Merge pull request #5538 from ellemouton/refreshPeerIP

multi: refresh peer IP during reconnect

docs/release-notes/release-notes-0.14.0.md

@@ -391,7 +391,7 @@ you.
 ## Bug Fixes
 
 * A bug has been fixed that would cause `lnd` to [try to bootstrap using the
-  currnet DNS seeds when in SigNet
+  current DNS seeds when in SigNet
   mode](https://github.com/lightningnetwork/lnd/pull/5564).
 
 * [A validation check for sane `CltvLimit` and `FinalCltvDelta` has been added
@@ -430,6 +430,10 @@ you.
   result in transactions being rebroadcast even after they had been confirmed. 
   [Lnd is updated to use the version of Neutrino containing this 
   fix](https://github.com/lightningnetwork/lnd/pull/5807).
+
+* A bug has been fixed that would result in nodes not [reconnecting to their
+  persistent outbound peers if the peer's IP
+  address changed](https://github.com/lightningnetwork/lnd/pull/5538).
 
 * [Use the change output index when validating the reserved wallet balance for
   SendCoins/SendMany calls](https://github.com/lightningnetwork/lnd/pull/5665)
@@ -444,6 +448,7 @@ change](https://github.com/lightningnetwork/lnd/pull/5613).
 * Alyssa Hertig
 * Andras Banki-Horvath
 * de6df1re
+* Elle Mouton
 * ErikEk
 * Eugene Siegel
 * Harsha Goli

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)
 
@@ -670,13 +670,31 @@ func (n *NetworkHarness) EnsureConnected(t *testing.T, a, b *HarnessNode) {
 	)
 }
 
-// ConnectNodes establishes an encrypted+authenticated p2p connection from node
+// ConnectNodes attempts to create a connection between nodes a and b.
+func (n *NetworkHarness) ConnectNodes(t *testing.T, a, b *HarnessNode) {
+	n.connectNodes(t, a, b, false)
+}
+
+// ConnectNodesPerm attempts to connect nodes a and b and sets node b as
+// a peer that node a should persistently attempt to reconnect to if they
+// become disconnected.
+func (n *NetworkHarness) ConnectNodesPerm(t *testing.T,
+	a, b *HarnessNode) {
+
+	n.connectNodes(t, a, b, true)
+}
+
+// connectNodes establishes an encrypted+authenticated p2p connection from node
 // a towards node b. The function will return a non-nil error if the connection
-// was unable to be established.
+// was unable to be established. If the perm parameter is set to true then
+// node a will persistently attempt to reconnect to node b if they get
+// disconnected.
 //
 // NOTE: This function may block for up to 15-seconds as it will not return
 // until the new connection is detected as being known to both nodes.
-func (n *NetworkHarness) ConnectNodes(t *testing.T, a, b *HarnessNode) {
+func (n *NetworkHarness) connectNodes(t *testing.T, a, b *HarnessNode,
+	perm bool) {
+
 	ctxb := context.Background()
 	ctx, cancel := context.WithTimeout(ctxb, DefaultTimeout)
 	defer cancel()
@@ -692,6 +710,7 @@ func (n *NetworkHarness) ConnectNodes(t *testing.T, a, b *HarnessNode) {
 			Pubkey: bobInfo.IdentityPubkey,
 			Host:   b.Cfg.P2PAddr(),
 		},
+		Perm: perm,
 	}
 
 	err = n.connect(ctx, req, a)

lntest/itest/assertions.go

@@ -471,43 +471,102 @@ func assertNumOpenChannelsPending(t *harnessTest,
 	require.NoError(t.t, err)
 }
 
-// assertNumConnections asserts number current connections between two peers.
-func assertNumConnections(t *harnessTest, alice, bob *lntest.HarnessNode,
-	expected int) {
+// checkPeerInPeersList returns true if Bob appears in Alice's peer list.
+func checkPeerInPeersList(ctx context.Context, alice,
+	bob *lntest.HarnessNode) (bool, error) {
+
+	peers, err := alice.ListPeers(ctx, &lnrpc.ListPeersRequest{})
+	if err != nil {
+		return false, fmt.Errorf(
+			"error listing %s's node (%v) peers: %v",
+			alice.Name(), alice.NodeID, err,
+		)
+	}
+
+	for _, peer := range peers.Peers {
+		if peer.PubKey == bob.PubKeyStr {
+			return true, nil
+		}
+	}
+
+	return false, nil
+}
+
+// assertConnected asserts that two peers are connected.
+func assertConnected(t *harnessTest, alice, bob *lntest.HarnessNode) {
 	ctxb := context.Background()
-	ctxt, _ := context.WithTimeout(ctxb, defaultTimeout)
+	ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
 
 	err := wait.NoError(func() error {
-		aNumPeers, err := alice.ListPeers(
-			ctxt, &lnrpc.ListPeersRequest{},
-		)
+		bobIsAlicePeer, err := checkPeerInPeersList(ctxt, alice, bob)
 		if err != nil {
+			return err
+		}
+
+		if !bobIsAlicePeer {
 			return fmt.Errorf(
-				"unable to fetch %s's node (%v) list peers %v",
-				alice.Name(), alice.NodeID, err,
+				"expected %s and %s to be connected "+
+					"but %s is not in %s's peer list",
+				alice.Name(), bob.Name(),
+				bob.Name(), alice.Name(),
 			)
 		}
 
-		bNumPeers, err := bob.ListPeers(ctxt, &lnrpc.ListPeersRequest{})
+		aliceIsBobPeer, err := checkPeerInPeersList(ctxt, bob, alice)
 		if err != nil {
+			return err
+		}
+
+		if !aliceIsBobPeer {
 			return fmt.Errorf(
-				"unable to fetch %s's node (%v) list peers %v",
-				bob.Name(), bob.NodeID, err,
+				"expected %s and %s to be connected "+
+					"but %s is not in %s's peer list",
+				alice.Name(), bob.Name(),
+				alice.Name(), bob.Name(),
 			)
 		}
 
-		if len(aNumPeers.Peers) != expected {
+		return nil
+
+	}, defaultTimeout)
+	require.NoError(t.t, err)
+}
+
+// assertNotConnected asserts that two peers are not connected.
+func assertNotConnected(t *harnessTest, alice, bob *lntest.HarnessNode) {
+	ctxb := context.Background()
+	ctxt, cancel := context.WithTimeout(ctxb, defaultTimeout)
+	defer cancel()
+
+	err := wait.NoError(func() error {
+		bobIsAlicePeer, err := checkPeerInPeersList(ctxt, alice, bob)
+		if err != nil {
+			return err
+		}
+
+		if bobIsAlicePeer {
 			return fmt.Errorf(
-				"number of peers connected to %s is "+
-					"incorrect: expected %v, got %v",
-				alice.Name(), expected, len(aNumPeers.Peers),
+				"expected %s and %s not to be "+
+					"connected but %s is in %s's "+
+					"peer list",
+				alice.Name(), bob.Name(),
+				bob.Name(), alice.Name(),
 			)
 		}
-		if len(bNumPeers.Peers) != expected {
+
+		aliceIsBobPeer, err := checkPeerInPeersList(ctxt, bob, alice)
+		if err != nil {
+			return err
+		}
+
+		if aliceIsBobPeer {
 			return fmt.Errorf(
-				"number of peers connected to %s is "+
-					"incorrect: expected %v, got %v",
-				bob.Name(), expected, len(bNumPeers.Peers),
+				"expected %s and %s not to be "+
+					"connected but %s is in %s's "+
+					"peer list",
+				alice.Name(), bob.Name(),
+				alice.Name(), bob.Name(),
 			)
 		}
 

lntest/itest/lnd_misc_test.go

@@ -49,7 +49,7 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) {
 	net.ConnectNodes(t.t, alice, bob)
 
 	// Check existing connection.
-	assertNumConnections(t, alice, bob, 1)
+	assertConnected(t, alice, bob)
 
 	// Give Alice some coins so she can fund a channel.
 	net.SendCoins(t.t, btcutil.SatoshiPerBitcoin, alice)
@@ -82,7 +82,7 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) {
 	time.Sleep(time.Millisecond * 300)
 
 	// Assert that the connection was torn down.
-	assertNumConnections(t, alice, bob, 0)
+	assertNotConnected(t, alice, bob)
 
 	fundingTxID, err := chainhash.NewHash(pendingUpdate.Txid)
 	if err != nil {
@@ -128,7 +128,7 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) {
 	}
 
 	// Check existing connection.
-	assertNumConnections(t, alice, bob, 0)
+	assertNotConnected(t, alice, bob)
 
 	// Reconnect both nodes before force closing the channel.
 	net.ConnectNodes(t.t, alice, bob)
@@ -152,14 +152,14 @@ func testDisconnectingTargetPeer(net *lntest.NetworkHarness, t *harnessTest) {
 			err)
 	}
 
-	// Check zero peer connections.
-	assertNumConnections(t, alice, bob, 0)
+	// Check that the nodes not connected.
+	assertNotConnected(t, alice, bob)
 
 	// Finally, re-connect both nodes.
 	net.ConnectNodes(t.t, alice, bob)
 
 	// Check existing connection.
-	assertNumConnections(t, alice, net.Bob, 1)
+	assertConnected(t, alice, bob)
 
 	// Cleanup by mining the force close and sweep transaction.
 	cleanupForceClose(t, net, alice, chanPoint)