Split on_block() and on_gossip_attestation() into verified and unverified variants

[pablodeymo]

Motivation

PR #113 proposed extracting signature verification out of on_block() into a separate precheck_block_signatures() function called by the network layer before processing. Four automated reviewers flagged a critical security flaw: 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.

PR #154 responded by taking the conservative approach: keep on_block() unchanged and document its validate-then-mutate structure. It argued against extraction.

This PR is the compromise: instead of removing verification from on_block(), we keep on_block() as the safe default (always verifies) and add on_block_without_verification() as an explicit opt-out. This follows Zeam's architecture where ForkChoice.onBlock() is signature-agnostic and verification happens at the chain layer, but inverted — verification is the default and opting out requires calling a function whose name makes the risk obvious.

This eliminates the skip-signature-verification compile-time feature flag entirely.

Description

store.rs — Split on_block()

Three functions replace the old on_block():

Function	Visibility	Verifies?	Stores sigs?	Used by
on_block()	public	Yes	Yes	Production (lib.rs), signature_spectests
on_block_without_verification()	public	No	No	forkchoice_spectests
on_block_core()	private	—	—	Shared logic for both

on_block_core() takes parent_state by value (avoids the previous .clone() — minor perf win). The 4-line dedup + parent-state preamble is duplicated in both public functions to avoid double deserialization (verification needs the state, core needs the state).

store.rs — Split on_gossip_attestation()

Same pattern:

Function	Visibility	Verifies?	Stores sigs?
on_gossip_attestation()	public	Yes	Yes
on_gossip_attestation_without_verification()	public	No	No

Both share the validation + on_attestation() + logging logic. Some duplication is acceptable to keep each function self-contained.

Cargo.toml — Remove feature flag

• Removed [features] section with skip-signature-verification
• Removed required-features = ["skip-signature-verification"] from forkchoice test entry

forkchoice_spectests.rs

• Changed store::on_block() → store::on_block_without_verification()
• Fixed 4 pre-existing collapsible_if clippy warnings (now surfaced because the test no longer requires a feature flag to compile, so clippy --all-targets sees it)

Makefile

• Removed --features skip-signature-verification from test target

CLAUDE.md and RELEASE.md

• Removed all skip-signature-verification references from test commands, gotchas, and Docker build examples

Relationship to #113 and #78

This PR supersedes #113's approach. Rather than extracting verification to a separate precheck (which creates the "forgotten precheck" security risk flagged by reviewers), it splits the function into safe and unsafe variants. The safe variant (on_block()) always verifies — callers can't accidentally skip verification. The unsafe variant (on_block_without_verification()) makes the risk explicit in its name. Issue #78's concern about signature verification is fully addressed: production paths always verify, and the compile-time feature flag is eliminated.

How to test

make fmt
make lint
make test
cargo test -p ethlambda-blockchain --test forkchoice_spectests
cargo test -p ethlambda-blockchain --test signature_spectests --release

Confirm no feature flag remnants:

grep -r "skip-signature-verification" crates/

Note: signature_spectests have pre-existing failures (fixture deserialization issue with ProposerSignature). These failures are identical on main and are unrelated to this change.

[github-actions]

🤖 Kimi Code Review

Review Summary

This PR removes the skip-signature-verification feature flag and replaces it with explicit test-only functions. The changes are well-structured and improve security by making signature verification behavior explicit.

Key Issues Found

1. Missing import in store.rs (line 214-215):

   • ValidatorSignature is used but not imported in the new on_gossip_attestation function
   • Add: use ethlambda_types::signature::ValidatorSignature;

2. Potential panic in slot conversion (line 218):

   • attestation.data.slot.try_into().expect("slot exceeds u32") can panic
   • Consider using ? with a proper error variant instead

3. Inconsistent error handling:

   • on_block_without_verification calls on_attestation which may still verify signatures
   • This could break fork choice tests if attestations are processed with verification

4. Code duplication:

   • The attestation processing logic is duplicated between on_gossip_attestation and on_gossip_attestation_without_verification
   • Consider extracting common validation logic

Minor Issues

• Line 347: Comment mentions "with signature verification" but should clarify this is the production path
• Line 502: The comment "TODO: validate attestations before processing" seems outdated since validation happens in on_attestation

Positive Changes

1. Security improvement: Removing feature flags for signature verification prevents accidental deployment without verification
2. Test clarity: Explicit *_without_verification functions make test intent clear
3. Code organization: The split into on_block/on_block_without_verification and on_block_core is clean
4. Documentation: Added clear doc comments explaining when to use each function

