fix(state-transition): reject malformed justification state with typed errors (leanSpec #1178)

[MegaRedHand]

What

Ports the reachable robustness guards from leanSpec #1178: reject a malformed justification-bookkeeping State in process_attestations with typed errors instead of silently mis-counting.

Why

process_attestations unpacks the flat justifications_validators bit list as one validator-sized segment per tracked root. A State reconstructed from untrusted bytes (checkpoint sync) satisfies SSZ yet can still break the cross-field invariant len(justifications_validators) == len(justifications_roots) * validator_count — SSZ decoding cannot enforce it. ethlambda's index-range unpack (.get(j)) does not crash on a mismatch, but it silently produces short/wrong vote segments. These guards close that gap.

Changes

crates/blockchain/state_transition/src/lib.rs, in process_attestations, before the unpack:

• Empty registry: reject validator_count == 0 (NoValidators). Belt-and-suspenders — the header stage rejects this first in the normal flow — but the unpack relies on a non-zero segment width directly.
• Vote-list length mismatch: reject justifications_validators.len() != justifications_roots.len() * validator_count (new JustificationVotesLengthMismatch).

Checks are ordered to match the spec (registry → length → zero-hash). The zero-hash-root guard and the totality of slot_is_justifiable_after (the other two items in #1178) were already present in ethlambda, so they are unchanged.

For a well-formed state none of these fire, so honest operation and state roots are unaffected.

Tests

• process_attestations_rejects_justification_votes_length_mismatch
• process_attestations_rejects_empty_validator_registry

cargo fmt, clippy -D warnings, and the state-transition lib tests pass.

[github-actions]

🤖 Kimi Code Review

This is a well-crafted defensive programming PR that closes an invariant violation vector reachable via malicious checkpoint-sync anchors. The validation logic is sound and the test coverage is comprehensive.

Security & Correctness

• Line 248: The multiplication justifications_roots.len() * validator_count could theoretically overflow usize. While SSZ list bounds make this practically impossible for Ethereum consensus data, consider using checked_mul for absolute robustness in safety-critical code:

let expected_vote_count = state
    .justifications_roots
    .len()
    .checked_mul(validator_count)
    .ok_or(Error::ArithmeticOverflow)?; // or expect() with a safety comment

• Lines 256-258: The empty validator registry check is necessary and correctly placed. Without it, the subsequent chunks(validator_count) call would panic or behave unpredictably when validator_count is 0.

• Lines 263-270: The length mismatch validation is critical. Without it, the zip operation at line 283 (in the original code) would silently truncate the justifications HashMap if the flat vote list is shorter than expected, leading to consensus failures or fork choice corruption.

Code Quality

• Line 39-43: The new error variant JustificationVotesLengthMismatch follows the existing enum style and provides actionable debugging data (expected vs. actual).
• Lines 241-250: Comments correctly cite leanSpec #1178 and explain the threat model (SSZ decoding vs. semantic validity).
• Tests: Both new tests use matches! correctly and verify the specific error variants rather than just checking for Err(_).

Nitpicks

• Line 248: Consider extracting state.justifications_roots.len() into a named variable (tracked_root_count) to improve readability of the length calculation.
• Line 887: The test comment mentions "belt-and-suspenders" which accurately describes the defense-in-depth approach with the header stage.

Verdict: Approved with minor suggestions. The overflow check is optional given SSZ bounds, but recommended for cryptographic code.

---

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

[github-actions]

🤖 Codex Code Review

1. crates/blockchain/state_transition/src/lib.rs fixes the invariant only inside process_attestations, but the same malformed anchor state is still admitted earlier by bin/ethlambda/src/checkpoint_sync.rs and crates/storage/src/store.rs. That matters because crates/blockchain/src/block_builder.rs still deserializes justifications_validators with the old silent-truncation behavior, so a buggy or malicious checkpoint-sync peer can bootstrap a node into a malformed state and affect proposer scoring before the first STF call rejects it. I’d move the justifications_roots / justifications_validators cross-field validation into shared anchor-state validation and call it from checkpoint sync and store bootstrap, not only from process_attestations.

Otherwise the patch itself looks sensible: the new typed error is clearer than silently fabricating short vote segments, and the added tests cover the intended failure modes well.

I couldn’t run the touched tests here because cargo needs to fetch the pinned Rust 1.92.0 toolchain and network access is blocked in this sandbox.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: PR 502 — reject malformed justification bookkeeping with typed errors

Summary: Clean, well-scoped defensive fix. Adds two guards in process_attestations (crates/blockchain/state_transition/src/lib.rs:250-268) that validate the cross-field invariant len(justifications_validators) == len(justifications_roots) * validator_count before the flat vote list is unpacked at line 280-291.

Correctness

• The invariant being enforced is real: serialize_justifications (line 516-518) is the writer that produces this exact flat layout, so validating it on read before the unpack in process_attestations is symmetric and correct.
• Check ordering (registry → length → zero-hash) matches the PR's stated intent and the spec's assert order. Verified: NoValidators fires before JustificationVotesLengthMismatch when both conditions hold (test process_attestations_rejects_empty_validator_registry implicitly confirms this since it doesn't hit the length check first).
• process_attestations is a private fn only reachable through process_block → process_block_header → process_attestations (lib.rs:118-119). Since process_block_header already rejects validator_count == 0 via current_proposer (line 145), the new NoValidators check at line 255-257 is indeed unreachable in the normal flow, exactly as the PR description states ("belt-and-suspenders"). This is honestly documented in both the code comment and PR body — no objection, but worth confirming reviewers understand it's dead code in practice, only reachable if a future caller invokes process_attestations directly.
• expected_vote_count = state.justifications_roots.len() * validator_count (line 261): both operands are bounded by HISTORICAL_ROOTS_LIMIT (2^18) and VALIDATOR_REGISTRY_LIMIT (4096), product ≈ 1.07B, well within usize on any realistic target — no overflow concern.

Tests

• Both new tests are precise and exercise the intended paths: process_attestations_rejects_justification_votes_length_mismatch crafts a 1-root/4-validator state with a 3-bit vote list (wrong width), and process_attestations_rejects_empty_validator_registry covers the zero-validator belt-and-suspenders case. Both assert on the exact error variant/fields rather than just is_err().

Style / consistency

• New Error::JustificationVotesLengthMismatch { expected, actual } follows the existing struct-variant convention (mirrors AggregationBitsOutOfBounds), and the error message reads clearly.
• Comments explain why (untrusted checkpoint-sync bytes, SSZ can't enforce cross-field invariants) rather than restating the code — consistent with the codebase's comment style.

No issues found — this is a solid, minimal, and correctly justified hardening fix.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[greptile-apps]

Greptile Summary

Ports the reachable robustness guards from leanSpec #1178 into process_attestations: rejects a State whose flat justification vote list violates the cross-field invariant len(justifications_validators) == len(justifications_roots) × validator_count with typed errors instead of silently producing misaligned vote segments.

• Adds a validator_count == 0 early-exit (reusing NoValidators) and a new JustificationVotesLengthMismatch variant checked before the flat-list unpack, in spec-prescribed order (registry → length → zero-hash).
• Moves the validator_count binding to the top of process_attestations to serve both new guards, and adds two unit tests that drive each new error path via a hand-crafted malformed State.

Confidence Score: 4/5

The guards and new error variant are correct; for any well-formed state none of the new checks fire, so normal operation is unaffected. Safe to merge after addressing the optional wording note.

The validation logic is sound, the checks are placed in spec-prescribed order, both new error paths have dedicated tests, and the moved validator_count binding is used consistently throughout the function. The only note is a minor grammatical ambiguity in the JustificationVotesLengthMismatch error format string where the {expected} interpolation reads as if it qualifies validator count rather than the computed product.

Only crates/blockchain/state_transition/src/lib.rs changed; the error message wording is worth a second look but nothing else requires attention.

Important Files Changed

Filename	Overview
crates/blockchain/state_transition/src/lib.rs	Adds two pre-flight guards to process_attestations (empty validator registry and flat-vote-list length mismatch) with a new typed error variant, plus matching tests; logic is correct, minor wording ambiguity in the error format string.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[process_attestations called] --> B{validator_count == 0?}
    B -- Yes --> C[Err: NoValidators]
    B -- No --> D{vote list len mismatch?}
    D -- Yes --> E[Err: JustificationVotesLengthMismatch]
    D -- No --> F{any root is zero hash?}
    F -- Yes --> G[Err: ZeroHashInJustificationRoots]
    F -- No --> H[Unpack flat vote list into HashMap]
    H --> I[Process attestations loop]
    I --> J[Update justification and finalization state]
    J --> K[Ok]

Loading

%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
    A[process_attestations called] --> B{validator_count == 0?}
    B -- Yes --> C[Err: NoValidators]
    B -- No --> D{vote list len mismatch?}
    D -- Yes --> E[Err: JustificationVotesLengthMismatch]
    D -- No --> F{any root is zero hash?}
    F -- Yes --> G[Err: ZeroHashInJustificationRoots]
    F -- No --> H[Unpack flat vote list into HashMap]
    H --> I[Process attestations loop]
    I --> J[Update justification and finalization state]
    J --> K[Ok]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
crates/blockchain/state_transition/src/lib.rs:40-42
The `{expected}` placeholder is positioned after "validator count" in the error string, so it reads as though `{expected}` *is* the validator count, when it is actually the product `roots.len() * validator_count`. For example, with 3 roots and 4 validators the message emits "…validator count 12", which could mislead a reader into thinking the validator set has 12 members. Breaking the sentence so that `{expected}` stands alone as the computed total removes the ambiguity.

```suggestion
    #[error(
        "justification vote list length {actual} does not equal expected {expected} (= tracked-root count × validator count)"
    )]
```

_{Reviews (1): Last reviewed commit: "fix(state-transition): reject malformed ..." | Re-trigger Greptile}