p2p/sentry: fix shared PeerStore version filter in findBestPeers, findPeerByMinBlock, PeerEvents#21394
Merged
Merged
Conversation
…findPeerByMinBlock, PeerEvents With the shared PeerStore introduced in #21335, all GrpcServers see every peer regardless of which eth protocol version they negotiated. Three rangePeers loops were missing the per-sentry version filter that Peers() and SimplePeerCount() already apply: - findBestPeersWithPermit: used by SendMessageByMinBlock to pick target peers. Without the filter an eth/71 sentry could select an eth/69 peer, encode the request with eth/71 wire codes, and send it over the eth/69 peer's rw — the remote peer would receive an unknown code and disconnect. - findPeerByMinBlock: same path, single-peer variant. - PeerEvents replay: emitted Connect events for every peer in the shared store, including peers owned by other-version sentries. Those peers never generate a Disconnect on this sentry's peersStreams, leaving the subscriber with ghost peers that are never cleaned up. Fix: apply the same `pv == 0 || pv != ss.ethVersion` guard used by Peers() and SimplePeerCount() to all three loops. Three new tests cover the fixed behaviour.
anacrolix
approved these changes
May 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up fixes for #21335 (merged).
Bugs fixed
With the shared
PeerStoreintroduced in #21335, everyGrpcServercansee all peers regardless of which eth protocol version they negotiated.
Three
rangePeersloops were missing the per-sentry version filter thatPeers()andSimplePeerCount()already apply correctly:1.
findBestPeersWithPermit/findPeerByMinBlockUsed by
SendMessageByMinBlockto select target peers. Without thefilter, an eth/71 sentry could pick an eth/69 peer:
messageCodeencodes the request with eth/71 wire codes.writePeersends it over the eth/69 peer'sethRw.code and disconnects.
Contrast:
SendMessageToRandomPeersandSendMessageToAllalreadyfilter with
protocolVersions.Contains(peerInfo.EthProtocol()).2.
PeerEventsreplayThe replay pass at the start of
PeerEventsiterated the entire sharedstore and emitted a
Connectevent for every peer. Subscribers foreth/68 would receive
Connectevents for eth/69 and eth/71 peers.Those peers never generate a
Disconnecton the eth/68 sentry'speersStreams, so the subscriber accumulates ghost peers that are nevercleaned up.
Fix
Apply the same two-condition guard already used by
Peers()andSimplePeerCount():Tests
Three new tests added to
sentry_grpc_server_test.go:TestGrpcServer_FindBestPeersWithPermit_FiltersVersionTestGrpcServer_FindPeerByMinBlock_FiltersVersionTestGrpcServer_PeerEvents_ReplayFiltersByVersionAll existing tests pass.
make lintclean ×2.