tapgarden: implement SealBatch #866

jharveyb · 2024-04-02T18:25:09Z

Addresses #722. Depends on #827.

Remaining TODOs:

Extend tapdb/asset_minting tests to cover new query params for script key, group key, etc.
Add tests for tapdb/asset_minting/CommitBatchTx, AddSeedlingGroups, FetchSeedlingGroups
Update SealBatch to reject being called more than once
Add itest that mints with custom asset group witnesses, custom script keys
Add itest that spends an asset minted with a custom script key
Update existing minting itests as needed
Add itest that spends an anchor amount with a custom script instead of a signature

jharveyb · 2024-04-03T20:26:17Z

Have an external group witness working now in unit tests. Still looking at ways to prevent SealBatch from being called multiple times.

Right now IIUC we don't explicitly reject new seedling requests after a batch is frozen, so one option is to just freeze the batch at the end of SealBatch + disallow all calls except for FinalizeBatch on a frozen batch. So after calling SealBatch, the only other calls available would be finalize or cancel.

guggero

Looks pretty good to me! Looking forward to seeing it in action with an itest 🎉
Main request is to potentially remove some code duplication by moving some of the complexity of the seedlings further down, instead of having two almost identical batch structs.

tapgarden/batch.go

tapgarden/planter_test.go

tapgarden/planter.go

taprpc/marshal.go

taprpc/mintrpc/mint.proto

tapgarden/planter.go

jharveyb · 2024-04-11T15:07:39Z

Looks like a possible new flake in custodian event handling?

--- FAIL: TestTransactionConfirmedOnly (123.22s)
    test_postgres.go:11: Creating new Postgres DB for testing
    custodian_test.go:220: 
        	Error Trace:	/home/runner/work/taproot-assets/taproot-assets/tapgarden/custodian_test.go:220
        	            				/home/runner/work/taproot-assets/taproot-assets/tapgarden/custodian_test.go:740
        	            				/home/runner/work/taproot-assets/taproot-assets/tapgarden/custodian_test.go:669
        	Error:      	Received unexpected error:
        	            	method did not return within the timeout
        	Test:       	TestTransactionConfirmedOnly
FAIL
FAIL	github.com/lightninglabs/taproot-assets/tapgarden	156.362s
FAIL

jharveyb · 2024-04-12T22:17:21Z

Posted some updates, mostly fixes where the group tapscript root wasn't being propogated correctly.

Also some changes in the MintAsset RPC to accomodate all the new fields.

Still working out some marshalling issues when generating group witnesses.

jharveyb · 2024-04-15T15:26:06Z

Posted my latest state. The issue with the version of the test I was working on before was internal keys not derived from the backing tapd node. Example for derivation here:

taproot-assets/itest/mint_fund_seal_test.go

Line 70 in 761212d

