From a57d8ed266c52fb4b3b8cb6fba5c981bdca8afd2 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 11:30:58 +0100 Subject: [PATCH 01/10] proof: verify inclusion and exclusion proofs of proof suffix This test is very useful for debugging user-supplied proofs that failed for some reason. We expand the test to also verify the inclusion and exclusion proofs. --- proof/proof_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/proof/proof_test.go b/proof/proof_test.go index 028b4a725..09d3f9e2a 100644 --- a/proof/proof_test.go +++ b/proof/proof_test.go @@ -758,6 +758,8 @@ func TestProofVerification(t *testing.T) { t.Logf("Proof asset JSON: %s", assetJSON) + // If we have a challenge witness, we can verify that without having the + // previous proof. if len(p.ChallengeWitness) > 0 { _, err = p.Verify( context.Background(), nil, MockHeaderVerifier, @@ -766,6 +768,11 @@ func TestProofVerification(t *testing.T) { require.NoError(t, err) } + // Verifying the inclusion and exclusion proofs can also be done without + // the previous proof. + _, err = p.VerifyProofs() + require.NoError(t, err) + // Ensure that verification of a proof of unknown version fails. p.Version = TransitionVersion(212) From c59f68b1b5a958bdf1df161cd26d941a25e06b78 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 11:52:38 +0100 Subject: [PATCH 02/10] tapfreighter+tapsend: fix error msg, don't shadown pkg name --- tapfreighter/wallet.go | 2 +- tapsend/send.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tapfreighter/wallet.go b/tapfreighter/wallet.go index d453c2d96..19035a6b8 100644 --- a/tapfreighter/wallet.go +++ b/tapfreighter/wallet.go @@ -752,7 +752,7 @@ func (f *AssetWallet) fundPacketWithInputs(ctx context.Context, } if err := tapsend.PrepareOutputAssets(ctx, vPkt); err != nil { - return nil, fmt.Errorf("unable to create split commit: %w", err) + return nil, fmt.Errorf("unable to prepare outputs: %w", err) } return &FundedVPacket{ diff --git a/tapsend/send.go b/tapsend/send.go index 87bb67a11..a71b419b0 100644 --- a/tapsend/send.go +++ b/tapsend/send.go @@ -735,8 +735,8 @@ func SignVirtualTransaction(vPkt *tappsbt.VPacket, signer tapscript.Signer, // Construct input set from all input assets. prevAssets := make(commitment.InputSet, len(inputs)) for idx := range vPkt.Inputs { - input := vPkt.Inputs[idx] - prevAssets[input.PrevID] = input.Asset() + vIn := vPkt.Inputs[idx] + prevAssets[vIn.PrevID] = vIn.Asset() } // Create a Taproot Asset virtual transaction representing the asset From cb45a9b0041213904f5f553560faa5d8834d0978 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 12:34:15 +0100 Subject: [PATCH 03/10] tappsbt: add AssetSpecifier to vPacket This helper method returns the asset specifier for the virtual packet. Because a single virtual packet is only supposed to carry inputs and outputs of a single asset ID, the TAP commitment key is valid for the whole vPacket. --- tappsbt/interface.go | 19 +++++++++ tappsbt/interface_test.go | 83 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 tappsbt/interface_test.go diff --git a/tappsbt/interface.go b/tappsbt/interface.go index b94bbe20f..61f958115 100644 --- a/tappsbt/interface.go +++ b/tappsbt/interface.go @@ -321,6 +321,25 @@ func (p *VPacket) AssetID() (asset.ID, error) { return firstID, nil } +// AssetSpecifier returns the asset specifier for the asset being spent by the +// first input of the virtual transaction. It returns an error if the asset ID +// of the virtual transaction is not set or if the asset of the first input is +// not set. +func (p *VPacket) AssetSpecifier() (asset.Specifier, error) { + assetID, err := p.AssetID() + if err != nil { + return asset.Specifier{}, err + } + + if p.Inputs[0].Asset() == nil { + return asset.Specifier{}, fmt.Errorf("no asset set for input 0") + } + + return asset.NewSpecifier( + &assetID, nil, p.Inputs[0].Asset().GroupKey, true, + ) +} + // Anchor is a struct that contains all the information about an anchor output. type Anchor struct { // Value is output value of the anchor output. diff --git a/tappsbt/interface_test.go b/tappsbt/interface_test.go new file mode 100644 index 000000000..e9868f4cf --- /dev/null +++ b/tappsbt/interface_test.go @@ -0,0 +1,83 @@ +package tappsbt + +import ( + "testing" + + "github.com/lightninglabs/taproot-assets/asset" + "github.com/lightninglabs/taproot-assets/internal/test" + "github.com/stretchr/testify/require" +) + +// TestAssetSpecifier tests the AssetSpecifier method of the VPacket struct. +func TestAssetSpecifier(t *testing.T) { + groupPubKey := test.RandPubKey(t) + groupKey := &asset.GroupKey{ + GroupPubKey: *groupPubKey, + } + + tests := []struct { + name string + vPacket *VPacket + expectErr bool + expected asset.Specifier + }{ + { + name: "valid input with group key", + vPacket: &VPacket{ + Inputs: []*VInput{ + { + asset: &asset.Asset{ + GroupKey: groupKey, + }, + PrevID: asset.PrevID{ + ID: asset.ID{1, 2, 3}, + }, + }, + }, + }, + expected: asset.NewSpecifierOptionalGroupPubKey( + asset.ID{1, 2, 3}, groupPubKey, + ), + }, + { + name: "valid input with asset ID only", + vPacket: &VPacket{ + Inputs: []*VInput{ + { + asset: &asset.Asset{}, + PrevID: asset.PrevID{ + ID: asset.ID{1, 2, 3}, + }, + }, + }, + }, + expected: asset.NewSpecifierFromId(asset.ID{1, 2, 3}), + }, + { + name: "no inputs", + vPacket: &VPacket{}, + expectErr: true, + }, + { + name: "no asset set for input", + vPacket: &VPacket{ + Inputs: []*VInput{ + {}, + }, + }, + expectErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(tt *testing.T) { + specifier, err := tc.vPacket.AssetSpecifier() + if tc.expectErr { + require.Error(tt, err) + } else { + require.NoError(tt, err) + require.Equal(tt, tc.expected, specifier) + } + }) + } +} From a23f0844e0f438a7ab8db86f4f6347b638e6366d Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 13:49:19 +0100 Subject: [PATCH 04/10] tapsend: disallow colliding script keys in packets To avoid asset leaves colliding on the same asset ID and script key. --- tapsend/send.go | 71 +++++++++++++++++ tapsend/send_test.go | 183 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+) diff --git a/tapsend/send.go b/tapsend/send.go index a71b419b0..8fd1363a4 100644 --- a/tapsend/send.go +++ b/tapsend/send.go @@ -130,6 +130,14 @@ var ( ErrNoRootLocator = errors.New( "cannot create split commitment without split root output", ) + + // ErrDuplicateScriptKeys is an error that is returned when the outputs + // of a virtual packet contain duplicate script keys. + ErrDuplicateScriptKeys = errors.New( + "send: duplicate script keys in outputs - cannot re-use same " + + "script key in same transaction (e.g. cannot send to " + + "same address more than once)", + ) ) var ( @@ -364,6 +372,60 @@ func ValidateInputs(inputCommitments tappsbt.InputCommitments, return isFullValueSpend, nil } +// ValidateCommitmentKeysUnique makes sure the outputs of a set of virtual +// packets don't lead to collisions in and of the trees (e.g. two asset outputs +// with the same asset ID and script key in the same anchor output) or with +// exclusion proofs (e.g. two asset outputs with the same asset ID and script +// key in different anchor outputs). +func ValidateCommitmentKeysUnique(packets []*tappsbt.VPacket) error { + // We'll keep track of all asset-level commitment keys (per top-level + // commitment key) that we've seen so far. The lower level commitment + // keys must be unique per top-level commitment key, otherwise + // collisions or problems with exclusion proofs can occur. For grouped + // assets, the asset ID is part of the asset-level commitment key, so + // it's okay to re-use the same script key (e.g. a zero amount tombstone + // change output) across different asset IDs. + commitmentKeys := make(map[[32]byte]map[[32]byte]struct{}) + + for _, vPkt := range packets { + specifier, err := vPkt.AssetSpecifier() + if err != nil { + return fmt.Errorf("unable to determine tap commitment "+ + "key: %w", err) + } + + tapCommitmentKey := asset.TapCommitmentKey(specifier) + perAssetMap, ok := commitmentKeys[tapCommitmentKey] + if !ok { + perAssetMap = make(map[[32]byte]struct{}) + commitmentKeys[tapCommitmentKey] = perAssetMap + } + + id, err := specifier.UnwrapIdOrErr() + if err != nil { + return fmt.Errorf("unable to unwrap asset ID: %w", err) + } + + hasGroupKey := specifier.HasGroupPubKey() + for idx := range vPkt.Outputs { + vOut := vPkt.Outputs[idx] + + // Check if the asset-level commitment key has already + // been used in this transaction. + assetCommitmentKey := asset.AssetCommitmentKey( + id, vOut.ScriptKey.PubKey, !hasGroupKey, + ) + if _, ok := perAssetMap[assetCommitmentKey]; ok { + return ErrDuplicateScriptKeys + } + + perAssetMap[assetCommitmentKey] = struct{}{} + } + } + + return nil +} + // PrepareOutputAssets prepares the assets of the given outputs depending on // the amounts set on the transaction. If a split is necessary (non-interactive // or partial amount send) it computes a split commitment with the given inputs @@ -977,6 +1039,15 @@ func CreateOutputCommitments( return nil, err } + // We need to make sure this transaction doesn't lead to collisions in + // any of the trees (e.g. two asset outputs with the same script key in + // the same anchor output) or with exclusion proofs (e.g. two asset + // outputs with the same script key in different anchor outputs). + err := ValidateCommitmentKeysUnique(packets) + if err != nil { + return nil, err + } + // And now we commit each packet to the respective anchor output // commitments. for _, vPkt := range packets { diff --git a/tapsend/send_test.go b/tapsend/send_test.go index 2273e2ad9..c3dcf5919 100644 --- a/tapsend/send_test.go +++ b/tapsend/send_test.go @@ -2952,3 +2952,186 @@ func TestValidateAnchorInputs(t *testing.T) { }) } } + +// TestValidateCommitmentKeysUnique tests that the commitment keys in a set of +// virtual packets are unique per TAP commitment key. +func TestValidateCommitmentKeysUnique(t *testing.T) { + groupPubKey1 := test.RandPubKey(t) + groupPubKey2 := test.RandPubKey(t) + groupKey1 := &asset.GroupKey{ + GroupPubKey: *groupPubKey1, + } + groupKey2 := &asset.GroupKey{ + GroupPubKey: *groupPubKey2, + } + assetID1 := asset.ID{1, 2, 3} + assetID2 := asset.ID{2, 3, 4} + + scriptKey1 := asset.NewScriptKey(test.RandPubKey(t)) + scriptKey2 := asset.NewScriptKey(test.RandPubKey(t)) + scriptKey3 := asset.NewScriptKey(test.RandPubKey(t)) + + makeVPacket := func(assetID asset.ID, groupKey *asset.GroupKey, + outputKeys []asset.ScriptKey) *tappsbt.VPacket { + + vPkt := &tappsbt.VPacket{ + ChainParams: &address.RegressionNetTap, + Inputs: []*tappsbt.VInput{ + { + PrevID: asset.PrevID{ + ID: assetID, + }, + }, + }, + Outputs: make([]*tappsbt.VOutput, len(outputKeys)), + } + var a asset.Asset + if groupKey != nil { + a.GroupKey = groupKey + } + vPkt.SetInputAsset(0, &a) + + for i, outputKey := range outputKeys { + vPkt.Outputs[i] = &tappsbt.VOutput{ + ScriptKey: outputKey, + } + } + + return vPkt + } + + tests := []struct { + name string + vPackets []*tappsbt.VPacket + expectErr bool + }{ + { + name: "no collision, single packet, unique keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, groupKey1, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + }, + }, + { + name: "no collision, multi group packets, same keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, groupKey1, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + makeVPacket( + assetID1, nil, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + }, + }, + { + name: "no collision, multi group packets 2, same keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, groupKey1, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + makeVPacket( + assetID1, groupKey2, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + }, + }, + { + name: "no collision, same group packets, unique keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, groupKey1, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + makeVPacket( + assetID2, groupKey1, + []asset.ScriptKey{ + scriptKey3, + }, + ), + }, + }, + { + name: "no collision, different asset packets, same " + + "keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, groupKey1, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + makeVPacket( + assetID2, groupKey1, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + }, + }, + { + name: "collision, same asset packets, same keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, nil, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + makeVPacket( + assetID1, nil, + []asset.ScriptKey{ + scriptKey1, scriptKey2, + }, + ), + }, + expectErr: true, + }, + { + name: "no collision, multi asset packets, same keys", + vPackets: []*tappsbt.VPacket{ + makeVPacket( + assetID1, nil, + []asset.ScriptKey{ + scriptKey1, + }, + ), + makeVPacket( + assetID2, nil, + []asset.ScriptKey{ + scriptKey1, + }, + ), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := tapsend.ValidateCommitmentKeysUnique(tt.vPackets) + if tt.expectErr { + require.ErrorIs( + t, err, tapsend.ErrDuplicateScriptKeys, + ) + } else { + require.NoError(t, err) + } + }) + } +} From 0628d0b7652b5bd045b40a89dc8fa22918748b7f Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 15:04:30 +0100 Subject: [PATCH 05/10] tapfreighter: log selected inputs correctly --- tapfreighter/coin_select.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/tapfreighter/coin_select.go b/tapfreighter/coin_select.go index 2e5408ba5..7ef111711 100644 --- a/tapfreighter/coin_select.go +++ b/tapfreighter/coin_select.go @@ -67,8 +67,14 @@ func (s *CoinSelect) SelectCoins(ctx context.Context, return nil, ErrMatchingAssetsNotFound } - log.Infof("Identified %v eligible asset inputs for send of %d to %v", - len(eligibleCommitments), constraints.MinAmt, constraints) + anchorInputs := fn.Map( + eligibleCommitments, func(c *AnchoredCommitment) string { + return c.AnchorPoint.String() + }, + ) + log.Infof("Identified %v eligible asset inputs for send of %d to %v: "+ + "%v", len(anchorInputs), constraints.MinAmt, + constraints.String(), anchorInputs) // Only select coins anchored in a compatible commitment. compatibleCommitments := fn.Filter( From 59841f063b6abbf650e4208c271a7a66f567f067 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 15:04:41 +0100 Subject: [PATCH 06/10] tapfreighter: add ReleaseCoins to asset wallet interface --- tapfreighter/wallet.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tapfreighter/wallet.go b/tapfreighter/wallet.go index 19035a6b8..852c4851e 100644 --- a/tapfreighter/wallet.go +++ b/tapfreighter/wallet.go @@ -129,6 +129,10 @@ type Wallet interface { // address.ErrInternalKeyNotFound is returned. FetchInternalKeyLocator(ctx context.Context, rawKey *btcec.PublicKey) (keychain.KeyLocator, error) + + // ReleaseCoins releases/unlocks coins that were previously leased and + // makes them available for coin selection again. + ReleaseCoins(ctx context.Context, utxoOutpoints ...wire.OutPoint) error } // AddrBook is an interface that provides access to the address book. @@ -1462,6 +1466,14 @@ func (f *AssetWallet) FetchInternalKeyLocator(ctx context.Context, return f.cfg.AddrBook.FetchInternalKeyLocator(ctx, rawKey) } +// ReleaseCoins releases/unlocks coins that were previously leased and makes +// them available for coin selection again. +func (f *AssetWallet) ReleaseCoins(ctx context.Context, + utxoOutpoints ...wire.OutPoint) error { + + return f.cfg.CoinSelector.ReleaseCoins(ctx, utxoOutpoints...) +} + // addAnchorPsbtInputs adds anchor information from all inputs to the PSBT // packet. This is called after the PSBT has been funded, but before signing. func addAnchorPsbtInputs(btcPkt *psbt.Packet, vPackets []*tappsbt.VPacket) { From 75e8264c533e98a23c1f2d56dcac437ad585b0a6 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 15:05:04 +0100 Subject: [PATCH 07/10] tapfreighter: release asset UTXOs on error Previously we only released/unlocked the BTC level outputs we leased from the lnd wallet when an error occurred. But if we're still in a state where nothing has been written to disk and the user can try again, we can safely release/unlock the asset level UTXOs as well to avoid them being locked for 10 minutes. --- tapfreighter/chain_porter.go | 51 +++++++++++++++++++++++++++++++++++- 1 file changed, 50 insertions(+), 1 deletion(-) diff --git a/tapfreighter/chain_porter.go b/tapfreighter/chain_porter.go index 3fbb6790d..36d8a78f6 100644 --- a/tapfreighter/chain_porter.go +++ b/tapfreighter/chain_porter.go @@ -1075,9 +1075,14 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { // At this point, we have everything we need to sign our _virtual_ // transaction on the Taproot Asset layer. case SendStateVirtualSign: + ctx, cancel := p.WithCtxQuitNoTimeout() + defer cancel() + vPackets := currentPkg.VirtualPackets err := tapsend.ValidateVPacketVersions(vPackets) if err != nil { + p.unlockInputs(ctx, ¤tPkg) + return nil, err } @@ -1091,6 +1096,8 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { _, err := p.cfg.AssetWallet.SignVirtualPacket(vPkt) if err != nil { + p.unlockInputs(ctx, ¤tPkg) + return nil, fmt.Errorf("unable to sign and "+ "commit virtual packet: %w", err) } @@ -1125,6 +1132,8 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { ctx, tapsend.SendConfTarget, ) if err != nil { + p.unlockInputs(ctx, ¤tPkg) + return nil, fmt.Errorf("unable to estimate "+ "fee: %w", err) } @@ -1148,6 +1157,8 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { currentPkg.InputCommitments, ) if err != nil { + p.unlockInputs(ctx, ¤tPkg) + return nil, fmt.Errorf("unable to create passive "+ "assets: %w", err) } @@ -1156,6 +1167,8 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { len(currentPkg.PassiveAssets)) err = wallet.SignPassiveAssets(currentPkg.PassiveAssets) if err != nil { + p.unlockInputs(ctx, ¤tPkg) + return nil, fmt.Errorf("unable to sign passive "+ "assets: %w", err) } @@ -1168,6 +1181,8 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { }, ) if err != nil { + p.unlockInputs(ctx, ¤tPkg) + return nil, fmt.Errorf("unable to anchor virtual "+ "transactions: %w", err) } @@ -1382,10 +1397,44 @@ func (p *ChainPorter) stateStep(currentPkg sendPackage) (*sendPackage, error) { // unlockInputs unlocks the inputs that were locked for the given package. func (p *ChainPorter) unlockInputs(ctx context.Context, pkg *sendPackage) { - if pkg == nil || pkg.AnchorTx == nil || pkg.AnchorTx.FundedPsbt == nil { + // Impossible state, but catch it anyway. + if pkg == nil { + return + } + + // If we haven't even attempted to broadcast yet, we're still in a state + // where we give feedback to the user synchronously, as we haven't + // created an on-chain transaction that we need to await confirmation. + // We also haven't written the transfer to disk yet, so we can just + // release/unlock the _asset_ level UTXOs so the user can try again. We + // sanity-check that we have known input commitments to unlock, since + // that might not always be the case (for example if another party + // contributes inputs). + if pkg.SendState < SendStateStorePreBroadcast && + len(pkg.InputCommitments) > 0 { + + for prevID := range pkg.InputCommitments { + log.Debugf("Unlocking input %v", prevID.OutPoint) + + err := p.cfg.AssetWallet.ReleaseCoins( + ctx, prevID.OutPoint, + ) + if err != nil { + log.Warnf("Unable to unlock input %v: %v", + prevID.OutPoint, err) + } + } + } + + // If we're in another state, the anchor transaction has been created, + // and we can't simply unlock the asset level inputs. This will likely + // require manual intervention. + if pkg.AnchorTx == nil || pkg.AnchorTx.FundedPsbt == nil { return } + // We need to unlock any _BTC_ level inputs we locked for the anchor + // transaction. for _, op := range pkg.AnchorTx.FundedPsbt.LockedUTXOs { err := p.cfg.Wallet.UnlockInput(ctx, op) if err != nil { From 0198a108f1c9ba6647512f2e1bb7449dd5156a10 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 15:06:15 +0100 Subject: [PATCH 08/10] itest: add test for duplicate address send This test demonstrates that we get the correct error when we attempt to send to the same address twice in the same transaction. This also shows that we can re-try normally if a send fails before it was written to disk. --- itest/send_test.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/itest/send_test.go b/itest/send_test.go index 26ed01f54..5d255d9bb 100644 --- a/itest/send_test.go +++ b/itest/send_test.go @@ -17,6 +17,7 @@ import ( "github.com/lightninglabs/taproot-assets/taprpc/mintrpc" "github.com/lightninglabs/taproot-assets/taprpc/tapdevrpc" unirpc "github.com/lightninglabs/taproot-assets/taprpc/universerpc" + "github.com/lightninglabs/taproot-assets/tapsend" "github.com/lightningnetwork/lnd/lntest/wait" "github.com/stretchr/testify/require" ) @@ -102,6 +103,16 @@ func testBasicSendUnidirectional(t *harnessTest) { }) require.NoError(t.t, err) + // Before we start sending, we test that we aren't allowed to send to + // the same address more than once within the same transfer. + _, err = t.tapd.SendAsset(ctxb, &taprpc.SendAssetRequest{ + TapAddrs: []string{ + bobAddr.Encoded, + bobAddr.Encoded, + }, + }) + require.ErrorContains(t.t, err, tapsend.ErrDuplicateScriptKeys.Error()) + for i := 0; i < numSends; i++ { t.t.Logf("Performing send procedure: %d", i) From f20a8754b00f64ba35859a2dd68241193dc59783 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Fri, 8 Nov 2024 15:56:30 +0100 Subject: [PATCH 09/10] itest: fix fee estimation test assumptions The fee estimation test assumed that UTXOs would be locked after an error and couldn't be used anymore. This is no longer the case and we need to adjust the test. We clean up the test a bit while we're at it. --- itest/assertions.go | 5 ++++- itest/fee_estimation_test.go | 24 +++++++++++++----------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/itest/assertions.go b/itest/assertions.go index 73905cef6..2c0abdd40 100644 --- a/itest/assertions.go +++ b/itest/assertions.go @@ -1108,7 +1108,10 @@ func AssertAssetOutboundTransferWithOutputs(t *testing.T, out := transfer.Outputs[idx] require.Contains(t, outpoints, out.Anchor.Outpoint) require.Contains(t, scripts, string(out.ScriptKey)) - require.Equal(t, expectedAmounts[idx], out.Amount) + require.Equalf( + t, expectedAmounts[idx], out.Amount, + "expected amounts: %v, transfer: %v", + expectedAmounts, toJSON(t, transfer)) } firstIn := transfer.Inputs[0] diff --git a/itest/fee_estimation_test.go b/itest/fee_estimation_test.go index 414c498c1..c05657ce9 100644 --- a/itest/fee_estimation_test.go +++ b/itest/fee_estimation_test.go @@ -66,9 +66,11 @@ func testFeeEstimation(t *harnessTest) { rpcAssets := MintAssetsConfirmBatch( t.t, t.lndHarness.Miner().Client, t.tapd, simpleAssets, ) + normalAsset := rpcAssets[0] + normalAssetId := normalAsset.AssetGenesis.AssetId // Check the final fee rate of the mint TX. - rpcMintOutpoint := rpcAssets[0].ChainAnchor.AnchorOutpoint + rpcMintOutpoint := normalAsset.ChainAnchor.AnchorOutpoint mintOutpoint, err := wire.NewOutPointFromString(rpcMintOutpoint) require.NoError(t.t, err) @@ -81,8 +83,7 @@ func testFeeEstimation(t *harnessTest) { ) // Split the normal asset to create a transfer with two anchor outputs. - normalAssetId := rpcAssets[0].AssetGenesis.AssetId - splitAmount := rpcAssets[0].Amount / 2 + splitAmount := normalAsset.Amount / 2 addr, stream := NewAddrWithEventStream( t.t, t.tapd, &taprpc.NewAddrRequest{ AssetId: normalAssetId, @@ -90,7 +91,7 @@ func testFeeEstimation(t *harnessTest) { }, ) - AssertAddrCreated(t.t, t.tapd, rpcAssets[0], addr) + AssertAddrCreated(t.t, t.tapd, normalAsset, addr) sendResp, sendEvents := sendAssetsToAddr(t, t.tapd, addr) transferIdx := 0 @@ -121,7 +122,7 @@ func testFeeEstimation(t *harnessTest) { }, ) - AssertAddrCreated(t.t, t.tapd, rpcAssets[0], addr2) + AssertAddrCreated(t.t, t.tapd, normalAsset, addr2) sendResp, sendEvents = sendAssetsToAddr(t, t.tapd, addr2) ConfirmAndAssertOutboundTransfer( @@ -157,7 +158,7 @@ func testFeeEstimation(t *harnessTest) { []*UTXORequest{initialUTXOs[3]}, ) - AssertAddrCreated(t.t, t.tapd, rpcAssets[0], addr3) + AssertAddrCreated(t.t, t.tapd, normalAsset, addr3) _, err = t.tapd.SendAsset(ctxt, &taprpc.SendAssetRequest{ TapAddrs: []string{addr3.Encoded}, }) @@ -166,7 +167,7 @@ func testFeeEstimation(t *harnessTest) { ) // The transfer should also be rejected if the manually-specified - // feerate fails the sanity check against the fee estimator's fee floor + // fee rate fails the sanity check against the fee estimator's fee floor // of 253 sat/kw, or 1.012 sat/vB. _, err = t.tapd.SendAsset(ctxt, &taprpc.SendAssetRequest{ TapAddrs: []string{addr3.Encoded}, @@ -174,15 +175,16 @@ func testFeeEstimation(t *harnessTest) { }) require.ErrorContains(t.t, err, "manual fee rate below floor") - // After failure at the high feerate, we should still be able to make a - // transfer at a very low feerate. + // After failure at the high fee rate, we should still be able to make a + // transfer at a very low fee rate. t.lndHarness.SetFeeEstimateWithConf(lowFeeRate, 6) sendResp, sendEvents = sendAssetsToAddr(t, t.tapd, addr3) ConfirmAndAssertOutboundTransfer( t.t, t.lndHarness.Miner().Client, t.tapd, sendResp, - normalAssetId, []uint64{thirdSplitAmount, thirdSplitAmount}, - transferIdx, transferIdx+1, + normalAssetId, []uint64{ + splitAmount - thirdSplitAmount, thirdSplitAmount, + }, transferIdx, transferIdx+1, ) transferIdx += 1 AssertNonInteractiveRecvComplete(t.t, t.tapd, transferIdx) From ae69e0a096b9907104421f00faab44b9fa6c0843 Mon Sep 17 00:00:00 2001 From: Oliver Gugger Date: Thu, 14 Nov 2024 14:43:33 +0100 Subject: [PATCH 10/10] tapfreighter: de-duplicate passive asset creation Because we now check all active and passive packets for uniqueness in their commitment keys, this issue has surfaced. If we're spending multiple different assets from the same on-chain input commitment, we used to create duplicate passive assets. That wasn't a problem until now because within the trees everything would just be overwritten and collapsed back to a single asset. But because have the uniqueness check on the virtual packet slice level, this duplication became apparent. --- tapfreighter/wallet.go | 17 ++++++++++++++--- 1 file changed, 14 insertions(+), 3 deletions(-) diff --git a/tapfreighter/wallet.go b/tapfreighter/wallet.go index 852c4851e..e309a7744 100644 --- a/tapfreighter/wallet.go +++ b/tapfreighter/wallet.go @@ -25,6 +25,7 @@ import ( "github.com/lightninglabs/taproot-assets/tapsend" "github.com/lightningnetwork/lnd/keychain" "github.com/lightningnetwork/lnd/lnwallet/chainfee" + "golang.org/x/exp/maps" ) const ( @@ -1210,7 +1211,7 @@ func (f *AssetWallet) CreatePassiveAssets(ctx context.Context, } // Gather passive assets found in each input Taproot Asset commitment. - var passivePackets []*tappsbt.VPacket + passivePackets := make(map[asset.PrevID]*tappsbt.VPacket) for prevID := range inputCommitments { tapCommitment := inputCommitments[prevID] @@ -1248,6 +1249,16 @@ func (f *AssetWallet) CreatePassiveAssets(ctx context.Context, "proof: %w", err) } + scriptKey := passiveAsset.ScriptKey.PubKey + passivePrevID := asset.PrevID{ + OutPoint: prevID.OutPoint, + ID: passiveAsset.ID(), + ScriptKey: asset.ToSerialized(scriptKey), + } + log.Tracef("Adding passive packet for asset_id=%v, "+ + "script_key=%x", passiveAsset.ID().String(), + scriptKey.SerializeCompressed()) + passivePacket, err := createPassivePacket( f.cfg.ChainParams, passiveAsset, activePackets, anchorOutIdx, *anchorOutDesc, prevID.OutPoint, @@ -1258,11 +1269,11 @@ func (f *AssetWallet) CreatePassiveAssets(ctx context.Context, "passive packet: %w", err) } - passivePackets = append(passivePackets, passivePacket) + passivePackets[passivePrevID] = passivePacket } } - return passivePackets, nil + return maps.Values(passivePackets), nil } // SignPassiveAssets signs the given passive asset packets.