server: gate handleDonePeerMsg on peerAdded, not VerAckReceived
What the developer wrote
Authored by Olaoluwa Osuntokun
server: gate handleDonePeerMsg on peerAdded, not VerAckReceived
peer.processRemoteVerAckMsg sets verAckReceived=true *before* it
invokes the OnVerAck listener that closes verAckCh. If the peer
disconnects in that window, the peerLifecycleHandler select may
pick Peer.Done() over verAckCh and skip the peerAdd send -- yet
VerAckReceived() still returns true. handleDonePeerMsg would then
call syncManager.DonePeer for a peer that was never NewPeer'd
(sync manager logs a warning and discards).
Add an explicit peerAdded atomic.Bool on serverPeer. Set it in
peerLifecycleHandler after peerAdd has been enqueued. Gate the
syncManager.DonePeer + orphan eviction in handleDonePeerMsg on
peerAdded.Load() so those side effects only fire for peers that
were actually registered.
This tightens the "peerAdd precedes peerDone, or peerAdd is
skipped entirely" invariant from this PR into something explicit
rather than something derived from a flag that races OnVerAck.
What changed, and why it matters
This commit fixes a subtle internal race condition in btcd's peer-handling logic. Previously, a peer could disconnect at exactly the wrong moment, causing the server to tell its sync manager that a peer had finished even though it had never been properly registered. That produced only a warning and skipped cleanup, but it weakened the invariant that every 'peer done' event matches a prior 'peer add' event. The patch adds an explicit flag so the server only runs cleanup side effects for peers that were actually registered.
Treat as a low-severity hardening fix. Reviewers should verify that peerAdded is set in exactly one place and that no other code paths rely on VerAckReceived() for the same side effects. No immediate incident response is warranted unless further analysis shows the mismatched DonePeer can be exploited for denial of service or state corruption.
Security signals we found
Race condition between verack flag and disconnect channel
Mismatched sync-manager peer lifecycle events (DonePeer without NewPeer)
Atomic flag added to enforce explicit peer-registration invariant
Cleanup side effects (orphan eviction) gated on actual registration
Evidence from the diff
serverPeer gains an atomic.Bool field peerAdded. peerLifecycleHandler sets it to true immediately after enqueueing a peerAdd event. handleDonePeerMsg now gates syncManager.DonePeer and orphan eviction on peerAdded.Load() instead of VerAckReceived(). The old check was racy because processRemoteVerAckMsg sets verAckReceived before invoking OnVerAck, which closes verAckCh; a disconnect in that window could cause peerLifecycleHandler to select Peer.Done() and skip peerAdd, yet VerAckReceived() would still return true, leading to a DonePeer call for an unregistered peer (currently a warning/no-op) and unnecessary orphan eviction.
Changed components
btcd server peer lifecycle handling (server.go)syncManager peer registration callbacksmempool orphan evictionInspect captured patch +19 / −2
diff --git a/server.go b/server.go
index 4a28460..a9268cb 100644
--- a/server.go
+++ b/server.go
@@ -302,6 +302,15 @@ type serverPeer struct {
verAckCh chan struct{}
verAckOnce sync.Once
+ // peerAdded is set by peerLifecycleHandler after a peerAdd event
+ // has been enqueued on s.peerLifecycle. handleDonePeerMsg reads
+ // this to decide whether to notify the sync manager and evict
+ // orphans: those side effects must only run for peers that were
+ // actually registered via syncManager.NewPeer, never for peers
+ // where peerAdd was skipped because the verAckCh/Peer.Done()
+ // select picked the disconnect case.
+ peerAdded atomic.Bool
+
// The following chans are used to sync blockmanager and server.
txProcessed chan struct{}
blockProcessed chan struct{}
@@ -1937,8 +1946,15 @@ func (s *server) handleDonePeerMsg(state *peerState, sp *serverPeer) {
}
// Notify the sync manager the peer is gone and evict any
- // remaining orphans that were sent by the peer.
- if sp.VerAckReceived() {
+ // remaining orphans that were sent by the peer. We gate on
+ // peerAdded rather than VerAckReceived() because the verack flag
+ // is set inside processRemoteVerAckMsg before OnVerAck closes
+ // verAckCh. If the peer disconnects in that window and the
+ // peerLifecycleHandler select picks Peer.Done() over verAckCh,
+ // no peerAdd is enqueued and the sync manager never sees
+ // NewPeer for this peer, so a matching DonePeer would be a
+ // no-op warning at best.
+ if sp.peerAdded.Load() {
s.syncManager.DonePeer(sp.Peer)
numEvicted := s.txMemPool.RemoveOrphansByTag(mempool.Tag(sp.ID()))
@@ -2329,6 +2345,7 @@ func (s *server) peerLifecycleHandler(sp *serverPeer) {
s.peerLifecycle <- peerLifecycleEvent{
action: peerAdd, sp: sp,
}
+ sp.peerAdded.Store(true)
case <-sp.Peer.Done():
// Disconnected before verack; no peerAdd needed.
Why this scored 32/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.