feat: respond to Status messages by MegaRedHand · Pull Request #8 · lambdaclass/ethlambda

MegaRedHand · 2026-01-07T16:28:18Z

No description provided.

…ttestation processing Three defensive fixes in process_attestations / try_finalize: 1. aggregation_bits OOB (finding #6): A malicious attestation could carry aggregation_bits longer than the validator set, causing votes[validator_id] to panic on out-of-bounds access. Now filter bits >= validator_count before indexing. 2. root_to_slot missing key (finding #7): try_finalize used direct HashMap index root_to_slot[root] which panics if a justification root is absent from historical_block_hashes (e.g. carried over from a prior finalization window). Replaced with .get() — missing roots are conservatively retained. 3. justifications[root] (finding #8): Reviewed and confirmed safe — the roots come from justifications.keys() so the key is always present. No change needed.

…on data (#228) ## Motivation Security audit findings #6, #7, #8: three sites in `process_attestations` / `try_finalize` use direct indexing on data structures whose keys or indices come from network-controlled attestations. A malicious peer can craft attestations that trigger panics, crashing the node. ## Description ### Fix 1 — Reject attestations with OOB `aggregation_bits` (finding #6) **File:** `crates/blockchain/state_transition/src/lib.rs` **Problem:** `aggregation_bits` is a `BitList<ValidatorRegistryLimit>` (max 4096 bits) deserialized from the network. `votes` is a `Vec<bool>` of length `validator_count` (actual validators in state). If a malicious attestation carries `aggregation_bits` with bits set beyond `validator_count`, direct indexing panics. **Spec and cross-client behavior:** | Implementation | Behavior | |---|---| | **Spec** (state.py:503-505) | Direct list access `justifications[target.root][validator_id]` — crashes on OOB (IndexError) | | **Zeam** (Zig) | Rejects attestation with error | | **Lantern** (C) | Rejects attestation with `return -1` | | **gean** (Go) | `Bitlist.Get()` returns false for OOB (silent skip) | **Fix:** Pre-loop length check that rejects the entire attestation when `aggregation_bits.len() > validator_count`, matching the spec (crash = implicit reject) and the majority of other clients (Zeam, Lantern). Direct indexing `votes[validator_id] = true` is safe after the bounds check. --- ### Fix 2 — Prune justification roots missing from `root_to_slot` (finding #7) **File:** `crates/blockchain/state_transition/src/lib.rs` (in `try_finalize`) **Problem:** `root_to_slot` is built from `historical_block_hashes` for slots after finalization. But `justifications` can contain roots carried over from a previous finalization window that no longer appear in `historical_block_hashes`. Direct HashMap index panics on missing key. **Spec and cross-client behavior:** | Implementation | Behavior | |---|---| | **Spec** (state.py:560-562) | `assert all(root in root_to_slot for root in justifications)` — crashes (invariant violation) | | **Lantern** (C) | Conservative retention | The spec treats missing roots as an invariant violation. A root absent from `root_to_slot` means its slot is at or below `finalized_slot` (already finalized). Conservatively retaining such roots (previous approach with `is_none_or`) would cause them to accumulate forever — they never reappear in `root_to_slot`, and `is_valid_vote` rejects new votes for them. **Fix:** Prune missing roots (return `false` in `retain`), matching the spec's filter intent: `root_to_slot[root] > finalized_slot` would be `false` for such roots. A warning is logged when this happens. --- ### Finding #8 — `justifications[root]` (no change needed) **File:** `crates/blockchain/state_transition/src/lib.rs` (in `serialize_justifications`) ```rust let justification_roots: Vec<H256> = justifications.keys().cloned().collect(); // ... .flat_map(|root| justifications[root].iter()) ``` Reviewed and confirmed safe: `justification_roots` is built from `justifications.keys()`, so every root used as index is guaranteed to be present in the map. No change needed. ## How to Test ```bash make fmt make lint cargo test --workspace --release ``` All 120 tests pass unchanged — valid attestations always have `aggregation_bits.len() <= validator_count` and all justification roots are in `root_to_slot`, so these guards only trigger on malformed/adversarial data not present in test fixtures.

* feat(p2p): serve beacon blocks by range and by root The beacon follower registered no block protocol, so a peer asking for one got a stream-negotiation refusal. Register `beacon_blocks_by_range/2` and `beacon_blocks_by_root/2`, answer both off the store, and add the request side for the anchor-to-head fetch to call. Both chains answer a block request as one chunk per block, so the loop, its per-chunk metrics and its skip-on-error-code rule move out of `lean::encoding` into `req_resp::encoding` and each chain now supplies only what differs: its `ChunkLimits` (whether a chunk carries a fork digest, and how many chunks an answer may run to), and how a chunk body becomes a block. `write_success_chunk` grows a `context` parameter for the same reason. That loop was also unbounded on both chains, reading until the peer closed, so it now stops at the protocol's own request ceiling. Two details of these two protocols are not guessable from the response they produce, and both are wire-visible: - The range request is still three fields. `step` is deprecated and must be 1, but altair says the v2 request is unchanged from phase0's and lighthouse pins the protocol's request length to `min == max == 24 bytes`, so a two-field body is refused before it is ever decoded. A body naming any other step is refused as malformed: the spec says a requester MUST set it to 1, and honouring the phase0 leniency would mean carrying a dead field through every layer above the codec. - The root request is a bare SSZ list, not a container holding one. The spec says this body is an SSZ *field* where the range body is an SSZ *container*, and a container prefixes its one variable-length field with a four-byte offset. Neither difference reaches above the codec. What a peer is asking for is the same question on either chain, so `req_resp::messages` owns one `BlocksByRangeRequest` and one `BlocksByRoot` variant for both wires and each chain's encoder converts to its own container: lean's has no `step` and fills it with 1 on the way in, beacon's carries all three fields. The shared struct is deliberately not SSZ-derived, so it cannot be written to either wire by accident. The root list needs no conversion at all, since `Root` *is* `H256` and both lists are `SszList<H256, 1024>`. `step` is carried as far as the handler rather than checked at decode, so a peer that sets it wrong gets `INVALID_REQUEST` and learns which rule it broke; refusing in the codec would close the stream with nothing on it. The answer is shared the same way. `ResponsePayload::Blocks` is one variant for all four block protocols: `SignedBeaconBlock` already carries a `Lean` variant, and both stores hand blocks back in that type, so the serving path no longer converts at all. The narrowing to lean's `SignedBlock` happens once, where a fetched block is handed to the chain actor, and lean's block writer refuses to put a block of any other fork on its wire. Neither enum records which chain a message came from. A node speaks one wire for its whole life, so `P2PServer::wire` already answers that, and the dispatch reads `Wire::is_beacon`. Every successful chunk carries a four-byte `ForkDigest` context, and it is the digest of that block's own epoch rather than the one this node runs on, so serving history needs `genesis_validators_root`: `BeaconWire` and the codec both carry it now. The codec is no longer `Default`, because a contextless one would negotiate these protocols and then fail every chunk; `build_swarm` decides the context in the same match that decides the protocol set. Reading a chunk goes the other way round. The fork comes from the slot inside the payload, through the decode path gossip already uses, and the context bytes are then checked against the digest that slot implies. That is a stronger test than using them as the decoder key: it catches a peer whose `genesis_validators_root` or fork schedule is not ours, which is what the digest exists to say and is not otherwise visible until a signature fails. Serving is live; asking is not driven from anywhere yet. This node has no beacon `BlockChain` actor to import an answer into, so a fetch loop would spend a peer's bandwidth on blocks it then drops. The two request functions are exported for the anchor-to-head fetch to call. A node whose data directory is not a beacon one refuses both protocols with `RESOURCE_UNAVAILABLE`, the spec's own code for a peer unable to reply to block requests. One gap this exposes rather than introduces: `build_status` still advertises zeroes for every checkpoint. That was the honest answer for a follower holding no chain, and is no longer one now that the anchor gives it a store to serve from. Peers pick who to ask for blocks by reading `Status`, so that understatement, not any limit in the serving path, is what will keep these two protocols quiet. Deriving `Status` from the store wants the same work that gives this node an importer. * feat(beacon): advertise the store this node already serves from `beacon_blocks_by_{range,root}/2` answer from the checkpoint-anchored store, but `build_status` reported zeroes for every checkpoint. That was the honest answer for a follower holding no chain and stopped being one once there was a store behind those handlers. Peers pick who to ask for blocks by reading `Status`, so the understatement, not any limit in the serving path, is what kept that path quiet. Derive it: head from `Store::beacon_head`, the finalized checkpoint from `Store::beacon_finalized_checkpoint`, and `earliest_available_slot` from `Store::latest_finalized`'s own slot rather than `finalized_epoch * SLOTS_PER_EPOCH`. This node anchors at a checkpoint and keeps only the unfinalized window above it, so the epoch's first slot is not necessarily one it holds, while the anchor checkpoint is exactly the oldest block the directory can still produce. Naming that slot tells a peer not to ask for anything older; naming zero would claim genesis is in reach. `Store::beacon_head` answers `None` in one window, between `init_beacon` seeding `KEY_HEAD` with the anchor root and fork choice inserting the block that root names. Every field but the fork digest falls back to zero there, which stays honest: lighthouse's relevance check exempts a zero `finalized_root` from its finalized-root comparison, reading it as "this peer is syncing" rather than as a conflicting chain, so the connection survives instead of earning an `IrrelevantPeer` disconnect. `handle_status_request` stops going through `beacon_wire_or_refuse`, which returns a `&BeaconWire` borrowed from the whole `&mut P2PServer` and so would make the `&server.store` this now needs a conflicting borrow. Reading `server.wire.beacon()` directly borrows that one field. * refactor(p2p): pick the by-root protocol from the wire `fetch_block_from_peer` asked for a missing block through `Request::BlocksByRoot` on `BLOCKS_BY_ROOT_PROTOCOL_V1` unconditionally. `Handler<FetchBlock>` calls it without knowing which chain is running, so on a beacon node that puts a lean-framed request on beacon streams. Peer selection stays where it is, since which peers already failed to answer for a root has nothing to do with which wire is speaking. Only the send branches, and the beacon arm goes through `request_beacon_block_by_root`, which was already written and exported with no in-tree caller. Its `outbound_requests` bookkeeping is its own, so the lean arm keeps recording that entry itself and the `pending_root_requests` entry that dedupes a repeated fetch is shared below both arms. Nothing exercises the beacon arm on this branch: reaching it needs a chain actor asking for a missing parent, and this one spawns none. It is here so that the request cannot go out mis-framed once one does. * feat(beacon): sync the block range from the anchor to a peer's head `Status` was recorded and nothing was driven off it, so this node asked no peer for the range between its anchor and their head. The two beacon request senders existed and were exported with no in-tree caller. A peer's Status now starts or extends a range session, reusing lean's `RangeSyncState` wholesale: same merge-or-create, same one-batch-in- flight rule, same per-peer failure elimination. What differs is only where the batch is sent and what "behind" is measured against. Sent through `request_beacon_blocks_by_range`, so the beacon protocol id applies and `MAX_REQUEST_BLOCKS_DENEB` is the real per-request ceiling rather than the larger `MAX_REQUEST_BLOCKS` that `next_batch` plans against. That sender records its own `outbound_requests` entry with whatever it actually sent, so `complete_batch` still advances by the true request span when a batch was clamped. Measured against a new `beacon_fetched_through` watermark, not the store's head. Nothing here imports, so the store's head never moves and a sync driven off it would re-request the same range forever; the watermark is seeded from the anchor at spawn and advances on arrival. The field's doc comment carries the rest of the reasoning, which holds once an importer does exist: delivery is a message and import is work, so a head-driven sync trails a delivered batch by the whole actor mailbox. `handle_beacon_blocks_response` grows the session bookkeeping to match: an empty answer fails the peer out of the session (or, for a by-root request, goes through `handle_fetch_failure` like lean's), a successful one completes the batch and asks for the next. An exhausted session clears back to `None`, which is what keeps the resync path usable, since both status handlers only ever merge into an existing session and never replace one that is stuck. That predicate is now `range_session_exhausted`, shared with the lean handler rather than spelled out twice. Blocks are still counted and dropped, exactly as gossiped beacon blocks are: there is no actor to import them on this branch. The range and root checks run anyway, since they are what makes an answer trustworthy. * docs(beacon): describe the wire that now asks, and advertises honestly Both notes said the request side had no in-tree caller and that `build_status` reported zeroes. Neither is true any more: a peer's Status starts a range session, `fetch_block_from_peer` picks its protocol from the wire, and Status is derived from the anchored store. What replaces them keeps the limitation that is still real and was previously implied by "no caller": the range path runs while nothing imports, so its blocks are checked and dropped. Worth saying outright, since it is bandwidth spent on discarded data rather than a path that merely sits idle. * refactor(p2p): answer a by-root response with one handler for both wires The two by-root paths had drifted, and only the beacon one had drifted into bugs. An empty answer re-inserted its `outbound_requests` entry for a failure event that never comes again, so the entry stayed forever. An answer carrying blocks under other roots fell through with no `handle_fetch_failure` at all, leaving the root in `pending_root_requests` where it deduplicates every later fetch of that block for the life of the process. Both follow from treating an empty answer as a case of its own. It is not one: a request carries exactly one root, so `find` over an empty response is already the no-match case, and collapsing the two leaves a single handler that serves either wire. The import at the end is where the chains genuinely part, and that split is already made for us, since `blockchain` is `None` on a beacon node. The dispatch now reads the request kind first and the wire second, which is what gives the root arm one callee. `handle_beacon_blocks_response` loses its root half and becomes `handle_beacon_blocks_by_range_response`, which is also what lean's side has always looked like. * refactor(p2p): read a canonical slot range through one function `canonical_beacon_blocks_by_range` was written as the counterpart of `canonical_blocks_by_range`, but there is nothing left for a counterpart to do: both return `SignedBeaconBlock` and both call the same store method, which dispatches on the chain itself. The only difference was a `count == 0` early return that `checked_sub(1)` already covers, since a count of zero has no last offset to add. The surviving name is the bare one, which is how this module already spells a handler that serves both wires. * docs(p2p): give the `Request` enum back its doc comment `BlocksByRangeRequest` was inserted between the enum's doc block and the enum, with no blank line, so rustdoc attached the whole "Every request either chain can send" block to the struct and `Request` documented nothing at all. The block moves back down to the item it describes; the struct keeps the paragraph that was always its own. * docs(p2p): correct the rustdoc this branch left behind `cargo doc --document-private-items` reported two links that no longer resolve, and four passages had gone stale under the same renames: - `Codec::for_wire` became `Codec::lean()` / `Codec::beacon()`. - `crate::lean::messages::BlocksByRangeRequest` became `LeanBlocksByRangeRequest`, and the shared request it was standing in for is `req_resp`'s own struct, which belongs to neither wire. Said in three places, all three now say it. - The `pub use` of the two senders still claimed nothing in the crate calls them. `fetch_block_from_peer` and `request_next_beacon_range_batch` both do; what is still true is that the answer stops here, with no importer to take it. - `decode_single_chunk` still claimed every registered beacon protocol is single-chunk, which the two block protocols are not. Its link to the multi-chunk shape did not resolve either, which predates this branch. `DISCOVERY_DIAL_INTERVAL` in `discovery/dial.rs` is left alone: it is the one unresolved link older than this branch and outside its diff. * fix(p2p): log a planned beacon batch as planned, not as sent `request_next_beacon_range_batch` traced the batch size `next_batch` planned, which is bounded by `MAX_REQUEST_BLOCKS`, under the name `count`, while the request actually sent is clamped to `MAX_REQUEST_BLOCKS_DENEB`. Reading the log, a clamped batch looked like a request for several times what went out. The field is now `planned` and the message says the batch is being planned. What went on the wire keeps being traced by `request_beacon_blocks_by_range`, which is where the clamp lives and so the only place that can report it without a second copy of the ceiling. * test(p2p): drop the range-session test that tested itself The second half of `a_range_session_exhausted_by_completing_its_batch_ clears_to_none` built its own `Option`, re-implemented the handlers' clear-to-`None` inline, and asserted that its own two lines had done what they say. No production code ran in it, so it could not fail for any reason worth knowing about. Its one real assertion, that a session whose whole range arrives in one batch is exhausted, is the `current_range.is_empty()` branch of `range_session_exhausted`, which no other test covered. It moves to the test that already covers the other branch, whose name now says which branches it checks rather than only the case it opens with. * chore: simplify doc

feat: respond to Status messages

b961704

MegaRedHand merged commit 682667a into main Jan 7, 2026

MegaRedHand deleted the handle-status-messages branch January 7, 2026 16:43

github-actions Bot mentioned this pull request Feb 2, 2026

feat: support requests for multiple blocks #70

Merged

pablodeymo mentioned this pull request Mar 13, 2026

Guard against out-of-bounds indexing on attacker-controlled attestation data #228

Merged

pablodeymo mentioned this pull request Aug 25, 2026

feat(p2p): add opt-in discv5 peer discovery #579

Merged

Navigation Menu

Repository navigation

feat: respond to Status messages - #8

feat: respond to Status messages#8
MegaRedHand merged 1 commit into
mainfrom
handle-status-messages

MegaRedHand commented Jan 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

MegaRedHand commented Jan 7, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant