sweep, rpcserver: optimize SendAll cost wise

itest/lnd_onchain_test.go

@@ -345,13 +345,43 @@ func testAnchorReservedValue(ht *lntest.HarnessTest) {
 	// The reserved value is now back in Alice's wallet.
 	aliceBalance := waitForConfirmedBalance()
 
+	// Send more coins to Alice.
+	ht.FundCoins(btcutil.SatoshiPerBitcoin, alice)
+
+	// Make sure wallet balance increased.
+	newBalance := waitForConfirmedBalance()
+	require.Greater(ht, newBalance, aliceBalance,
+		"Alice's balance did not increase after receiving a coin")
+
+	// Send all to external address. We already have a coin of value
+	// reserved to bump anchor outputs, so fee efficient SendAll should keep
+	// that coin untouched instead of recreating new reserved output.
+	sweepReq = &lnrpc.SendCoinsRequest{
+		Addr:       minerAddr.String(),
+		SendAll:    true,
+		TargetConf: 6,
+	}
+	alice.RPC.SendCoins(sweepReq)
+
+	// We'll mine a block which should include the sweep transaction we
+	// generated above.
+	block = ht.MineBlocksAndAssertNumTxes(1, 1)[0]
+
+	// The sweep transaction should have 1 input and 1 output. Reserved
+	// coin is not touched.
+	sweepTx = block.Transactions[1]
+	assertNumTxInAndTxOut(sweepTx, 1, 1)
+
+	// Record wallet balance before closing channels.
+	aliceBalance = waitForConfirmedBalance()
+
 	// Alice closes channel, should now be allowed to send everything to an
 	// external address.
 	for _, chanPoint := range chanPoints {
 		ht.CloseChannel(alice, chanPoint)
 	}
 
-	newBalance := waitForConfirmedBalance()
+	newBalance = waitForConfirmedBalance()
 	require.Greater(ht, newBalance, aliceBalance,
 		"Alice's balance did not increase after channel close")
 

rpcserver.go

@@ -1356,12 +1356,11 @@ func (r *rpcServer) SendCoins(ctx context.Context,
 		// transaction that will sweep ALL outputs from the wallet in a
 		// single transaction. This will be generated in a concurrent
 		// safe manner, so no need to worry about locking. The tx will
-		// pay to the change address created above if we needed to
-		// reserve any value, the rest will go to targetAddr.
+		// pay everything to targetAddr.
 		sweepTxPkg, err := sweep.CraftSweepAllTx(
 			feePerKw, maxFeeRate, uint32(bestHeight), nil,
 			targetAddr, wallet, wallet, wallet.WalletController,
-			r.server.cc.Signer, minConfs,
+			r.server.cc.Signer, minConfs, 0,
 		)
 		if err != nil {
 			return nil, err
@@ -1387,38 +1386,66 @@ func (r *rpcServer) SendCoins(ctx context.Context,
 			sweepTxPkg.CancelSweepAttempt()
 
 			rpcsLog.Debugf("Reserved value %v not satisfied after "+
-				"send_all, trying with change output",
+				"send_all, trying to exclude one input",
 				reservedVal)
 
-			// We'll request a change address from the wallet,
-			// where we'll send this reserved value back to. This
-			// ensures this is an address the wallet knows about,
-			// allowing us to pass the reserved value check.
-			changeAddr, err := r.server.cc.Wallet.NewAddress(
-				lnwallet.TaprootPubkey, true,
-				lnwallet.DefaultAccountName,
+			// Try to exlude an input of value exactly reservedVal.
+			// It is likely to exist as an output of previous
+			// SendAll tx.
+			sweepTxPkg, err = sweep.CraftSweepAllTx(
+				feePerKw, maxFeeRate, uint32(bestHeight), nil,
+				targetAddr, wallet, wallet,
+				wallet.WalletController, r.server.cc.Signer,
+				minConfs, reservedVal,
 			)
-			if err != nil {
+			missingInput := errors.Is(
+				err, sweep.ErrMissingInputToSkip,
+			)
+			if err != nil && !missingInput {
+				// Unexpected error.
 				return nil, err
 			}
 
-			// Send the reserved value to this change address, the
-			// remaining funds will go to the targetAddr.
-			outputs := []sweep.DeliveryAddr{
-				{
-					Addr: changeAddr,
-					Amt:  reservedVal,
-				},
-			}
+			// If there is no such input to skip of needed value to
+			// satisfy reservation requirements, add an output.
+			if missingInput {
+				rpcsLog.Debugf("Can not find an input of value"+
+					" %v for send_all, trying with change "+
+					"output", reservedVal)
+
+				// We'll request a change address from the
+				// wallet, where we'll send this reserved value
+				// back to. This ensures this is an address the
+				// wallet knows about, allowing us to pass the
+				// reserved value check.
+				newAddress := r.server.cc.Wallet.NewAddress
+				changeAddr, err := newAddress(
+					lnwallet.TaprootPubkey, true,
+					lnwallet.DefaultAccountName,
+				)
+				if err != nil {
+					return nil, err
+				}
 
-			sweepTxPkg, err = sweep.CraftSweepAllTx(
-				feePerKw, maxFeeRate, uint32(bestHeight),
-				outputs, targetAddr, wallet, wallet,
-				wallet.WalletController,
-				r.server.cc.Signer, minConfs,
-			)
-			if err != nil {
-				return nil, err
+				// Send the reserved value to this change
+				// address, the remaining funds will go to the
+				// targetAddr.
+				outputs := []sweep.DeliveryAddr{
+					{
+						Addr: changeAddr,
+						Amt:  reservedVal,
+					},
+				}
+
+				sweepTxPkg, err = sweep.CraftSweepAllTx(
+					feePerKw, maxFeeRate,
+					uint32(bestHeight), outputs, targetAddr,
+					wallet, wallet, wallet.WalletController,
+					r.server.cc.Signer, minConfs, 0,
+				)
+				if err != nil {
+					return nil, err
+				}
 			}
 
 			// Sanity check the new tx by re-doing the check.

sweep/walletsweep.go

@@ -24,6 +24,10 @@ var (
 	// ErrFeePreferenceConflict is returned when both a fee rate and a conf
 	// target is set for a fee preference.
 	ErrFeePreferenceConflict = errors.New("fee preference conflict")
+
+	// ErrMissingInputToSkip is returned by CraftSweepAllTx if argument
+	// skipInputAmount is not 0 and there is no such an input of that value.
+	ErrMissingInputToSkip = errors.New("no input of desired value")
 )
 
 // FeePreference defines an interface that allows the caller to specify how the
@@ -212,12 +216,15 @@ type DeliveryAddr struct {
 // leftover amount after these outputs and transaction fee, is sent to a single
 // output, as specified by the change address. The sweep transaction will be
 // crafted with the target fee rate, and will use the utxoSource and
-// outputLeaser as sources for wallet funds.
+// outputLeaser as sources for wallet funds. If skipInputAmount is not 0, one
+// input of value exactly skipInputAmount is skipped, if it is present, or an
+// error wrapping ErrMissingInputToSkip is returned and coins are not locked.
 func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight,
 	blockHeight uint32, deliveryAddrs []DeliveryAddr,
 	changeAddr btcutil.Address, coinSelectLocker CoinSelectionLocker,
 	utxoSource UtxoSource, outputLeaser OutputLeaser,
-	signer input.Signer, minConfs int32) (*WalletSweepPackage, error) {
+	signer input.Signer, minConfs int32, skipInputAmount btcutil.Amount) (
+	*WalletSweepPackage, error) {
 
 	// TODO(roasbeef): turn off ATPL as well when available?
 
@@ -260,6 +267,27 @@ func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight,
 
 		log.Trace("[WithCoinSelectLock] finished fetching UTXOs")
 
+		if skipInputAmount != 0 {
+			// Filter out one input of value skipInputAmount.
+			utxos2 := make([]*lnwallet.Utxo, 0, len(utxos))
+			skipped := false
+			for _, utxo := range utxos {
+				if !skipped && utxo.Value == skipInputAmount {
+					log.Trace("skipped UTXO %v of value %v",
+						utxo.OutPoint, utxo.Value)
+					skipped = true
+					continue
+				}
+				utxos2 = append(utxos2, utxo)
+			}
+			utxos = utxos2
+
+			// Make sure we have found and excluded a coin.
+			if !skipped {
+				return ErrMissingInputToSkip
+			}
+		}
+
 		// We'll now lock each UTXO to ensure that other callers don't
 		// attempt to use these UTXOs in transactions while we're
 		// crafting out sweep all transaction.
@@ -288,7 +316,7 @@ func CraftSweepAllTx(feeRate, maxFeeRate chainfee.SatPerKWeight,
 		unlockOutputs()
 
 		return nil, fmt.Errorf("unable to fetch+lock wallet "+
-			"utxos: %v", err)
+			"utxos: %w", err)
 	}
 
 	// Now that we've locked all the potential outputs to sweep, we'll

sweep/walletsweep_test.go

@@ -339,7 +339,7 @@ func TestCraftSweepAllTxCoinSelectFail(t *testing.T) {
 
 	_, err := CraftSweepAllTx(
 		0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLeaser,
-		nil, 0,
+		nil, 0, 0,
 	)
 
 	// Since we instructed the coin select locker to fail above, we should
@@ -365,7 +365,7 @@ func TestCraftSweepAllTxUnknownWitnessType(t *testing.T) {
 
 	_, err := CraftSweepAllTx(
 		0, 0, 10, nil, nil, coinSelectLocker, utxoSource, utxoLeaser,
-		nil, 0,
+		nil, 0, 0,
 	)
 
 	// Since passed in a p2wsh output, which is unknown, we should fail to
@@ -399,7 +399,7 @@ func TestCraftSweepAllTx(t *testing.T) {
 
 	sweepPkg, err := CraftSweepAllTx(
 		0, 0, 10, nil, deliveryAddr, coinSelectLocker, utxoSource,
-		utxoLeaser, signer, 0,
+		utxoLeaser, signer, 0, 0,
 	)
 	require.NoError(t, err, "unable to make sweep tx")
 
@@ -440,3 +440,62 @@ func TestCraftSweepAllTx(t *testing.T) {
 	sweepPkg.CancelSweepAttempt()
 	assertUtxosReleased(t, utxoLeaser, testUtxos[:2])
 }
+
+// TestCraftSweepAllTxSkipInput tests that we'll skip the input of requested
+// value if it exists and return wrapped ErrMissingInputToSkip, if it doesn't
+// exist. In the later case coins are not locked.
+func TestCraftSweepAllTxSkipInput(t *testing.T) {
+	t.Parallel()
+
+	// First, we'll make a mock signer along with a fee estimator, We'll
+	// use zero fees to we can assert a precise output value.
+	signer := &mock.DummySigner{}
+
+	// For our UTXO source, we'll pass in all the UTXOs that we know of,
+	// other than the final one which is of an unknown witness type.
+	targetUTXOs := testUtxos[:2]
+	utxoSource := newMockUtxoSource(targetUTXOs)
+	coinSelectLocker := &mockCoinSelectionLocker{}
+	utxoLeaser := newMockOutputLeaser()
+
+	// Try to skip a coin of value 1500. There is no such coin.
+	const skipInputAmountMissing = 1500
+	_, err := CraftSweepAllTx(
+		0, 0, 10, nil, deliveryAddr, coinSelectLocker, utxoSource,
+		utxoLeaser, signer, 0, skipInputAmountMissing,
+	)
+	require.ErrorIs(t, err, ErrMissingInputToSkip)
+
+	// No UTXOs should be leased.
+	require.Empty(t, utxoLeaser.leasedOutputs)
+
+	// Now skip a coin of value 1000. There is a coin of that value.
+	const skipInputAmount = 1000
+	sweepPkg, err := CraftSweepAllTx(
+		0, 0, 10, nil, deliveryAddr, coinSelectLocker, utxoSource,
+		utxoLeaser, signer, 0, skipInputAmount,
+	)
+	require.NoError(t, err, "unable to make sweep tx")
+
+	// At this point, the second UTXO that we made above should be locked
+	// and none of them unlocked.
+	assertUtxosLeased(t, utxoLeaser, testUtxos[1:2])
+	assertNoUtxosReleased(t, utxoLeaser, testUtxos[1:2])
+
+	// The transaction is expected to have the second UTXO as its input
+	// and to have a single output.
+	sweepTx := sweepPkg.SweepTx
+	require.Equal(t, 1, len(sweepTx.TxIn))
+
+	// We should have a single output that pays to our sweep script
+	// generated above.
+	require.Equal(t, 1, len(sweepTx.TxOut))
+	output := sweepTx.TxOut[0]
+	require.Equal(t, int64(2000), output.Value)
+	require.Equal(t, sweepScript, output.PkScript)
+
+	// If we cancel the sweep attempt, then we should find that the second
+	// UTXO is now unlocked.
+	sweepPkg.CancelSweepAttempt()
+	assertUtxosReleased(t, utxoLeaser, testUtxos[1:2])
+}