deriveRandomKey := func(lnd *node.HarnessNode) keychain.KeyDescriptor {

The posted version generates a sig for a siglock external to the planter, but I could not get a similar witness working if that key was derived from the backing lnd node, for both the Taproot Assets key family, or a random key family.

I'm at the last steps of the PSBT flow, which is very similar to the multisig test.

jharveyb · 2024-04-16T18:13:31Z

Remaining tasks:

freeze batch at the end of SealBatch to prevent further calls to add seedlings, fund, or freeze
Add CLI support

There's a path toward external key support in the itest via some changes in buildGroupReqs and how the key descriptor is set in the seedling:

#883 (comment)

jharveyb · 2024-04-23T16:46:22Z

I have a branch that resolves #883 , but the better fix for that would be switching to SignPsbt for the VirtualTxSigner.

From offline discussion, I think for this PR we'll continue to accept a non-local group internal key without any constraints on the keyDescriptor / won't require mutual exclusion of the pubkey and keyLocator. I think having the family and index in the DB for a non-local internal key is useful in some cases, and shouldn't cause issues if it's for the group internal key.

jharveyb · 2024-05-01T00:42:40Z

CLI commands are added, but they omit the fields with non-trivial formatting like KeyDescriptor or asset.ScriptKey. Similar to the commands we have for generating addresses, some functionality is only available via the RPCs directly.

lightninglabs-deploy · 2024-05-08T01:44:47Z

@GeorgeTsagk: review reminder
@guggero: review reminder

guggero

Very nice! tACK, LGTM 🎉

Mostly nits and nice-to-haves.

tapgarden/planter.go

tapgarden/planter_test.go

tapgarden/planter.go

itest/mint_fund_seal_test.go

cmd/tapcli/assets.go

guggero · 2024-05-10T11:51:35Z

cmd/tapcli/assets.go

+var sealBatchCommand = cli.Command{
+	Name:        "seal",
+	Usage:       "seal a batch",
+	Description: "Attempt to seal the pending batch.",


Does this command really do anything on the command line other than sanity check that we can generate witnesses for all group keys in the batch? Not sure if we should hide (Hidden: true) the command and only keep it for debugging? Since it probably doesn't make sense to support submitting the actual witnesses through the CLI (only makes sense through RPC).

Hmm, yeh seems odd given that it doesn't have any acutal args in the proto.

Yeah I agree it doesn't have much use...

I suppose we want to still expose fund as we'll add other features to that later.

Update: added more to the descriptions and hid this cmd. In practice you could do fund + finalize from the CLI if we don't accept witness in seal.

taprpc/taprootassets.proto

itest/assertions.go

Roasbeef · 2024-05-14T22:59:00Z

itest/mint_fund_seal_test.go

+// also want to reissue assets with a custom asset group witness, and spend
+// an asset with a custom script key witness. An asset group with a tapscript
+// root should also be imported into a new node correctly.
+func testMintFundSealAssets(t *harnessTest) {


Great itest, but I think it would be easier to follow if some of the sub-components were split down into helper functions. So eg:

Making the tapscript root for the group key.

Making the root for the funding output tapscript leaf.

Signing on both levels,

The multi-sig sign and transfer at the end,

etc

Roasbeef · 2024-05-14T22:59:49Z

cmd/tapcli/assets.go

+var sealBatchCommand = cli.Command{
+	Name:        "seal",
+	Usage:       "seal a batch",
+	Description: "Attempt to seal the pending batch.",


Hmm, yeh seems odd given that it doesn't have any acutal args in the proto.

Roasbeef · 2024-05-14T23:03:45Z

rpcserver.go

+
+func (r *rpcServer) SealBatch(ctx context.Context,
+	req *mintrpc.SealBatchRequest) (*mintrpc.SealBatchResponse, error) {
+


Shouldn't this require that group witnesses re provided? Otherwise, how can you seal a batch w/o providing the witness?

The check for all assets that need a witness, having a witness, is in the planter:

taproot-assets/tapgarden/planter.go

Line 1360 in 4b15443

switch {

For all assets without an external witness, we try to derive the default witness. If we are unable to, DeriveGroupKey fails and no groups would be saved to disk.

So this call would fail if at least one external witness is needed. From the RPC layer, we can't really know if any external witnesses are needed.

In this commit, we add a verbosity flag to ListBatches, which will show the group key request and virtual TX needed for a user to produce an asset group witness for a grouped seedling.

In this commit, we add an itest to exercise the FundBatch and SealBatch RPCs. Specifically, we use asset script and group internal keys derived outside of tapd, pass asset group witnesses via SealBatch, and use a tweaked asset script key set during minting to complete an asset transfer. We also use a tapscript sibling set during minting to provide a script witness for anchoring an asset transfer.

jharveyb · 2024-05-16T15:38:47Z

Updated to address all comments, itest should be more readable now + other small fixes.

Roasbeef

LGTM 🍵

jharveyb added this to the v0.4 milestone Apr 2, 2024

jharveyb self-assigned this Apr 2, 2024

dstadulis added enhancement New feature or request batching labels Apr 2, 2024

jharveyb force-pushed the mint_seal_batch branch from 3f53f04 to a6fc23f Compare April 2, 2024 23:26

jharveyb force-pushed the mint_early_sprouting branch from c2e1cfd to 42b7bde Compare April 3, 2024 18:06

jharveyb force-pushed the mint_seal_batch branch from a6fc23f to c922d04 Compare April 3, 2024 18:13

jharveyb force-pushed the mint_early_sprouting branch from 42b7bde to 132a2d0 Compare April 9, 2024 16:48

jharveyb force-pushed the mint_seal_batch branch from c922d04 to cf332e1 Compare April 9, 2024 16:50

Base automatically changed from mint_early_sprouting to main April 9, 2024 17:19

jharveyb force-pushed the mint_seal_batch branch from cf332e1 to 78baa21 Compare April 9, 2024 18:45

jharveyb requested a review from guggero April 10, 2024 01:19

guggero reviewed Apr 10, 2024

View reviewed changes

jharveyb force-pushed the mint_seal_batch branch 2 times, most recently from 1f28703 to 434b33a Compare April 10, 2024 22:14

jharveyb force-pushed the mint_seal_batch branch from 3909a99 to 67ade9f Compare April 12, 2024 20:24

jharveyb force-pushed the mint_seal_batch branch from 67ade9f to 761212d Compare April 15, 2024 15:21

jharveyb force-pushed the mint_seal_batch branch 2 times, most recently from fb3ce36 to 00530e2 Compare April 15, 2024 19:28

jharveyb force-pushed the mint_seal_batch branch from 00530e2 to 0dd4dd0 Compare April 25, 2024 18:58

jharveyb force-pushed the mint_seal_batch branch 2 times, most recently from 69b4b7e to c74bf0b Compare May 1, 2024 00:41

jharveyb requested review from GeorgeTsagk and guggero May 1, 2024 00:43

jharveyb marked this pull request as ready for review May 1, 2024 00:43

jharveyb linked an issue May 1, 2024 that may be closed by this pull request

[feature]: custom Group Key API for minting #722

Closed

3 tasks

guggero approved these changes May 10, 2024

View reviewed changes

Roasbeef reviewed May 14, 2024

View reviewed changes

dstadulis mentioned this pull request May 15, 2024

[Tracking]: tapgarden: implement SealBatch #899

Closed

jharveyb mentioned this pull request May 15, 2024

[feature]: allow deriving group key upfront to simplify minting #749

Closed

jharveyb added 6 commits May 15, 2024 13:35

tapgarden: show asset group info in ListBatches

3d8bf85

In this commit, we add a verbosity flag to ListBatches, which will show the group key request and virtual TX needed for a user to produce an asset group witness for a grouped seedling.

tapgarden+tapdb: implement SealBatch

9a0ac1a

taprpc: add protos for FundBatch, SealBatch RPCs

0ca85fb

rpcserver: implement Fund and Seal functions

551e96b

itest: update ListBatches calls

c972893

tapdb: expose tapscript root for chain assets

9befae0

jharveyb force-pushed the mint_seal_batch branch from c74bf0b to 4b15443 Compare May 15, 2024 17:39

jharveyb added 2 commits May 16, 2024 10:53

cmd/tapcli: add fund and seal minting subcommands

c5f425e

jharveyb force-pushed the mint_seal_batch branch from 4b15443 to c5f425e Compare May 16, 2024 14:53

jharveyb requested review from Roasbeef and removed request for GeorgeTsagk May 16, 2024 15:38

Roasbeef approved these changes May 16, 2024

View reviewed changes

Roasbeef merged commit 057880e into main May 16, 2024

guggero deleted the mint_seal_batch branch May 17, 2024 06:40

jharveyb mentioned this pull request Jun 10, 2024

tapgarden: batch restart fixes #941

Merged

jharveyb mentioned this pull request Jul 1, 2024

tapgarden: minting batch display for pre-0.3.3 batches #986

Closed


		func (r *rpcServer) SealBatch(ctx context.Context,
		req mintrpc.SealBatchRequest) (mintrpc.SealBatchResponse, error) {

tapgarden: implement SealBatch #866

tapgarden: implement SealBatch #866

Uh oh!

Conversation

jharveyb commented Apr 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jharveyb commented Apr 3, 2024

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jharveyb commented Apr 11, 2024

Uh oh!

jharveyb commented Apr 12, 2024

Uh oh!

jharveyb commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jharveyb commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jharveyb commented Apr 23, 2024

Uh oh!

jharveyb commented May 1, 2024

Uh oh!

lightninglabs-deploy commented May 8, 2024

Uh oh!

guggero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guggero May 10, 2024

Choose a reason for hiding this comment

Uh oh!

Roasbeef May 14, 2024

Choose a reason for hiding this comment

Uh oh!

jharveyb May 15, 2024

Choose a reason for hiding this comment

Uh oh!

jharveyb May 15, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Roasbeef May 14, 2024

Choose a reason for hiding this comment

Uh oh!

Roasbeef May 14, 2024

Choose a reason for hiding this comment

Uh oh!

Roasbeef May 14, 2024

Choose a reason for hiding this comment

Uh oh!

jharveyb May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jharveyb commented May 16, 2024

Uh oh!

Roasbeef left a comment

Choose a reason for hiding this comment

jharveyb commented Apr 2, 2024 •

edited

Loading

jharveyb commented Apr 15, 2024 •

edited

Loading

jharveyb commented Apr 16, 2024 •

edited

Loading

jharveyb May 14, 2024 •

edited

Loading