Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

ffranr · 2025-01-03T23:19:32Z

This PR builds on Oli's WIP to demonstrate asset minting using the new V1 asset group key formulation. It integrates chantools to mint assets with an external asset group signing key (taproot internal key), offering a viable option for users who wish to keep their asset group signing key in cold storage.

A new integration test is included to simulate user behavior by leveraging chantools via the command line. The test verifies that the minter can successfully sign an asset into the asset group using the external asset group signing key.

Notes for Reviewers

Work which will be included in a future PR(s), ordered by most important first:

Tweaked NUMS key asset group key formulation.
This TODO in the doc:

TODO(guggero/ffranr): Replace this step by allowing the CLI to take the signed
PSBT instead. We can list the batch to get the asset ID of each pending asset
and match the PSBT's input previous output to find out what signed PSBT belongs
to what asset (in case there are multiple).

itest: ensure minter can spend asset minted using external key.
Unit tests for ExternalKey type and methods (draft: Add unit test for method ExternalKey.PubKey #1284)
Unit tests for method PendingAssetGroup.PSBT.

coveralls · 2025-01-08T15:46:45Z

Pull Request Test Coverage Report for Build 12709993838

Details

198 of 645 (30.7%) changed or added relevant lines in 10 files are covered.
22 unchanged lines in 6 files lost coverage.
Overall coverage decreased (-0.2%) to 40.763%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
proof/mint.go	7	10	70.0%
tapdb/assets_common.go	26	35	74.29%
cmd/tapcli/assets.go	0	49	0.0%
rpcserver.go	0	68	0.0%
tapgarden/planter.go	105	192	54.69%
asset/asset.go	46	161	28.57%
itest/chantools_harness.go	0	116	0.0%

Files with Coverage Reduction	New Missed Lines	%
tappsbt/create.go	2	53.22%
commitment/tap.go	2	84.09%
asset/asset.go	2	75.34%
tapgarden/planter.go	3	71.13%
rpcserver.go	3	0.0%
universe/interface.go	10	52.81%

Totals
Change from base Build 12650018423:	-0.2%
Covered Lines:	26495
Relevant Lines:	64997

💛 - Coveralls

Introduce a new method to support future usage in the tapgarden package.

Refactored the `FundBatch` method to use a general return type, `FundBatchResp`. This change improves code health and allows for easier extension by enabling the state machine to return additional fields in the future.

Introduced a `newVerboseBatch` function to create a verbose mint batch from a regular mint batch. This functionality is extracted from the existing list batch logic as part of a refactor. The function will be used in a future commit to include a verbose batch in the response of the FundBatch RPC endpoint.

Added comments to the planter function `buildGroupReqs` to improve code readability and explain the logic more clearly.

Introduce a guide outlining the workflow for using external group keys. Future commits in this PR will add features and implement changes to support the described workflow.

The FundBatch RPC endpoint now returns a verbose batch instead of a regular batch. The verbose batch includes additional asset group information required by external signers.

Added a new `ExternalKey` message type to represent an external key used for deriving and managing HD wallet addresses per BIP-86. A new field of type `ExternalKey` was added to the `MintAsset` message, enabling the specification of an external key when a mint request is added to the batch.

Introduce a new `ExternalKey` type and add external key fields of this type to `Seedling` and `GroupKeyRequest`.

Add unmarshalling support for the new `Asset.ExternalGroupKey` field in the `MintAsset` RPC endpoint. Also populate the external key in the `Seedling` instance.

Add an `external_key` field to the `GroupKeyRequest` RPC message, which will also make it available in the `UnsealedAsset` RPC message. Include marshalling functionality for the new field.

Added the following command line minting flags: - group_key_xpub - group_key_derivation_path - group_key_fingerprint

Updated the documentation for the `MintAsset.group_tapscript_root` field to clarify its purpose: it now represents the custom tapscript subtree root for V1 group key reveals and serves as the tapscript tree root for V0 group key reveals.

Add the `CustomTapscriptRoot` field to both the `GroupKey` and `GroupKeyRequest` types. This field stores the user-defined custom tapscript subtree root, which is committed to by the asset group key. Update the `buildGroupReqs` function to leverage this new field when constructing group key requests for generating asset group keys.

Introduce a new `GroupKeyVersion` type to represent the group key version in `asset.GroupKey` and `asset.GroupKeyRequest`. Add a `version` field to both `asset.GroupKey` and `asset.GroupKeyRequest`, with logic to populate it. Enhance the verification process by adding version-specific checks.

Update `GroupKeyRequest` methods to support the generation of version 1 (V1) group keys.

Add a new PSBT field to the `UnsealedAssets` RPC message type, which contains the byte-serialized PSBT equivalent of the group virtual transaction for unsealed assets. As a result, the `FundBatch` and `ListBatches` RPC endpoints now return group virtual PSBTs. Include logic to generate the group virtual PSBT. The PSBT is unsigned and is provided to allow signing with an external cold private key.

ffranr · 2025-01-09T02:35:04Z

I think these are the necessary changes. This PR can't really be made smaller in any meaningful way.

Extended the `asset_groups` database table with two new columns: `rows_version` and `custom_subtree_root`. These fields are required to store and retrieve new GroupKey data for use during mint proof generation, where the group key reveal is formulated.

Ensured the `version` and `custom_subtree_root` columns are populated with the appropriate GroupKey data when storing a group key in the database.

Implemented parsing logic for the `version` and `custom_subtree_root` fields when reading from the database.

Added support for group key reveal V1 alongside the existing group key reveal V0 when generating mint proofs.

Adds a test to ensure the new PSBT field introduced in the previous commit can be used to derive a transaction identical to the group virtual transaction.

Update the `build-itest` Makefile target, used as a subcommand during `make itest`, to ensure `chantools` is properly set up for integration tests. Additionally, update `.gitignore` to exclude the `chantools` build directory.

Add a test harness to execute the chantools binary via the command line and parse its output.

Added a new integration test, `testMintExternalGroupKeyChantools`, to verify the ability to mint an asset and generate an asset group signature using chantools with an externally managed signing key.

ffranr · 2025-01-10T13:02:14Z

As mentioned during our last call, I’ve updated taproot-assets/tapdb/sqlc/migrations/000026_asset_group_version_customsubtree.down.sql to include the recreation of the previous version of key_group_info_view as part of the down migration. While we’re not actively using down migrations, it’s still beneficial to have this in place as a precaution.

Roasbeef · 2025-01-12T21:57:15Z

taprpc/mintrpc/mint.proto

+    This key enables signing operations to be performed externally, outside
+    the daemon.
+    */
+    taprpc.ExternalKey external_group_key = 14;


Is this field mutually exclusive with the existing group_key field above?

Roasbeef · 2025-01-12T21:59:28Z

asset/asset.go

@@ -883,6 +966,10 @@ type GroupKeyRequest struct {
 	// has been applied.
 	RawKey keychain.KeyDescriptor

+	// ExternalKey specifies a public key that, when provided, is used to
+	// externally sign the group virtual transaction outside of tapd.
+	ExternalKey fn.Option[ExternalKey]


Same comment here re the proto: can RawKey and ExternalKey be specified at the same time?

Roasbeef · 2025-01-12T22:03:53Z

cmd/tapcli/assets.go

@@ -138,6 +139,21 @@ var mintAssetCommand = cli.Command{
 				"in order to avoid printing a large amount " +
 				"of data in case of large batches",
 		},
+		cli.StringFlag{
+			Name: "group_key_xpub",


Do we also need to be able to specify optional leaves for the custom tapscript root?

Roasbeef · 2025-01-12T22:08:51Z

cmd/tapcli/assets.go

-	})
+	}
+
+	// TODO(guggero): Actually just ask for the signed PSBT back and extract


