bugfix: fix two long-existing edge case bugs

[guggero]

Fixes #1740.

Fixes two bugs that have been in the code base for a long time but only have been triggered recently by the test mentioned here: #1740

1. Use correct input asset: When an asset is a split output and then in the next transfer becomes a passive asset, the input asset used when signing was not the correct one (missing the split commitment), which lead to the invalid transfer asset witness error. The fix is using the correct input asset from the input proof instead of the asset from the trimmed input commitment.
2. Store the split commitment root in the DB when importing asset from proof: This is super old and shows that so far we've only ever kept the split root of a transfer on our local node (as change or tombstone) and have never sent it out to another node as part of an interactive transfer. The fix is storing the split commitment root hash and value when importing an asset from a proof file. This fixes the following error: unable to fund address send: error funding packet: unable to list eligible coins: unable to query commitments: mismatch of managed utxo and constructed tap commitment root.

I've also included the integration test from ZZiigguurraatt@d6ef2ac which reproduced the first bug reliably (thanks a lot, @ZZiigguurraatt, that was super helpful!).

cc @bhandras, not sure if you perhaps ran into either of these bugs in your work?

[coveralls]

Pull Request Test Coverage Report for Build 17062493703

Details

• 70 of 76 (92.11%) changed or added relevant lines in 5 files are covered.
• 8942 unchanged lines in 124 files lost coverage.
• Overall coverage decreased (-0.02%) to 56.645%

---

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
tappsbt/create.go	33	35	94.29%
tapdb/assets_store.go	6	10	60.0%

Files with Coverage Reduction	New Missed Lines	%
authmailbox/client.go	2	69.84%
commitment/proof.go	2	87.29%
fn/retry.go	2	92.5%
tapdb/sqlc/transfers.sql.go	2	83.33%
tapsend/proof.go	2	85.99%
universe/syncer.go	2	82.73%
fn/recv.go	3	65.12%
itest/multisig.go	3	97.91%
universe/interface.go	3	60.63%
commitment/encoding.go	4	68.75%

Totals
Change from base Build 17057360543:	-0.02%
Covered Lines:	60721
Relevant Lines:	107195

---

💛 - Coveralls

[ZZiigguurraatt]

2. Store the split commitment root in the DB when importing asset from proof: This is super old and shows that so far we've only ever kept the split root of a transfer on our local node (as change or tombstone) and have never sent it out to another node as part of an interactive transfer. The fix is storing the split commitment root hash and value when importing an asset from a proof file. This fixes the following error: unable to fund address send: error funding packet: unable to list eligible coins: unable to query commitments: mismatch of managed utxo and constructed tap commitment root.

Is this the cause of step 10 (the second problem) in #1736 ?

[Roasbeef]

An initial pass, still trying to grok the exact gaps here which led to this bug. I also plan to add some additional documentation here for posterity.

[Roasbeef]

Here's an extra unit test that we can add (at the `tapdb` level, covers the issue where import proof didn't write the split commit info):

// createTestProof creates an annotated proof for testing.
func createTestProof(t *testing.T,
	testAsset *asset.Asset) *proof.AnnotatedProof {

	return createTestProofWithAnchor(t, testAsset, 0)
}

// createTestProofWithAnchor creates an annotated proof with specific anchor
// index.
func createTestProofWithAnchor(t *testing.T, testAsset *asset.Asset,
	anchorIndex uint32) *proof.AnnotatedProof {

	var blockHash chainhash.Hash
	_, err := rand.Read(blockHash[:])
	require.NoError(t, err)

	anchorTx := wire.NewMsgTx(2)
	anchorTx.AddTxIn(&wire.TxIn{})

	// Add enough outputs to cover the requested index
	for i := uint32(0); i <= anchorIndex; i++ {
		anchorTx.AddTxOut(&wire.TxOut{
			PkScript: bytes.Repeat([]byte{0x01}, 34),
			Value:    10,
		})
	}

	assetRoot, err := commitment.NewAssetCommitment(testAsset)
	require.NoError(t, err)

	commitVersion := test.RandFlip(nil, fn.Ptr(commitment.TapCommitmentV2))
	taprootAssetRoot, err := commitment.NewTapCommitment(
		commitVersion, assetRoot,
	)
	require.NoError(t, err)

	testProof := randProof(t, testAsset)
	proofBlob, err := testProof.Bytes()
	require.NoError(t, err)

	assetID := testAsset.ID()
	anchorPoint := wire.OutPoint{
		Hash:  anchorTx.TxHash(),
		Index: anchorIndex,
	}

	return &proof.AnnotatedProof{
		Locator: proof.Locator{
			AssetID:   &assetID,
			ScriptKey: *testAsset.ScriptKey.PubKey,
		},
		Blob: proofBlob,
		AssetSnapshot: &proof.AssetSnapshot{
			Asset:             testAsset,
			OutPoint:          anchorPoint,
			AnchorBlockHash:   blockHash,
			AnchorBlockHeight: uint32(test.RandIntn(1000) + 1),
			AnchorTxIndex:     test.RandInt[uint32](),
			AnchorTx:          anchorTx,
			OutputIndex:       anchorIndex,
			InternalKey:       test.RandPubKey(t),
			ScriptRoot:        taprootAssetRoot,
		},
	}
}

