Merge pull request #9967 from ellemouton/peerBootstrapItest

itest: test automatic peer bootstrapping

docs/release-notes/release-notes-0.20.0.md

@@ -125,6 +125,13 @@ circuit. The indices are only available for forwarding events saved after v0.20.
 
 ## Testing
 
+* Previously, automatic peer bootstrapping was disabled for simnet, signet and
+  regtest networks even if the `--nobootstrap` flag was not set. This automatic
+  disabling has now been 
+  [removed](https://github.com/lightningnetwork/lnd/pull/9967) meaning that any 
+  test network scripts that rely on bootstrapping being disabled will need to 
+  explicitly define the `--nobootstrap` flag.
+
 ## Database
 
 ## Code Health

itest/list_on_test.go

@@ -707,6 +707,10 @@ var allTestCases = []*lntest.TestCase{
 		Name:     "partially specified route blinded invoice",
 		TestFunc: testPartiallySpecifiedBlindedPath,
 	},
+	{
+		Name:     "peer bootstrapping",
+		TestFunc: testPeerBootstrapping,
+	},
 }
 
 // appendPrefixed is used to add a prefix to each test name in the subtests

itest/lnd_graph.go

@@ -0,0 +1,77 @@
+package itest
+
+import (
+	"time"
+
+	"github.com/btcsuite/btcd/btcutil"
+	"github.com/lightningnetwork/lnd/lntest"
+	"github.com/lightningnetwork/lnd/lntest/wait"
+	"github.com/stretchr/testify/require"
+)
+
+// testPeerBootstrapping tests that a node is able to use persisted node
+// announcements to bootstrap its peer connections. This is done by
+// connecting a node to a channel network so that it syncs the necessary gossip
+// and then:
+//  1. Asserting that on a restart where bootstrapping is _disabled_, the node
+//     does not connect to any peers.
+//  2. Asserting that on a restart where bootstrapping is _enabled_, the node
+//     does connect to peers.
+func testPeerBootstrapping(ht *lntest.HarnessTest) {
+	// 1) Set up the following node/channel network.
+	// 	Alice <- Bob <- Charlie
+	_, nodes := ht.CreateSimpleNetwork(
+		[][]string{nil, nil, nil}, lntest.OpenChannelParams{
+			Amt: btcutil.Amount(100000),
+		},
+	)
+	carol, bob, alice := nodes[0], nodes[1], nodes[2]
+
+	// Assert that they all know about each other and the channels.
+	ht.AssertNumEdges(alice, 2, false)
+	ht.AssertNumEdges(bob, 2, false)
+	ht.AssertNumEdges(carol, 2, false)
+
+	// Spin up a new node, Dave.
+	dave := ht.NewNode("Dave", nil)
+
+	// Explicitly assert that Dave was started with bootstrapping disabled.
+	require.False(ht, dave.Cfg.WithPeerBootstrap)
+
+	// Assert that Dave's current view of the graph is empty.
+	ht.AssertNumEdges(dave, 0, false)
+
+	// Now, connect Dave to Alice and wait for gossip sync to complete.
+	ht.EnsureConnected(dave, alice)
+	ht.AssertNumEdges(dave, 2, false)
+
+	// Disconnect Dave from Alice and restart Dave. Since Alice and Dave
+	// did not have channels between them, the nodes should not reconnect
+	// to each other.
+	ht.DisconnectNodes(dave, alice)
+	ht.RestartNode(dave)
+
+	// Dave still has bootstrapping disabled and so it should not connect
+	// to any peers.
+	err := wait.Invariant(func() bool {
+		peerResp := dave.RPC.ListPeers()
+
+		return len(peerResp.Peers) == 0
+	}, time.Second*5)
+	require.NoError(ht, err)
+
+	// Dave should still know about the full graph though since it was
+	// synced previously from Alice.
+	ht.AssertNumEdges(dave, 2, false)
+
+	// Restart Dave again but this time with bootstrapping enabled.
+	dave.Cfg.WithPeerBootstrap = true
+	ht.RestartNode(dave)
+
+	// Show that Dave now does connect to some peers. The default minimum
+	// number of peers that will be connected to is 3, so we
+	// expect Dave to connect to all three nodes in the network.
+	ht.AssertConnected(dave, alice)
+	ht.AssertConnected(dave, bob)
+	ht.AssertConnected(dave, carol)
+}

lntest/node/config.go