Recommendations

1. Fix the missing import and potential panic
2. Ensure on_block_without_verification uses on_attestation_without_verification if such a function exists
3. Consider creating a shared helper for attestation validation to reduce duplication

---

Automated review by Kimi (Moonshot AI) · custom prompt

[github-actions]

🤖 Codex Code Review

Findings:

1. High: persistent side effect before block acceptance

• In store.rs:389, proposer signature is inserted into store before on_block_core runs.
• If state_transition fails later in store.rs:448, the block is rejected but signature cache is still mutated.
• Impact: cache pollution / unbounded growth from invalid blocks, and potential reuse of signatures from blocks that never became valid.
• Suggestion: move insert_gossip_signature into the success path inside on_block_core after STF + block/state insertion.

2. Medium: verification-bypass APIs are public in non-test builds

• on_gossip_attestation_without_verification and on_block_without_verification are pub and always compiled.
• After removing the feature gate, any downstream caller can bypass XMSS checks by accident (or misuse), which is risky for consensus code.
• Suggestion: restrict to pub(crate) + test-only wrappers, or gate with #[cfg(test)]/dedicated non-production feature.

3. Low: panic-on-input in signature epoch conversion

• store.rs:219 still does try_into().expect("slot exceeds u32").
• For network-facing paths, panic is a DoS risk; prefer returning a typed StoreError instead of expect.

What looks good:

• Refactor cleanly separates verified and non-verified block paths.
• Forkchoice spec tests now explicitly call the no-verification path, which is clearer than feature-driven behavior.

Validation note:

• I could not run tests in this environment due rustup permission errors (/home/runner/.rustup/tmp not writable).

---

Automated review by OpenAI Codex · custom prompt

[github-actions]

🤖 Claude Code Review

Now I have a clear picture of all the changes. Let me write up my review.

---

Review: PR #157 — Split on_block() / on_gossip_attestation() into verified and unverified variants

Summary

The design approach is correct and the security improvement is sound. Eliminating the compile-time skip-signature-verification feature flag in favour of an explicit _without_verification naming convention is the right trade-off: verification is the default and opt-out requires calling a function whose name makes the risk obvious. The on_block_core() extraction is clean.

There are a few issues worth addressing before merging.

---

Issues

1. Correctness: proposer signature stored before state transition can fail

In on_block(), store.insert_gossip_signature(...) is called at lines 393–397, before on_block_core() (which contains the state transition). If on_block_core returns Err (e.g., the state transition function rejects the block), the gossip signature store is left with a stale entry for a block root that was never committed to the chain. A future block builder iterating stored gossip signatures could attempt to reuse a signature for a block that does not exist.

// store.rs ~393
store.insert_gossip_signature(           // ← stored unconditionally
    &proposer_attestation.data,
    proposer_attestation.validator_id,
    proposer_sig,
);

on_block_core(store, signed_block, parent_state)  // ← can still return Err

The signature store should only be mutated after on_block_core succeeds. One option is to move the insert_gossip_signature call into on_block_core (passing the signature in), or to roll back on error.

---

2. Performance: block_root computed twice for every accepted block

block.tree_hash_root() is a full Merkle hash over the block. It is now computed:

• once in on_block() / on_block_without_verification() (for the duplicate check)
• once again inside on_block_core() at line 443

// on_block(), line 369
let block_root = block.tree_hash_root();   // first hash

// on_block_core(), line 443
let block_root = block.tree_hash_root();   // second hash — redundant

Passing block_root into on_block_core as a parameter would eliminate this. This is a regression introduced by the refactoring (the old single-function code only hashed once).

---

3. Dead code: on_gossip_attestation_without_verification has no callers

on_gossip_attestation_without_verification is pub and defined but never called anywhere in the codebase — not in any test file, not in lib.rs. The fork choice spec tests only call on_block_without_verification. If no test currently exercises attestation processing without signatures, the function should either be removed until it is needed, or a test that uses it should be added alongside it.

---

4. Behavioural divergence: on_gossip_attestation_without_verification silently skips the validator bounds check

on_gossip_attestation (the verified variant) includes an explicit bounds check and pubkey decoding before attempting signature verification:

