What the developer wrote
Authored by Olaoluwa Osuntokun
server: guard OnVerAck with sync.Once
The prior select/default+close() guard on verAckCh is correct only
under the invariant that OnVerAck is invoked from a single goroutine
(peer.processRemoteVerAckMsg on the input handler). The two steps
are not atomic: any future change that invokes listeners off the
input goroutine would let two concurrent callers both observe
default and panic on double-close.
Replace the guard with sync.Once. This makes the close-once
contract obviously correct rather than correct-by-distant-invariant
and drops the dead "called more than once" log path.
What changed, and why it matters
This commit hardens a Bitcoin peer handshake callback so it cannot accidentally close the same notification channel twice if future code changes call it from multiple goroutines. The old design was safe only because of a distant, unenforced rule about which goroutine could call it. The new design uses Go's sync.Once to make the 'close only once' guarantee obvious and robust. There is no currently reachable bug; it is a defensive correctness fix.
Treat as a low-risk defensive hardening patch. No urgent action required. Reviewers should verify that verAckCh is no longer closed elsewhere and that sync.Once is initialized correctly for each serverPeer.
Security signals we found
Defensive concurrency hardening
Potential double-close panic under changed invocation assumptions
No currently reachable vulnerability in present code paths per commit message
Removal of dead error-handling log path
Evidence from the diff
serverPeer.OnVerAck previously used a select/default+close() pattern on verAckCh to ensure the channel was closed only once. That pattern is non-atomic and relies on the invariant that OnVerAck is always invoked from a single goroutine (peer.processRemoteVerAckMsg on the input handler). The commit replaces it with sync.Once, making the once-close contract atomic and self-evident. It also removes the now-dead error log path for ‘called more than once’. The change is defensive: the commit message explicitly states the prior guard is correct under the current invariant, but could panic on double-close if future changes invoke listeners off the input goroutine.
Changed components
btcd/server.goserverPeer.OnVerAckserverPeer.verAckChInspect captured patch +10 / −11
diff --git a/server.go b/server.go
index 2a22dea..4a28460 100644
--- a/server.go
+++ b/server.go
@@ -296,9 +296,12 @@ type serverPeer struct {
addressesMtx sync.RWMutex
knownAddresses lru.Cache
banScore connmgr.DynamicBanScore
- quit chan struct{}
- // Closed when OnVerAck fires.
- verAckCh chan struct{}
+ quit chan struct{}
+
+ // Closed by verAckOnce when OnVerAck fires.
+ verAckCh chan struct{}
+ verAckOnce sync.Once
+
// The following chans are used to sync blockmanager and server.
txProcessed chan struct{}
blockProcessed chan struct{}
@@ -558,15 +561,11 @@ func (sp *serverPeer) OnVersion(_ *peer.Peer, msg *wire.MsgVersion) *wire.MsgRej
// OnVerAck is invoked when a peer receives a verack bitcoin message.
// It signals the peer's lifecycle handler that the handshake is
-// complete so it can register the peer with the server.
+// complete so it can register the peer with the server. The
+// sync.Once guard ensures verAckCh is closed at most once even if
+// OnVerAck is ever invoked more than once for a given peer.
func (sp *serverPeer) OnVerAck(_ *peer.Peer, _ *wire.MsgVerAck) {
- select {
- case <-sp.verAckCh:
- peerLog.Errorf("OnVerAck called more than once "+
- "for peer %v", sp)
- default:
- close(sp.verAckCh)
- }
+ sp.verAckOnce.Do(func() { close(sp.verAckCh) })
}
// OnMemPool is invoked when a peer receives a mempool bitcoin message.
Why this scored 25/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.