From 567fa9ed107939e89220d9a7f50436b78aeee045 Mon Sep 17 00:00:00 2001
From: yyforyongyu <yy2452@columbia.edu>
Date: Thu, 3 Sep 2020 18:59:08 +0800
Subject: [PATCH] itest: disable node retrying to connect to miner
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Previously the chainbackend connected to the miner using a permanent
connection, which would retry the connection when it’s disconnected.
It would leave multiple connections between the chainbackend and the
miner, causing difficulties in debugging. Currently, there are two
occasions when disconnection happens. One happens when running the open
channel reorg test, the miner is connected/disconnected multiple times
on purpose. The other happens when the chainbackend receives a
MSG_WITNESS_TX type from the miner, which would be considered as an
invalid type and disconnects the miner. With the latter one being fixed
in btcd, the chainbackend will still be disconnected from the miner if
it reaches the ban score by requesting too many notfound messages in a
short time which is why the `--nobanning` flag is added.
---
 lntest/bitcoind.go       |  8 --------
 lntest/btcd.go           | 13 ++++++++-----
 lntest/itest/lnd_test.go |  8 ++++++++
 lntest/neutrino.go       |  2 +-
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/lntest/bitcoind.go b/lntest/bitcoind.go
index 86d59ef4a..566708929 100644
--- a/lntest/bitcoind.go
+++ b/lntest/bitcoind.go
@@ -174,14 +174,6 @@ func NewBackend(miner string, netParams *chaincfg.Params) (
 			err)
 	}
 
-	// We start by adding the miner to the bitcoind addnode list. We do
-	// this instead of connecting using command line flags, because it will
-	// allow us to disconnect the miner using the AddNode command later.
-	if err := client.AddNode(miner, rpcclient.ANAdd); err != nil {
-		cleanUp()
-		return nil, nil, fmt.Errorf("unable to add node: %v", err)
-	}
-
 	bd := BitcoindBackendConfig{
 		rpcHost:      rpcHost,
 		rpcUser:      rpcUser,
diff --git a/lntest/btcd.go b/lntest/btcd.go
index 3b75b7bcc..b3ec7b450 100644
--- a/lntest/btcd.go
+++ b/lntest/btcd.go
@@ -17,11 +17,11 @@ import (
 // logDir is the name of the temporary log directory.
 const logDir = "./.backendlogs"
 
-// perm is used to signal we want to establish a permanent connection using the
+// temp is used to signal we want to establish a temporary connection using the
 // btcd Node API.
 //
 // NOTE: Cannot be const, since the node API expects a reference.
-var perm = "perm"
+var temp = "temp"
 
 // BtcdBackendConfig is an implementation of the BackendConfig interface
 // backed by a btcd node.
@@ -56,12 +56,12 @@ func (b BtcdBackendConfig) GenArgs() []string {
 
 // ConnectMiner is called to establish a connection to the test miner.
 func (b BtcdBackendConfig) ConnectMiner() error {
-	return b.harness.Node.Node(btcjson.NConnect, b.minerAddr, &perm)
+	return b.harness.Node.Node(btcjson.NConnect, b.minerAddr, &temp)
 }
 
 // DisconnectMiner is called to disconnect the miner.
 func (b BtcdBackendConfig) DisconnectMiner() error {
-	return b.harness.Node.Node(btcjson.NRemove, b.minerAddr, &perm)
+	return b.harness.Node.Node(btcjson.NDisconnect, b.minerAddr, &temp)
 }
 
 // Name returns the name of the backend type.
@@ -81,8 +81,11 @@ func NewBackend(miner string, netParams *chaincfg.Params) (
 		"--trickleinterval=100ms",
 		"--debuglevel=debug",
 		"--logdir=" + logDir,
-		"--connect=" + miner,
 		"--nowinservice",
+		// The miner will get banned and disconnected from the node if
+		// its requested data are not found. We add a nobanning flag to
+		// make sure they stay connected if it happens.
+		"--nobanning",
 	}
 	chainBackend, err := rpctest.New(netParams, nil, args)
 	if err != nil {
diff --git a/lntest/itest/lnd_test.go b/lntest/itest/lnd_test.go
index a85894502..0d151b712 100644
--- a/lntest/itest/lnd_test.go
+++ b/lntest/itest/lnd_test.go
@@ -2467,6 +2467,7 @@ func testOpenChannelAfterReorg(net *lntest.NetworkHarness, t *harnessTest) {
 		"--rejectnonstd",
 		"--txindex",
 		"--nowinservice",
+		"--nobanning",
 	}
 	tempMiner, err := rpctest.New(
 		harnessNetParams, &rpcclient.NotificationHandlers{}, args,
@@ -14407,6 +14408,7 @@ func TestLightningNetworkDaemon(t *testing.T) {
 		"--logdir=" + minerLogDir,
 		"--trickleinterval=100ms",
 		"--nowinservice",
+		"--nobanning",
 	}
 	handlers := &rpcclient.NotificationHandlers{
 		OnTxAccepted: func(hash *chainhash.Hash, amt btcutil.Amount) {
@@ -14458,6 +14460,12 @@ func TestLightningNetworkDaemon(t *testing.T) {
 		ht.Fatalf("unable to request transaction notifications: %v", err)
 	}
 
+	// Connect chainbackend to miner.
+	require.NoError(
+		t, chainBackend.ConnectMiner(),
+		"failed to connect to miner",
+	)
+
 	binary := itestLndBinary
 	if runtime.GOOS == "windows" {
 		// Windows (even in a bash like environment like git bash as on
diff --git a/lntest/neutrino.go b/lntest/neutrino.go
index 25f2af247..d8e4596ca 100644
--- a/lntest/neutrino.go
+++ b/lntest/neutrino.go
@@ -29,7 +29,7 @@ func (b NeutrinoBackendConfig) GenArgs() []string {
 
 // ConnectMiner is called to establish a connection to the test miner.
 func (b NeutrinoBackendConfig) ConnectMiner() error {
-	return fmt.Errorf("unimplemented")
+	return nil
 }
 
 // DisconnectMiner is called to disconnect the miner.