server+lntest: use all addrs during reconnect

In this commit, all advertised addresses of a peer are used during
reconnection. This fixes a bug previously demonstrated in an itest.

lntest/itest/lnd_network_test.go

@@ -60,11 +60,8 @@ func testNetworkConnectionTimeout(net *lntest.NetworkHarness, t *harnessTest) {
 	assertTimeoutError(ctxt, t, dave, req)
 }
 
-// testReconnectAfterIPChange verifies that if an inbound node changes its
+// testReconnectAfterIPChange verifies that if a persistent inbound node changes
-// IP address to an IP address not listed first in its advertised address list
+// its listening address then it's peer will still be able to reconnect to it.
-// 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
@@ -197,10 +194,9 @@ func testReconnectAfterIPChange(net *lntest.NetworkHarness, t *harnessTest) {
 	err := net.RestartNode(dave, nil)
 	require.NoError(t.t, err)
 
-	// assert that Dave and Charlie do not reconnect after Dave's
+	// assert that Dave and Charlie reconnect successfully after Dave
-	// changes to his second advertised address. This will be fixed in
+	// changes to his second advertised address.
-	// a following commit.
+	assertConnected(t, dave, charlie)
-	assertNotConnected(t, dave, charlie)
 }
 
 // assertTimeoutError asserts that a connection timeout error is raised. A

server.go

