Merge pull request #1181 from lightninglabs/enforce-unique-script-key

[tapsend]: Enforce unique script keys

Author: guggero

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]

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,16 +83,15 @@ 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,
 			Amt:     splitAmount,
 		},
 	)
 
-	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,23 +167,24 @@ 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},
 		FeeRate:  uint32(chainfee.FeePerKwFloor) - 1,
 	})
 	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)

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)
 

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)
 

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, &currentPkg)
+
 			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, &currentPkg)
+
 				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, &currentPkg)
+
 				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, &currentPkg)
+
 			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, &currentPkg)
+
 			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, &currentPkg)
+
 			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 {

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(

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 (
@@ -129,6 +130,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.
@@ -752,7 +757,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{
@@ -1206,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]
 
@@ -1244,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,
@@ -1254,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.
@@ -1462,6 +1477,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) {

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.