lntest: show that multi addresses are not used

This commit adds an itest to demonstrate that if a peer advertises
multiplie external IP addresses, then they will not all be used to
reconnect to the peer during reconnection. This will be fixed in a
follow-up commit.

lntest/harness.go

@@ -346,10 +346,10 @@ func (n *NetworkHarness) NewNodeEtcd(name string, etcdCfg *etcd.Config,
 // current instance of the network harness. The created node is running, but
 // not yet connected to other nodes within the network.
 func (n *NetworkHarness) NewNode(t *testing.T,
-	name string, extraArgs []string) *HarnessNode {
+	name string, extraArgs []string, opts ...NodeOption) *HarnessNode {
 
 	node, err := n.newNode(
-		name, extraArgs, false, nil, n.dbBackend, true,
+		name, extraArgs, false, nil, n.dbBackend, true, opts...,
 	)
 	require.NoErrorf(t, err, "unable to create new node for %s", name)
 

lntest/itest/lnd_network_test.go

@@ -60,6 +60,149 @@ func testNetworkConnectionTimeout(net *lntest.NetworkHarness, t *harnessTest) {
 	assertTimeoutError(ctxt, t, dave, req)
 }
 
+// testReconnectAfterIPChange verifies that if an inbound node changes its
+// IP address to an IP address not listed first in its advertised address list
+// then its peer will not reconnect to it. This test asserts that this bug
+// exists and will be changed to assert that peers are able to reconnect in the
+// commit that fixes the bug.
+func testReconnectAfterIPChange(net *lntest.NetworkHarness, t *harnessTest) {
+	// In this test, the following network will be set up. A single
+	// dash line represents a peer connection and a double dash line
+	// represents a channel.
+	// Charlie will create a connection to Dave so that Dave is the inbound
+	// peer. This will be made a persistent connection for Charlie so that
+	// Charlie will attempt to reconnect to Dave if Dave restarts.
+	// A channel will be opened between Dave and Alice to ensure that any
+	// NodeAnnouncements that Dave sends will reach Alice.
+	// The connection between Alice and Charlie ensures that Charlie
+	// receives all of Dave's NodeAnnouncements.
+	// The desired behaviour is that if Dave changes his P2P IP address then
+	// Charlie should still be able to reconnect to him.
+	//
+	//    /------- Charlie <-----\
+	//    |                      |
+	//    v                      |
+	//   Dave <===============> Alice
+
+	// The first thing we will test is the case where Dave advertises two
+	// external IP addresses and then switches from the first one listed
+	// to the second one listed. The desired behaviour is that Charlie will
+	// attempt both of Dave's advertised addresses when attempting to
+	// reconnect.
+
+	// Create a new node, Charlie.
+	charlie := net.NewNode(t.t, "Charlie", nil)
+	defer shutdownAndAssert(net, t, charlie)
+
+	// We derive two ports for Dave, and we initialise his node with
+	// these ports advertised as `--externalip` arguments.
+	ip1 := lntest.NextAvailablePort()
+	ip2 := lntest.NextAvailablePort()
+
+	advertisedAddrs := []string{
+		fmt.Sprintf("127.0.0.1:%d", ip1),
+		fmt.Sprintf("127.0.0.1:%d", ip2),
+	}
+
+	var daveArgs []string
+	for _, addr := range advertisedAddrs {
+		daveArgs = append(daveArgs, "--externalip="+addr)
+	}
+
+	// withP2PPort is a helper closure used to set the P2P port that a node
+	// should use.
+	var withP2PPort = func(port int) lntest.NodeOption {
+		return func(cfg *lntest.NodeConfig) {
+			cfg.P2PPort = port
+		}
+	}
+
+	// Create a new node, Dave, and ensure that his initial P2P port is
+	// ip1 derived above.
+	dave := net.NewNode(t.t, "Dave", daveArgs, withP2PPort(ip1))
+	defer shutdownAndAssert(net, t, dave)
+
+	// Subscribe to graph notifications from Charlie so that we can tell
+	// when he receives Dave's NodeAnnouncements.
+	ctxb := context.Background()
+	charlieSub := subscribeGraphNotifications(ctxb, t, charlie)
+	defer close(charlieSub.quit)
+
+	// Connect Alice to Dave and Charlie.
+	net.ConnectNodes(t.t, net.Alice, dave)
+	net.ConnectNodes(t.t, net.Alice, charlie)
+
+	// We'll then go ahead and open a channel between Alice and Dave. This
+	// ensures that Charlie receives the node announcement from Alice as
+	// part of the announcement broadcast.
+	chanPoint := openChannelAndAssert(
+		t, net, net.Alice, dave, lntest.OpenChannelParams{
+			Amt: 1000000,
+		},
+	)
+	defer closeChannelAndAssert(t, net, net.Alice, chanPoint, false)
+
+	// waitForNodeAnnouncement is a closure used to wait on the given graph
+	// subscription for a node announcement from a node with the given
+	// public key. It also waits for the node announcement that advertises
+	// a particular set of addresses.
+	waitForNodeAnnouncement := func(graphSub graphSubscription,
+		nodePubKey string, addrs []string) {
+
+		for {
+			select {
+			case graphUpdate := <-graphSub.updateChan:
+			nextUpdate:
+				for _, update := range graphUpdate.NodeUpdates {
+					if update.IdentityKey != nodePubKey {
+						continue
+					}
+
+					addrMap := make(map[string]bool)
+					for _, addr := range update.NodeAddresses {
+						addrMap[addr.GetAddr()] = true
+					}
+
+					for _, addr := range addrs {
+						if !addrMap[addr] {
+							continue nextUpdate
+						}
+					}
+
+					return
+				}
+
+			case err := <-graphSub.errChan:
+				t.Fatalf("unable to recv graph update: %v", err)
+
+			case <-time.After(defaultTimeout):
+				t.Fatalf("did not receive node ann update")
+			}
+		}
+	}
+
+	// Wait for Charlie to receive Dave's initial NodeAnnouncement.
+	waitForNodeAnnouncement(charlieSub, dave.PubKeyStr, advertisedAddrs)
+
+	// Now create a persistent connection between Charlie and Bob with no
+	// channels. Charlie is the outbound node and Bob is the inbound node.
+	net.ConnectNodesPerm(t.t, charlie, dave)
+
+	// Assert that Dave and Charlie are connected
+	assertConnected(t, dave, charlie)
+
+	// Change Dave's P2P port to the second IP address that he advertised
+	// and restart his node.
+	dave.Cfg.P2PPort = ip2
+	err := net.RestartNode(dave, nil)
+	require.NoError(t.t, err)
+
+	// assert that Dave and Charlie do not reconnect after Dave's
+	// changes to his second advertised address. This will be fixed in
+	// a following commit.
+	assertNotConnected(t, dave, charlie)
+}
+
 // assertTimeoutError asserts that a connection timeout error is raised. A
 // context with a default timeout is used to make the request. If our customized
 // connection timeout is less than the default, we won't see the request context

lntest/itest/lnd_test_list_on_test.go

@@ -40,6 +40,10 @@ var allTestCases = []*testCase{
 		name: "disconnecting target peer",
 		test: testDisconnectingTargetPeer,
 	},
+	{
+		name: "reconnect after ip change",
+		test: testReconnectAfterIPChange,
+	},
 	{
 		name: "graph topology notifications",
 		test: testGraphTopologyNotifications,