Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tapgarden: add FundBatch and SealBatch requests #827

Merged
merged 16 commits into from
Apr 9, 2024
Merged

Conversation

jharveyb
Copy link
Contributor

@jharveyb jharveyb commented Mar 11, 2024

Closes #820

Update the minting state machines to move batch funding into a separate command that can execute before batch finalization. Add a new command to supply group anchor witnesses before batch finalization.

Remaining TODOs:

  • derive seedling script keys in prepAssetSeedling
  • DB migration to add seedling script key reference field
  • Update DB queries to write & fetch seedling script key
  • Update DB queries to write & fetch batch TX
  • derive group internal key in prepAssetSeedling
  • Update DB queries to write & fetch seedling group internal key
  • Extend tapdb/asset_minting tests to cover new query params
  • Add tests for tapdb/asset_minting/CommitBatchTx
  • Resolve FinalizeParams handling wrt. fundBatch
  • Extend tapgarden/planter_test test to cover FundBatch
  • Implement SealBatch
  • Remove batch ticker from caretaker and tapgarden tests

@dstadulis
Copy link
Collaborator

Will be pulled out of draft and merged this sprint

@jharveyb jharveyb force-pushed the mint_early_sprouting branch 2 times, most recently from 3cdbe5c to 1ab4428 Compare March 20, 2024 00:51
@jharveyb jharveyb marked this pull request as ready for review March 20, 2024 01:17
@jharveyb jharveyb requested a review from guggero March 20, 2024 01:18
@jharveyb
Copy link
Contributor Author

Feature work is done IMO, follow-on work can happen in a separate PR as this one has already ballooned quite a bit.

Remaining tasks are just unit test coverage AFAICT, should add some usage in an itest as well.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! This gets us quite far in terms of re-arranging the state machine. Some commits need some love/cleanup, but apart from that the code looks pretty good.
Will need to do a second pass once we have the follow-up PR building on top of this that exposes the RPC methods to interact with the new states.

tapgarden/planter.go Show resolved Hide resolved
tapgarden/planter_test.go Show resolved Hide resolved
tapgarden/planter_test.go Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
tapgarden/caretaker.go Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the mint_early_sprouting branch 2 times, most recently from 4c02fc5 to 3064415 Compare March 21, 2024 15:04
@jharveyb
Copy link
Contributor Author

One DB change is actually missing from this PR, related to group key handling. We need to also associate a [32]byte with a Seedling, which would later map to a GroupKey.TapscriptRoot. Without that, a user could not later provide a custom tapscript root for a group key, and if they generated witness using such a root they would not verify with the group virtual TXs made during sealBatch.

@jharveyb jharveyb force-pushed the mint_early_sprouting branch 3 times, most recently from 0a6065a to e3a322f Compare March 22, 2024 16:19
tapgarden/interface.go Show resolved Hide resolved
tapgarden/planter.go Show resolved Hide resolved
tapgarden/planter.go Show resolved Hide resolved
tapgarden/planter.go Show resolved Hide resolved
tapdb/asset_minting.go Outdated Show resolved Hide resolved
tapgarden/seedling.go Show resolved Hide resolved
tapdb/asset_minting.go Show resolved Hide resolved
tapgarden/planter.go Outdated Show resolved Hide resolved
tapgarden/planter.go Show resolved Hide resolved
tapgarden/planter.go Show resolved Hide resolved
@guggero guggero self-requested a review March 25, 2024 16:55
@jharveyb jharveyb force-pushed the mint_early_sprouting branch 2 times, most recently from b341ded to 973ac8b Compare March 26, 2024 17:39
@jharveyb
Copy link
Contributor Author

jharveyb commented Mar 26, 2024

Updated to address some feedback and update tests to exercise query changes. Also fixed some issues with not accepting externally-set seedling script keys and group internal keys.

Hitting some issues with minting DB tests around using the seedling group tapscript root.

From discussion offline, I think the simplest behavior for FundBatch and SealBatch would be that they can be called exactly once before Finalize. I don't see a clear use case for making those calls multiple times. Its straightforward to detect if a batch is funded or sealed, so it should also be straightforward to return an error here and require the batch to be resubmitted.

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending existing comments and successful CI run.

tapdb/asset_minting.go Show resolved Hide resolved
tapdb/assets_common.go Show resolved Hide resolved
asset/asset.go Show resolved Hide resolved
asset/asset.go Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the mint_early_sprouting branch 2 times, most recently from 409b9c8 to d234399 Compare March 29, 2024 19:59
@jharveyb jharveyb force-pushed the mint_early_sprouting branch from c2e1cfd to 42b7bde Compare April 3, 2024 18:06
@jharveyb
Copy link
Contributor Author

jharveyb commented Apr 3, 2024

Rebased.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🦚

@Roasbeef Roasbeef added this pull request to the merge queue Apr 8, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 8, 2024
@guggero
Copy link
Member

guggero commented Apr 9, 2024

Needs another rebase to apply new linter rules, then a couple of fixes for the new rules.

jharveyb added 16 commits April 9, 2024 11:44
In this commit, we introduce two new functions that allow batch funding
before finalization, and supplying custom group witnesses for asset
groups being created in a batch.
In this commit, we export a query from the MintingStore that allows a TX
to be bound to a batch before finalization. We also write the TX of a
batch, if present, when we are first writing a batch.
In this commit, we move batch funding from the caretaker to the planter,
where it can occur before batch finalization. We optionally create a new
batch during funding, and we fund batches during finalization before
starting a caretaker.
In this commit, we stop using the batch ticker in any minter unit tests.
This changed some test behavior, as finalizing via the ticker is
non-blocking. We update some assertions to read the DB directly instead
of querying the planter to maintain similar guarantees as before.
Finally, we remove a test section checking for batch cancellation after
finalize, which is not possible as finalize is blocking. This only
worked before because it used the batch ticker.
In this commit, we add logic to the planter to produce asset group
witnesses for all seedlings in a batch associated with an asset group.
We also store the asset groups so they can be fetched by the caretaker
during batch finalization.
In this commit, we add a unit test that funds a batch before finalize.
This test also provides external keys for some seedlings to ensure that
these are handled correctly during minting. We also update the planter
to reject fund and finalize calls if a batch is already funded.
@jharveyb jharveyb force-pushed the mint_early_sprouting branch from 42b7bde to 132a2d0 Compare April 9, 2024 16:48
@jharveyb jharveyb enabled auto-merge April 9, 2024 17:02
@jharveyb jharveyb added this pull request to the merge queue Apr 9, 2024
Merged via the queue into main with commit dc8932d Apr 9, 2024
14 checks passed
@guggero guggero deleted the mint_early_sprouting branch April 9, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

tapgarden: earlier handling of asset script keys
4 participants