p2p/sentry: kick peers sending oversized NewBlockHashes#21476
Open
yperbasis wants to merge 8 commits into
Open
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>
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