Merge pull request #4589 from yyforyongyu/itest-proper-cleanup

Itest: proper connect/disconnect miner

go.mod

@@ -5,7 +5,7 @@ require (
 	github.com/NebulousLabs/fastrand v0.0.0-20181203155948-6fb6489aac4e // indirect
 	github.com/NebulousLabs/go-upnp v0.0.0-20180202185039-29b680b06c82
 	github.com/Yawning/aez v0.0.0-20180114000226-4dad034d9db2
-	github.com/btcsuite/btcd v0.20.1-beta.0.20200730232343-1db1b6f8217f
+	github.com/btcsuite/btcd v0.20.1-beta.0.20200903105316-61634447e719
 	github.com/btcsuite/btclog v0.0.0-20170628155309-84c8d2346e9f
 	github.com/btcsuite/btcutil v1.0.2
 	github.com/btcsuite/btcutil/psbt v1.0.3-0.20200826194809-5f93e33af2b0

go.sum

@@ -29,6 +29,8 @@ github.com/btcsuite/btcd v0.20.1-beta/go.mod h1:wVuoA8VJLEcwgqHBwHmzLRazpKxTv13P
 github.com/btcsuite/btcd v0.20.1-beta.0.20200513120220-b470eee47728/go.mod h1:wVuoA8VJLEcwgqHBwHmzLRazpKxTv13Px/pDuV7OomQ=
 github.com/btcsuite/btcd v0.20.1-beta.0.20200730232343-1db1b6f8217f h1:m/GhMTvDQLbID616c4TYdHyt0MZ9lH5B/nf9Lu3okCY=
 github.com/btcsuite/btcd v0.20.1-beta.0.20200730232343-1db1b6f8217f/go.mod h1:ZSWyehm27aAuS9bvkATT+Xte3hjHZ+MRgMY/8NJ7K94=
+github.com/btcsuite/btcd v0.20.1-beta.0.20200903105316-61634447e719 h1:EVCN2/T2EhbccMSuV3iM6cVcVVYSzmsx4EP3fWgdFGQ=
+github.com/btcsuite/btcd v0.20.1-beta.0.20200903105316-61634447e719/go.mod h1:ZSWyehm27aAuS9bvkATT+Xte3hjHZ+MRgMY/8NJ7K94=
 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 h1:yJzD/yFppdVCf6ApMkVy8cUxV0XrxdP9rVf6D87/Mng=

lntest/bitcoind.go

@@ -3,6 +3,7 @@
 package lntest
 
 import (
+	"errors"
 	"fmt"
 	"io/ioutil"
 	"math/rand"
@@ -71,7 +72,7 @@ func (b BitcoindBackendConfig) Name() string {
 // NewBackend starts a bitcoind node and returns a BitoindBackendConfig for
 // that node.
 func NewBackend(miner string, netParams *chaincfg.Params) (
-	*BitcoindBackendConfig, func(), error) {
+	*BitcoindBackendConfig, func() error, error) {
 
 	if netParams != &chaincfg.RegressionNetParams {
 		return nil, nil, fmt.Errorf("only regtest supported")
@@ -121,21 +122,32 @@ func NewBackend(miner string, netParams *chaincfg.Params) (
 		return nil, nil, fmt.Errorf("couldn't start bitcoind: %v", err)
 	}
 
-	cleanUp := func() {
+	cleanUp := func() error {
 		bitcoind.Process.Kill()
 		bitcoind.Wait()
 
+		var errStr string
 		// After shutting down the chain backend, we'll make a copy of
 		// the log file before deleting the temporary log dir.
 		err := CopyFile("./output_bitcoind_chainbackend.log", logFile)
 		if err != nil {
-			fmt.Printf("unable to copy file: %v\n", err)
+			errStr += fmt.Sprintf("unable to copy file: %v\n", err)
 		}
 		if err = os.RemoveAll(logDir); err != nil {
-			fmt.Printf("Cannot remove dir %s: %v\n", logDir, err)
+			errStr += fmt.Sprintf(
+				"cannot remove dir %s: %v\n", logDir, err,
+			)
 		}
-
+		if err := os.RemoveAll(tempBitcoindDir); err != nil {
-		os.RemoveAll(tempBitcoindDir)
+			errStr += fmt.Sprintf(
+				"cannot remove dir %s: %v\n",
+				tempBitcoindDir, err,
+			)
+		}
+		if errStr != "" {
+			return errors.New(errStr)
+		}
+		return nil
 	}
 
 	// Allow process to start.
@@ -162,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,

lntest/btcd.go

@@ -4,6 +4,7 @@ package lntest
 
 import (
 	"encoding/hex"
+	"errors"
 	"fmt"
 	"os"
 
@@ -16,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.
@@ -55,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.
@@ -72,7 +73,7 @@ func (b BtcdBackendConfig) Name() string {
 // that node. miner should be set to the P2P address of the miner to connect
 // to.
 func NewBackend(miner string, netParams *chaincfg.Params) (
-	*BtcdBackendConfig, func(), error) {
+	*BtcdBackendConfig, func() error, error) {
 
 	args := []string{
 		"--rejectnonstd",
@@ -80,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 {
@@ -98,19 +102,28 @@ func NewBackend(miner string, netParams *chaincfg.Params) (
 		minerAddr: miner,
 	}
 
-	cleanUp := func() {
+	cleanUp := func() error {
-		chainBackend.TearDown()
+		var errStr string
+		if err := chainBackend.TearDown(); err != nil {
+			errStr += err.Error() + "\n"
+		}
 
 		// After shutting down the chain backend, we'll make a copy of
 		// the log file before deleting the temporary log dir.
 		logFile := logDir + "/" + netParams.Name + "/btcd.log"
 		err := CopyFile("./output_btcd_chainbackend.log", logFile)
 		if err != nil {
-			fmt.Printf("unable to copy file: %v\n", err)
+			errStr += fmt.Sprintf("unable to copy file: %v\n", err)
 		}
 		if err = os.RemoveAll(logDir); err != nil {
-			fmt.Printf("Cannot remove dir %s: %v\n", logDir, err)
+			errStr += fmt.Sprintf(
+				"cannot remove dir %s: %v\n", logDir, err,
+			)
 		}
+		if errStr != "" {
+			return errors.New(errStr)
+		}
+		return nil
 	}
 
 	return bd, cleanUp, nil

lntest/itest/lnd_test.go

@@ -2283,6 +2283,7 @@ func testOpenChannelAfterReorg(net *lntest.NetworkHarness, t *harnessTest) {
 		"--rejectnonstd",
 		"--txindex",
 		"--nowinservice",
+		"--nobanning",
 	}
 	tempMiner, err := rpctest.New(
 		harnessNetParams, &rpcclient.NotificationHandlers{}, args,
@@ -2293,7 +2294,12 @@ func testOpenChannelAfterReorg(net *lntest.NetworkHarness, t *harnessTest) {
 	if err := tempMiner.SetUp(false, 0); err != nil {
 		t.Fatalf("unable to set up mining node: %v", err)
 	}
-	defer tempMiner.TearDown()
+	defer func() {
+		require.NoError(
+			t.t, tempMiner.TearDown(),
+			"failed to tear down temp miner",
+		)
+	}()
 
 	// We start by connecting the new miner to our original miner,
 	// such that it will sync to our original chain.
@@ -13936,6 +13942,7 @@ func TestLightningNetworkDaemon(t *testing.T) {
 		"--logdir=" + minerLogDir,
 		"--trickleinterval=100ms",
 		"--nowinservice",
+		"--nobanning",
 	}
 	handlers := &rpcclient.NotificationHandlers{
 		OnTxAccepted: func(hash *chainhash.Hash, amt btcutil.Amount) {
@@ -13948,7 +13955,9 @@ func TestLightningNetworkDaemon(t *testing.T) {
 		ht.Fatalf("unable to create mining node: %v", err)
 	}
 	defer func() {
-		miner.TearDown()
+		require.NoError(
+			t, miner.TearDown(), "failed to tear down miner",
+		)
 
 		// After shutting down the miner, we'll make a copy of the log
 		// file before deleting the temporary log dir.
@@ -13972,7 +13981,11 @@ func TestLightningNetworkDaemon(t *testing.T) {
 	if err != nil {
 		ht.Fatalf("unable to start backend: %v", err)
 	}
-	defer cleanUp()
+	defer func() {
+		require.NoError(
+			t, cleanUp(), "failed to clean up chain backend",
+		)
+	}()
 
 	if err := miner.SetUp(true, 50); err != nil {
 		ht.Fatalf("unable to set up mining node: %v", err)
@@ -13981,6 +13994,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

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.
@@ -44,12 +44,12 @@ func (b NeutrinoBackendConfig) Name() string {
 
 // NewBackend starts and returns a NeutrinoBackendConfig for the node.
 func NewBackend(miner string, _ *chaincfg.Params) (
-	*NeutrinoBackendConfig, func(), error) {
+	*NeutrinoBackendConfig, func() error, error) {
 
 	bd := &NeutrinoBackendConfig{
 		minerAddr: miner,
 	}
 
-	cleanUp := func() {}
+	cleanUp := func() error { return nil }
 	return bd, cleanUp, nil
 }