From 789f00746bcc7b168ffda93de477cd4d17292136 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 3 Nov 2021 14:50:39 +0200 Subject: [PATCH 1/4] server: cleanup persistentPeerAddr when pruned Delete all the stored addresses from the persistentPeerAddr map when we prune the persistent peer. --- server.go | 1 + 1 file changed, 1 insertion(+) diff --git a/server.go b/server.go index 354b90dba..aba368e28 100644 --- a/server.go +++ b/server.go @@ -2858,6 +2858,7 @@ func (s *server) prunePersistentPeerConnection(compressedPubKey [33]byte) { if perm, ok := s.persistentPeers[pubKeyStr]; ok && !perm { delete(s.persistentPeers, pubKeyStr) delete(s.persistentPeersBackoff, pubKeyStr) + delete(s.persistentPeerAddrs, pubKeyStr) s.cancelConnReqs(pubKeyStr, nil) s.mu.Unlock() From b9b7ea2a6a4938f37e25007ec90bbce214bf2a55 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 3 Nov 2021 15:02:23 +0200 Subject: [PATCH 2/4] server: use one func for peer re-connection Let the connectToPersistentPeer func handle all connection attempts to persistent peers so that that logic is in once place. --- server.go | 56 ++++++++++++++++++++++++++----------------------------- 1 file changed, 26 insertions(+), 30 deletions(-) diff --git a/server.go b/server.go index aba368e28..24c101c82 100644 --- a/server.go +++ b/server.go @@ -2798,34 +2798,24 @@ func (s *server) establishPersistentConnections() error { IdentityKey: nodeAddr.pubKey, Address: address, } - srvrLog.Debugf("Attempting persistent connection to "+ - "channel peer %v", lnAddr) - // Send the persistent connection request to the - // connection manager, saving the request itself so we - // can cancel/restart the process as needed. - connReq := &connmgr.ConnReq{ - Addr: lnAddr, - Permanent: true, - } + s.persistentPeerAddrs[pubStr] = append( + s.persistentPeerAddrs[pubStr], lnAddr) + } - s.persistentConnReqs[pubStr] = append( - s.persistentConnReqs[pubStr], connReq) + // We'll connect to the first 10 peers immediately, then + // randomly stagger any remaining connections if the + // stagger initial reconnect flag is set. This ensures + // that mobile nodes or nodes with a small number of + // channels obtain connectivity quickly, but larger + // nodes are able to disperse the costs of connecting to + // all peers at once. + if numOutboundConns < numInstantInitReconnect || + !s.cfg.StaggerInitialReconnect { - // We'll connect to the first 10 peers immediately, then - // randomly stagger any remaining connections if the - // stagger initial reconnect flag is set. This ensures - // that mobile nodes or nodes with a small number of - // channels obtain connectivity quickly, but larger - // nodes are able to disperse the costs of connecting to - // all peers at once. - if numOutboundConns < numInstantInitReconnect || - !s.cfg.StaggerInitialReconnect { - - go s.connMgr.Connect(connReq) - } else { - go s.delayInitialReconnect(connReq) - } + go s.connectToPersistentPeer(pubStr) + } else { + go s.delayInitialReconnect(pubStr) } numOutboundConns++ @@ -2834,16 +2824,15 @@ func (s *server) establishPersistentConnections() error { return nil } -// delayInitialReconnect will attempt a reconnection using the passed connreq -// after sampling a value for the delay between 0s and the -// maxInitReconnectDelay. +// delayInitialReconnect will attempt a reconnection to the given peer after +// sampling a value for the delay between 0s and the maxInitReconnectDelay. // // NOTE: This method MUST be run as a goroutine. -func (s *server) delayInitialReconnect(connReq *connmgr.ConnReq) { +func (s *server) delayInitialReconnect(pubStr string) { delay := time.Duration(prand.Intn(maxInitReconnectDelay)) * time.Second select { case <-time.After(delay): - s.connMgr.Connect(connReq) + s.connectToPersistentPeer(pubStr) case <-s.quit: } } @@ -3822,9 +3811,16 @@ func (s *server) connectToPersistentPeer(pubKeyStr string) { Permanent: true, } + srvrLog.Debugf("Attempting persistent connection to "+ + "channel peer %v", addr) + + // Send the persistent connection request to the connection + // manager, saving the request itself so we can cancel/restart + // the process as needed. s.persistentConnReqs[pubKeyStr] = append( s.persistentConnReqs[pubKeyStr], connReq, ) + go s.connMgr.Connect(connReq) } } From 6ec9c3c58fea8ca80a84e6622f3bc97541a37ccb Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 3 Nov 2021 15:06:58 +0200 Subject: [PATCH 3/4] server: stagger multi addr connection attempts In this commit, we stagger the connection attempts to the multiple addresses of a peer. --- server.go | 60 +++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 17 deletions(-) diff --git a/server.go b/server.go index 24c101c82..3b108d250 100644 --- a/server.go +++ b/server.go @@ -95,6 +95,10 @@ const ( // value used or a particular peer will be chosen between 0s and this // value. maxInitReconnectDelay = 30 + + // multiAddrConnectionStagger is the number of seconds to wait between + // attempting to a peer with each of its advertised addresses. + multiAddrConnectionStagger = 10 * time.Second ) var ( @@ -3803,26 +3807,48 @@ func (s *server) connectToPersistentPeer(pubKeyStr string) { s.persistentConnReqs[pubKeyStr] = updatedConnReqs + cancelChan, ok := s.persistentRetryCancels[pubKeyStr] + if !ok { + cancelChan = make(chan struct{}) + s.persistentRetryCancels[pubKeyStr] = cancelChan + } + // 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 addrMap { - connReq := &connmgr.ConnReq{ - Addr: addr, - Permanent: true, + // If there is more than one address in the address map, stagger the + // creation of the connection requests for those. + go func() { + ticker := time.NewTicker(multiAddrConnectionStagger) + + for _, addr := range addrMap { + // Send the persistent connection request to the + // connection manager, saving the request itself so we + // can cancel/restart the process as needed. + connReq := &connmgr.ConnReq{ + Addr: addr, + Permanent: true, + } + + s.mu.Lock() + s.persistentConnReqs[pubKeyStr] = append( + s.persistentConnReqs[pubKeyStr], connReq, + ) + s.mu.Unlock() + + srvrLog.Debugf("Attempting persistent connection to "+ + "channel peer %v", addr) + + go s.connMgr.Connect(connReq) + + select { + case <-s.quit: + return + case <-cancelChan: + return + case <-ticker.C: + } } - - srvrLog.Debugf("Attempting persistent connection to "+ - "channel peer %v", addr) - - // Send the persistent connection request to the connection - // manager, saving the request itself so we can cancel/restart - // the process as needed. - 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 From ac4fe5ff2e58c4ba04b089239084f03481d14b50 Mon Sep 17 00:00:00 2001 From: Elle Mouton Date: Wed, 3 Nov 2021 15:49:49 +0200 Subject: [PATCH 4/4] docs: update release notes --- docs/release-notes/release-notes-0.14.0.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/docs/release-notes/release-notes-0.14.0.md b/docs/release-notes/release-notes-0.14.0.md index 757090b42..27d54e2db 100644 --- a/docs/release-notes/release-notes-0.14.0.md +++ b/docs/release-notes/release-notes-0.14.0.md @@ -600,6 +600,11 @@ messages directly. There is no routing/path finding involved. * [Fix pathfinding crash when inbound policy is unknown]( https://github.com/lightningnetwork/lnd/pull/5922) +* [Stagger connection attempts to multi-address peers to ensure that the peer + doesn't close the first successful connection in favour of the next if + the first one was successful]( + https://github.com/lightningnetwork/lnd/pull/5925) + ## Documentation The [code contribution guidelines have been updated to mention the new