// on_gossip_attestation, lines 210–215
if validator_id >= target_state.validators.len() as u64 {
    return Err(StoreError::InvalidValidatorIndex);
}
let validator_pubkey = target_state.validators[validator_id as usize]
    .get_pubkey()
    .map_err(|_| StoreError::PubkeyDecodingFailed(validator_id))?;

on_gossip_attestation_without_verification omits this entirely and goes directly to on_attestation. If the function is ever used in a test with invalid validator indices the error will come from a different location (or potentially panic) rather than from InvalidValidatorIndex. This makes the unverified variant behave differently beyond just skipping crypto, which could hide bugs in tests. The bounds check is cheap and not signature-related — it should be present in both variants.

---

Minor observations

• The collapsible_if fixes in forkchoice_spectests.rs (nested if let Some → if let Some(...) && ...) are correct and clean.
• The double validate_attestation call (once in on_gossip_attestation/on_gossip_attestation_without_verification and once inside on_attestation) is pre-existing and not introduced here, but it is now more visible — worth a follow-up TODO.
• slot in on_block_core (line 444) is used only for the log line at line 514 and the attestation warning at line 494, which is fine.
• Documentation (CLAUDE.md, RELEASE.md, Makefile comments) is clean and accurate.

---

Automated review by Claude (Anthropic) · custom prompt

[greptile-apps]

Greptile Summary

This PR successfully eliminates the skip-signature-verification compile-time feature flag by splitting on_block() and on_gossip_attestation() into verified and unverified variants. The approach is sound: verification is the default, and opting out requires explicitly calling functions with _without_verification suffixes.

Key improvements:

• Security-first design: on_block() and on_gossip_attestation() always verify signatures by default
• Explicit opt-out: *_without_verification() variants make the risk obvious in the function name
• Performance optimization: on_block_core() takes parent_state by value, eliminating one clone operation
• Complete cleanup: All feature flag references removed from code, tests, and documentation

Code organization:

• Production code (lib.rs) uses verified variants exclusively
• Fork choice spec tests use unverified variants (appropriate since test fixtures lack valid signatures)
• Signature spec tests use verified variants (appropriate for testing signature verification)

Changes include:

• Split functions in store.rs with shared on_block_core() logic
• Updated forkchoice_spectests.rs to use on_block_without_verification() and fixed clippy warnings
• Removed feature flag from Cargo.toml
• Updated documentation in CLAUDE.md, RELEASE.md, and Makefile

Confidence Score: 4/5

• This PR is safe to merge with one verification needed regarding signature storage timing.
• The refactoring is well-structured and successfully achieves its goal of eliminating the feature flag while maintaining security. The default-safe approach (always verify unless explicitly opting out) is the correct design choice. Documentation is thorough and complete. However, there's one behavior change where proposer signatures are now stored earlier in the execution flow, which could potentially cause consistency issues if block processing fails. This warrants verification before merge.
• Pay close attention to crates/blockchain/src/store.rs - verify that early signature storage (before block processing) doesn't cause issues.

Important Files Changed

Filename	Overview
crates/blockchain/src/store.rs	Successfully splits on_block() and on_gossip_attestation() into verified/unverified variants with shared logic, eliminating feature flags. Minor timing change in signature storage warrants verification.
crates/blockchain/tests/forkchoice_spectests.rs	Correctly updated to use on_block_without_verification() and fixed pre-existing clippy warnings for collapsible if statements using modern Rust syntax.
crates/blockchain/Cargo.toml	Cleanly removes skip-signature-verification feature flag and associated test requirements. No issues found.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Block Received] --> B{Production or Test?}
    B -->|Production| C[on_block]
    B -->|Fork Choice Tests| D[on_block_without_verification]
    
    C --> E[Check for Duplicate]
    D --> F[Check for Duplicate]
    
    E --> G[Get Parent State]
    F --> H[Get Parent State]
    
    G --> I[verify_signatures]
    I --> J[Store Proposer Signature]
    J --> K[on_block_core]
    
    H --> K
    
    K --> L[State Transition]
    L --> M[Store Block & State]
    M --> N[Process Attestations]
    N --> O[Update Fork Choice Head]
    O --> P[Process Proposer Attestation]
    P --> Q[Complete]
    
    style C fill:#90EE90
    style D fill:#FFB6C1
    style I fill:#87CEEB
    style J fill:#FFD700
    style K fill:#DDA0DD

Loading

_{Last reviewed commit: aff6594}

[greptile-apps]

_{6 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[MegaRedHand]

LGTM