From 11b84f5cb57a9edc1a86109344636b5598df192d Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 11 Oct 2019 18:17:21 -0400 Subject: [PATCH 1/4] server: signal SyncManager with new peer within AddPeer This makes the logic a bit more unified as previously it was possible we for us to report the new peer to the SyncManager, but for whatever reason failed to track the peer in the server internally within AddPeer. This change ensures this can no longer happen. --- server.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server.go b/server.go index 9f4c54d3..17dc2783 100644 --- a/server.go +++ b/server.go @@ -490,9 +490,6 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej // the local clock to keep the network time in sync. sp.server.timeSource.AddTimeSample(sp.Addr(), msg.Timestamp) - // Signal the sync manager this peer is a new sync candidate. - sp.server.syncManager.NewPeer(sp.Peer) - // Choose whether or not to relay transactions before a filter command // is received. sp.setDisableRelayTx(msg.DisableRelayTx) @@ -1657,6 +1654,9 @@ func (s *server) handleAddPeerMsg(state *peerState, sp *serverPeer) bool { s.addrManager.Connected(sp.NA()) } + // Signal the sync manager this peer is a new sync candidate. + s.syncManager.NewPeer(sp.Peer) + return true } From a1a5bfa81957965f2b65ba661bd8632d25be6a64 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 11 Oct 2019 18:19:59 -0400 Subject: [PATCH 2/4] server: add new peers within OnVerAck instead of within OnVersion This change is needed as part of requiring peers to also send a verack message following their version message during protocol negotiation. Peers were previously added to the SyncManager before their message queues were started, causing the server to stall if a peer didn't provide a timely verack response following their version. We now do this within OnVerAck, which happens shortly before peer message queues are started. --- server.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/server.go b/server.go index 17dc2783..bff032d9 100644 --- a/server.go +++ b/server.go @@ -494,11 +494,15 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej // is received. sp.setDisableRelayTx(msg.DisableRelayTx) - // Add valid peer to the server. - sp.server.AddPeer(sp) return nil } +// OnVerAck is invoked when a peer receives a verack bitcoin message and is used +// to kick start communication with them. +func (sp *serverPeer) OnVerAck(_ *peer.Peer, _ *wire.MsgVerAck) { + sp.server.AddPeer(sp) +} + // OnMemPool is invoked when a peer receives a mempool bitcoin message. // It creates and sends an inventory message with the contents of the memory // pool up to the maximum inventory allowed per message. When the peer has a @@ -1966,6 +1970,7 @@ func newPeerConfig(sp *serverPeer) *peer.Config { return &peer.Config{ Listeners: peer.MessageListeners{ OnVersion: sp.OnVersion, + OnVerAck: sp.OnVerAck, OnMemPool: sp.OnMemPool, OnTx: sp.OnTx, OnBlock: sp.OnBlock, From 769c4e152f0711aba7560a5d68005b0779b5f677 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 11 Oct 2019 18:24:31 -0400 Subject: [PATCH 3/4] server: request addresses from new peers once they can process messages This was previously done within the OnVersion listener, which should not be a blocking operation. It turns out that requesting these messages there can lead to blocking due to peers not being able to process messages since their message queues have yet to start. Therefore, we'll now request within handleAddPeerMsg, which should allow it to go through. --- server.go | 59 +++++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/server.go b/server.go index bff032d9..630937fb 100644 --- a/server.go +++ b/server.go @@ -438,11 +438,6 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej return wire.NewMsgReject(msg.Command(), wire.RejectNonstandard, reason) } - // Update the address manager and request known addresses from the - // remote peer for outbound connections. This is skipped when running - // on the simulation test network since it is only intended to connect - // to specified peers and actively avoids advertising and connecting to - // discovered peers. if !cfg.SimNet && !isInbound { // After soft-fork activation, only make outbound // connection to peers if they flag that they're segwit @@ -461,29 +456,6 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej sp.Disconnect() return nil } - - // Advertise the local address when the server accepts incoming - // connections and it believes itself to be close to the best known tip. - if !cfg.DisableListen && sp.server.syncManager.IsCurrent() { - // Get address that best matches. - lna := addrManager.GetBestLocalAddress(remoteAddr) - if addrmgr.IsRoutable(lna) { - // Filter addresses the peer already knows about. - addresses := []*wire.NetAddress{lna} - sp.pushAddrMsg(addresses) - } - } - - // Request known addresses if the server address manager needs - // more and the peer has a protocol version new enough to - // include a timestamp with addresses. - hasTimestamp := sp.ProtocolVersion() >= wire.NetAddressTimeVersion - if addrManager.NeedMoreAddresses() && hasTimestamp { - sp.QueueMessage(wire.NewMsgGetAddr(), nil) - } - - // Mark the address as a known good address. - addrManager.Good(remoteAddr) } // Add the remote peer time as a sample for creating an offset against @@ -1661,6 +1633,37 @@ func (s *server) handleAddPeerMsg(state *peerState, sp *serverPeer) bool { // Signal the sync manager this peer is a new sync candidate. s.syncManager.NewPeer(sp.Peer) + // Update the address manager and request known addresses from the + // remote peer for outbound connections. This is skipped when running on + // the simulation test network since it is only intended to connect to + // specified peers and actively avoids advertising and connecting to + // discovered peers. + if !cfg.SimNet && !sp.Inbound() { + // Advertise the local address when the server accepts incoming + // connections and it believes itself to be close to the best + // known tip. + if !cfg.DisableListen && s.syncManager.IsCurrent() { + // Get address that best matches. + lna := s.addrManager.GetBestLocalAddress(sp.NA()) + if addrmgr.IsRoutable(lna) { + // Filter addresses the peer already knows about. + addresses := []*wire.NetAddress{lna} + sp.pushAddrMsg(addresses) + } + } + + // Request known addresses if the server address manager needs + // more and the peer has a protocol version new enough to + // include a timestamp with addresses. + hasTimestamp := sp.ProtocolVersion() >= wire.NetAddressTimeVersion + if s.addrManager.NeedMoreAddresses() && hasTimestamp { + sp.QueueMessage(wire.NewMsgGetAddr(), nil) + } + + // Mark the address as a known good address. + s.addrManager.Good(sp.NA()) + } + return true } From baeb789a7d789cf095b6c465c2a5754cf6e4dff7 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Fri, 11 Oct 2019 18:29:09 -0400 Subject: [PATCH 4/4] server: prevent adding peers if already disconnected This addresses an issue where the server ends up tracking a peer that has been disconnected due to it processing a peer's `done` message before its `add` message. --- server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server.go b/server.go index 630937fb..1d7fdf9b 100644 --- a/server.go +++ b/server.go @@ -1563,7 +1563,7 @@ func (s *server) handleUpdatePeerHeights(state *peerState, umsg updatePeerHeight // handleAddPeerMsg deals with adding new peers. It is invoked from the // peerHandler goroutine. func (s *server) handleAddPeerMsg(state *peerState, sp *serverPeer) bool { - if sp == nil { + if sp == nil || !sp.Connected() { return false }