Skip to content

p2p/sentry: cap and rate-limit inbound NewBlockHashes per peer#21557

Merged
taratorio merged 4 commits into
mainfrom
yperbasis/sentry-newblockhashes-guards
Jun 2, 2026
Merged

p2p/sentry: cap and rate-limit inbound NewBlockHashes per peer#21557
taratorio merged 4 commits into
mainfrom
yperbasis/sentry-newblockhashes-guards

Conversation

@yperbasis

@yperbasis yperbasis commented Jun 1, 2026

Copy link
Copy Markdown
Member

Summary

Harden the per-peer inbound loop (runPeer) against abusive NewBlockHashes:

  • Oversize (bytes): reject a packet whose wire size exceeds maxNewBlockHashesBytes before the payload is buffered — a multi-MB announcement is dropped without being allocated.
  • Entry cap: after the byte-bounded read, walk the RLP list (no allocation) and reject packets with more than maxBlockHashesPerMsg (4096) entries. The byte ceiling alone doesn't bound the entry count (RLP entries are variable-length), so this enforces the hard cap.
  • Flood: a per-peer token-bucket limiter (10/s, burst 30) kicks peers flooding announcements; it is local to runPeer, so its state is bounded and freed on disconnect.

The peer is disconnected on any of these, and nothing is forwarded to subscribers (eth multi-client or the polygon execution/p2p observer). The size and rate gates run before the hasSubscribers short-circuit, so abusive peers are kicked regardless of sync state.

The handler-level peek (closed PR #21476) was obviated by #21505, which removed the eth block-download handler; this places the protection at the framing layer where it also covers the surviving execution/p2p observer path.

Scope

The live consumer of inbound NewBlockHashes is polygon/Bor (polygon/sync tip-events via execution/p2p), which still gossips blocks over devp2p; post-Merge Ethereum doesn't use these messages — the eth multi-client drops them (#21505) — so there it's generic defense-in-depth against peers sending unsolicited announcements. The thresholds sit well above honest Bor rates (~1 announcement per block), so legitimate peers are unaffected.

Closes erigontech/security#69
Closes erigontech/security#68

Test plan

  • TestRunPeer_OversizedNewBlockHashesKicksPeer — oversized (bytes) packet kicks peer (PeerErrorMessageSizeLimit) and is not forwarded to subscribers
  • TestRunPeer_TooManyNewBlockHashesEntriesKicksPeer — a packet under the byte cap but over the 4096-entry cap kicks the peer
  • TestRunPeer_NewBlockHashesFloodKicksPeer — flood past burst kicks peer (PeerErrorInvalidMessage)
  • TestRunPeer_NormalNewBlockHashesForwarded — compliant traffic is forwarded and not penalized
  • go test -race ./p2p/sentry/ (new tests, -count=5)
  • make lint clean; make erigon integration

🤖 Generated with Claude Code

## Summary
- Reject NewBlockHashes packets larger than maxNewBlockHashesBytes
  (~4096 entries) in the per-peer framing loop (runPeer), before the
  payload is buffered, so an oversized announcement is dropped at the
  sentry layer and never reaches the eth multi-client or polygon
  execution/p2p observer subscribers. The peer is kicked.
- Add a per-peer token-bucket limiter (10 packets/s, burst 30) that
  kicks peers flooding NewBlockHashes. The limiter is local to runPeer,
  so its state is bounded and freed on disconnect.
- Both gates run before the hasSubscribers short-circuit, so abusive
  peers are kicked regardless of sync state.

The handler-level peek (closed PR #21476) is obviated by #21505, which
removed the eth block-download handler; this places the protection at
the framing layer where it also covers the surviving execution/p2p
observer path.

Closes erigontech/security#69
Closes erigontech/security#68

## Test plan
- [x] TestRunPeer_OversizedNewBlockHashesKicksPeer — oversized packet
  kicks peer (PeerErrorMessageSizeLimit) and is not forwarded
- [x] TestRunPeer_NewBlockHashesFloodKicksPeer — flood past burst kicks
  peer (PeerErrorInvalidMessage)
- [x] TestRunPeer_NormalNewBlockHashesForwarded — compliant traffic is
  forwarded and not penalized
- [x] go test -race ./p2p/sentry/ (new tests, -count=5)
- [x] make lint clean; make erigon integration

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yperbasis yperbasis marked this pull request as draft June 1, 2026 12:10
@yperbasis yperbasis requested a review from Copilot June 1, 2026 12:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds sentry-layer protections against abusive eth.NewBlockHashes traffic by rejecting oversized announcements before buffering and disconnecting peers that flood announcements, with accompanying unit tests for the new behavior.

Changes:

  • Add a per-peer size gate for inbound NewBlockHashes messages in runPeer (kick on oversize, discard payload early).
  • Add a per-peer token-bucket rate limiter for inbound NewBlockHashes in runPeer (kick on flood).
  • Add new runPeer tests covering oversize, flood, and normal forwarding behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
p2p/sentry/sentry_grpc_server.go Adds per-peer NewBlockHashes byte cap and rate limiting in runPeer.
p2p/sentry/sentry_grpc_server_test.go Adds helper + tests verifying oversize drop/kick, flood kick, and normal forwarding.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread p2p/sentry/sentry_grpc_server.go
The byte ceiling alone let a peer pack more than maxBlockHashesPerMsg
entries under maxNewBlockHashesBytes, since RLP entries are variable-length.
Walk the byte-bounded payload and disconnect peers whose NewBlockHashes list
exceeds maxBlockHashesPerMsg, so the documented per-message cap is actually
enforced. The byte cap stays as a pre-read bound so the walk never sees more
than maxNewBlockHashesBytes.

Addresses Copilot review feedback on #21557.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@yperbasis yperbasis marked this pull request as ready for review June 1, 2026 13:12
@taratorio taratorio added this pull request to the merge queue Jun 2, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 2, 2026
@taratorio taratorio enabled auto-merge June 2, 2026 03:07
@taratorio taratorio added this pull request to the merge queue Jun 2, 2026
Merged via the queue into main with commit cc4105c Jun 2, 2026
89 checks passed
@taratorio taratorio deleted the yperbasis/sentry-newblockhashes-guards branch June 2, 2026 11:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants