Document on_block() validate-then-mutate structure and resolve stale TODOs

[pablodeymo]

Summary

• Documents the existing two-phase (validate-then-mutate) structure of on_block() that was previously implicit
• Removes 3 stale TODOs that proposed changes incompatible with the protocol
• Replaces each TODO with an accurate comment explaining why the current behavior is correct

Context

This is a follow-up to the investigation started in #113 by @MegaRedHand (which addresses #78). That PR proposed extracting signature verification from on_block() into a separate precheck_block_signatures() function called earlier in the pipeline. Four automated reviewers flagged a critical issue: removing verify_signatures() from on_block() means any caller that forgets to call precheck_block_signatures() first can import blocks without cryptographic validation. There are 3 call sites for on_block() (production process_block(), and 2 test files), and only 1 would get the precheck.

During this rework, we also attempted the two other TODOs in the function: rejecting blocks when block body or proposer attestations fail validation. Both turned out to be incorrect — forkchoice spec tests revealed that attestation validation failures are normal and expected:

• Block body attestations can legitimately have source.slot > target.slot when the attesting validator's justified checkpoint (source) advanced past their chosen target. 3 spec tests (test_reorg_on_newly_justified_slot, test_all_validators_attest_in_single_aggregation, test_multiple_specs_same_target_merge_into_one) exercise this exact scenario.
• Proposer attestations can reference the block being processed as their head, which doesn't exist in the store at pre-validation time.

The current warn-and-skip behavior (let _ = on_attestation(...)) is the correct design.

Changes

crates/blockchain/src/store.rs (comments only, zero executable code changes):

1. Updated on_block() docstring — documents the two-phase structure:

   • Phase 1 (read-only): parent state lookup, signature verification, state transition
   • Phase 2 (mutations): store block/state, process attestations, update head

2. Removed TODO: "extract signature verification to a pre-checks function" (was at line 352) — signature verification stays inside on_block(), which is the only safe design. Extracting it creates a "forgotten precheck" risk across all 3 call sites.

3. Removed TODO: "fail the block if an attestation is invalid" (was at line 385) — replaced with a comment explaining that block body attestations fail validation for legitimate reasons (unseen forks, source exceeding target after justification advances).

4. Removed TODO: "validate attestations before processing" (was at line 434) — replaced with a comment explaining that proposer attestations can reference the block being processed or have stale checkpoint data.