Will this be addressed in this PR?

So accepting the signed PSBT to extract the signatures manually ourselves.

One other related thing is that we want to add some basic meta data to the group key minting PSBT to allow external software to display information such as the asset ID, name, and units to be minted.

Roasbeef · 2025-01-12T22:10:26Z

taprpc/mintrpc/mint.proto

+    group key. This allows for future asset issuance authorized using a
+    script witness.
+
+    If an external group key is provided, the V1 scheme for group key script


Why this assumption vs a version + default?

Not sure if I understand @Roasbeef's suggestion here, but at least mention the default V0 scheme so that it is clear what we the change in behavior entails.

Roasbeef · 2025-01-12T22:24:36Z

tapgarden/planter.go

+		return nil, fmt.Errorf("error producing group virtual PSBT "+
+			"from tx: %w", err)
+	}
+


Re my other comment, this is where we'd add the new custom global fields to allow external signers/coordinates to display meta data w.r.t what it actually being minted. We can also provide the information needed to derive the virtual tx in the first place (for additional validation).

Roasbeef · 2025-01-12T22:25:40Z

taprpc/mintrpc/mint.proto

@@ -124,6 +124,9 @@ message UnsealedAsset {

    // The group virtual transaction for the asset.
    taprpc.GroupVirtualTx group_virtual_tx = 3;
+
+    // The byte serialized PSBT equivalent of the group virtual transaction.
+    string group_virtual_psbt = 4;


SealBatch can also optionally accept this.

Roasbeef · 2025-01-12T22:29:03Z

tapdb/sqlc/migrations/000026_asset_group_version_customsubtree.up.sql

+-- is used to store the root of the custom subtree for the asset group. The
+-- custom subtree, if provided, represents a subtree of the final tapscript
+-- tree.
+ALTER TABLE asset_groups ADD COLUMN custom_subtree_root BLOB;


Why isn't the existing field sufficient?

taproot-assets/tapdb/sqlc/migrations/000002_assets.up.sql

Line 95 in a00fd22

tapscript_root BLOB,

I think we only need the version here added for this migration.

Roasbeef · 2025-01-12T22:30:43Z

tapdb/assets_common.go

+		GroupPubKey:         *tweakedGroupKey,
+		TapscriptRoot:       tapscriptRoot,
+		Witness:             groupWitness,
+		CustomTapscriptRoot: customSubtreeRootHash,


Should expand assertions for db tests to make sure these new fields are read/write properly.

Although I don't think we need the new custom tapscript root at all. We can get away with just one field, the existing one, instead of having to lug around two fields where one is a transformation of the other.

Roasbeef · 2025-01-12T22:32:21Z

itest/mint_fund_seal_test.go

@@ -501,6 +505,188 @@ func testMintFundSealAssets(t *harnessTest) {
 	})
 }

