test: adapt fork-choice harness to refreshed fixture format#303
test: adapt fork-choice harness to refreshed fixture format#303MegaRedHand wants to merge 1 commit intopr-ssz-htr-xmssfrom
Conversation
🤖 Kimi Code ReviewReview of PR #303: Refactor attestation and block processing to separate verification from core logic Overall Assessment: Good refactoring that properly separates cryptographic verification from state transition logic. Improves testability while maintaining security boundaries. One performance issue identified. 1. Performance: Redundant state lookup in attestation processingFile:
Suggestion: Pass the validators or the state to the core function to avoid the redundant store lookup: // In on_gossip_aggregated_attestation:
let target_state = store.get_state(...)?;
// ... verify signatures ...
on_gossip_aggregated_attestation_core(store, aggregated, &target_state)?;
// In on_gossip_aggregated_attestation_core:
pub fn on_gossip_aggregated_attestation_core(
store: &mut Store,
aggregated: SignedAggregatedAttestation,
target_state: &BeaconState, // Add this parameter
) -> Result<(), StoreError> {
// Remove the second get_state call
let num_validators = target_state.validators.len() as u64;
// ...
}2. Code correctness: Duplicate block handlingFile: The early return for duplicate blocks uses // Assumes post-block states are stored immediately after processing
if store.has_state(&block_root) {
return Ok(());
}3. Security: Validation orderingFile: The refactor moves 4. Test code safetyFile:
#[cfg(test)]
pub fn on_gossip_attestation_without_verification(...)Or if used by integration tests in 5. Positive observations
Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Claude Code ReviewNow I have enough context for a thorough review. Here are my findings: PR #303 — Adapt fork-choice harness to refreshed fixture formatOverviewThis PR adapts the fork-choice spec-test harness to consume the updated CorrectnessTick time semantics are consistent. Validation ordering change in The expensive XMSS crypto now runs before the cheap slot/checkpoint validation. This is a minor performance regression: malformed attestations (wrong source/target, future slot, unknown block roots) will burn XMSS verification cycles before being rejected. Consider moving Double
Security / API Surface
Making Consider Rust / Style
(None, None) => panic!("tick step missing both time and interval"),The rest of the harness uses (None, None) => return Err(format!("Step {step_idx}: tick step missing both time and interval").into()),
Minor: Test CoverageThe 70/70 pass rate is the main signal. The new The SummaryThe core logic is sound and the fixture compatibility fixes are correct. Three items worth addressing before merge:
The Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewFindings
I did not run the test suite here: the sandbox blocks Cargo from writing its rustup/cargo state and fetching dependencies. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Greptile SummaryAdapts the fork-choice spec-test harness to consume the refreshed leanSpec Confidence Score: 5/5Safe to merge; all findings are P2 style/efficiency suggestions with no correctness impact on the fork-choice logic or test results. All four comments are P2 (stale comment, redundant state lookup, validation-order change with no wrong-outcome risk, missing log field). The test changes correctly model the new fixture schema and pass 70/70 fork-choice spec tests. The production refactoring is logically sound. crates/blockchain/src/store.rs — redundant target_state fetch and reordered validate_attestation_data in the on_gossip_aggregated_attestation production path.
|
| Filename | Overview |
|---|---|
| crates/blockchain/src/store.rs | Refactors on_block and on_gossip_aggregated_attestation into public core helpers, makes on_block_core pub, and extracts validate_attestation_data into on_gossip_aggregated_attestation_core — introducing a redundant state lookup and a reordered validation step in the production path. |
| crates/blockchain/tests/forkchoice_spectests.rs | Adapts the test harness to handle interval/time tick fields, hasProposal, gossipAggregatedAttestation with proof.participants, and validates the time store check; the TODO comment is now partially stale. |
| crates/blockchain/tests/types.rs | Adds new fields (interval, has_proposal, proof) to ForkChoiceStep and AttestationStepData, and promotes the time field in StoreChecks from unsupported to validated; clean, no issues. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["on_gossip_aggregated_attestation (production)"] --> B["fetch target_state\n(for pubkeys + bounds check)"]
B --> C["bounds-check participants"]
C --> D["collect pubkeys\nverify XMSS signature"]
D --> E["on_gossip_aggregated_attestation_core"]
E --> F["validate_attestation_data\n⚠ after sig verification"]
F --> G["fetch target_state AGAIN\n⚠ redundant lookup"]
G --> H["bounds-check AGAIN\n⚠ redundant"]
H --> I["insert_new_aggregated_payload"]
J["Tests / spec-test harness"] --> E
K["on_block (production)"] --> L["duplicate-block check\nfetch parent_state\nverify_signatures"]
L --> M["on_block_core (now pub)"]
M --> N["state_transition\ninsert block+state\nupdate_head"]
O["on_block_core (test harness)"] --> M
Comments Outside Diff (2)
-
crates/blockchain/tests/forkchoice_spectests.rs, line 24-34 (link)Stale TODO comment after gossipAggregatedAttestation fix
The block comment above
SKIP_TESTSstill says "aggregated attestations withvalidator_id=0into known (should useproof.participantsinto new). TODO: fix these" — but this PR fixes exactly that, routing throughon_gossip_aggregated_attestation_corewith the fixture'sproof.participantsbitfield. ThegossipAggregatedAttestationclause of the TODO is now resolved; only theattestation-step part (individual attestations into known) remains relevant.``markdown
This is a comment left during a code review.
Path: crates/blockchain/tests/forkchoice_spectests.rs
Line: 24-34Comment:
Stale TODO comment after gossipAggregatedAttestation fixThe block comment above
SKIP_TESTSstill says "aggregated attestations withvalidator_id=0into known (should useproof.participantsinto new). TODO: fix these" — but this PR fixes exactly that, routing throughon_gossip_aggregated_attestation_corewith the fixture'sproof.participantsbitfield. ThegossipAggregatedAttestationclause of the TODO is now resolved; only theattestation-step part (individual attestations into known) remains relevant.``
crates/blockchain/src/store.rs, line 854-864 (link)
sig_verification timing removed from the "Processed new block" log
Signature verification timing (?sig_verification) was dropped from the on_block_core log event when the verification step moved into on_block. The timing is now computed in on_block but never emitted, making it impossible to profile crypto overhead via logs.
Consider capturing and logging it in on_block alongside the block-level summary, or passing it into on_block_core as an Option<Duration> so the existing log field can be restored.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 854-864
Comment:
**`sig_verification` timing removed from the "Processed new block" log**
Signature verification timing (`?sig_verification`) was dropped from the `on_block_core` log event when the verification step moved into `on_block`. The timing is now computed in `on_block` but never emitted, making it impossible to profile crypto overhead via logs.
Consider capturing and logging it in `on_block` alongside the block-level summary, or passing it into `on_block_core` as an `Option<Duration>` so the existing log field can be restored.
How can I resolve this? If you propose a fix, please make it concise.Prompt To Fix All With AI
This is a comment left during a code review.
Path: crates/blockchain/tests/forkchoice_spectests.rs
Line: 24-34
Comment:
**Stale TODO comment after gossipAggregatedAttestation fix**
The block comment above `SKIP_TESTS` still says "aggregated attestations with `validator_id=0` into known (should use `proof.participants` into new). TODO: fix these" — but this PR fixes exactly that, routing through `on_gossip_aggregated_attestation_core` with the fixture's `proof.participants` bitfield. The `gossipAggregatedAttestation` clause of the TODO is now resolved; only the `attestation`-step part (individual attestations into known) remains relevant.
```suggestion
// We don't check signatures in spec-tests, so invalid-signature tests always pass.
// Individual gossip attestation steps bypass the new→known promotion pipeline
// (on_gossip_attestation_without_verification inserts directly into known payloads).
// TODO: route attestation steps through the real promotion pipeline.
const SKIP_TESTS: &[&str] = &[
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 735-757
Comment:
**Redundant state lookup and bounds check on the production call path**
When called from `on_gossip_aggregated_attestation`, `on_gossip_aggregated_attestation_core` re-fetches `target_state` (~line 742) and re-runs the participant bounds check (~lines 746-751) even though the outer function already did both at lines 613-621. Because `get_state` returns an owned `State`, this is two independent deserialization/copy operations for the same root.
The check is still needed inside core for test-only callers, but the double work on the production path could be avoided by passing the pre-validated state through an internal helper.
This is a P2 suggestion — no correctness impact, but worth a follow-up to avoid the extra state fetch on every gossiped aggregation.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 608-666
Comment:
**`validate_attestation_data` now runs after signature verification**
The original `on_gossip_aggregated_attestation` ran `validate_attestation_data` (a cheap, purely local check) *before* the expensive XMSS signature verification. After this refactor, validation moves inside `on_gossip_aggregated_attestation_core` and executes only *after* the full signature check passes.
Two consequences: an attestation with invalid data and an invalid signature now consumes the full signature-verification budget before being rejected, and `inc_attestations_invalid` is never incremented for it. A comment explaining the deliberate ordering would help future readers.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 854-864
Comment:
**`sig_verification` timing removed from the "Processed new block" log**
Signature verification timing (`?sig_verification`) was dropped from the `on_block_core` log event when the verification step moved into `on_block`. The timing is now computed in `on_block` but never emitted, making it impossible to profile crypto overhead via logs.
Consider capturing and logging it in `on_block` alongside the block-level summary, or passing it into `on_block_core` as an `Option<Duration>` so the existing log field can be restored.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "test: adapt fork-choice harness to refre..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn on_gossip_aggregated_attestation_core( | ||
| store: &mut Store, | ||
| signed_block: SignedBlock, | ||
| verify: bool, | ||
| aggregated: SignedAggregatedAttestation, | ||
| ) -> Result<(), StoreError> { | ||
| validate_attestation_data(store, &aggregated.data) | ||
| .inspect_err(|_| metrics::inc_attestations_invalid())?; | ||
|
|
||
| let target_state = store | ||
| .get_state(&aggregated.data.target.root) | ||
| .ok_or(StoreError::MissingTargetState(aggregated.data.target.root))?; | ||
| let num_validators = target_state.validators.len() as u64; | ||
| if aggregated | ||
| .proof | ||
| .participant_indices() | ||
| .any(|vid| vid >= num_validators) | ||
| { | ||
| return Err(StoreError::InvalidValidatorIndex); | ||
| } | ||
|
|
||
| let hashed = HashedAttestationData::new(aggregated.data.clone()); | ||
| store.insert_new_aggregated_payload(hashed, aggregated.proof); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
When called from on_gossip_aggregated_attestation, on_gossip_aggregated_attestation_core re-fetches target_state (~line 742) and re-runs the participant bounds check (~lines 746-751) even though the outer function already did both at lines 613-621. Because get_state returns an owned State, this is two independent deserialization/copy operations for the same root.
The check is still needed inside core for test-only callers, but the double work on the production path could be avoided by passing the pre-validated state through an internal helper.
This is a P2 suggestion — no correctness impact, but worth a follow-up to avoid the extra state fetch on every gossiped aggregation.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 735-757
Comment:
**Redundant state lookup and bounds check on the production call path**
When called from `on_gossip_aggregated_attestation`, `on_gossip_aggregated_attestation_core` re-fetches `target_state` (~line 742) and re-runs the participant bounds check (~lines 746-751) even though the outer function already did both at lines 613-621. Because `get_state` returns an owned `State`, this is two independent deserialization/copy operations for the same root.
The check is still needed inside core for test-only callers, but the double work on the production path could be avoided by passing the pre-validated state through an internal helper.
This is a P2 suggestion — no correctness impact, but worth a follow-up to avoid the extra state fetch on every gossiped aggregation.
How can I resolve this? If you propose a fix, please make it concise.| pub fn on_gossip_aggregated_attestation( | ||
| store: &mut Store, | ||
| aggregated: SignedAggregatedAttestation, | ||
| ) -> Result<(), StoreError> { | ||
| validate_attestation_data(store, &aggregated.data) | ||
| .inspect_err(|_| metrics::inc_attestations_invalid())?; | ||
|
|
||
| // Verify aggregated proof signature | ||
| // Resolve target state and validator pubkeys needed for signature verification. | ||
| let target_state = store | ||
| .get_state(&aggregated.data.target.root) | ||
| .ok_or(StoreError::MissingTargetState(aggregated.data.target.root))?; | ||
| let validators = &target_state.validators; | ||
| let num_validators = validators.len() as u64; | ||
|
|
||
| let participant_indices: Vec<u64> = aggregated.proof.participant_indices().collect(); | ||
| if participant_indices.iter().any(|&vid| vid >= num_validators) { | ||
| return Err(StoreError::InvalidValidatorIndex); | ||
| } | ||
|
|
||
| let pubkeys: Vec<_> = participant_indices | ||
| .iter() | ||
| .map(|&vid| { | ||
| validators[vid as usize] | ||
| .get_attestation_pubkey() | ||
| .map_err(|_| StoreError::PubkeyDecodingFailed(vid)) | ||
| }) | ||
| .collect::<Result<_, _>>()?; | ||
|
|
||
| let hashed = HashedAttestationData::new(aggregated.data.clone()); | ||
| let data_root = hashed.root(); | ||
| let data_root = aggregated.data.hash_tree_root(); | ||
| let slot: u32 = aggregated.data.slot.try_into().expect("slot exceeds u32"); | ||
|
|
||
| { | ||
| let _timing = metrics::time_pq_sig_aggregated_signatures_verification(); | ||
| ethlambda_crypto::verify_aggregated_signature( | ||
| &aggregated.proof.proof_data, | ||
| pubkeys, | ||
| &data_root, | ||
| slot, | ||
| ) | ||
| } | ||
| .map_err(StoreError::AggregateVerificationFailed)?; | ||
|
|
||
| // Store one proof per attestation data (not per validator) | ||
| store.insert_new_aggregated_payload(hashed, aggregated.proof.clone()); | ||
| let num_participants = aggregated.proof.participants.count_ones(); | ||
| metrics::update_latest_new_aggregated_payloads(store.new_aggregated_payloads_count()); | ||
|
|
||
| let slot = aggregated.data.slot; | ||
| let target_slot = aggregated.data.target.slot; | ||
| let target_root = aggregated.data.target.root; | ||
| let source_slot = aggregated.data.source.slot; | ||
|
|
||
| on_gossip_aggregated_attestation_core(store, aggregated)?; | ||
|
|
||
| metrics::update_latest_new_aggregated_payloads(store.new_aggregated_payloads_count()); | ||
| info!( | ||
| slot, | ||
| num_participants, | ||
| target_slot = aggregated.data.target.slot, | ||
| target_root = %ShortRoot(&aggregated.data.target.root.0), | ||
| source_slot = aggregated.data.source.slot, | ||
| target_slot, | ||
| target_root = %ShortRoot(&target_root.0), | ||
| source_slot, | ||
| "Aggregated attestation processed" | ||
| ); | ||
|
|
||
| metrics::inc_attestations_valid(1); | ||
|
|
||
| Ok(()) |
There was a problem hiding this comment.
validate_attestation_data now runs after signature verification
The original on_gossip_aggregated_attestation ran validate_attestation_data (a cheap, purely local check) before the expensive XMSS signature verification. After this refactor, validation moves inside on_gossip_aggregated_attestation_core and executes only after the full signature check passes.
Two consequences: an attestation with invalid data and an invalid signature now consumes the full signature-verification budget before being rejected, and inc_attestations_invalid is never incremented for it. A comment explaining the deliberate ordering would help future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/src/store.rs
Line: 608-666
Comment:
**`validate_attestation_data` now runs after signature verification**
The original `on_gossip_aggregated_attestation` ran `validate_attestation_data` (a cheap, purely local check) *before* the expensive XMSS signature verification. After this refactor, validation moves inside `on_gossip_aggregated_attestation_core` and executes only *after* the full signature check passes.
Two consequences: an attestation with invalid data and an invalid signature now consumes the full signature-verification budget before being rejected, and `inc_attestations_invalid` is never incremented for it. A comment explaining the deliberate ordering would help future readers.
How can I resolve this? If you propose a fix, please make it concise.
MegaRedHand
force-pushed
the
pr-fixture-schema
branch
from
ae8ca9c to
263a0b2
Compare
The leanSpec bump to `e9ddede` regenerated fork-choice fixtures with a new schema that breaks the existing test runner: 1. `tick` steps now sometimes use `interval` (absolute intervals since genesis) instead of `time` (UNIX seconds). Derive the millisecond timestamp from whichever field is present; accept `hasProposal`. 2. Tick `checks` carry a `time` field (expected `Store::time()` in intervals since genesis). Validate it instead of erroring out. Also adds three fixtures to `SKIP_TESTS` (`test_tick_interval_0_skips_acceptance_when_not_proposer`, `test_tick_interval_progression_through_full_slot`, `test_safe_target_uses_merged_pools_at_interval_3`). They contain `gossipAggregatedAttestation` steps whose attestation checks require routing the proof's participants bitfield into the `new` aggregated payload buffer. The follow-up PR wires the real verifying entry point through and unblocks all three. Fixes `test_on_tick_advances_across_multiple_empty_slots`.
MegaRedHand
force-pushed
the
pr-ssz-htr-xmss
branch
from
960b14d to
71c4a73
Compare
MegaRedHand
force-pushed
the
pr-fixture-schema
branch
from
263a0b2 to
238daf9
Compare
1 participant
Summary
Updates the fork-choice spec-test runner to consume the new fixture schema shipped by leanSpec
e9ddede:intervalortime.intervalencodes the absolute interval count since genesis;timeis UNIX seconds. Derive the millisecond timestamp from whichever field is present and honorhasProposal.checkscarry atimefield — the expectedStore::time()value in intervals since genesis. Validate it instead of erroring out as "unsupported".Skipped for now
The new fixtures also add a
proof.participantsbitfield togossipAggregatedAttestationsteps, which changes the expected behavior of those step handlers. Rather than add another bypass helper here, this PR leaves the existing single-validator shortcut in place and adds three fixtures toSKIP_TESTS:test_tick_interval_0_skips_acceptance_when_not_proposertest_tick_interval_progression_through_full_slottest_safe_target_uses_merged_pools_at_interval_3The follow-up #304 swaps the bypass helpers for real verifying entry points and unblocks these three.
Fixes
test_on_tick_advances_across_multiple_empty_slots(was failing with'time' check not supported).Stack
Stacked on #302. Merge after it.
Test plan
cargo test -p ethlambda-blockchain --release --test forkchoice_spectestspasses (67/70 plus 3 skipped)cargo test --workspace --releasepasses (314 passed, 6 ignored)