server: address review feedback on peer lifecycle handling
What the developer wrote
Authored by Or Aharonee
server: address review feedback on peer lifecycle handling
Prioritize verAckCh in peerLifecycleHandler select to avoid
nondeterministic peerAdd skipping when both channels are ready.
Guard OnVerAck against double-close by checking the channel before
closing, logging an error instead of panicking.
Adjust peerLifecycleEvent comment to reflect that peerAdd may be
skipped when the peer disconnects before or concurrently with verack.
Fix verAckCh field comment formatting.
What changed, and why it matters
This commit fixes two internal timing bugs in how btcd manages peer connections during the Bitcoin handshake. It prevents a peer-registration step from being accidentally skipped when handshake messages arrive at the same time as a disconnect, and it stops the server from crashing if a peer somehow sends its final handshake acknowledgment twice. These are robustness fixes rather than obvious remote-attack paths, but they remove conditions that could destabilize a node.
Treat as a stability/robustness fix worth including in a release. Monitor for related denial-of-service reports, but no immediate emergency response is warranted based solely on this diff.
Security signals we found
Double-close of Go channel converted from panic to logged error
Select-statement ordering changed to prevent non-deterministic peer registration skip
Peer lifecycle race between verack and disconnect addressed
No explicit security framing by vendor in commit message
Evidence from the diff
The patch modifies server.go in btcd. First, it prioritizes the verAckCh case in peerLifecycleHandler’s select so that peerAdd is not skipped when both verAckCh and the peer’s quit/done channels are ready simultaneously. Second, it guards OnVerAck against double-close of verAckCh by checking whether the channel is already closed; if so it logs an error instead of calling close() again, which would panic. The comment for peerLifecycleEvent is updated to note that peerAdd may be skipped when the peer disconnects before or concurrently with verack.
Changed components
server.goserverPeer.OnVerAckpeerLifecycleHandlerverAckChInspect captured patch +13 / −4
diff --git a/server.go b/server.go
index a8a5fe0..2a22dea 100644
--- a/server.go
+++ b/server.go
@@ -161,8 +161,10 @@ const (
// peerLifecycleEvent represents a peer connection or disconnection
// event. Both event types for a given peer are sent by a single
-// goroutine (peerLifecycleHandler), guaranteeing that peerAdd is
-// always enqueued before peerDone.
+// goroutine (peerLifecycleHandler), guaranteeing that if peerAdd is
+// sent, it is always enqueued before peerDone. peerAdd may be
+// skipped entirely when the peer disconnects before or concurrently
+// with verack.
type peerLifecycleEvent struct {
action peerLifecycleAction
sp *serverPeer
@@ -295,7 +297,8 @@ type serverPeer struct {
knownAddresses lru.Cache
banScore connmgr.DynamicBanScore
quit chan struct{}
- verAckCh chan struct{} // closed when OnVerAck fires
+ // Closed when OnVerAck fires.
+ verAckCh chan struct{}
// The following chans are used to sync blockmanager and server.
txProcessed chan struct{}
blockProcessed chan struct{}
@@ -557,7 +560,13 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej
// It signals the peer's lifecycle handler that the handshake is
// complete so it can register the peer with the server.
func (sp *serverPeer) OnVerAck(_ *peer.Peer, _ *wire.MsgVerAck) {
- close(sp.verAckCh)
+ select {
+ case <-sp.verAckCh:
+ peerLog.Errorf("OnVerAck called more than once "+
+ "for peer %v", sp)
+ default:
+ close(sp.verAckCh)
+ }
}
// OnMemPool is invoked when a peer receives a mempool bitcoin message.
Why this scored 40/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.