Merge pull request #9549 from yyforyongyu/fix-bitcond-test

Fix unit test flake `TestHistoricalConfDetailsTxIndex`

Makefile

@@ -276,18 +276,18 @@ unit-bench: $(BTCD_BIN)
 # FLAKE HUNTING
 # =============
 
-#? flakehunter: Run the integration tests continuously until one fails
-flakehunter: build-itest
+#? flakehunter-itest: Run the integration tests continuously until one fails
+flakehunter-itest: build-itest
 	@$(call print, "Flake hunting ${backend} integration tests.")
 	while [ $$? -eq 0 ]; do make itest-only icase='${icase}' backend='${backend}'; done
 
-#? flake-unit: Run the unit tests continuously until one fails
-flake-unit:
-	@$(call print, "Flake hunting unit tests.")
-	while [ $$? -eq 0 ]; do GOTRACEBACK=all $(UNIT) -count=1; done
+#? flakehunter-unit: Run the unit tests continuously until one fails
+flakehunter-unit:
+	@$(call print, "Flake hunting unit test.")
+	scripts/unit-test-flake-hunter.sh ${pkg} ${case}
 
-#? flakehunter-parallel: Run the integration tests continuously until one fails, running up to ITEST_PARALLELISM test tranches in parallel (default 4)
-flakehunter-parallel:
+#? flakehunter-itest-parallel: Run the integration tests continuously until one fails, running up to ITEST_PARALLELISM test tranches in parallel (default 4)
+flakehunter-itest-parallel:
 	@$(call print, "Flake hunting ${backend} integration tests in parallel.")
 	while [ $$? -eq 0 ]; do make itest-parallel tranches=1 parallel=${ITEST_PARALLELISM} icase='${icase}' backend='${backend}'; done
 

chainntnfs/bitcoindnotify/bitcoind_test.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/btcsuite/btcd/chaincfg/chainhash"
 	"github.com/btcsuite/btcd/integration/rpctest"
+	"github.com/btcsuite/btcd/rpcclient"
 	"github.com/btcsuite/btcwallet/chain"
 	"github.com/lightningnetwork/lnd/blockcache"
 	"github.com/lightningnetwork/lnd/chainntnfs"
@@ -103,6 +104,54 @@ func syncNotifierWithMiner(t *testing.T, notifier *BitcoindNotifier,
 				"err=%v, minerHeight=%v, bitcoindHeight=%v",
 				err, minerHeight, bitcoindHeight)
 		}
+
+		// Get the num of connections the miner has. We expect it to
+		// have at least one connection with the chain backend.
+		count, err := miner.Client.GetConnectionCount()
+		require.NoError(t, err)
+		if count != 0 {
+			continue
+		}
+
+		// Reconnect the miner and the chain backend.
+		//
+		// NOTE: The connection should have been made before we perform
+		// the `syncNotifierWithMiner`. However, due to an unknown
+		// reason, the miner may refuse to process the inbound
+		// connection made by the bitcoind node, causing the connection
+		// to fail. It's possible there's a bug in the handshake between
+		// the two nodes.
+		//
+		// A normal flow is, bitcoind starts a v2 handshake flow, which
+		// btcd will fail and disconnect. Upon seeing this
+		// disconnection, bitcoind will try a v1 handshake and succeeds.
+		// The failed flow is, upon seeing the v2 handshake, btcd
+		// doesn't seem to perform the disconnect. Instead an EOF
+		// websocket error is found.
+		//
+		// TODO(yy): Fix the above bug in `btcd`. This can be reproduced
+		// using `make flakehunter-unit pkg=$pkg case=$case`, with,
+		// `case=TestHistoricalConfDetailsNoTxIndex/rpc_polling_enabled`
+		// `pkg=chainntnfs/bitcoindnotify`.
+		// Also need to modify the temp dir logic so we can save the
+		// debug logs.
+		// This bug is likely to be fixed when we implement the
+		// encrypted p2p conn, or when we properly fix the shutdown
+		// issues in all our RPC conns.
+		t.Log("Expected to the chain backend to have one conn with " +
+			"the miner, instead it's disconnected!")
+
+		// We now ask the miner to add the chain backend back.
+		host := fmt.Sprintf(
+			"127.0.0.1:%s", notifier.chainParams.DefaultPort,
+		)
+
+		// NOTE:AddNode must take a host that has the format
+		// `host:port`, otherwise the default port will be used. Check
+		// `normalizeAddress` in btcd for details.
+		err = miner.Client.AddNode(host, rpcclient.ANAdd)
+		require.NoError(t, err, "Failed to connect miner to the chain "+
+			"backend")
 	}
 }
 