+// testMintExternalGroupKeyChantools tests that we're able to mint an asset
+// using an external asset signing group key derived and managed by chantools.
+func testMintExternalGroupKeyChantools(t *harnessTest) {


gijswijs

Great work. I have added some small comments throughout.

nit: the commit message of commit a993070 claims the commit populates the external key in the seedling instance, but I don't think that happens in that commit.

I would love to see a itest that tests the re-issuance of an asset with cold minting.

Would also like to see a unit test that tests both the V0 and the V1 flow re GroupTapscriptRoot and a custom tapscript root. Whatever approach we take regarding implicit or explicit flow @Roasbeef suggested, we will end up with two separate flows that are currently not fully covered by unit tests.

gijswijs · 2025-01-10T18:08:49Z

asset/asset.go

+
+	if len(e.DerivationPath) != 5 {
+		return fmt.Errorf("derivation path must have exactly 5 " +
+			"componenets")


Suggested change

"componenets")

"components")

gijswijs · 2025-01-13T14:37:53Z

taprpc/mintrpc/mint.proto

+    group key. This allows for future asset issuance authorized using a
+    script witness.
+
+    If an external group key is provided, the V1 scheme for group key script


Not sure if I understand @Roasbeef's suggestion here, but at least mention the default V0 scheme so that it is clear what we the change in behavior entails.

