From 06baabe5da28d17a1dd357594e2b8f5cc7ea3091 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 10 Oct 2019 12:03:47 -0400 Subject: [PATCH 1/4] server: mark address attempted on attempt rather than upon connection We should mark addresses as attempted when we attempt to connect to them, not once we establish a connection with said address. --- server.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/server.go b/server.go index bed4de95..7d5da9c1 100644 --- a/server.go +++ b/server.go @@ -2032,7 +2032,6 @@ func (s *server) outboundPeerConnected(c *connmgr.ConnReq, conn net.Conn) { sp.isWhitelisted = isWhitelisted(conn.RemoteAddr()) sp.AssociateConnection(conn) go s.peerDoneHandler(sp) - s.addrManager.Attempt(sp.NA()) } // peerDoneHandler handles peer disconnects by notifiying the server that it's @@ -2802,6 +2801,9 @@ func newServer(listenAddrs, agentBlacklist, agentWhitelist []string, continue } + // Mark an attempt for the valid address. + s.addrManager.Attempt(addr.NetAddress()) + addrString := addrmgr.NetAddressKey(addr.NetAddress()) return addrStringToNetAddr(addrString) } From ab6f3089f667f38172d9b45936206f0b4bc6f215 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 10 Oct 2019 12:10:52 -0400 Subject: [PATCH 2/4] server: mark address as connected within handleAddPeerMsg We do this to ensure the address manager contains live addresses. Previously, addresses with which we established connections with would not be marked as connected because it would be done once we disconnect peers. Given that we don't process all of the disconnect logic when we're shutting down, addresses of stable and good peers would never be marked as connected unless the connection was lost during operation. --- server.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/server.go b/server.go index 7d5da9c1..eaaa1133 100644 --- a/server.go +++ b/server.go @@ -1651,6 +1651,12 @@ func (s *server) handleAddPeerMsg(state *peerState, sp *serverPeer) bool { } } + // Update the address' last seen time if the peer has acknowledged + // our version and has sent us its version as well. + if sp.VerAckReceived() && sp.VersionKnown() && sp.NA() != nil { + s.addrManager.Connected(sp.NA()) + } + return true } @@ -1681,12 +1687,6 @@ func (s *server) handleDonePeerMsg(state *peerState, sp *serverPeer) { s.connManager.Disconnect(sp.connReq.ID()) } - // Update the address' last seen time if the peer has acknowledged - // our version and has sent us its version as well. - if sp.VerAckReceived() && sp.VersionKnown() && sp.NA() != nil { - s.addrManager.Connected(sp.NA()) - } - // If we get here it means that either we didn't know about the peer // or we purposefully deleted it. } From 45d66d46f9c5c917b643eab43a061f35157ece6b Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 10 Oct 2019 13:18:03 -0400 Subject: [PATCH 3/4] server: standardize use of connmanager's Disconnect and Remove methods The Disconnect method would still attempt to reconnect to the same peer, which could cause us to reconnect to bad/unstable peers if we came across them. Instead, we'll now use Remove whenever we intend to remove a peer that is not persistent. --- server.go | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/server.go b/server.go index eaaa1133..b518a663 100644 --- a/server.go +++ b/server.go @@ -1671,24 +1671,26 @@ func (s *server) handleDonePeerMsg(state *peerState, sp *serverPeer) { } else { list = state.outboundPeers } + + // Regardless of whether the peer was found in our list, we'll inform + // our connection manager about the disconnection. This can happen if we + // process a peer's `done` message before its `add`. + if !sp.Inbound() { + if sp.persistent { + s.connManager.Disconnect(sp.connReq.ID()) + } else { + s.connManager.Remove(sp.connReq.ID()) + } + } + if _, ok := list[sp.ID()]; ok { if !sp.Inbound() && sp.VersionKnown() { state.outboundGroups[addrmgr.GroupKey(sp.NA())]-- } - if !sp.Inbound() && sp.connReq != nil { - s.connManager.Disconnect(sp.connReq.ID()) - } delete(list, sp.ID()) srvrLog.Debugf("Removed peer %s", sp) return } - - if sp.connReq != nil { - s.connManager.Disconnect(sp.connReq.ID()) - } - - // If we get here it means that either we didn't know about the peer - // or we purposefully deleted it. } // handleBanPeerMsg deals with banning peers. It is invoked from the @@ -2025,7 +2027,12 @@ func (s *server) outboundPeerConnected(c *connmgr.ConnReq, conn net.Conn) { p, err := peer.NewOutboundPeer(newPeerConfig(sp), c.Addr.String()) if err != nil { srvrLog.Debugf("Cannot create outbound peer %s: %v", c.Addr, err) - s.connManager.Disconnect(c.ID()) + if c.Permanent { + s.connManager.Disconnect(c.ID()) + } else { + s.connManager.Remove(c.ID()) + } + return } sp.Peer = p sp.connReq = c From 0d00cdf82c3266f014ec7ef7a7717daf28e69de6 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Thu, 10 Oct 2019 13:25:19 -0400 Subject: [PATCH 4/4] server: request new peer after disconnection of non-persistent peers Doing so ensures we reach our target number of outbound peers as soon as possible. This is only necessary after calls to connmgr.Remove, as these won't request a new peer connection. --- server.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/server.go b/server.go index b518a663..9f4c54d3 100644 --- a/server.go +++ b/server.go @@ -1680,6 +1680,7 @@ func (s *server) handleDonePeerMsg(state *peerState, sp *serverPeer) { s.connManager.Disconnect(sp.connReq.ID()) } else { s.connManager.Remove(sp.connReq.ID()) + go s.connManager.NewConnReq() } } @@ -2031,6 +2032,7 @@ func (s *server) outboundPeerConnected(c *connmgr.ConnReq, conn net.Conn) { s.connManager.Disconnect(c.ID()) } else { s.connManager.Remove(c.ID()) + go s.connManager.NewConnReq() } return }