p2p/sentry: kick peers sending oversized NewBlockHashes#21476
Closed
yperbasis wants to merge 8 commits into
Closed
p2p/sentry: kick peers sending oversized NewBlockHashes#21476yperbasis wants to merge 8 commits into
yperbasis wants to merge 8 commits into
Conversation
The eth/68 NewBlockHashes handler decoded each inbound packet into a flat slice and fired one synchronous GetBlockHeaders RPC per entry, with no per-message item cap. The 10 MiB wire envelope allows ~275k entries per packet, so a single peer looping such packets drove unbounded heap growth and triggered the OOM killer. Hoist a 4096-entry cap to the top of newBlockHashes66 (matching the existing convention on NewPooledTransactionHashes). The check peeks the outer-list payload via rlp.ParseList and lower-bounds the count by dividing by the minimum [hash, number] pair size, so oversized packets are rejected without allocating the full slice. Offending peers are kicked via PenaltyKind_Kick. The gate runs before the disableBlockDownload and Hd.InitialCycle short-circuits so abusive peers are dropped regardless of internal sync state. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an early guard in the sentry multi-client NewBlockHashes handler to reject oversized inbound announcements before they can fan out into many header requests.
Changes:
- Introduces constants and a helper to detect oversized
NewBlockHashespayloads. - Kicks peers sending oversized packets before block-download short-circuits.
- Adds tests for oversized peer kicking and small-packet non-penalization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
p2p/sentry/sentry_multi_client/sentry_multi_client.go |
Adds the oversize detection and peer penalty path in newBlockHashes66. |
p2p/sentry/sentry_multi_client/sentry_multi_client_test.go |
Adds unit tests for oversized and normal-sized NewBlockHashes packets. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…sitive The previous oversize check divided the outer-list payload by the minimum [hash, number] pair size (35 bytes) and kicked when the result exceeded the cap. Dividing total payload by the minimum element size gives an UPPER bound on entry count, so the check fired on legitimate packets whose pairs encoded to more than the absolute minimum — e.g. a valid 4096-entry packet announcing mainnet-scale (~22M) block numbers (38-byte pairs, ~155 KB total) was kicked because 155648/35 = 4447 > 4096. Replace the size-based estimate with peekNewBlockHashesCount, which walks the outer list's pair prefixes to return the exact entry count without decoding or allocating entry contents. Mirrors the peekAnnouncementCount precedent in txnprovider/txpool/rlp_eth.go for NewPooledTransactionHashes68. Add TestNewBlockHashes66_AtCapNotKicked: exactly maxBlockHashesPerMsg entries with realistic block numbers must not be kicked — this is the boundary the previous helper false-positived on. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the helper walked every pair prefix to the end of the outer list even when the count had already exceeded maxBlockHashesPerMsg, so a 10 MiB packet could still force ~275k rlp.ParseList calls before being kicked. The whole point of this path is the DoS guard, so return once count exceeds the cap and reject oversized packets after bounded work. Add TestPeekNewBlockHashesCount_StopsAtCap: a packet with 4*cap entries must return count = cap+1, proving the helper does not walk past the bail point. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Post-bail, the count returned by peekNewBlockHashesCount was always maxBlockHashesPerMsg+1, so the "count" field logged on the kick path was misleading and the int return carried no useful information. Switch the helper to newBlockHashesExceedsCap returning (bool, error), and drop the TestPeekNewBlockHashesCount_StopsAtCap unit test — its only assertion (count == cap+1) was the externally observable witness of the early-bail, no longer expressible with a bool return; the same oversize path is already exercised end-to-end by TestNewBlockHashes66_OversizedKicksPeer. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address Copilot review feedback on the nonstandard "false-positived" wording in TestNewBlockHashes66_AtCapNotKicked. Drop the forensic clause about an earlier payload-size estimate (that implementation no longer exists; the history belongs in the PR description, not in source where it rots). Also rewrite "false-positively penalizing" in the parallel TestNewBlockHashes66_NormalSizeIgnored comment for consistency. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
newBlockHashesExceedsCap uses rlp.ParseList, whose ErrParse-wrapped errors are not in the set checked by rlp.IsInvalidRLPError — so the centralized kick path in HandleInboundMessage does not catch them. Even without that gap, the disableBlockDownload / InitialCycle short-circuits between the peek and rlp.DecodeBytes intercept malformed packets before the standard decoder ever runs. Result: a peer sending unparseable NewBlockHashes RLP was previously not penalized. Treat a peek error as a kickable offense, mirroring the existing oversize kick path. Add TestNewBlockHashes66_MalformedKicksPeer with a non-list payload (0x80) to lock the behavior in. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
rlp.ParseList only validates the first top-level list, so a payload shaped like a valid outer list followed by stray bytes (e.g. 0xc0 0x42) slipped through newBlockHashesExceedsCap as (false, nil). Under the disableBlockDownload / InitialCycle short-circuits — where rlp.DecodeBytes is never reached — such packets were silently ignored instead of kicking the offending peer. Verify the outer list consumes the entire payload and return an ErrParse-wrapped error otherwise, mirroring the "exactly one value, no trailing data" contract of rlp.DecodeBytes. Add TestNewBlockHashes66_TrailingBytesKickPeer covering the short-circuit path with disableBlockDownload: true. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Superseded by #21505 ("execution: remove legacy headerdownload, bodydownload and dataflow packages"), which removed the legacy block-download-over-devp2p path entirely. Inbound case sentryproto.MessageId_NEW_BLOCK_HASHES_66,
sentryproto.MessageId_BLOCK_HEADERS_66,
sentryproto.MessageId_NEW_BLOCK_66,
sentryproto.MessageId_BLOCK_BODIES_66:
// Block download is handled by the execution layer (engine API), not over devp2p.
return nilThe DoS this PR guarded against — |
6 tasks
Sahil-4555
pushed a commit
to Sahil-4555/erigon
that referenced
this pull request
Jun 2, 2026
…ntech#21557) ## 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 erigontech#21476) was obviated by erigontech#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 (erigontech#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 - [x] `TestRunPeer_OversizedNewBlockHashesKicksPeer` — oversized (bytes) packet kicks peer (`PeerErrorMessageSizeLimit`) and is not forwarded to subscribers - [x] `TestRunPeer_TooManyNewBlockHashesEntriesKicksPeer` — a packet under the byte cap but over the 4096-entry cap kicks the peer - [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` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: milen <94537774+taratorio@users.noreply.github.com>
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
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.
Closes ethereum-bounty/erigon#11
Summary
NewBlockHasheshandler previously decoded every inbound packet into a flat slice and fired one synchronousGetBlockHeadersRPC per entry with no per-message item cap. The 10 MiB wire envelope allows ~275k entries, so a single peer looping such packets drove unbounded heap growth.newBlockHashes66(matching the existing convention onNewPooledTransactionHashes) and kick offending peers viaPenaltyKind_Kick.rlp.ParseListand lower-bounds the count by dividing by the minimum[hash, number]pair size, so oversized packets are rejected without allocating the full slice. The gate runs before thedisableBlockDownload/Hd.InitialCycleshort-circuits so abusive peers are dropped regardless of internal sync state.Defense-in-depth follow-ups are tracked privately.
Test plan
TestNewBlockHashes66_OversizedKicksPeer(new) — oversized packet kicks the peer and produces zero outbound header requestsTestNewBlockHashes66_NormalSizeIgnored(new) — small 2-entry packet is not falsely penalizedgo test ./p2p/sentry/sentry_multi_client/...passesmake lintclean (0 issues, run twice)make erigon integrationbuilds cleanly🤖 Generated with Claude Code