gijswijs · 2025-01-13T15:04:15Z

asset/asset.go

-	)
+	// Construct an asset group pub key.
+	genesisAssetID := req.AnchorGen.ID()
+	groupPubKey, err := req.NewGroupPubKey(genesisAssetID)


👍 I don't mind these small commits. Your commit narrative is very clear to me.

gijswijs · 2025-01-13T15:15:20Z

tapdb/sqlc/queries/assets.sql

-    assets.asset_id AS asset_primary_key, assets.genesis_id, version, spent,
+    assets.asset_id AS asset_primary_key,
+    assets.genesis_id, assets.version,
+    spent,


ultra-nit: What are our coding style guidelines re line breaks and such for SQL?

gijswijs · 2025-01-13T15:21:51Z

tapdb/sqlc/migrations/000026_asset_group_version_customsubtree.up.sql

+-- assets we care about. We obtain only the assets found in the batch
+-- above, with the WHERE query at the bottom.


This is copy pasta from the original migration file. The comment assets found in the batch above doesn't make sense here.

Suggested change

-- assets we care about. We obtain only the assets found in the batch

-- above, with the WHERE query at the bottom.

-- assets we care about. We obtain the assets with the WHERE query at the

-- bottom.

ffranr self-assigned this Jan 3, 2025

dstadulis requested a review from GeorgeTsagk January 6, 2025 15:32

dstadulis added this to the v0.6 milestone Jan 6, 2025

dstadulis added wallet psbt group_key labels Jan 6, 2025

ffranr force-pushed the cold-group-key branch 3 times, most recently from c766bbe to cabfd1d Compare January 8, 2025 15:30

dstadulis mentioned this pull request Jan 9, 2025

docs: Clarify various aspects of external-group-key.md and add specificity #1282

Open

6 tasks

ffranr and others added 17 commits January 9, 2025 01:58

multi: fix formatting and doc comments

58f7f05

asset: add GroupKeyRevealTapscript.Root method

a74692e

Introduce a new method to support future usage in the tapgarden package.

tapgarden: introduce FundBatchResp return type for FundBatch

702dc6e

Refactored the `FundBatch` method to use a general return type, `FundBatchResp`. This change improves code health and allows for easier extension by enabling the state machine to return additional fields in the future.

tapgarden: clarify buildGroupReqs with additional comments

8d3a435

Added comments to the planter function `buildGroupReqs` to improve code readability and explain the logic more clearly.

docs: add guide for using external group keys

e891588

Introduce a guide outlining the workflow for using external group keys. Future commits in this PR will add features and implement changes to support the described workflow.

multi: update FundBatch RPC to return verbose batch

66b442b

The FundBatch RPC endpoint now returns a verbose batch instead of a regular batch. The verbose batch includes additional asset group information required by external signers.

asset+tapgarden: add ExternalKey field to Seedling and GroupKeyRequest

74decd6

Introduce a new `ExternalKey` type and add external key fields of this type to `Seedling` and `GroupKeyRequest`.

rpcserver: unmarshal Asset.ExternalGroupKey in MintAsset endpoint

a993070

Add unmarshalling support for the new `Asset.ExternalGroupKey` field in the `MintAsset` RPC endpoint. Also populate the external key in the `Seedling` instance.

taprpc: add external_key field to GroupKeyRequest message

c682091

Add an `external_key` field to the `GroupKeyRequest` RPC message, which will also make it available in the `UnsealedAsset` RPC message. Include marshalling functionality for the new field.

cmd/tapcli: add minting flags for external key asset group support

a235fb8