@@ -130,7 +179,7 @@ func testHistoricalConfDetailsTxIndex(t *testing.T, rpcPolling bool) {
 	)
 
 	bitcoindConn := unittest.NewBitcoindBackend(
-		t, unittest.NetParams, miner.P2PAddress(), true, rpcPolling,
+		t, unittest.NetParams, miner, true, rpcPolling,
 	)
 
 	hintCache := initHintCache(t)
@@ -140,8 +189,6 @@ func testHistoricalConfDetailsTxIndex(t *testing.T, rpcPolling bool) {
 		t, bitcoindConn, hintCache, hintCache, blockCache,
 	)
 
-	syncNotifierWithMiner(t, notifier, miner)
-
 	// A transaction unknown to the node should not be found within the
 	// txindex even if it is enabled, so we should not proceed with any
 	// fallback methods.
@@ -230,13 +277,15 @@ func testHistoricalConfDetailsNoTxIndex(t *testing.T, rpcpolling bool) {
 	miner := unittest.NewMiner(t, unittest.NetParams, nil, true, 25)
 
 	bitcoindConn := unittest.NewBitcoindBackend(
-		t, unittest.NetParams, miner.P2PAddress(), false, rpcpolling,
+		t, unittest.NetParams, miner, false, rpcpolling,
 	)
 
 	hintCache := initHintCache(t)
 	blockCache := blockcache.NewBlockCache(10000)
 
-	notifier := setUpNotifier(t, bitcoindConn, hintCache, hintCache, blockCache)
+	notifier := setUpNotifier(
+		t, bitcoindConn, hintCache, hintCache, blockCache,
+	)
 
 	// Since the node has its txindex disabled, we fall back to scanning the
 	// chain manually. A transaction unknown to the network should not be
@@ -245,7 +294,11 @@ func testHistoricalConfDetailsNoTxIndex(t *testing.T, rpcpolling bool) {
 	copy(unknownHash[:], bytes.Repeat([]byte{0x10}, 32))
 	unknownConfReq, err := chainntnfs.NewConfRequest(&unknownHash, testScript)
 	require.NoError(t, err, "unable to create conf request")
-	broadcastHeight := syncNotifierWithMiner(t, notifier, miner)
+
+	// Get the current best height.
+	_, broadcastHeight, err := miner.Client.GetBestBlock()
+	require.NoError(t, err, "unable to retrieve miner's current height")
+
 	_, txStatus, err := notifier.historicalConfDetails(
 		unknownConfReq, uint32(broadcastHeight), uint32(broadcastHeight),
 	)

chainntnfs/test/test_interface.go

@@ -1932,7 +1932,7 @@ func TestInterfaces(t *testing.T, targetBackEnd string) {
 		case "bitcoind":
 			var bitcoindConn *chain.BitcoindConn
 			bitcoindConn = unittest.NewBitcoindBackend(
-				t, unittest.NetParams, p2pAddr, true, false,
+				t, unittest.NetParams, miner, true, false,
 			)
 			newNotifier = func() (chainntnfs.TestChainNotifier, error) {
 				return bitcoindnotify.New(
@@ -1944,7 +1944,7 @@ func TestInterfaces(t *testing.T, targetBackEnd string) {
 		case "bitcoind-rpc-polling":
 			var bitcoindConn *chain.BitcoindConn
 			bitcoindConn = unittest.NewBitcoindBackend(
-				t, unittest.NetParams, p2pAddr, true, true,
+				t, unittest.NetParams, miner, true, true,
 			)
 			newNotifier = func() (chainntnfs.TestChainNotifier, error) {
 				return bitcoindnotify.New(

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

@@ -317,6 +317,9 @@ The underlying functionality between those two options remain the same.
   now documented and
   [fixed](https://github.com/lightningnetwork/lnd/pull/9368).
 
+* [Fixed](https://github.com/lightningnetwork/lnd/pull/9549) a long standing
+  unit test flake found in the `chainntnfs/bitcoindnotify` package.
+
 ## Database
 
 * [Migrate the mission control