5. Added phase boundary marker (// === Store mutations ===) at the exact point where store mutations begin, making the validate-then-mutate structure grep-able.

Relationship to #113 and #78

This PR supersedes #113's approach. Rather than extracting verification to a separate precheck (which creates safety risks), it documents that the current structure already achieves the goal: all validation happens before any store mutations, so on_block() either rejects early (no side effects) or succeeds fully. Issue #78's concern about "accepting blocks with invalid attestations" is addressed by documenting that this is intentional protocol behavior, not a bug.

Test plan

• make fmt passes
• make lint passes
• 26/26 forkchoice spectests pass
• All workspace unit tests pass
• Verified the 3 failing tests that motivated keeping let _ = (test_reorg_on_newly_justified_slot, test_all_validators_attest_in_single_aggregation, test_multiple_specs_same_target_merge_into_one)
• Confirmed signature spectests fail identically on main (pre-existing fixture deserialization issue, unrelated)

[github-actions]

🤖 Kimi Code Review

Review Summary

The PR improves documentation and clarifies the two-phase validation approach in on_block(). However, several security and correctness issues remain:

Critical Issues

1. State mutation before validation complete (lines 355-367): The post-state is computed and modified (post_state.latest_block_header.state_root = state_root) before all validation passes. This violates the claimed two-phase approach where mutations only happen after validation.

2. Attestation processing race condition (lines 394-419): When processing block body attestations, the code calls on_attestation() which mutates the store, but this happens during the "validation" phase according to the documentation. This could lead to inconsistent state if later validation fails.

3. Missing atomicity (lines 367-437): If any step after state mutation fails (e.g., database write fails), the store could be left in an inconsistent state. The function should use transactions or rollback mechanisms.

Security Concerns

4. Signature verification bypass (lines 357-360): The cfg! macro is compile-time only. Consider using a runtime flag to allow disabling signature verification for testing without recompilation.

5. Unchecked attestation processing (lines 394-419, 440-447): Invalid attestations are logged but not rejected. While this matches the spec, ensure this behavior is intentional and documented, as it could allow spam.

Code Quality

6. Inconsistent error handling (lines 387-447): Some operations use let _ = to ignore errors while others use ?. Standardize error handling approach.

7. Dead code risk (lines 430-432): The proposer_attestation construction happens regardless of whether it will be used. Consider moving inside the on_attestation call.

Suggested Fixes

// Move state mutation after all validation
let state_root = block.state_root;
// Don't modify post_state here - do it after validation

// After all validation passes:
post_state.latest_block_header.state_root = state_root;
// Then proceed with mutations

Consider wrapping the entire mutation phase in a transaction or implementing rollback mechanisms for critical failures.

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

No functional changes detected — this diff only updates comments in crates/blockchain/src/store.rs around on_block. No code paths, validations, or state transitions changed. I don’t see new correctness, security, performance, or memory-safety risks introduced by this PR.

Notes:

• The new comments correctly describe the existing best‑effort attestation processing and proposer attestation handling. Behavior remains unchanged, including “log and skip” on invalid attestations. See crates/blockchain/src/store.rs around the on_block body and attestation loops (approx. lines 314–450).

Given the above, I have no requested changes. The PR looks good from a code‑review perspective.

---

Automated review by OpenAI Codex · custom prompt

[greptile-apps]

Greptile Summary

This PR makes documentation-only improvements to on_block(), removing three stale TODOs that proposed changes incompatible with the Ethereum forkchoice protocol:

• Documented the two-phase structure: The updated docstring clearly explains that on_block() performs read-only validation first (parent state lookup, signature verification, state transition), then applies mutations only if all validation passes (store block/state, process attestations, update head). A phase boundary marker comment makes this structure grep-able.

• Removed incorrect TODO about signature extraction: The TODO suggested extracting signature verification to a separate precheck function. The current design correctly keeps verification inside on_block() to prevent the "forgotten precheck" risk where callers might skip verification.

• Removed incorrect TODOs about failing on invalid attestations: Both block body and proposer attestations can legitimately fail validation for protocol-compliant reasons (unseen fork blocks, source exceeding target after justification advances, referencing the block being processed). The new comments accurately explain why the warn-and-skip behavior is correct, as confirmed by forkchoice spec tests.

All changes are comments-only with zero executable code modifications. The explanations are technically accurate and improve code maintainability.

Confidence Score: 5/5

• This PR is completely safe to merge - it only modifies comments and documentation with no executable code changes
• Score of 5 reflects that this is a documentation-only PR with zero risk: no executable code was modified, all comments are technically accurate and improve clarity, and all tests pass. The changes remove incorrect TODOs and replace them with accurate explanations backed by spec test evidence.
• No files require special attention

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Documentation-only changes that clarify the validate-then-mutate structure and remove three incorrect TODOs, replacing them with accurate explanations of protocol behavior

_{Last reviewed commit: 4122c73}

[github-actions]

🤖 Claude Code Review

I have everything I need. Here is the review:

---

PR 154 Review: Document on_block() validate-then-mutate structure

Summary: Documentation-only PR — zero executable code changes. The core reasoning is sound, and all three TODO removals are well-justified. A few issues worth noting below.

---

Findings

1. Pre-existing metrics label bug made more visible by new comments (lines 444–449)

The inspect_err for the proposer attestation increments "block" but the proposer attestation is explicitly processed as gossip (is_from_block = false):

let _ = on_attestation(store, proposer_attestation, false)
    .inspect(|_| metrics::inc_attestations_valid("gossip"))     // ← "gossip"
    .inspect_err(|err| {
        warn!(%slot, %err, "Invalid proposer attestation in block");
        metrics::inc_attestations_invalid("block");             // ← "block" — mismatch
    });

The success path correctly uses "gossip", but the error path uses "block". This was pre-existing, but the new surrounding comment ("this can fail validation for legitimate reasons") now draws attention to this code path. It should be inc_attestations_invalid("gossip").

---

2. Inaccurate comment about failure reason for proposer attestation (lines 440–443)

The comment says:

e.g., referencing the block being processed

However, by the time on_attestation is called at line 444, the block has already been stored (lines 383–384: insert_signed_block / insert_state). So the head block root IS in the store and this specific failure mode cannot occur at that point.

The more accurate reason the proposer attestation can fail is the strict slot guard inside on_attestation for non-block attestations:

// on_attestation, is_from_block = false branch (lines 295–300):
let current_slot = store.time() / SECONDS_PER_SLOT;
if attestation_slot > current_slot {
    return Err(StoreError::AttestationTooFarInFuture { ... });
}

When a block arrives early in its slot — before store.time() has advanced to that slot — the proposer attestation's slot (= block.slot) exceeds current_slot and is rejected. This is the real failure mode, not the block being unknown in the store.

Suggested replacement wording:

// The proposer's attestation slot equals the block slot. If the block
// arrives before the store's clock has advanced to that slot, the attestation
// will be rejected as "too far in future" by on_attestation's non-block path.
// Source/target/head blocks from unseen forks are also valid failure reasons.

---

3. Docstring Phase 2 step 4 bundles operations in misleading order (lines 324–328)

The docstring says:

4. Storing the block, state, and updating checkpoints

But in the actual code (lines 373–384), checkpoint updates happen before block and state storage:

// Lines 373–380: checkpoints updated FIRST
store.update_checkpoints(ForkCheckpoints::new(...));

// Lines 383–384: block and state stored AFTER
store.insert_signed_block(block_root, signed_block.clone());
store.insert_state(block_root, post_state);

Splitting into two docstring items would be more precise:

4. Updating justified/finalized checkpoints
5. Storing the block and post-state

(with the remaining items renumbered accordingly)

---

What's correct and well-reasoned

• Keeping verify_signatures inside on_block(): The PR's argument is correct. Extracting it to a precheck creates a "forgotten precheck" vulnerability across 3 call sites. Security-critical decision.
• let _ = on_attestation(...) for block body attestations: The explanation is accurate. The 3 failing spec tests (test_reorg_on_newly_justified_slot, test_all_validators_attest_in_single_aggregation, test_multiple_specs_same_target_merge_into_one) are real and validate this behavior. The comment accurately captures source.slot > target.slot as expected after justified checkpoint advancement.
• // === Store mutations === boundary marker: Good grep target. Placement is exactly right — before the first mutation.
• Two-phase structure characterization: Accurate from the store's perspective. Phase 1 is read-only on the store even though post_state is mutated locally.

---

Verdict

The PR logic is correct and the documentation adds real value. Items 1 (metrics label) and 2 (inaccurate comment) are worth fixing before merge. Item 3 is a minor nit.

---

Automated review by Claude (Anthropic) · custom prompt

[MegaRedHand]

Superseded by #157