Refactor: Split Pre-Commitment Logic to Support New Asset Groups

[ffranr]

Closes #1586

---

Problem

The entire pre-commitment process—both adding the output to the transaction and linking it to an asset group—was executed in the funding step. This approach was flawed because it incorrectly assumed the asset group already existed on disk. This caused a failure when handling universe commitments for newly created assets, as the required group data was not yet available.

Solution

This PR refactors the logic to align with the minting batch lifecycle, separating transaction construction from data association.

• During the funding step: The pre-commitment output is still created and added to the minting transaction, as before. This part of the logic, which deals with the transaction itself, in principle remains unchanged.

• During the seal step: We now introduce logic to upsert the relationship between the pre-commitment output and the asset group. This association is intentionally deferred to the seal step because this is the first point at which we can guarantee a new asset group has been successfully created and its data is available.

This separation of concerns ensures the feature works reliably for both new and existing asset groups.

---

This change is 

[coveralls]

Pull Request Test Coverage Report for Build 15737474312

Details

• 126 of 537 (23.46%) changed or added relevant lines in 9 files are covered.
• 25 unchanged lines in 9 files lost coverage.
• Overall coverage decreased (-0.03%) to 37.703%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
itest/utils.go	0	3	0.0%
tapgarden/caretaker.go	16	20	80.0%
tapdb/sqlc/assets.sql.go	31	39	79.49%
tapgarden/batch.go	2	10	20.0%
tapdb/assets_common.go	0	19	0.0%
itest/assertions.go	0	22	0.0%
rpcserver.go	0	32	0.0%
tapdb/asset_minting.go	1	152	0.66%
tapgarden/planter.go	76	240	31.67%

Files with Coverage Reduction	New Missed Lines	%
rpcserver.go	1	0.0%
tapdb/asset_minting.go	1	57.2%
tapgarden/batch.go	1	71.63%
asset/group_key.go	2	57.52%
commitment/tap.go	2	71.82%
itest/assertions.go	2	0.0%
tapgarden/caretaker.go	2	68.49%
tapdb/multiverse.go	6	53.03%
tapgarden/planter.go	8	58.45%

Totals
Change from base Build 15736920214:	-0.03%
Covered Lines:	28178
Relevant Lines:	74736

---

💛 - Coveralls

[Roasbeef]

So this handles the case where you're creating the very first tranche, that's unconfirmed, but then you attempt to create a subsequent tranche?

[ffranr]

Yes, it can handle issuing into an existing asset group and also creating a new asset group.

[Roasbeef]

At the time of validation, the group key may not yet exist and could be
generated later during batch sealing.

How can the group key be generated later on? We need to know the group key upfront to create any of the set of relevant commitments.

[ffranr]

Previously, the if !newSeedling.HasGroupKey() check within ChainPlanter.prepAssetSeedling was used to verify that a seedling had its GroupInfo set upon being added to a batch. This worked correctly for seedlings requested through c.seedlingReqs that already had a group key. However, this check created an issue for seedlings whose group key is generated later, during the batch seal step. In this scenario, the seedlings would not yet have their GroupInfo set, causing the HasGroupKey() check to fail incorrectly.

[ffranr]

I've revised this once more in the latest commits. It's now:

if !newSeedling.EnableEmission && !newSeedling.HasGroupKey() {
			return fmt.Errorf("universe commitment enabled: " +
				"seedling must either create a new asset " +
				"group or issue into an existing one")
}

which i think is what we want.

[Roasbeef]

Does this change violate the assumptions of any of the other tests?

[ffranr]

This check was somewhat odd. The assertion logic attempted to reconstruct what it believed the asset’s metadata reveal should be, then asserted that the resulting metadata hash matched. However, if the supply commitment delegation key is generated during minting and not specified in the minting request, the assertion logic can't reliably know the full metadata reveal in advance. We need to clarify what this metadata hash assertion is actually verifying, given that some metadata fields are only determined after minting completes.

[ffranr]

Failing itest is: testMintFundSealAssets. It's flakey, it doesn't always fail.
Earliest commit I've been able to reproduce so far:

tapdb+tapgarden: update pre-commit output db entry as part of batch seal ffranr 06/06/2025, 23:57 39674350

Error message:

2025-06-11 17:15:55.399 [ERR] GRDN caretaker.go:390: unable to advance state machine: unable to construct minting proofs: invalid proof file generated: group key reveal doesn't match group key

It fails locally and in github CI.

[Roasbeef]

Reviewed 19 of 19 files at r2, all commit messages.
Reviewable status: 19 of 25 files reviewed, 14 unresolved discussions (waiting on @ffranr and @guggero)

---