Added the following command line minting flags: - group_key_xpub - group_key_derivation_path - group_key_fingerprint

asset: add support for V1 group keys in GroupKeyRequest methods

cb985f7

Update `GroupKeyRequest` methods to support the generation of version 1 (V1) group keys.

ffranr force-pushed the cold-group-key branch 2 times, most recently from 8af293f to 4666b23 Compare January 9, 2025 02:19

ffranr marked this pull request as ready for review January 9, 2025 02:19

ffranr requested a review from gijswijs January 9, 2025 02:19

ffranr force-pushed the cold-group-key branch 2 times, most recently from 1e543e2 to e61d638 Compare January 9, 2025 02:29

ffranr changed the title ~~Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support~~ Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support Jan 9, 2025

This was referenced Jan 9, 2025

Add unit test for method ExternalKey.PubKey #1284

Draft

Add PSBT field to UnsealedAsset RPC message #1214

Closed

ffranr and others added 8 commits January 10, 2025 12:59

tapdb: write new GroupKey fields to database rows

f030426

Ensured the `version` and `custom_subtree_root` columns are populated with the appropriate GroupKey data when storing a group key in the database.

tapdb: read new GroupKey fields from database rows

650efb5

Implemented parsing logic for the `version` and `custom_subtree_root` fields when reading from the database.

proof: support group key reveal V1 in mint proof generation

d49f4c1

Added support for group key reveal V1 alongside the existing group key reveal V0 when generating mint proofs.

itest: verify group virtual transaction can be derived from PSBT

06ec377

Adds a test to ensure the new PSBT field introduced in the previous commit can be used to derive a transaction identical to the group virtual transaction.

Makefile: enhance build-itest target for chantools setup

18bbcf3

Update the `build-itest` Makefile target, used as a subcommand during `make itest`, to ensure `chantools` is properly set up for integration tests. Additionally, update `.gitignore` to exclude the `chantools` build directory.

itest: add harness for invoking chantools binary

3b7a2ec

Add a test harness to execute the chantools binary via the command line and parse its output.

itest: add test for minting with external group key via chantools

c0c6795

Added a new integration test, `testMintExternalGroupKeyChantools`, to verify the ability to mint an asset and generate an asset group signature using chantools with an externally managed signing key.

ffranr force-pushed the cold-group-key branch from e61d638 to c0c6795 Compare January 10, 2025 12:59

ffranr mentioned this pull request Jan 10, 2025

PoC: Use tweaked NUMS key for non-spendable Taproot script leaves #1289

Draft

Roasbeef requested changes Jan 12, 2025

View reviewed changes

gijswijs reviewed Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

ffranr commented Jan 3, 2025 •

edited

Loading

coveralls commented Jan 8, 2025 •

edited

Loading

ffranr commented Jan 9, 2025 •

edited

Loading

ffranr commented Jan 10, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

gijswijs Jan 13, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

Roasbeef Jan 12, 2025

gijswijs left a comment

gijswijs Jan 10, 2025

gijswijs Jan 13, 2025

gijswijs Jan 13, 2025

gijswijs Jan 13, 2025

gijswijs Jan 13, 2025

		-- assets we care about. We obtain only the assets found in the batch
		-- above, with the WHERE query at the bottom.

Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

Are you sure you want to change the base?

Part 1: Asset Minting with V1 Asset Group Key and Chantools Cold Storage Support #1272

Conversation

ffranr commented Jan 3, 2025 • edited Loading

Notes for Reviewers

coveralls commented Jan 8, 2025 • edited Loading

Pull Request Test Coverage Report for Build 12709993838

Details

💛 - Coveralls

ffranr commented Jan 9, 2025 • edited Loading

ffranr commented Jan 10, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gijswijs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffranr commented Jan 3, 2025 •

edited

Loading

coveralls commented Jan 8, 2025 •

edited

Loading

ffranr commented Jan 9, 2025 •

edited

Loading