@@ -146,8 +146,9 @@ type BaseNodeConfig struct {
 	ReadMacPath    string
 	InvoiceMacPath string
 
-	SkipUnlock bool
-	Password   []byte
+	SkipUnlock        bool
+	Password          []byte
+	WithPeerBootstrap bool
 
 	P2PPort     int
 	RPCPort     int
@@ -285,7 +286,6 @@ func (cfg *BaseNodeConfig) GenArgs() []string {
 	args = append(args, backendArgs...)
 
 	nodeArgs := []string{
-		"--nobootstrap",
 		"--debuglevel=debug",
 		"--bitcoin.defaultchanconfs=1",
 		"--accept-keysend",
@@ -326,6 +326,10 @@ func (cfg *BaseNodeConfig) GenArgs() []string {
 		args = append(args, "--noseedbackup")
 	}
 
+	if !cfg.WithPeerBootstrap {
+		args = append(args, "--nobootstrap")
+	}
+
 	switch cfg.DBBackend {
 	case BackendEtcd:
 		args = append(args, "--db.backend=etcd")

server.go

@@ -2644,7 +2644,7 @@ func (s *server) Start(ctx context.Context) error {
 		// configure the set of active bootstrappers, and launch a
 		// dedicated goroutine to maintain a set of persistent
 		// connections.
-		if shouldPeerBootstrap(s.cfg) {
+		if !s.cfg.NoNetBootstrap {
 			bootstrappers, err := initNetworkBootstrappers(s)
 			if err != nil {
 				startErr = err
@@ -5378,20 +5378,6 @@ func newSweepPkScriptGen(
 	}
 }
 
-// shouldPeerBootstrap returns true if we should attempt to perform peer
-// bootstrapping to actively seek our peers using the set of active network
-// bootstrappers.
-func shouldPeerBootstrap(cfg *Config) bool {
-	isSimnet := cfg.Bitcoin.SimNet
-	isSignet := cfg.Bitcoin.SigNet
-	isRegtest := cfg.Bitcoin.RegTest
-	isDevNetwork := isSimnet || isSignet || isRegtest
-
-	// TODO(yy): remove the check on simnet/regtest such that the itest is
-	// covering the bootstrapping process.
-	return !cfg.NoNetBootstrap && !isDevNetwork
-}
-
 // fetchClosedChannelSCIDs returns a set of SCIDs that have their force closing
 // finished.
 func (s *server) fetchClosedChannelSCIDs() map[lnwire.ShortChannelID]struct{} {

server_test.go

@@ -1,82 +0,0 @@
-package lnd
-
-import (
-	"testing"
-
-	"github.com/lightningnetwork/lnd/lncfg"
-)
-
-// TestShouldPeerBootstrap tests that we properly skip network bootstrap for
-// the developer networks, and also if bootstrapping is explicitly disabled.
-func TestShouldPeerBootstrap(t *testing.T) {
-	t.Parallel()
-
-	testCases := []struct {
-		cfg            *Config
-		shouldBoostrap bool
-	}{
-		// Simnet active, no bootstrap.
-		{
-			cfg: &Config{
-				Bitcoin: &lncfg.Chain{
-					SimNet: true,
-				},
-			},
-		},
-
-		// Regtest active, no bootstrap.
-		{
-			cfg: &Config{
-				Bitcoin: &lncfg.Chain{
-					RegTest: true,
-				},
-			},
-		},
-
-		// Signet active, no bootstrap.
-		{
-			cfg: &Config{
-				Bitcoin: &lncfg.Chain{
-					SigNet: true,
-				},
-			},
-		},
-
-		// Mainnet active, but bootstrap disabled, no bootstrap.
-		{
-			cfg: &Config{
-				Bitcoin: &lncfg.Chain{
-					MainNet: true,
-				},
-				NoNetBootstrap: true,
-			},
-		},
-
-		// Mainnet active, should bootstrap.
-		{
-			cfg: &Config{
-				Bitcoin: &lncfg.Chain{
-					MainNet: true,
-				},
-			},
-			shouldBoostrap: true,
-		},
-
-		// Testnet active, should bootstrap.
-		{
-			cfg: &Config{
-				Bitcoin: &lncfg.Chain{
-					TestNet3: true,
-				},
-			},
-			shouldBoostrap: true,
-		},
-	}
-	for i, testCase := range testCases {
-		bootstrapped := shouldPeerBootstrap(testCase.cfg)
-		if bootstrapped != testCase.shouldBoostrap {
-			t.Fatalf("#%v: expected bootstrap=%v, got bootstrap=%v",
-				i, testCase.shouldBoostrap, bootstrapped)
-		}
-	}
-}