tapgarden/batch.go line 427 at r2 (raw file):

		// it violates the constraints of the universe commitment
		// feature.
		if !newSeedling.EnableEmission && !newSeedling.HasGroupKey() {

So the prior logic here was essentially an OR, what prompted the change to instead assert the conjunction?'

The error message here also seems to indicate that the block here was still intended to be an OR?

---

tapdb/asset_minting.go line 1476 at r2 (raw file):

// public key.
func (a *AssetMintingStore) FetchDelegationKey(ctx context.Context,
	groupKey btcec.PublicKey) (fn.Option[tapgarden.DelegationKey], error) {

IIRC, we actually set things up s.t this key can change across minted batches. If this is the case, then we may need to update the logic here to return a slice, and factor in that the delegation key can change over time.

For ref, I understand the delegation key to serve two roles:

1. The internal key used in the pre commitment output.
2. The key used to sign the new blob for grouped assets that opt into an on-chain universe commitment.

---

tapgarden/planter.go line 584 at r2 (raw file):

				// the pending batch.
				if err == nil && sealedBatch != nil {
					batch = sealedBatch

Do we need to fold the update call below into this block? As either way we're updating the state of the batch, which is then passed into newCaretakerForBatch.

---

tapgarden/planter.go line 1986 at r2 (raw file):

				// the pending batch.
				if err == nil && sealedBatch != nil {
					c.pendingBatch = sealedBatch

Ah so then the main shifted here is returning a new batch vs mutating the param tha was passed in.

---

tapgarden/planter.go line 2453 at r2 (raw file):

	}

	// Assign each newly created asset group to its corresponding seedling.

Can we split out this new section into a new helper function? This method is already rather long as is.

[ffranr]

Reviewable status: 18 of 25 files reviewed, 14 unresolved discussions (waiting on @ffranr, @guggero, and @Roasbeef)

---

tapdb/asset_minting.go line 1476 at r2 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

IIRC, we actually set things up s.t this key can change across minted batches. If this is the case, then we may need to update the logic here to return a slice, and factor in that the delegation key can change over time.

For ref, I understand the delegation key to serve two roles:

1. The internal key used in the pre commitment output.
2. The key used to sign the new blob for grouped assets that opt into an on-chain universe commitment.

My view when working on the pre-commit aspect of the supply commit feature was to keep things simple for now. Setting the delegation key isn’t possible via the existing CLI or RPC interface, but we’re not ruling out support for delegation key updates in the future.

In this version, the delegation key is assumed to be fixed for a given group key. Later on, we could extend the feature to allow delegation key changes. I'd like to focus on getting the supply commitment feature working end to end before making it any more complicated.

Does that strategy sound reasonable to you?

---

tapgarden/batch.go line 427 at r2 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

So the prior logic here was essentially an OR, what prompted the change to instead assert the conjunction?'

The error message here also seems to indicate that the block here was still intended to be an OR?

We only raise an error in this if-clause when the seedling is neither creating a new asset group nor adding an asset to an existing one. In other words, if the seedling does not relate to any asset group and the supply/uni commitment feature is enabled, we return an error.

• !newSeedling.EnableEmission -> does the seedling NOT generate a new asset group
• !newSeedling.HasGroupKey() -> does the seedling NOT add an asset to an exiting asset group.

---

tapgarden/planter.go line 584 at r2 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Do we need to fold the update call below into this block? As either way we're updating the state of the batch, which is then passed into newCaretakerForBatch.

I've updated my code block as follows:

// If the sealBatch call returned a sealed
// batch, update the pending batch accordingly.
if sealedBatch != nil {
    batch = sealedBatch
}

// Any pending batch that was funded and sealed
// can now be set as frozen. We are already not
// able to add new seedlings to the batch.
batch.UpdateState(BatchStateFrozen)

If the seal batch function returns ErrBatchAlreadySealed then sealedBatch == nil. That's the only edge case we need to avoid. Otherwise, batch = sealedBatch should be fine.

---

tapgarden/planter.go line 1986 at r2 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Ah so then the main shifted here is returning a new batch vs mutating the param tha was passed in.

Before my changes, method ChainPlanter.sealBatch did return batchWithGroupInfo which was a copy of the working batch. So effectively, and it did not mutate the workingBatch param with batchWithGroupInfo.

---

tapgarden/planter.go line 2453 at r2 (raw file):

Previously, Roasbeef (Olaoluwa Osuntokun) wrote…

Can we split out this new section into a new helper function? This method is already rather long as is.

Will do 👍

[guggero]

Great refactor work! Found at least one bug that we need to fix. The rest is mostly optimization suggestions

[guggero]

Why do we unpack this when we put it back as an option? If we just want to validate it's there, we can do if !groupKeyOpt.IsSome(). Same for the preCommitDesc. Probably doesn't matter too much, but might visually simplify the logic a little bit.

[ffranr]

I've gone with:

	// Formulate the pre-commitment output descriptor with the group key.
	preCommitDesc := fn.MapOptionZ(
		fundedAnchor.PreCommitmentOutput,
		// nolint: lll
		func(preCommit PreCommitmentOutput) fn.Option[PreCommitmentOutput] {
			preCommit.GroupPubKey = fn.Some(groupKey)
			return fn.Some(preCommit)
		},
	)

does that work for you?

[guggero]

Very nice, LGTM 🎉