// TestUpsertAssetsWithSplitCommitments tests that split commitment data is
// properly stored and retrieved during asset upsert operations.
func TestUpsertAssetsWithSplitCommitments(t *testing.T) {
	t.Parallel()

	testCases := []struct {
		name           string
		hasSplitCommit bool
		splitValue     uint64
	}{
		{
			name:           "asset_with_split_commitment",
			hasSplitCommit: true,
			splitValue:     12345,
		},
		{
			name:           "asset_without_split_commitment",
			hasSplitCommit: false,
		},
	}

	for _, tc := range testCases {
		tc := tc
		t.Run(tc.name, func(t *testing.T) {
			t.Parallel()

			ctx := context.Background()
			dbHandle := NewDbHandle(t)

			testAsset := randAsset(t)

			var expectedHash mssmt.NodeHash
			if tc.hasSplitCommit {
				splitHash := test.RandBytes(32)
				copy(expectedHash[:], splitHash)

				testAsset.SplitCommitmentRoot = mssmt.NewComputedNode( //nolint:lll
					expectedHash, tc.splitValue,
				)
			}

			annotatedProof := createTestProof(
				t, testAsset,
			)

			err := dbHandle.AssetStore.ImportProofs(
				ctx, proof.MockVerifierCtx, false,
				annotatedProof,
			)
			require.NoError(t, err)

			assets, err := dbHandle.AssetStore.FetchAllAssets(ctx, false, false, nil)
			require.NoError(t, err)
			require.Len(t, assets, 1)

			dbAsset := assets[0].Asset

			if tc.hasSplitCommit {
				require.NotNil(
					t, dbAsset.SplitCommitmentRoot,
				)
				gotHash := dbAsset.SplitCommitmentRoot.NodeHash()
				require.Equal(t, expectedHash, gotHash)
				require.Equal(
					t, tc.splitValue,
					dbAsset.SplitCommitmentRoot.NodeSum(),
				)
			} else {
				require.Nil(
					t, dbAsset.SplitCommitmentRoot,
					"Split commitment root should be nil",
				)
			}
		})
	}
}

[guggero]

Is this the cause of step 10 (the second problem) in #1736 ?

Yes, I'm pretty sure that's the bug you ran into. At least the error message is identical (but different cases could potentially lead to that error message, but hoping this is the one fixed here, otherwise please make sure to report again).

[guggero]

Here's an extra unit test that we can add (at the tapdb level, covers the issue where import proof didn't write the split commit info):

Awesome, thanks you! Added.

[ZZiigguurraatt]

I've also included the integration test from ZZiigguurraatt@d6ef2ac which reproduced the first bug reliably (thanks a lot, @ZZiigguurraatt, that was super helpful!).

If this issue has been around for a while, should we re-write the itest to do a non-group key send? I was able to reproduce the issue sending the individual asset tranches using a non-group key V1 address, but I did not try to do the multi asset id send with V1 address. Perhaps that could be done by creating 3 V1 addresses and then paying to them all in the same invocation of SendAsset since we can pass multiple addresses to it at once (

taproot-assets/taprpc/taprootassets.proto

Lines 1524 to 1531 in ca434e4

	// The list of TAP addresses to send assets to. The amount to send to each
	// address is determined by the amount specified in the address itself. For
	// V2 addresses that are allowed to not specify an amount, use the
	// addresses_with_amounts list to specify the amount to send to each
	// address. The tap_addrs and addresses_with_amounts lists are mutually
	// exclusive, meaning that if addresses_with_amounts is set, then tap_addrs
	// must be empty, and vice versa.
	repeated string tap_addrs = 1;

)?

[guggero]

I've also included the integration test from ZZiigguurraatt@d6ef2ac which reproduced the first bug reliably (thanks a lot, @ZZiigguurraatt, that was super helpful!).

If this issue has been around for a while, should we re-write the itest to do a non-group key send? I was able to reproduce the issue sending the individual asset tranches using a non-group key V1 address, but I did not try to do the multi asset id send with V1 address. Perhaps that could be done by creating 3 V1 addresses and then paying to them all in the same invocation of SendAsset since we can pass multiple addresses to it at once (

taproot-assets/taprpc/taprootassets.proto

Lines 1524 to 1531 in ca434e4

	// The list of TAP addresses to send assets to. The amount to send to each
	// address is determined by the amount specified in the address itself. For
	// V2 addresses that are allowed to not specify an amount, use the
	// addresses_with_amounts list to specify the amount to send to each
	// address. The tap_addrs and addresses_with_amounts lists are mutually
	// exclusive, meaning that if addresses_with_amounts is set, then tap_addrs
	// must be empty, and vice versa.
	repeated string tap_addrs = 1;

)?

No, I don't think that will work. Because you need multiple split outputs being in the same on-chain output. And you can't do that with V0/V1 addresses, each address will get its own on-chain output.
We can do it with the vPSBT API and I did add a reproduction itest case for that, in addition to your V2 itest case. So I think this case is now covered at least twice, which is much better than before.

[ZZiigguurraatt]

We can do it with the vPSBT API

So this was the only way that this bug could have been previously exposed? You say this PR fixes two long standing edge cases, but before V2 addresses actually getting them to happen would be very hard for the average user and only a really advanced user could have even found them?

[guggero]

So this was the only way that this bug could have been previously exposed? You say this PR fixes two long standing edge cases, but before V2 addresses actually getting them to happen would be very hard for the average user and only a really advanced user could have even found them?

Correct, so we're lucky nobody ran into these two cases yet.

[Roasbeef]

LGTM 🧬

[GeorgeTsagk]

LGTM, ty for the fixes! 💯