Merge pull request #9609 from yyforyongyu/fix-listunspent

Fix inaccurate `listunspent` result

Author: yyforyongyu

docs/release-notes/release-notes-0.19.0.md

@@ -91,6 +91,9 @@
 * [Pass through](https://github.com/lightningnetwork/lnd/pull/9601) the unused
   `MaxPeers` configuration variable for neutrino mode.
 
+* [Fixed](https://github.com/lightningnetwork/lnd/pull/9609) a bug that may
+  cause `listunspent` to give inaccurate wallet UTXOs.
+
 # New Features
 
 * Add support for [archiving channel backup](https://github.com/lightningnetwork/lnd/pull/9232)

go.mod

@@ -11,7 +11,7 @@ require (
 	github.com/btcsuite/btcd/chaincfg/chainhash v1.1.0
 	github.com/btcsuite/btclog v0.0.0-20241003133417-09c4e92e319c
 	github.com/btcsuite/btclog/v2 v2.0.1-0.20250110154127-3ae4bf1cb318
-	github.com/btcsuite/btcwallet v0.16.10
+	github.com/btcsuite/btcwallet v0.16.11
 	github.com/btcsuite/btcwallet/wallet/txauthor v1.3.5
 	github.com/btcsuite/btcwallet/wallet/txrules v1.2.2
 	github.com/btcsuite/btcwallet/walletdb v1.4.4

go.sum

@@ -95,8 +95,8 @@ github.com/btcsuite/btclog v0.0.0-20241003133417-09c4e92e319c/go.mod h1:w7xnGOhw
 github.com/btcsuite/btclog/v2 v2.0.1-0.20250110154127-3ae4bf1cb318 h1:oCjIcinPt7XQ644MP/22JcjYEC84qRc3bRBH0d7Hhd4=
 github.com/btcsuite/btclog/v2 v2.0.1-0.20250110154127-3ae4bf1cb318/go.mod h1:XItGUfVOxotJL8kkuk2Hj3EVow5KCugXl3wWfQ6K0AE=
 github.com/btcsuite/btcutil v0.0.0-20190425235716-9e5f4b9a998d/go.mod h1:+5NJ2+qvTyV9exUAL/rxXi3DcLg2Ts+ymUAY5y4NvMg=
-github.com/btcsuite/btcwallet v0.16.10 h1:SDMS0Gp7oEJVvyZNQ6gDYkpkvp/cSMRcAAy3fvO3vTk=
-github.com/btcsuite/btcwallet v0.16.10/go.mod h1:1HJXYbjJzgumlnxOC2+ViR1U+gnHWoOn7WeK5OfY1eU=
+github.com/btcsuite/btcwallet v0.16.11 h1:c8RgW/HO79if8P+KFQLQE00ITmFnLPKAzIy9FwxE37A=
+github.com/btcsuite/btcwallet v0.16.11/go.mod h1:1HJXYbjJzgumlnxOC2+ViR1U+gnHWoOn7WeK5OfY1eU=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.5 h1:Rr0njWI3r341nhSPesKQ2JF+ugDSzdPoeckS75SeDZk=
 github.com/btcsuite/btcwallet/wallet/txauthor v1.3.5/go.mod h1:+tXJ3Ym0nlQc/iHSwW1qzjmPs3ev+UVWMbGgfV1OZqU=
 github.com/btcsuite/btcwallet/wallet/txrules v1.2.2 h1:YEO+Lx1ZJJAtdRrjuhXjWrYsmAk26wLTlNzxt2q0lhk=

itest/flakes.go

@@ -3,9 +3,7 @@ package itest
 import (
 	"time"
 
-	"github.com/btcsuite/btcd/btcutil"
 	"github.com/lightningnetwork/lnd/lntest"
-	"github.com/lightningnetwork/lnd/lntest/node"
 )
 
 // flakePreimageSettlement documents a flake found when testing the preimage
@@ -34,19 +32,6 @@ func flakePreimageSettlement(ht *lntest.HarnessTest) {
 	time.Sleep(2 * time.Second)
 }
 
-// flakeFundExtraUTXO documents a flake found when testing the sweeping behavior
-// of the given node, which would fail due to no wallet UTXO available, while
-// there should be enough.
-//
-// TODO(yy): remove it once the issue is resolved.
-func flakeFundExtraUTXO(ht *lntest.HarnessTest, node *node.HarnessNode) {
-	// The node should have enough wallet UTXOs here to sweep the HTLC in
-	// the end of this test. However, due to a known issue, the node's
-	// wallet may report there's no UTXO available. For details,
-	// - https://github.com/lightningnetwork/lnd/issues/8786
-	ht.FundCoins(btcutil.SatoshiPerBitcoin, node)
-}
-
 // flakeTxNotifierNeutrino documents a flake found when running force close
 // tests using neutrino backend, which is a race between two notifications - one
 // for the spending notification, the other for the block which contains the
@@ -127,3 +112,28 @@ func flakePaymentStreamReturnEarly() {
 	// commitment.
 	time.Sleep(2 * time.Second)
 }
+
+// flakeRaceInBitcoinClientNotifications documents a bug found that the
+// `ListUnspent` gives inaccurate results. In specific,
+//   - an output is confirmed in block X, which is under the process of being
+//     credited to our wallet.
+//   - `ListUnspent` is called between the above process, returning an
+//     inaccurate result, causing the sweeper to think there's no wallet utxo.
+//   - the sweeping will fail at block X due to not enough inputs.
+//
+// Under the hood, the RPC client created for handling wallet txns and handling
+// block notifications are independent. For the block notification, which is
+// registered via `RegisterBlockEpochNtfn`, is managed by the `chainntnfs`,
+// which is hooked to a bitcoind client created at startup. For the wallet, it
+// uses another bitcoind client to receive online events. Although they share
+// the same bitcoind RPC conn, these two clients are acting independently.
+// With this setup, it means there's no coordination between the two system -
+// `lnwallet` and `chainntnfs` can disagree on the latest onchain state for a
+// short period, causing an inconsistent state which leads to the failed
+// sweeping attempt.
+//
+// TODO(yy): We need to adhere to the SSOT principle, and make the effort to
+// ensure the whole system only uses one bitcoind client.
+func flakeRaceInBitcoinClientNotifications(ht *lntest.HarnessTest) {
+	ht.MineEmptyBlocks(1)
+}

itest/list_exclude_test.go

@@ -69,6 +69,13 @@ var excludedTestsWindows = []string{
 	"query blinded route",
 
 	"data loss protection",
+
+	// The following restart cases will fail in windows due to aggregation
+	// can sometimes miss grouping one or two inputs in the same sweeping
+	// tx, which is likely caused by how the blocks are notified in windows,
+	// more investigation is needed.
+	"channel force close-anchor restart",
+	"channel force close-simple taproot restart",
 }
 
 // filterWindowsFlakyTests filters out the flaky tests that are excluded from

itest/lnd_multi-hop_force_close_test.go

@@ -341,8 +341,6 @@ func runLocalClaimOutgoingHTLC(ht *lntest.HarnessTest,
 		ht.FundCoins(btcutil.SatoshiPerBitcoin, bob)
 	}
 
-	flakeFundExtraUTXO(ht, bob)
-
 	// Now that our channels are set up, we'll send two HTLC's from Alice
 	// to Carol. The first HTLC will be universally considered "dust",
 	// while the second will be a proper fully valued HTLC.
@@ -685,7 +683,6 @@ func runMultiHopReceiverPreimageClaim(ht *lntest.HarnessTest,
 
 	// Fund Carol one UTXO so she can sweep outputs.
 	ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
-	flakeFundExtraUTXO(ht, carol)
 
 	// If this is a taproot channel, then we'll need to make some manual
 	// route hints so Alice can actually find a route.
@@ -1601,8 +1598,6 @@ func runLocalClaimIncomingHTLC(ht *lntest.HarnessTest,
 
 	// Fund Carol one UTXO so she can sweep outputs.
 	ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
-	flakeFundExtraUTXO(ht, carol)
-	flakeFundExtraUTXO(ht, bob)
 
 	// If this is a taproot channel, then we'll need to make some manual
 	// route hints so Alice can actually find a route.
@@ -1897,7 +1892,6 @@ func runLocalClaimIncomingHTLCLeased(ht *lntest.HarnessTest,
 
 	// Fund Carol one UTXO so she can sweep outputs.
 	ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
-	flakeFundExtraUTXO(ht, carol)
 
 	// With the network active, we'll now add a new hodl invoice at Carol's
 	// end. Make sure the cltv expiry delta is large enough, otherwise Bob
@@ -2230,7 +2224,6 @@ func runLocalPreimageClaim(ht *lntest.HarnessTest,
 
 	// Fund Carol one UTXO so she can sweep outputs.
 	ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
-	flakeFundExtraUTXO(ht, carol)
 
 	// If this is a taproot channel, then we'll need to make some manual
 	// route hints so Alice can actually find a route.
@@ -2490,7 +2483,6 @@ func runLocalPreimageClaimLeased(ht *lntest.HarnessTest,
 
 	// Fund Carol one UTXO so she can sweep outputs.
 	ht.FundCoins(btcutil.SatoshiPerBitcoin, carol)
-	flakeFundExtraUTXO(ht, carol)
 
 	// With the network active, we'll now add a new hodl invoice at Carol's
 	// end. Make sure the cltv expiry delta is large enough, otherwise Bob
@@ -2842,7 +2834,6 @@ func runHtlcAggregation(ht *lntest.HarnessTest,
 	// We need one additional UTXO to create the sweeping tx for the
 	// second-level success txes.
 	ht.FundCoins(btcutil.SatoshiPerBitcoin, bob)
-	flakeFundExtraUTXO(ht, bob)
 
 	// If this is a taproot channel, then we'll need to make some manual
 	// route hints so Alice+Carol can actually find a route.

itest/lnd_sweep_test.go

@@ -115,8 +115,6 @@ func testSweepCPFPAnchorOutgoingTimeout(ht *lntest.HarnessTest) {
 		ht.FundCoins(btcutil.SatoshiPerBitcoin, bob)
 	}
 
-	flakeFundExtraUTXO(ht, bob)
-
 	// Subscribe the invoice.
 	streamCarol := carol.RPC.SubscribeSingleInvoice(payHash[:])
 
@@ -342,6 +340,8 @@ func testSweepCPFPAnchorOutgoingTimeout(ht *lntest.HarnessTest) {
 	// needed to clean up the mempool.
 	ht.MineBlocksAndAssertNumTxes(1, 2)
 
+	flakeRaceInBitcoinClientNotifications(ht)
+
 	// The above mined block should confirm Bob's force close tx, and his
 	// contractcourt will offer the HTLC to his sweeper. We are not testing
 	// the HTLC sweeping behaviors so we just perform a simple check and
@@ -435,8 +435,6 @@ func testSweepCPFPAnchorIncomingTimeout(ht *lntest.HarnessTest) {
 		ht.FundCoins(btcutil.SatoshiPerBitcoin, bob)
 	}
 
-	flakeFundExtraUTXO(ht, bob)
-
 	// Subscribe the invoice.
 	streamCarol := carol.RPC.SubscribeSingleInvoice(payHash[:])