From 18909e1a8ee5cae1b6d03841f296b303347f9364 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Tue, 31 Aug 2021 14:48:33 +0200 Subject: [PATCH] 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 | 4 +- lntest/itest/lnd_network_test.go | 143 ++++++++++++++++++++++++++ lntest/itest/lnd_test_list_on_test.go | 4 + 3 files changed, 149 insertions(+), 2 deletions(-) diff --git a/lntest/harness.go b/lntest/harness.go index 105f5c269..1c8386d60 100644 --- a/lntest/harness.go +++ b/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) diff --git a/lntest/itest/lnd_network_test.go b/lntest/itest/lnd_network_test.go index 63f401859..2464f3137 100644 --- a/lntest/itest/lnd_network_test.go +++ b/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 diff --git a/lntest/itest/lnd_test_list_on_test.go b/lntest/itest/lnd_test_list_on_test.go index a1b187556..cd9780502 100644 --- a/lntest/itest/lnd_test_list_on_test.go +++ b/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,