Merge pull request #8545 from ziggie1984/dont-use-sweeper-unconfirmed…

…-utxos

dont use sweeper unconfirmed utxos

docs/release-notes/release-notes-0.18.0.md

@@ -118,6 +118,14 @@
 
 * [Fixed a bug in `btcd` that caused an incompatibility with
   `bitcoind v27.0`](https://github.com/lightningnetwork/lnd/pull/8573).
+
+* [Fixed](https://github.com/lightningnetwork/lnd/pull/8609) a function call 
+  where arguments were swapped.
+
+* [Fixed](https://github.com/lightningnetwork/lnd/pull/8545) utxo selection
+  for the internal channel funding flow (Single and Batch Funding Flow). Now
+  utxos which are unconfirmed and originated from the sweeper subsystem are not
+  selected because they bear the risk of being replaced (BIP 125 RBF).
 
 # New Features
 ## Functional Enhancements

funding/manager.go

@@ -537,6 +537,12 @@ type Config struct {
 	// AliasManager is an implementation of the aliasHandler interface that
 	// abstracts away the handling of many alias functions.
 	AliasManager aliasHandler
+
+	// IsSweeperOutpoint queries the sweeper store for successfully
+	// published sweeps. This is useful to decide for the internal wallet
+	// backed funding flow to not use utxos still being swept by the sweeper
+	// subsystem.
+	IsSweeperOutpoint func(wire.OutPoint) bool
 }
 
 // Manager acts as an orchestrator/bridge between the wallet's
@@ -4600,10 +4606,26 @@ func (f *Manager) handleInitFundingMsg(msg *InitFundingMsg) {
 		MinConfs:          msg.MinConfs,
 		CommitType:        commitType,
 		ChanFunder:        msg.ChanFunder,
-		ZeroConf:          zeroConf,
-		OptionScidAlias:   scid,
-		ScidAliasFeature:  scidFeatureVal,
-		Memo:              msg.Memo,
+		// Unconfirmed Utxos which are marked by the sweeper subsystem
+		// are excluded from the coin selection because they are not
+		// final and can be RBFed by the sweeper subsystem.
+		AllowUtxoForFunding: func(u lnwallet.Utxo) bool {
+			// Utxos with at least 1 confirmation are safe to use
+			// for channel openings because they don't bare the risk
+			// of being replaced (BIP 125 RBF).
+			if u.Confirmations > 0 {
+				return true
+			}
+
+			// Query the sweeper storage to make sure we don't use
+			// an unconfirmed utxo still in use by the sweeper
+			// subsystem.
+			return !f.cfg.IsSweeperOutpoint(u.OutPoint)
+		},
+		ZeroConf:         zeroConf,
+		OptionScidAlias:  scid,
+		ScidAliasFeature: scidFeatureVal,
+		Memo:             msg.Memo,
 	}
 
 	reservation, err := f.cfg.Wallet.InitChannelReservation(req)

funding/manager_test.go

@@ -556,6 +556,11 @@ func createTestFundingManager(t *testing.T, privKey *btcec.PrivateKey,
 			return nil, nil
 		},
 		AliasManager: aliasMgr,
+		// For unit tests we default to false meaning that no funds
+		// originated from the sweeper.
+		IsSweeperOutpoint: func(wire.OutPoint) bool {
+			return false
+		},
 	}
 
 	for _, op := range options {

go.mod

@@ -10,7 +10,7 @@ require (
 	github.com/btcsuite/btcd/btcutil/psbt v1.1.8
 	github.com/btcsuite/btcd/chaincfg/chainhash v1.1.0
 	github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f
-	github.com/btcsuite/btcwallet v0.16.10-0.20240410030101-6fe19a472a62
+	github.com/btcsuite/btcwallet v0.16.10-0.20240404104514-b2f31f9045fb
 	github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4
 	github.com/btcsuite/btcwallet/wallet/txrules v1.2.1
 	github.com/btcsuite/btcwallet/walletdb v1.4.2

go.sum

@@ -92,8 +92,8 @@ github.com/btcsuite/btcd/chaincfg/chainhash v1.1.0/go.mod h1:7SFka0XMvUgj3hfZtyd
 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f h1:bAs4lUbRJpnnkd9VhRV3jjAVU7DJVjMaK+IsvSeZvFo=
 github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f/go.mod h1:TdznJufoqS23FtqVCzL0ZqgP5MqXbb4fg/WgDys70nA=
 github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg=
-github.com/btcsuite/btcwallet v0.16.10-0.20240410030101-6fe19a472a62 h1:MtcTVTcDbGdTJhfDc7LLikojyl0PYtSRNLwoRaLVbWI=
-github.com/btcsuite/btcwallet v0.16.10-0.20240410030101-6fe19a472a62/go.mod h1:2C3Q/MhYAKmk7F+Tey6LfKtKRTdQsrCf8AAAzzDPmH4=
+github.com/btcsuite/btcwallet v0.16.10-0.20240404104514-b2f31f9045fb h1:qoIOlBPRZWtfpcbQlNFf67Wz8ZlXo+mxQc9Pnbm/iqU=
+github.com/btcsuite/btcwallet v0.16.10-0.20240404104514-b2f31f9045fb/go.mod h1:2C3Q/MhYAKmk7F+Tey6LfKtKRTdQsrCf8AAAzzDPmH4=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4 h1:poyHFf7+5+RdxNp5r2T6IBRD7RyraUsYARYbp/7t4D8=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.4/go.mod h1:GETGDQuyq+VFfH1S/+/7slLM/9aNa4l7P4ejX6dJfb0=
 github.com/btcsuite/btcwallet/wallet/txrules v1.2.1 h1:UZo7YRzdHbwhK7Rhv3PO9bXgTxiOH45edK5qdsdiatk=

itest/list_on_test.go

@@ -117,6 +117,14 @@ var allTestCases = []*lntest.TestCase{
 		Name:     "batch channel funding",
 		TestFunc: testBatchChanFunding,
 	},
+	{
+		Name:     "open channel with unstable utxos",
+		TestFunc: testChannelFundingWithUnstableUtxos,
+	},
+	{
+		Name:     "open psbt channel with unstable utxos",
+		TestFunc: testPsbtChanFundingWithUnstableUtxos,
+	},
 	{
 		Name:     "update channel policy",
 		TestFunc: testUpdateChannelPolicy,

itest/lnd_channel_funding_utxo_selection_test.go

@@ -330,7 +330,8 @@ func runUtxoSelectionTestCase(ht *lntest.HarnessTest, alice,
 	// When re-selecting a spent output for funding another channel we
 	// expect the respective error message.
 	if tc.reuseUtxo {
-		expectedErrStr := fmt.Sprintf("outpoint already spent: %s:%d",
+		expectedErrStr := fmt.Sprintf("outpoint already spent or "+
+			"locked by another subsystem: %s:%d",
 			selectedOutpoints[0].TxidStr,
 			selectedOutpoints[0].OutputIndex)
 		expectedErr := fmt.Errorf(expectedErrStr)

itest/lnd_funding_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/btcsuite/btcd/txscript"
 	"github.com/btcsuite/btcd/wire"
 	"github.com/lightningnetwork/lnd/chainreg"
+	"github.com/lightningnetwork/lnd/fn"
 	"github.com/lightningnetwork/lnd/funding"
 	"github.com/lightningnetwork/lnd/input"
 	"github.com/lightningnetwork/lnd/labels"
@@ -1250,3 +1251,211 @@ func deriveFundingShim(ht *lntest.HarnessTest, carol, dave *node.HarnessNode,
 
 	return fundingShim, chanPoint
 }
+
+// testChannelFundingWithUnstableUtxos tests channel openings with restricted
+// utxo selection. Internal wallet utxos might be restricted due to another
+// subsystems still using it therefore it would be unsecure to use them for
+// channel openings. This test focuses on unconfirmed utxos which are still
+// being used by the sweeper subsystem hence should only be used when confirmed.
+func testChannelFundingWithUnstableUtxos(ht *lntest.HarnessTest) {
+	// Select funding amt below wumbo size because we later use fundMax to
+	// open a channel with the total balance.
+	fundingAmt := btcutil.Amount(3_000_000)
+
+	// We use STATIC_REMOTE_KEY channels because anchor sweeps would
+	// interfere and create additional utxos.
+	// Although its the current default we explicitly signal it.
+	cType := lnrpc.CommitmentType_STATIC_REMOTE_KEY
+
+	// First, we'll create two new nodes that we'll use to open channel
+	// between for this test.
+	carol := ht.NewNode("carol", nil)
+	// We'll attempt at max 2 pending channels, so Dave will need to accept
+	// two pending ones.
+	dave := ht.NewNode("dave", []string{
+		"--maxpendingchannels=2",
+	})
+	ht.EnsureConnected(carol, dave)
+
+	// Fund Carol's wallet with a confirmed utxo.
+	ht.FundCoins(fundingAmt, carol)
+
+	// Now spend the coins to create an unconfirmed transaction. This is
+	// necessary to test also the neutrino behaviour. For neutrino nodes
+	// only unconfirmed transactions originating from this node will be
+	// recognized as unconfirmed.
+	req := &lnrpc.NewAddressRequest{Type: AddrTypeTaprootPubkey}
+	resp := carol.RPC.NewAddress(req)
+
+	sendCoinsResp := carol.RPC.SendCoins(&lnrpc.SendCoinsRequest{
+		Addr:        resp.Address,
+		SendAll:     true,
+		SatPerVbyte: 1,
+	})
+
+	walletUtxo := ht.AssertNumUTXOsUnconfirmed(carol, 1)[0]
+	require.EqualValues(ht, sendCoinsResp.Txid, walletUtxo.Outpoint.TxidStr)
+
+	// We will attempt to open 2 channels at a time.
+	chanSize := btcutil.Amount(walletUtxo.AmountSat / 3)
+
+	// Open a channel to dave with an unconfirmed utxo. Although this utxo
+	// is unconfirmed it can be used to open a channel because it did not
+	// originated from the sweeper subsystem.
+	update := ht.OpenChannelAssertPending(carol, dave,
+		lntest.OpenChannelParams{
+			Amt:              chanSize,
+			SpendUnconfirmed: true,
+			CommitmentType:   cType,
+		})
+	chanPoint1 := lntest.ChanPointFromPendingUpdate(update)
+
+	// Verify that both nodes know about the channel.
+	ht.AssertNumPendingOpenChannels(carol, 1)
+	ht.AssertNumPendingOpenChannels(dave, 1)
+
+	// We open another channel on the fly, funds are unconfirmed but because
+	// the tx was not created by the sweeper we can use it and open another
+	// channel. This is a common use case when opening zeroconf channels,
+	// so unconfirmed utxos originated from prior channel opening are safe
+	// to use because channel opening should not be RBFed, at least not for
+	// now.
+	update = ht.OpenChannelAssertPending(carol, dave,
+		lntest.OpenChannelParams{
+			Amt:              chanSize,
+			SpendUnconfirmed: true,
+			CommitmentType:   cType,
+		})
+
+	chanPoint2 := lntest.ChanPointFromPendingUpdate(update)
+
+	ht.AssertNumPendingOpenChannels(carol, 2)
+	ht.AssertNumPendingOpenChannels(dave, 2)
+
+	// We expect the initial funding tx to confirm and also the two
+	// unconfirmed channel openings.
+	ht.MineBlocksAndAssertNumTxes(1, 3)
+
+	// Now we create an unconfirmed utxo which originated from the sweeper
+	// subsystem and hence is not safe to use for channel openings. We do
+	// that by dave force-closing the channel. Which let's carol sweep its
+	// to_remote output which is not encumbered by any relative locktime.
+	ht.CloseChannelAssertPending(dave, chanPoint2, true)
+	// Mine the force close commitment transaction.
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// Mine one block to trigger the sweep transaction.
+	ht.MineEmptyBlocks(1)
+
+	// We need to wait for carol initiating the sweep of the to_remote
+	// output of chanPoint2.
+	utxos := ht.AssertNumUTXOsUnconfirmed(carol, 1)
+
+	// We filter for the unconfirmed utxo and try to open a channel with
+	// that utxo.
+	utxoOpt := fn.Find(func(u *lnrpc.Utxo) bool {
+		return u.Confirmations == 0
+	}, utxos)
+	fundingUtxo := utxoOpt.UnwrapOrFail(ht.T)
+
+	// Now try to open the channel with this utxo and expect an error.
+	expectedErr := fmt.Errorf("outpoint already spent or "+
+		"locked by another subsystem: %s:%d",
+		fundingUtxo.Outpoint.TxidStr,
+		fundingUtxo.Outpoint.OutputIndex)
+
+	ht.OpenChannelAssertErr(carol, dave,
+		lntest.OpenChannelParams{
+			FundMax:          true,
+			SpendUnconfirmed: true,
+			Outpoints: []*lnrpc.OutPoint{
+				fundingUtxo.Outpoint,
+			},
+		}, expectedErr)
+
+	// The channel opening failed because the utxo was unconfirmed and
+	// originated from the sweeper subsystem. Now we confirm the
+	// to_remote sweep and expect the channel opening to work.
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// Try opening the channel with the same utxo (now confirmed) again.
+	update = ht.OpenChannelAssertPending(carol, dave,
+		lntest.OpenChannelParams{
+			FundMax:          true,
+			SpendUnconfirmed: true,
+			Outpoints: []*lnrpc.OutPoint{
+				fundingUtxo.Outpoint,
+			},
+		})
+
+	chanPoint3 := lntest.ChanPointFromPendingUpdate(update)
+	ht.AssertNumPendingOpenChannels(carol, 1)
+	ht.AssertNumPendingOpenChannels(dave, 1)
+
+	// We expect chanPoint3 to confirm.
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// Force Close the channel and test the opening flow without preselected
+	// utxos.
+	// Before we tested the channel funding with a selected coin, now we
+	// want to make sure that our internal coin selection also adheres to
+	// the restictions of unstable utxos.
+	// We create the unconfirmed sweeper originating utxo just like before
+	// by force-closing a channel from dave's side.
+	ht.CloseChannelAssertPending(dave, chanPoint3, true)
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	// Mine one block to trigger the sweep transaction.
+	ht.MineEmptyBlocks(1)
+
+	// Wait for the to_remote sweep tx to show up in carol's wallet.
+	ht.AssertNumUTXOsUnconfirmed(carol, 1)
+
+	// Calculate the maximum amount our wallet has for the channel funding
+	// so that we will use all utxos.
+	carolBalance := carol.RPC.WalletBalance()
+
+	// Now calculate the fee for the channel opening transaction. We don't
+	// have to keep a channel reserve because we are using STATIC_REMOTE_KEY
+	// channels.
+	// NOTE: The TotalBalance includes the unconfirmed balance as well.
+	chanSize = btcutil.Amount(carolBalance.TotalBalance) -
+		fundingFee(2, false)
+
+	// We are trying to open a channel with the maximum amount and expect it
+	// to fail because one of the utxos cannot be used because it is
+	// unstable.
+	expectedErr = fmt.Errorf("not enough witness outputs to create " +
+		"funding transaction")
+
+	ht.OpenChannelAssertErr(carol, dave,
+		lntest.OpenChannelParams{
+			Amt:              chanSize,
+			SpendUnconfirmed: true,
+			CommitmentType:   cType,
+		}, expectedErr)
+
+	// Confirm the to_remote sweep utxo.
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	ht.AssertNumUTXOsConfirmed(carol, 2)
+
+	// Now after the sweep utxo is confirmed it is stable and can be used
+	// for channel openings again.
+	update = ht.OpenChannelAssertPending(carol, dave,
+		lntest.OpenChannelParams{
+			Amt:              chanSize,
+			SpendUnconfirmed: true,
+			CommitmentType:   cType,
+		})
+	chanPoint4 := lntest.ChanPointFromPendingUpdate(update)
+
+	// Verify that both nodes know about the channel.
+	ht.AssertNumPendingOpenChannels(carol, 1)
+	ht.AssertNumPendingOpenChannels(dave, 1)
+
+	ht.MineBlocksAndAssertNumTxes(1, 1)
+
+	ht.CloseChannel(carol, chanPoint1)
+	ht.CloseChannel(carol, chanPoint4)
+}