diff --git a/docs/release-notes/release-notes-0.18.0.md b/docs/release-notes/release-notes-0.18.0.md index 9559053542..f59ae179b6 100644 --- a/docs/release-notes/release-notes-0.18.0.md +++ b/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 diff --git a/funding/manager.go b/funding/manager.go index e110269a7b..b012de48d9 100644 --- a/funding/manager.go +++ b/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) diff --git a/funding/manager_test.go b/funding/manager_test.go index db61c30006..69e23eeb83 100644 --- a/funding/manager_test.go +++ b/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 { diff --git a/go.mod b/go.mod index b8a658edd7..54827142b7 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index c2f3fdb804..e5e56d2c1a 100644 --- a/go.sum +++ b/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= diff --git a/itest/list_on_test.go b/itest/list_on_test.go index b4f586fa21..4696fc80ec 100644 --- a/itest/list_on_test.go +++ b/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, diff --git a/itest/lnd_channel_funding_utxo_selection_test.go b/itest/lnd_channel_funding_utxo_selection_test.go index 4ba4a557bd..8504a17cb9 100644 --- a/itest/lnd_channel_funding_utxo_selection_test.go +++ b/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) diff --git a/itest/lnd_funding_test.go b/itest/lnd_funding_test.go index 97613429eb..38ca71d92f 100644 --- a/itest/lnd_funding_test.go +++ b/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) +} diff --git a/itest/lnd_psbt_test.go b/itest/lnd_psbt_test.go index a3b5f757b9..a6f15fd357 100644 --- a/itest/lnd_psbt_test.go +++ b/itest/lnd_psbt_test.go @@ -1592,3 +1592,321 @@ func testPsbtChanFundingFailFlow(ht *lntest.HarnessTest) { // funding workflow with an internal error. ht.ReceiveOpenChannelError(chanUpdates, chanfunding.ErrRemoteCanceled) } + +// testPsbtChanFundingWithUnstableUtxos tests that channel openings with +// unstable utxos, in this case in particular unconfirmed utxos still in use by +// the sweeper subsystem, are not considered when opening a channel. They bear +// the risk of being RBFed and are therefore not safe to open a channel with. +func testPsbtChanFundingWithUnstableUtxos(ht *lntest.HarnessTest) { + fundingAmt := btcutil.Amount(2_000_000) + + // First, we'll create two new nodes that we'll use to open channel + // between for this test. + carol := ht.NewNode("carol", nil) + dave := ht.NewNode("dave", nil) + ht.EnsureConnected(carol, dave) + + // Fund Carol's wallet with a confirmed utxo. + ht.FundCoins(fundingAmt, carol) + + ht.AssertNumUTXOs(carol, 1) + + // 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) + + chanSize := btcutil.Amount(walletUtxo.AmountSat / 2) + + // We use STATIC_REMOTE_KEY channels to easily generate sweeps without + // anchor sweeps interfering. + cType := lnrpc.CommitmentType_STATIC_REMOTE_KEY + + // We open a normal channel so that we can force-close it and produce + // a sweeper originating utxo. + update := ht.OpenChannelAssertPending(carol, dave, + lntest.OpenChannelParams{ + Amt: chanSize, + SpendUnconfirmed: true, + }) + channelPoint := lntest.ChanPointFromPendingUpdate(update) + ht.MineBlocksAndAssertNumTxes(1, 2) + + // Now force close the channel by dave to generate a utxo which is + // swept by the sweeper. We have STATIC_REMOTE_KEY Channel Types. + ht.CloseChannelAssertPending(dave, channelPoint, true) + ht.MineBlocksAndAssertNumTxes(1, 1) + + // Mine one block to trigger the sweep transaction. + ht.MineEmptyBlocks(1) + + // We wait for the to_remote sweep tx. + ht.AssertNumUTXOsUnconfirmed(carol, 1) + + // We need the maximum funding amount to ensure we are opening the next + // channel with all available utxos. + carolBalance := carol.RPC.WalletBalance() + + // The max chan size needs to account for the fee opening the channel + // itself. + // NOTE: We need to always account for a change here, because their is + // an inaccurarcy in the backend code. + chanSize = btcutil.Amount(carolBalance.TotalBalance) - + fundingFee(2, true) + + // Now open a channel of this amount via a psbt workflow. + // At this point, we can begin our PSBT channel funding workflow. We'll + // start by generating a pending channel ID externally that will be used + // to track this new funding type. + pendingChanID := ht.Random32Bytes() + + // Now that we have the pending channel ID, Carol will open the channel + // by specifying a PSBT shim. We expect it to fail because we try to + // fund a channel with the maximum amount of our wallet, which also + // includes an unstable utxo originating from the sweeper. + chanUpdates, tempPsbt := ht.OpenChannelPsbt( + carol, dave, lntest.OpenChannelParams{ + Amt: chanSize, + FundingShim: &lnrpc.FundingShim{ + Shim: &lnrpc.FundingShim_PsbtShim{ + PsbtShim: &lnrpc.PsbtShim{ + PendingChanId: pendingChanID, + }, + }, + }, + CommitmentType: cType, + SpendUnconfirmed: true, + }, + ) + + fundReq := &walletrpc.FundPsbtRequest{ + Template: &walletrpc.FundPsbtRequest_Psbt{ + Psbt: tempPsbt, + }, + Fees: &walletrpc.FundPsbtRequest_SatPerVbyte{ + SatPerVbyte: 50, + }, + MinConfs: 0, + SpendUnconfirmed: true, + } + carol.RPC.FundPsbtAssertErr(fundReq) + + // We confirm the sweep transaction and make sure we see it as confirmed + // from the perspective of the underlying wallet. + ht.MineBlocksAndAssertNumTxes(1, 1) + + // We expect 2 confirmed utxos, the change of the prior successful + // channel opening and the confirmed to_remote output. + ht.AssertNumUTXOsConfirmed(carol, 2) + + // We fund the psbt request again and now all utxo are stable and can + // finally be used to fund the channel. + fundResp := carol.RPC.FundPsbt(fundReq) + + // We verify the psbt before finalizing it. + carol.RPC.FundingStateStep(&lnrpc.FundingTransitionMsg{ + Trigger: &lnrpc.FundingTransitionMsg_PsbtVerify{ + PsbtVerify: &lnrpc.FundingPsbtVerify{ + PendingChanId: pendingChanID, + FundedPsbt: fundResp.FundedPsbt, + }, + }, + }) + + // Now we'll ask Carol's wallet to sign the PSBT so we can finish the + // funding flow. + finalizeReq := &walletrpc.FinalizePsbtRequest{ + FundedPsbt: fundResp.FundedPsbt, + } + finalizeRes := carol.RPC.FinalizePsbt(finalizeReq) + + // We've signed our PSBT now, let's pass it to the intent again. + carol.RPC.FundingStateStep(&lnrpc.FundingTransitionMsg{ + Trigger: &lnrpc.FundingTransitionMsg_PsbtFinalize{ + PsbtFinalize: &lnrpc.FundingPsbtFinalize{ + PendingChanId: pendingChanID, + SignedPsbt: finalizeRes.SignedPsbt, + }, + }, + }) + + // Consume the "channel pending" update. This waits until the funding + // transaction was fully compiled. + updateResp := ht.ReceiveOpenChannelUpdate(chanUpdates) + upd, ok := updateResp.Update.(*lnrpc.OpenStatusUpdate_ChanPending) + require.True(ht, ok) + channelPoint2 := &lnrpc.ChannelPoint{ + FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{ + FundingTxidBytes: upd.ChanPending.Txid, + }, + OutputIndex: upd.ChanPending.OutputIndex, + } + + var finalTx wire.MsgTx + err := finalTx.Deserialize(bytes.NewReader(finalizeRes.RawFinalTx)) + require.NoError(ht, err) + + txHash := finalTx.TxHash() + block := ht.MineBlocksAndAssertNumTxes(1, 1)[0] + ht.Miner.AssertTxInBlock(block, &txHash) + + // Now we do the same but instead use preselected utxos to verify that + // these utxos respects the utxo restrictions on sweeper unconfirmed + // inputs as well. + + // Now force close the channel by dave to generate a utxo which is + // swept by the sweeper. We have STATIC_REMOTE_KEY Channel Types. + ht.CloseChannelAssertPending(dave, channelPoint2, true) + ht.MineBlocksAndAssertNumTxes(1, 1) + + // Mine one block to trigger the sweep transaction. + ht.MineEmptyBlocks(1) + + // We wait for the to_remote sweep tx of channelPoint2. + utxos := ht.AssertNumUTXOsUnconfirmed(carol, 1) + + // We need the maximum funding amount to ensure we are opening the next + // channel with all available utxos. + carolBalance = carol.RPC.WalletBalance() + + // The max chan size needs to account for the fee opening the channel + // itself. + // NOTE: We need to always account for a change here, because their is + // an inaccurarcy in the backend code calculating the fee of a 1 input + // one output transaction, it always account for a channge in that case + // as well. + chanSize = btcutil.Amount(carolBalance.TotalBalance) - + fundingFee(2, true) + + // Now open a channel of this amount via a psbt workflow. + // At this point, we can begin our PSBT channel funding workflow. We'll + // start by generating a pending channel ID externally that will be used + // to track this new funding type. + pendingChanID = ht.Random32Bytes() + + // Now that we have the pending channel ID, Carol will open the channel + // by specifying a PSBT shim. We expect it to fail because we try to + // fund a channel with the maximum amount of our wallet, which also + // includes an unstable utxo originating from the sweeper. + chanUpdates, tempPsbt = ht.OpenChannelPsbt( + carol, dave, lntest.OpenChannelParams{ + Amt: chanSize, + FundingShim: &lnrpc.FundingShim{ + Shim: &lnrpc.FundingShim_PsbtShim{ + PsbtShim: &lnrpc.PsbtShim{ + PendingChanId: pendingChanID, + }, + }, + }, + CommitmentType: cType, + SpendUnconfirmed: true, + }, + ) + // Add selected utxos to the funding intent. + decodedPsbt, err := psbt.NewFromRawBytes( + bytes.NewReader(tempPsbt), false, + ) + require.NoError(ht, err) + + for _, input := range utxos { + txHash, err := chainhash.NewHashFromStr(input.Outpoint.TxidStr) + require.NoError(ht, err) + decodedPsbt.UnsignedTx.TxIn = append( + decodedPsbt.UnsignedTx.TxIn, &wire.TxIn{ + PreviousOutPoint: wire.OutPoint{ + Hash: *txHash, + Index: input.Outpoint.OutputIndex, + }, + }) + + // The inputs we are using to fund the transaction are known to + // the internal wallet that's why we just append an empty input + // element so that the parsing of the psbt package succeeds. + decodedPsbt.Inputs = append(decodedPsbt.Inputs, psbt.PInput{}) + } + + var psbtBytes bytes.Buffer + err = decodedPsbt.Serialize(&psbtBytes) + require.NoError(ht, err) + + fundReq = &walletrpc.FundPsbtRequest{ + Template: &walletrpc.FundPsbtRequest_Psbt{ + Psbt: psbtBytes.Bytes(), + }, + Fees: &walletrpc.FundPsbtRequest_SatPerVbyte{ + SatPerVbyte: 50, + }, + MinConfs: 0, + SpendUnconfirmed: true, + } + carol.RPC.FundPsbtAssertErr(fundReq) + + ht.MineBlocksAndAssertNumTxes(1, 1) + + // We expect 2 confirmed utxos, the change of the last successful + // channel opening and the confirmed to_remote output of channelPoint2. + ht.AssertNumUTXOsConfirmed(carol, 2) + + // After the confirmation of the sweep to_remote output the funding + // will now proceed. + fundResp = carol.RPC.FundPsbt(fundReq) + + // We verify the funded psbt. + carol.RPC.FundingStateStep(&lnrpc.FundingTransitionMsg{ + Trigger: &lnrpc.FundingTransitionMsg_PsbtVerify{ + PsbtVerify: &lnrpc.FundingPsbtVerify{ + PendingChanId: pendingChanID, + FundedPsbt: fundResp.FundedPsbt, + }, + }, + }) + + // Now we'll ask Carol's wallet to sign the PSBT so we can finish the + // funding flow. + finalizeReq = &walletrpc.FinalizePsbtRequest{ + FundedPsbt: fundResp.FundedPsbt, + } + finalizeRes = carol.RPC.FinalizePsbt(finalizeReq) + + // We've signed our PSBT now, let's pass it to the intent again. + carol.RPC.FundingStateStep(&lnrpc.FundingTransitionMsg{ + Trigger: &lnrpc.FundingTransitionMsg_PsbtFinalize{ + PsbtFinalize: &lnrpc.FundingPsbtFinalize{ + PendingChanId: pendingChanID, + SignedPsbt: finalizeRes.SignedPsbt, + }, + }, + }) + + // Consume the "channel pending" update. This waits until the funding + // transaction was fully compiled. + updateResp = ht.ReceiveOpenChannelUpdate(chanUpdates) + upd, ok = updateResp.Update.(*lnrpc.OpenStatusUpdate_ChanPending) + require.True(ht, ok) + channelPoint3 := &lnrpc.ChannelPoint{ + FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{ + FundingTxidBytes: upd.ChanPending.Txid, + }, + OutputIndex: upd.ChanPending.OutputIndex, + } + + err = finalTx.Deserialize(bytes.NewReader(finalizeRes.RawFinalTx)) + require.NoError(ht, err) + + txHash = finalTx.TxHash() + block = ht.MineBlocksAndAssertNumTxes(1, 1)[0] + ht.Miner.AssertTxInBlock(block, &txHash) + + ht.CloseChannel(carol, channelPoint3) +} diff --git a/lnrpc/walletrpc/walletkit_server.go b/lnrpc/walletrpc/walletkit_server.go index b8fcbc776d..d3f5131b08 100644 --- a/lnrpc/walletrpc/walletkit_server.go +++ b/lnrpc/walletrpc/walletkit_server.go @@ -1551,21 +1551,82 @@ func (w *WalletKit) fundPsbtInternalWallet(account string, return err } + // filterFn makes sure utxos which are unconfirmed and + // still used by the sweeper are not used. + filterFn := func(u *lnwallet.Utxo) bool { + // Confirmed utxos are always allowed. + if u.Confirmations > 0 { + return true + } + + // Unconfirmed utxos in use by the sweeper are + // not stable to use because they can be + // replaced. + if w.cfg.Sweeper.IsSweeperOutpoint(u.OutPoint) { + log.Warnf("Cannot use unconfirmed "+ + "utxo=%v because it is "+ + "unstable and could be "+ + "replaced", u.OutPoint) + + return false + } + + return true + } + + eligibleUtxos := fn.Filter(filterFn, utxos) + // Validate all inputs against our known list of UTXOs // now. - err = verifyInputsUnspent(packet.UnsignedTx.TxIn, utxos) + err = verifyInputsUnspent( + packet.UnsignedTx.TxIn, eligibleUtxos, + ) if err != nil { return err } } + // currentHeight is needed to determine whether the internal + // wallet utxo is still unconfirmed. + _, currentHeight, err := w.cfg.Chain.GetBestBlock() + if err != nil { + return fmt.Errorf("unable to retrieve current "+ + "height: %v", err) + } + + // restrictUnstableUtxos is a filter function which disallows + // the usage of unconfirmed outputs published (still in use) by + // the sweeper. + restrictUnstableUtxos := func(utxo wtxmgr.Credit) bool { + // Wallet utxos which are unmined have a height + // of -1. + if utxo.Height != -1 && utxo.Height <= currentHeight { + // Confirmed utxos are always allowed. + return true + } + + // Utxos used by the sweeper are not used for + // channel openings. + allowed := !w.cfg.Sweeper.IsSweeperOutpoint( + utxo.OutPoint, + ) + if !allowed { + log.Warnf("Cannot use unconfirmed "+ + "utxo=%v because it is "+ + "unstable and could be "+ + "replaced", utxo.OutPoint) + } + + return allowed + } + // We made sure the input from the user is as sane as possible. // We can now ask the wallet to fund the TX. This will not yet // lock any coins but might still change the wallet DB by // generating a new change address. changeIndex, err := w.cfg.Wallet.FundPsbt( - packet, minConfs, feeSatPerKW, account, - keyScope, strategy, + packet, minConfs, feeSatPerKW, account, keyScope, + strategy, restrictUnstableUtxos, ) if err != nil { return fmt.Errorf("wallet couldn't fund PSBT: %w", err) diff --git a/lntest/harness.go b/lntest/harness.go index c291e7cf39..5c2bef4889 100644 --- a/lntest/harness.go +++ b/lntest/harness.go @@ -1205,6 +1205,7 @@ func (h *HarnessTest) OpenChannelAssertErr(srcNode, destNode *node.HarnessNode, // Receive an error to be sent from the stream. _, err := h.receiveOpenChannelUpdate(respStream) + require.NotNil(h, err, "expected channel opening to fail") // Use string comparison here as we haven't codified all the RPC errors // yet. diff --git a/lntest/mock/walletcontroller.go b/lntest/mock/walletcontroller.go index 21d78add37..52aecb3820 100644 --- a/lntest/mock/walletcontroller.go +++ b/lntest/mock/walletcontroller.go @@ -208,7 +208,8 @@ func (w *WalletController) ListLeasedOutputs() ([]*base.ListLeasedOutputResult, // FundPsbt currently does nothing. func (w *WalletController) FundPsbt(*psbt.Packet, int32, chainfee.SatPerKWeight, - string, *waddrmgr.KeyScope, base.CoinSelectionStrategy) (int32, error) { + string, *waddrmgr.KeyScope, base.CoinSelectionStrategy, + func(utxo wtxmgr.Credit) bool) (int32, error) { return 0, nil } diff --git a/lntest/rpc/wallet_kit.go b/lntest/rpc/wallet_kit.go index 79ccdcbdf4..967679d664 100644 --- a/lntest/rpc/wallet_kit.go +++ b/lntest/rpc/wallet_kit.go @@ -68,6 +68,16 @@ func (h *HarnessRPC) FundPsbt( return resp } +// FundPsbtAssertErr makes a RPC call to the node's FundPsbt and asserts an +// error is returned. +func (h *HarnessRPC) FundPsbtAssertErr(req *walletrpc.FundPsbtRequest) { + ctxt, cancel := context.WithTimeout(h.runCtx, DefaultTimeout) + defer cancel() + + _, err := h.WalletKit.FundPsbt(ctxt, req) + require.Error(h, err, "expected error returned") +} + // FinalizePsbt makes a RPC call to node's FinalizePsbt and asserts. func (h *HarnessRPC) FinalizePsbt( req *walletrpc.FinalizePsbtRequest) *walletrpc.FinalizePsbtResponse { diff --git a/lnwallet/btcwallet/psbt.go b/lnwallet/btcwallet/psbt.go index 0fdf76c382..ec88cd92ff 100644 --- a/lnwallet/btcwallet/psbt.go +++ b/lnwallet/btcwallet/psbt.go @@ -15,6 +15,7 @@ import ( "github.com/btcsuite/btcd/wire" "github.com/btcsuite/btcwallet/waddrmgr" "github.com/btcsuite/btcwallet/wallet" + "github.com/btcsuite/btcwallet/wtxmgr" "github.com/lightningnetwork/lnd/input" "github.com/lightningnetwork/lnd/keychain" "github.com/lightningnetwork/lnd/lnwallet" @@ -60,6 +61,9 @@ var ( // imported public keys. For custom account, no key scope should be provided // as the coin selection key scope will always be used to generate the change // address. +// The function argument `allowUtxo` specifies a filter function for utxos +// during coin selection. It should return true for utxos that can be used and +// false for those that should be excluded. // // NOTE: If the packet doesn't contain any inputs, coin selection is performed // automatically. The account parameter must be non-empty as it determines which @@ -74,7 +78,8 @@ var ( func (b *BtcWallet) FundPsbt(packet *psbt.Packet, minConfs int32, feeRate chainfee.SatPerKWeight, accountName string, changeScope *waddrmgr.KeyScope, - strategy wallet.CoinSelectionStrategy) (int32, error) { + strategy wallet.CoinSelectionStrategy, + allowUtxo func(wtxmgr.Credit) bool) (int32, error) { // The fee rate is passed in using units of sat/kw, so we'll convert // this to sat/KB as the CreateSimpleTx method requires this unit. @@ -130,6 +135,9 @@ func (b *BtcWallet) FundPsbt(packet *psbt.Packet, minConfs int32, if changeScope != nil { opts = append(opts, wallet.WithCustomChangeScope(changeScope)) } + if allowUtxo != nil { + opts = append(opts, wallet.WithUtxoFilter(allowUtxo)) + } // Let the wallet handle coin selection and/or fee estimation based on // the partial TX information in the packet. diff --git a/lnwallet/chanfunding/wallet_assembler.go b/lnwallet/chanfunding/wallet_assembler.go index d4ee080bcc..4f2aa759b2 100644 --- a/lnwallet/chanfunding/wallet_assembler.go +++ b/lnwallet/chanfunding/wallet_assembler.go @@ -334,7 +334,8 @@ func (w *WalletAssembler) ProvisionChannel(r *Request) (Intent, error) { } for _, coin := range manuallySelectedCoins { if _, ok := unspent[coin.OutPoint]; !ok { - return fmt.Errorf("outpoint already spent: %v", + return fmt.Errorf("outpoint already spent or "+ + "locked by another subsystem: %v", coin.OutPoint) } } diff --git a/lnwallet/interface.go b/lnwallet/interface.go index 59e6f5aab0..a48e925607 100644 --- a/lnwallet/interface.go +++ b/lnwallet/interface.go @@ -468,7 +468,8 @@ type WalletController interface { FundPsbt(packet *psbt.Packet, minConfs int32, feeRate chainfee.SatPerKWeight, account string, changeScope *waddrmgr.KeyScope, - strategy base.CoinSelectionStrategy) (int32, error) + strategy base.CoinSelectionStrategy, + allowUtxo func(wtxmgr.Credit) bool) (int32, error) // SignPsbt expects a partial transaction with all inputs and outputs // fully declared and tries to sign all unsigned inputs that have all diff --git a/lnwallet/mock.go b/lnwallet/mock.go index 0146df57ea..1873de79a8 100644 --- a/lnwallet/mock.go +++ b/lnwallet/mock.go @@ -217,7 +217,8 @@ func (w *mockWalletController) ListLeasedOutputs() ( // FundPsbt currently does nothing. func (w *mockWalletController) FundPsbt(*psbt.Packet, int32, chainfee.SatPerKWeight, string, *waddrmgr.KeyScope, - base.CoinSelectionStrategy) (int32, error) { + base.CoinSelectionStrategy, func(utxo wtxmgr.Credit) bool) (int32, + error) { return 0, nil } diff --git a/lnwallet/test/test_interface.go b/lnwallet/test/test_interface.go index 401c46683a..dd1db5f20c 100644 --- a/lnwallet/test/test_interface.go +++ b/lnwallet/test/test_interface.go @@ -2947,6 +2947,11 @@ func waitForWalletSync(r *rpctest.Harness, w *lnwallet.LightningWallet) error { func testSingleFunderExternalFundingTx(miner *rpctest.Harness, alice, bob *lnwallet.LightningWallet, t *testing.T) { + // Define a filter function without any restrictions. + allowUtxo := func(lnwallet.Utxo) bool { + return true + } + // First, we'll obtain multi-sig keys from both Alice and Bob which // simulates them exchanging keys on a higher level. aliceFundingKey, err := alice.DeriveNextKey(keychain.KeyFamilyMultiSig) @@ -2963,7 +2968,9 @@ func testSingleFunderExternalFundingTx(miner *rpctest.Harness, // we'll create a new chanfunding.Assembler hacked by Alice's wallet. aliceChanFunder := chanfunding.NewWalletAssembler( chanfunding.WalletConfig{ - CoinSource: lnwallet.NewCoinSource(alice), + CoinSource: lnwallet.NewCoinSource( + alice, allowUtxo, + ), CoinSelectLocker: alice, CoinLeaser: alice, Signer: alice.Cfg.Signer, diff --git a/lnwallet/wallet.go b/lnwallet/wallet.go index 7786791f93..b99b6eed2f 100644 --- a/lnwallet/wallet.go +++ b/lnwallet/wallet.go @@ -184,6 +184,15 @@ type InitFundingReserveMsg struct { // used. ChanFunder chanfunding.Assembler + // AllowUtxoForFunding enables the channel funding workflow to restrict + // the selection of utxos when selecting the inputs for the channel + // opening. This does ONLY apply for the internal wallet backed channel + // opening case. + // + // NOTE: This is very useful when opening channels with unconfirmed + // inputs to make sure stable non-replaceable inputs are used. + AllowUtxoForFunding func(Utxo) bool + // ZeroConf is a boolean that is true if a zero-conf channel was // negotiated. ZeroConf bool @@ -849,7 +858,9 @@ func (l *LightningWallet) handleFundingReserveRequest(req *InitFundingReserveMsg // P2WPKH dust limit and to avoid threading through two // different dust limits. cfg := chanfunding.WalletConfig{ - CoinSource: &CoinSource{l}, + CoinSource: NewCoinSource( + l, req.AllowUtxoForFunding, + ), CoinSelectLocker: l, CoinLeaser: l, Signer: l.Cfg.Signer, @@ -2525,12 +2536,16 @@ func (l *LightningWallet) CancelRebroadcast(txid chainhash.Hash) { // CoinSource is a wrapper around the wallet that implements the // chanfunding.CoinSource interface. type CoinSource struct { - wallet *LightningWallet + wallet *LightningWallet + allowUtxo func(Utxo) bool } // NewCoinSource creates a new instance of the CoinSource wrapper struct. -func NewCoinSource(w *LightningWallet) *CoinSource { - return &CoinSource{wallet: w} +func NewCoinSource(w *LightningWallet, allowUtxo func(Utxo) bool) *CoinSource { + return &CoinSource{ + wallet: w, + allowUtxo: allowUtxo, + } } // ListCoins returns all UTXOs from the source that have between @@ -2546,7 +2561,18 @@ func (c *CoinSource) ListCoins(minConfs int32, } var coins []wallet.Coin + for _, utxo := range utxos { + // If there is a filter function supplied all utxos not adhering + // to these conditions will be discared. + if c.allowUtxo != nil && !c.allowUtxo(*utxo) { + walletLog.Infof("Cannot use unconfirmed "+ + "utxo=%v because it is unstable and could be "+ + "replaced", utxo.OutPoint) + + continue + } + coins = append(coins, wallet.Coin{ TxOut: wire.TxOut{ Value: int64(utxo.Value), diff --git a/rpcserver.go b/rpcserver.go index 7f2ca16da9..810ac26304 100644 --- a/rpcserver.go +++ b/rpcserver.go @@ -1890,7 +1890,7 @@ func newFundingShimAssembler(chanPointShim *lnrpc.ChanPointShim, initiator bool, // newFundingShimAssembler returns a new fully populated // chanfunding.PsbtAssembler using a FundingShim obtained from an RPC caller. -func newPsbtAssembler(req *lnrpc.OpenChannelRequest, normalizedMinConfs int32, +func newPsbtAssembler(req *lnrpc.OpenChannelRequest, psbtShim *lnrpc.PsbtShim, netParams *chaincfg.Params) ( chanfunding.Assembler, error) { @@ -1904,11 +1904,6 @@ func newPsbtAssembler(req *lnrpc.OpenChannelRequest, normalizedMinConfs int32, if len(psbtShim.PendingChanId) != 32 { return nil, fmt.Errorf("pending chan ID not set") } - if normalizedMinConfs != 1 { - return nil, fmt.Errorf("setting non-default values for " + - "minimum confirmation is not supported for PSBT " + - "funding") - } if req.SatPerByte != 0 || req.SatPerVbyte != 0 || req.TargetConf != 0 { // nolint:staticcheck return nil, fmt.Errorf("specifying fee estimation parameters " + "is not supported for PSBT funding") @@ -2351,8 +2346,12 @@ func (r *rpcServer) OpenChannel(in *lnrpc.OpenChannelRequest, // chanfunding.PsbtAssembler to construct the funding // transaction. copy(req.PendingChanID[:], psbtShim.PendingChanId) + + // NOTE: For the PSBT case we do also allow unconfirmed + // utxos to fund the psbt transaction because we make + // sure we only use stable utxos. req.ChanFunder, err = newPsbtAssembler( - in, req.MinConfs, psbtShim, + in, psbtShim, &r.server.cc.Wallet.Cfg.NetParams, ) if err != nil { diff --git a/server.go b/server.go index e2c0b831a2..2979880a29 100644 --- a/server.go +++ b/server.go @@ -1489,8 +1489,9 @@ func newServer(cfg *Config, listenAddrs []net.Addr, EnableUpfrontShutdown: cfg.EnableUpfrontShutdown, MaxAnchorsCommitFeeRate: chainfee.SatPerKVByte( s.cfg.MaxCommitFeeRateAnchors * 1000).FeePerKWeight(), - DeleteAliasEdge: deleteAliasEdge, - AliasManager: s.aliasMgr, + DeleteAliasEdge: deleteAliasEdge, + AliasManager: s.aliasMgr, + IsSweeperOutpoint: s.sweeper.IsSweeperOutpoint, }) if err != nil { return nil, err diff --git a/sweep/sweeper.go b/sweep/sweeper.go index 2754485ea1..e8e7d77a4f 100644 --- a/sweep/sweeper.go +++ b/sweep/sweeper.go @@ -1735,3 +1735,26 @@ func (s *UtxoSweeper) handleBumpEvent(r *BumpResult) error { return nil } + +// IsSweeperOutpoint determines whether the outpoint was created by the sweeper. +// +// NOTE: It is enough to check the txid because the sweeper will create +// outpoints which solely belong to the internal LND wallet. +func (s *UtxoSweeper) IsSweeperOutpoint(op wire.OutPoint) bool { + found, err := s.cfg.Store.IsOurTx(op.Hash) + // In case there is an error fetching the transaction details from the + // sweeper store we assume the outpoint is still used by the sweeper + // (worst case scenario). + // + // TODO(ziggie): Ensure that confirmed outpoints are deleted from the + // bucket. + if err != nil && !errors.Is(err, errNoTxHashesBucket) { + log.Errorf("failed to fetch info for outpoint(%v:%d) "+ + "with: %v, we assume it is still in use by the sweeper", + op.Hash, op.Index, err) + + return true + } + + return found +}