@@ -194,6 +194,7 @@ type server struct {
 
 	persistentPeers        map[string]bool
 	persistentPeersBackoff map[string]time.Duration
+	persistentPeerAddrs    map[string][]*lnwire.NetAddress
 	persistentConnReqs     map[string][]*connmgr.ConnReq
 	persistentRetryCancels map[string]chan struct{}
 
@@ -465,6 +466,7 @@ func newServer(cfg *Config, listenAddrs []net.Addr,
 		persistentPeers:         make(map[string]bool),
 		persistentPeersBackoff:  make(map[string]time.Duration),
 		persistentConnReqs:      make(map[string][]*connmgr.ConnReq),
+		persistentPeerAddrs:     make(map[string][]*lnwire.NetAddress),
 		persistentRetryCancels:  make(map[string]chan struct{}),
 		peerErrors:              make(map[string]*queue.CircularBuffer),
 		ignorePeerTermination:   make(map[*peer.Brontide]struct{}),
@@ -3521,21 +3523,18 @@ func (s *server) peerTerminationWatcher(p *peer.Brontide, ready chan struct{}) {
 		return
 	}
 
-	// We'll only need to re-launch a connection request if one
+	// Get the last address that we used to connect to the peer.
-	// isn't already currently pending.
+	addrs := []net.Addr{
-	if _, ok := s.persistentConnReqs[pubStr]; ok {
+		p.NetAddress().Address,
-		return
 	}
 
-	// We'll ensure that we locate an advertised address to use
+	// We'll ensure that we locate all the peers advertised addresses for
-	// within the peer's address for reconnection purposes.
+	// reconnection purposes.
-	//
+	advertisedAddrs, err := s.fetchNodeAdvertisedAddrs(pubKey)
-	// TODO(roasbeef): use them all?
-	advertisedAddr, err := s.fetchNodeAdvertisedAddr(pubKey)
 	switch {
-	// We found an advertised address, so use it.
+	// We found advertised addresses, so use them.
 	case err == nil:
-		p.SetAddress(advertisedAddr)
+		addrs = advertisedAddrs
 
 	// The peer doesn't have an advertised address.
 	case err == errNoAdvertisedAddr:
@@ -3570,14 +3569,28 @@ func (s *server) peerTerminationWatcher(p *peer.Brontide, ready chan struct{}) {
 			err)
 	}
 
-	// Otherwise, we'll launch a new connection request in order to
+	// Make an easy lookup map so that we can check if an address
-	// attempt to maintain a persistent connection with this peer.
+	// is already in the address list that we have stored for this peer.
-	connReq := &connmgr.ConnReq{
+	existingAddrs := make(map[string]bool)
-		Addr:      p.NetAddress(),
+	for _, addr := range s.persistentPeerAddrs[pubStr] {
-		Permanent: true,
+		existingAddrs[addr.String()] = true
+	}
+
+	// Add any missing addresses for this peer to persistentPeerAddr.
+	for _, addr := range addrs {
+		if existingAddrs[addr.String()] {
+			continue
+		}
+
+		s.persistentPeerAddrs[pubStr] = append(
+			s.persistentPeerAddrs[pubStr],
+			&lnwire.NetAddress{
+				IdentityKey: p.IdentityKey(),
+				Address:     addr,
+				ChainNet:    p.NetAddress().ChainNet,
+			},
+		)
 	}
-	s.persistentConnReqs[pubStr] = append(
-		s.persistentConnReqs[pubStr], connReq)
 
 	// Record the computed backoff in the backoff map.
 	backoff := s.nextPeerBackoff(pubStr, p.StartTime())
@@ -3596,7 +3609,7 @@ func (s *server) peerTerminationWatcher(p *peer.Brontide, ready chan struct{}) {
 	// outbound connection attempt is being made.
 	go func() {
 		srvrLog.Debugf("Scheduling connection re-establishment to "+
-			"persistent peer %v in %s", p, backoff)
+			"persistent peer %v in %s", p.IdentityKey(), backoff)
 
 		select {
 		case <-time.After(backoff):
@@ -3607,12 +3620,83 @@ func (s *server) peerTerminationWatcher(p *peer.Brontide, ready chan struct{}) {
 		}
 
 		srvrLog.Debugf("Attempting to re-establish persistent "+
-			"connection to peer %v", p)
+			"connection to peer %v", p.IdentityKey())
 
-		s.connMgr.Connect(connReq)
+		s.connectToPersistentPeer(pubStr)
 	}()
 }
 
+// connectToPersistentPeer uses all the stored addresses for a peer to attempt
+// to connect to the peer. It creates connection requests if there are
+// currently none for a given address and it removes old connection requests
+// if the associated address is no longer in the latest address list for the
+// peer.
+func (s *server) connectToPersistentPeer(pubKeyStr string) {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+
+	// Create an easy lookup map of the addresses we have stored for the
+	// peer. We will remove entries from this map if we have existing
+	// connection requests for the associated address and then any leftover
+	// entries will indicate which addresses we should create new
+	// connection requests for.
+	addrMap := make(map[string]*lnwire.NetAddress)
+	for _, addr := range s.persistentPeerAddrs[pubKeyStr] {
+		addrMap[addr.String()] = addr
+	}
+
+	// Go through each of the existing connection requests and
+	// check if they correspond to the latest set of addresses. If
+	// there is a connection requests that does not use one of the latest
+	// advertised addresses then remove that connection request.
+	var updatedConnReqs []*connmgr.ConnReq
+	for _, connReq := range s.persistentConnReqs[pubKeyStr] {
+		lnAddr := connReq.Addr.(*lnwire.NetAddress).Address.String()
+
+		switch _, ok := addrMap[lnAddr]; ok {
+		// If the existing connection request is using one of the
+		// latest advertised addresses for the peer then we add it to
+		// updatedConnReqs and remove the associated address from
+		// addrMap so that we don't recreate this connReq later on.
+		case true:
+			updatedConnReqs = append(
+				updatedConnReqs, connReq,
+			)
+			delete(addrMap, lnAddr)
+
+		// If the existing connection request is using an address that
+		// is not one of the latest advertised addresses for the peer
+		// then we remove the connecting request from the connection
+		// manager.
+		case false:
+			srvrLog.Info(
+				"Removing conn req:", connReq.Addr.String(),
+			)
+			s.connMgr.Remove(connReq.ID())
+		}
+	}
+
+	s.persistentConnReqs[pubKeyStr] = updatedConnReqs
+
+	// Any addresses left in addrMap are new ones that we have not made
+	// connection requests for. So create new connection requests for those.
+	for _, addr := range s.persistentPeerAddrs[pubKeyStr] {
+		if _, ok := addrMap[addr.String()]; !ok {
+			continue
+		}
+
+		connReq := &connmgr.ConnReq{
+			Addr:      addr,
+			Permanent: true,
+		}
+
+		s.persistentConnReqs[pubKeyStr] = append(
+			s.persistentConnReqs[pubKeyStr], connReq,
+		)
+		go s.connMgr.Connect(connReq)
+	}
+}
+
 // removePeer removes the passed peer from the server's state of all active
 // peers.
 func (s *server) removePeer(p *peer.Brontide) {
@@ -3927,8 +4011,8 @@ func computeNextBackoff(currBackoff, maxBackoff time.Duration) time.Duration {
 // advertised address of a node, but they don't have one.
 var errNoAdvertisedAddr = errors.New("no advertised address found")
 
-// fetchNodeAdvertisedAddr attempts to fetch an advertised address of a node.
+// fetchNodeAdvertisedAddrs attempts to fetch the advertised addresses of a node.
-func (s *server) fetchNodeAdvertisedAddr(pub *btcec.PublicKey) (net.Addr, error) {
+func (s *server) fetchNodeAdvertisedAddrs(pub *btcec.PublicKey) ([]net.Addr, error) {
 	vertex, err := route.NewVertexFromBytes(pub.SerializeCompressed())
 	if err != nil {
 		return nil, err
@@ -3943,7 +4027,7 @@ func (s *server) fetchNodeAdvertisedAddr(pub *btcec.PublicKey) (net.Addr, error)
 		return nil, errNoAdvertisedAddr
 	}
 
-	return node.Addresses[0], nil
+	return node.Addresses, nil
 }
 
 // fetchLastChanUpdate returns a function which is able to retrieve our latest