Repository navigation
fix: dos protection for NetworkProcessor - #10039
Conversation
Performance Report✔️ no performance regression detected Full benchmark results
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae2fb3f081
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
twoeths
left a comment
twoeths
left a comment
There was a problem hiding this comment.
@lodekeeper please review
lodekeeper
left a comment
lodekeeper
left a comment
There was a problem hiding this comment.
Did a focused security pass on the DOS logic at b8c3e39bc6, reading the actual code rather than trusting the fix-commit messages (since this closes a bounty finding). Verdict on the three Codex findings:
- #1 (keep multiple unvalidated payload candidates) — fixed:
execution_payloadper-root cap is raised to 2, so a malformed-first envelope no longer starves the valid one. Minor headroom note in-thread (not blocking). - #2 (buffer cap bypass via
searchUnknownRoot) — fixed:bufferedRootsBySlotis now tracked separately and populated only when a message is actually buffered, so resending a rejected root can't climb to the 32-root search cap. Solid. - #3 (cross-slot prune deleting the wrong waits) — the flagged block↔payload case is fixed via the split wait maps +
blockPeerIds/envelopePeerIdsgating. I did find one low-severity residual of the same pattern within a single wait-type (root-keyed maps pruned wholesale by slot) — detailed inline; non-blocking since it errs toward dropping within a 3-slot window, no OOM/DOS risk.
The caps read sensibly (5 buffered / 32 searched / per-topic message caps), and the +308-line networkProcessor.test.ts is a good addition. Overall LGTM — the two P2s and the P1 are genuinely closed; the only open item is the optional #3 residual for you to decide on.
(Minor bookkeeping: the SHA cited for finding #2 is 5da6834738, but that commit is the MAX_AWAITING_MESSAGES_PER_ROOT change; #2's actual fix is b8c3e39bc6.)
| * Max distinct roots we BUFFER messages for, per slot. Kept tight to avoid the OOM | ||
| * risk. We don't support super forky condition where there are more than this many roots per slot via gossip. | ||
| */ | ||
| export const MAX_BUFFERED_ROOTS_PER_SLOT = 5; |
There was a problem hiding this comment.
haven't seen this many forks in any devnet so I think this is enough but I'd check with @nflaig
There was a problem hiding this comment.
no I think this is totally fine, I doubt we would need more than 5 ever, but my concern with such a limit is rather that an attacker can fill this up quite easily and then lodestar does not queue the actual messages?
There was a problem hiding this comment.
Verified against head 5af43b15. Your premise is correct for the raw message: once bufferedRootsBySlot.get(slot).size reaches MAX_BUFFERED_ROOTS_PER_SLOT (5), tryReserveBufferedRoot returns false (L636-642) and maybeAwaitBlock/maybeAwaitPayload drop the gossip message with reject{reason: reached_root_limit} (L589-592 / L614-617). An attacker can evict the buffered-replay path for a legit 6th+ root.
But message buffering and block/envelope acquisition are two independent gates on the same incoming path, and the acquisition one is deliberately looser:
searchUnknownRootfires before the buffer step (L434/488/507/518/537/543) and is gated only bytooManySearchedRoots→MAX_SEARCHED_ROOTS_PER_SLOT= 32 (L627-631, L651) — not the 5-root buffer cap.- So a legit root 6–32 still gets an active
searchUnknownBlock/searchUnknownEnvelopeeven though its passive gossip message was dropped. The block/envelope is recovered via sync; what's lost is the fast replay of that one waiting message (att vote / column / envelope-forward), which falls back to the slower sync/sampling path.
That's the intended tradeoff — the L70-73 comment calls out this exact case ("the genuine root may come after the first MAX_BUFFERED_ROOTS_PER_SLOT roots ... if we cannot search, we'll penalize peers in UnknownBlockInput"). The 32>5 gap exists so a buffer-fill attack degrades to slow-path acquisition instead of dropping the block.
Residual exposure, to be upfront:
- Roots beyond 32/slot get neither buffered nor searched — but 32 distinct roots/slot is well past any real fork, so that's an attacker-only regime.
- Garbage-root spammers aren't penalized yet — the
UnknownBlockInputscoring you flagged in the sibling thread (r3965403952), tracked as follow-up (per-pruneFinalized).
So not a "block never queued" hole — legit blocks within the 32-budget still get fetched — but it is a fast→slow degradation under attack, whose cost to the attacker depends on that follow-up peer-scoring landing.
There was a problem hiding this comment.
yeah this is how I understand it works after reviewing the code, cc @wemeetagain for another sanity check
| * - in the attack scenario, the genuine root may comes after the first MAX_BUFFERED_ROOTS_PER_SLOT roots | ||
| * - if we cannot search, we'll penalize peers in UnknownBlockInput, not in NetworkProcessor | ||
| */ | ||
| export const MAX_SEARCHED_ROOTS_PER_SLOT = 32; |
There was a problem hiding this comment.
this can only happen in some attack scenarios, we need to bound it anyway
right now UnknownBlockInput sync does not penalize peers
but we should safely do it per pruneFinalized() (not right after a failed download, it's not safe to do so)
will do it next
There was a problem hiding this comment.
Makes sense — bounding the search budget here and delegating peer penalization to UnknownBlockInput sync is the right split. Agree pruneFinalized() is the safe cadence: by finalization a genuine root would've resolved through forkchoice/sync, so anything still unresolved is safe to penalize, and it avoids the false positive of scoring a peer right after a download that only failed because the EL wasn't ready yet (same caution as the existing "removing the block(s) without penalizing the peers" path). Not a blocker for this PR — tracking it as follow-up.
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
my main concern is that this could be abused too, but it's much better and should prevent the dos. As for the queuing in general, the important messages are data columns and payloads, on mainnet, we do not expect them to arrive before the beacon block, but we definitely need to have this soaking on a devnet for a while and see if it causes any issues in the happy path, could also do some attack scenarios on kurtosis if we have the time for it
| const MAX_UNKNOWN_ROOTS_SLOT_CACHE_SIZE = 3; | ||
|
|
||
| /** | ||
| * Max distinct roots we BUFFER messages for, per slot. Kept tight to avoid the OOM |
There was a problem hiding this comment.
the term "buffer" is a bit ambiguous
There was a problem hiding this comment.
Agree it's ambiguous — "buffer" reads like a queue you drain, but here it's the set of unknown roots we hold pending gossip messages for until the block shows up. The test file already leans on "hold" language ("how many unknown blocks we hold messages for, per slot"), so aligning the JSDoc/const comment on that — e.g. "Max distinct unknown roots we hold pending messages for, per slot" — would remove the ambiguity without renaming the symbol.
|
|
||
| /** | ||
| * Max distinct roots we BUFFER messages for, per slot. Kept tight to avoid the OOM | ||
| * risk. We don't support super forky condition where there are more than this many roots per slot via gossip. |
There was a problem hiding this comment.
I have never seen more than 5 tbh, even in holesky, while there were more than 5 forks, most of those were dead and didn't produce messages at the head of each chain, so I think this should be plenty, cc @wemeetagain to double check
There was a problem hiding this comment.
The exact number matters less than it looks because of the backstop: MAX_SEARCHED_ROOTS_PER_SLOT (32) is separate from and higher than the hold limit, so even if honest forks occasionally push past 5 held roots, real blocks beyond the hold limit still get looked up by root and recover (the "keeps looking up a real block even after it stops holding messages for it" test covers this). So 5 for the hold buffer looks safe regardless of the precise live-fork count — leaving the empirical "is 5 enough" to @wemeetagain's check.
| const MAX_DATA_COLUMN_PER_ROOT = MAX_AWAITING_MESSAGES_PER_ROOT[GossipType.data_column_sidecar] as number; | ||
|
|
||
| // When a gossip message points at a block we don't have yet, we hold the message and look the block up. | ||
| // A malicious peer could flood us with messages pointing at blocks that will never arrive, so we put two |
There was a problem hiding this comment.
can we test a malicious peer polluting our queue for a block root, so if they real messages arrives, we will not queue it, right?
There was a problem hiding this comment.
This is already covered in networkProcessor.test.ts, split across two cases:
- "stops holding messages once too many different blocks are unknown in the same slot" — floods 5 distinct (garbage) roots then a 6th;
bufferedBlockCount()stays at 5, i.e. the 6th root's real messages are not held/queued. That's exactly your "real message arrives → we won't queue it". - "keeps looking up a real block even after it stops holding messages for it" — same flood, but asserts the 6th root is still looked up (
unknownBlockRootfired 6×), so the legit block is fetched via sync and the node recovers.
The same-root flood variant (attacker piles junk onto one root) is bounded by the per-(root,topic) cap and covered too — "holds a limited number of data column / execution payload / bid messages for one block".
So the behavior holds: a real message for an over-limit root is dropped, but the block itself is still recovered via lookup. If you'd prefer it spelled out as a single explicitly-named "malicious peer pollutes the buffer, then the legit victim's message is dropped but still looked up" case, happy to add that (or @twoeths may want to fold it in) — the mechanics are all there, it'd just consolidate the narrative.
On the review-body point re abuse + soaking: agree that's the right validation. Static read is the happy path is unaffected (columns/payloads/bids for a known-soon block don't depend on the hold buffer surviving an attack, and the abuse case degrades to slow-path lookup rather than dropping the block), but a devnet soak + a kurtosis flood scenario is the way to confirm it under real timing.
matthewkeil
moved this from In Progress
to Awaiting Author
in Lodestar Team Coordination
|
|
going to merge this, @wemeetagain feel free to do a post-merge review |
github-project-automation
Bot
moved this from Awaiting Author
to Done
in Lodestar Team Coordination
|
deployed to feat groups an devnets, nothing caught my eyes
the PR mainly handles the DOS resistant, it should not affect the normal scenario |
|
🎉 This PR is included in v1.48.0 🎉 |
5 participants
Motivation
Description
bufferedRootsBySlot = new MapDef<Slot, Set<RootHex>>pruneFinalized()searchedRootsBySlot = new MapDef<Slot, MapDef<RootHex, SearchedRootEntry>>unknownBlocksBySlotandunknownEnvelopesBySlottosearchedRootsBySlotso that we can cap roots per slot easierCloses https://github.com/ethereum-bounty/lodestar/issues/19
AI Assistance Disclosure