test(spec): harden fixture parsing and assert new fork-choice checks (leanSpec #1189)

[MegaRedHand]

Why

The spec-test fixture types silently dropped any JSON field they did not model. When leanSpec adds a new fixture check, ethlambda kept passing while verifying nothing new. That masked drift is exactly how PR #1189's canonicalEquivocationHeadAmong (and a dozen other already-shipped fields) landed in the 07-11 fixtures without ethlambda asserting any of them.

Stacked on #510 (branch fix/attestation-reject-head-off-finalized, the HEAD_NOT_DESCENDANT_OF_FINALIZED fix that makes all fork-choice fixtures pass). This PR targets that branch and will be retargeted to main once #510 merges.

What

1. deny_unknown_fields hardening (fail loudly on drift)

Added #[serde(deny_unknown_fields)] to every fixture-facing struct and modeled every field the current (07-11) fixtures actually carry, so a future upstream schema addition fails at parse time instead of being silently ignored.

Structs that got deny_unknown_fields:

• common.rs: Container<T>, Config, Checkpoint, BlockHeader, Validator, TestState, Block, BlockBody, AggregatedAttestation, AggregationBits, AttestationData, TestInfo.
• fork_choice.rs: ForkChoiceTest, ForkChoiceStep, AttestationStepData, ProofStepData, HexByteList, BlockStepData, StoreChecks, AttestationCheck, plus new BlockAttestationCheck, StoreSnapshot, BlockWeightEntry, AggregatedPayloadEntry, AttestationSignatureEntry.
• verify_signatures.rs: VerifySignaturesTest, TestSignedBlock, MergedProof, HexBytes.
• state_transition/tests/types.rs: StateTransitionTest (PostState already had it).

The flatten wrapper vectors (ForkChoiceTestVector, etc.) are intentionally left without the attribute (serde forbids deny_unknown_fields + flatten). The Hive driver's own request wrappers (StateTransitionRunRequest/InitForkChoiceRequest/VerifySignaturesRequest) are also left without it, so a stray top-level field (e.g. _info) is still tolerated. Note, however, that these wrappers embed the shared fixture types (TestState, Block, TestSignedBlock, ForkChoiceStep, …), which this PR does harden: any unknown field nested inside anchorState/anchorBlock/pre/blocks/signedBlock (or a fork-choice step body) now fails deserialization at the Axum handler rather than being silently dropped. This is intentional: silently dropping a replay-relevant field would make the driver report misleading results, so failing loudly on simulator-side schema drift is the safer boundary for a conformance driver.

Previously-dropped fields now modeled include: _info.keySetDigest, top-level proofSetting / rejectionReason / postStateRoot, per-step storeSnapshot / rejectionReason, attestationChecks[].sourceRootLabel, and the full new StoreChecks batch.

The fixture crate is a production dependency of the RPC Hive test-driver, so this also hardens the binary's parsing of simulator-delivered JSON. The driver returns a store snapshot and applies no StoreChecks itself (the leanSpec simulator does the comparisons), so no driver logic changed.

2. Newly asserted checks in the fork-choice runner

Check	Notes
canonicalEquivocationHeadAmong	leanSpec #1189. Head must sit on the fork whose targeting attestation in the accepted (known) pool carries the largest hash_tree_root. Scheme-independent; mirrors lexicographicHeadAmong. Covers the 3 equivocation fixtures.
latestKnownAggregatedTargetSlots	Sorted-unique target slots of the known aggregated pool.
attestationTargetRootLabel	Attestation target root resolves to the labeled block.
attestationChecks[].sourceRootLabel	Per-validator source checkpoint root by label.
labelsInStore	Labeled blocks still present in the block tree.
reorgDepth	ancestors(old_head) \ ancestors(new_head).
blockAttestations / blockAttestationCount	Per-aggregate participant/slot checks + count on the block imported this step.

Also seeds the block registry with the anchor under the label "genesis", matching leanSpec's harness, so label-based checks resolve (true even for checkpoint-sync anchors past slot 0).

3. Parsed but not asserted (each carries a TODO)

latestNewAggregatedTargetSlots, attestationSignatureTargetSlots, newPoolProofParticipants, storeSnapshot, filledBlockRootLabel.

These read the pending (new) aggregated pool, the raw gossip-signature pool, or the identity of a freshly-built block. The offline runner advances ticks without the aggregator role and folds block-borne votes straight into the known pool, so those pools/built-block do not track leanSpec's harness. The accepted (known) pool — which the runner does populate — is asserted. storeSnapshot is a whole-store snapshot (692 steps) that would need substantial new Store plumbing to compare.

Verification

• cargo test --release -p ethlambda-blockchain --test forkchoice_spectests → 122 passed
• cargo test --release -p ethlambda-state-transition --test stf_spectests → 74 passed
• cargo test --release -p ethlambda-blockchain --test signature_spectests → 3 passed
• make test → fully green
• make fmt / make lint (clippy -D warnings) clean; whole workspace (incl. Hive driver binary) builds.
• Confirmed the canonicalEquivocationHeadAmong assertion executes and is meaningful: inverting the winner selection (max→min) fails exactly the 3 equivocation fixtures; poisoning the computed reorgDepth/blockAttestationCount actuals fails exactly the fixtures carrying those checks.

[github-actions]

🤖 Kimi Code Review

The PR is a high-quality update to the consensus test harness that improves schema strictness and adds comprehensive fork-choice validation. No critical issues found.

Correctness & Consensus Safety

1. Ancestor Set Calculation (forkchoice_spectests.rs:433-448): The ancestor_set function correctly walks parent links until reaching a root not present in the block tree (the anchor). Termination is guaranteed because the anchor's parent is H256::ZERO, which is never inserted into the store's block map. This correctly mirrors the leanSpec _ancestor_set helper.

2. Equivocation Tie-Breaking (forkchoice_spectests.rs:513-588): The validate_canonical_equivocation_head_among function correctly implements the lexicographic tie-break rule (leanSpec #1189). It selects the fork with the maximum attestation_data_root among those targeted by the accepted pool. Since these are cryptographic hashes, using max_by_key is safe and deterministic.

3. Reorg Depth (forkchoice_spectests.rs:396-408): The set difference old_ancestors.difference(&new_ancestors) correctly counts blocks that were reachable from the previous head but are no longer reachable from the new head, matching the standard definition of reorg depth.

4. Block Body Validation (forkchoice_spectests.rs:453-510): validate_block_attestations correctly uses BTreeSet<u64> for participant comparison, ensuring validator index order does not affect matching. The logic correctly pairs expected checks with actual aggregates by participant set equality.

Rust Best Practices

1. Schema Evolution Safety: The addition of #[serde(deny_unknown_fields)] to test fixture structs (e.g., types.rs:26, common.rs:24, fork_choice.rs:36) is excellent defensive programming. It ensures the test runner fails fast on fixture schema changes rather than silently ignoring new fields.

2. Explicit Dead Code Allowance: New fields captured solely for forward-compatibility (e.g., post_state_root, rejection_reason, proof_setting) are appropriately marked with #[allow(dead_code)] and documented. This clarifies intent and prevents lint noise.

3. Error Context: The resolve_label helper (forkchoice_spectests.rs:420-431) provides rich error messages including the available registry keys, which will significantly speed up debugging when fixtures reference missing labels.

Minor Suggestions

1. Bijective Check (validate_block_attestations): Currently, the function checks that every expected aggregate exists in the block. Consider also verifying that the number of expected checks equals block.body.attestations.len() to ensure no unexpected aggregates are present. This can be added if the spec tests require it.

2. Memory Allocation (ancestor_set): The function allocates a new HashSet for every invocation. While acceptable for test code, if this were ever moved to production hot paths, consider passing a pre-allocated buffer or using an iterator-based approach. No change required for tests.

Security Notes

• Determinism: The use of BTreeSet for sorting target slots (forkchoice_spectests.rs:365) and participant sets ensures deterministic validation across platforms, which is critical for consensus test reproducibility.
• Label Resolution: The explicit registry lookup with resolve_label prevents silent misconfiguration where a typo in a fixture label might otherwise resolve to a default value.

Summary

The code is idiomatic, well-documented, and correctly implements the consensus validation logic. The addition of deny_unknown_fields significantly improves maintainability. Approved for merge.

---

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

[github-actions]

🤖 Codex Code Review

1. reorgDepth is computed from the post-step LiveChain, so it can undercount or drop to zero whenever the step prunes the old branch during finalization. In forkchoice_spectests.rs, old_head is captured before the step, but its ancestor set is rebuilt afterward from st.get_live_chain(). That API explicitly returns only non-finalized blocks (store.rs), so once the old branch is pruned it disappears from the data used for the calculation. Snapshot the old ancestor set before executing the step, or query block ancestry from persistent block headers instead of LiveChain.

2. canonicalEquivocationHeadAmong is not checking the same rule the store actually uses for canonical same-slot votes. In forkchoice_spectests.rs, the winner is chosen as “largest attestation-data root targeting each fork”. Fork choice weight, however, is derived from extract_latest_known_attestations(), which resolves votes per validator by descending (slot, data_root) (store.rs). A high-root attestation on fork A contributes no weight if that validator has an even higher-root vote on fork B, but this checker still treats fork A as competitive. That can make the spectest fail even when fork choice is correct, or pass while masking a real regression. The check should derive the winner from the extracted per-validator latest attestations, not from known_aggregated_payloads() maxima.

3. validate_block_attestations matches expected entries by participant set only and reuses the first match for every check. In forkchoice_spectests.rs, .find(...) does not consume the matched aggregate, so two expected checks with the same participants but different attestationSlot/targetSlot cannot be distinguished. That matters exactly in equivocation-style cases, where the same validators may appear in multiple aggregates with different data. Matching should either include the slot fields in the lookup key or remove each matched aggregate once used.

No production-path security issues stand out in this PR; the risk is mostly incorrect spec-test assertions reducing confidence in fork-choice coverage.

I couldn’t run the affected cargo tests here because the environment blocks writes to the default Rust/Cargo cache locations under /home/runner, so compile verification wasn’t possible in this session.

---

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

[greptile-apps]

Greptile Summary

This PR hardens fixture parsing and adds new fork-choice fixture checks.

• Adds deny_unknown_fields to fixture-facing structs.
• Models newly emitted leanSpec fixture fields.
• Adds fork-choice checks for labels, aggregate pools, reorg depth, and block attestations.
• Seeds the block registry with the genesis label.

Confidence Score: 4/5

The fork-choice test runner has a contained matching bug in the new block-attestation check.

• One actual aggregate can satisfy multiple expected blockAttestations entries with the same participant set.
• The rest of the changed parsing and added checks are scoped to fixture validation.

crates/blockchain/tests/forkchoice_spectests.rs

Important Files Changed

Filename	Overview
crates/blockchain/state_transition/tests/types.rs	Adds strict parsing and optional metadata fields for state-transition fixtures.
crates/common/test-fixtures/src/common.rs	Adds strict parsing to shared fixture structs and models _info.keySetDigest.
crates/common/test-fixtures/src/fork_choice.rs	Adds strict fork-choice fixture schemas and new store/check fields.
crates/common/test-fixtures/src/verify_signatures.rs	Adds strict parsing and models proofSetting for verify-signatures fixtures.
crates/blockchain/tests/forkchoice_spectests.rs	Adds new fork-choice assertions; block-attestation matching can reuse one actual aggregate for repeated participant sets.

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/tests/forkchoice_spectests.rs:610-612
**Repeated Aggregate Match Reuse**

When two expected `blockAttestations` entries have the same participant set, this lookup can match both expectations against the first actual aggregate. A fixture that is trying to check distinct aggregates for the same participants can then validate the wrong slot/target data or let one aggregate satisfy multiple expected entries.

_{Reviews (1): Last reviewed commit: "test(forkchoice): propagate Result from ..." | Re-trigger Greptile}

[github-actions]

🤖 Claude Code Review

Review: PR 511 — harden fixture parsing, assert new fork-choice checks

This PR is test/fixture-only in terms of source it touches, but one of those "test" crates is a stated production dependency of the RPC Hive test-driver, which is where the one substantive issue lives.

1. deny_unknown_fields hardening leaks into the production Hive test-driver, contradicting the PR's own claim (High)

The PR description says:

The Hive driver's StateTransitionRunRequest/InitForkChoiceRequest/VerifySignaturesRequest are also left as-is: they deliberately extract a subset from the full fixture body delivered over HTTP.

That's true only at the top level of those request structs. Look at what they actually contain:

• crates/net/rpc/src/test_driver.rs:126-141 — InitForkChoiceRequest { anchor_state: TestState, anchor_block: FixtureBlock, .. } and VerifySignaturesRequest { anchor_state: TestState, signed_block: TestSignedBlock }.
• crates/common/test-fixtures/src/state_transition.rs:19-24 — StateTransitionRunRequest { pre: TestState, blocks: Vec<Block>, .. }, whose own doc comment states "Extra fields ... are ignored by serde."

TestState, Block/BlockBody/AggregatedAttestation/AggregationBits/AttestationData/Checkpoint/Validator/Config (common.rs), and TestSignedBlock/MergedProof/HexBytes (verify_signatures.rs) are exactly the types this PR just annotated with #[serde(deny_unknown_fields)]. The wrapper request structs themselves don't have the attribute (so a stray top-level field like _info is still tolerated, matching the doc comment), but any unknown field nested inside anchor_state, anchor_block, pre, blocks, or signed_block will now hard-fail deserialization at the Axum handler.

Previously this was a robust boundary: the driver silently dropped anything it didn't model from the simulator's JSON. After this PR, the moment leanSpec's schema gains a field the driver's shared types don't model — even one irrelevant to STF/fork-choice replay — every Hive-driven request that includes it 400s instead of degrading gracefully. That's the exact failure mode the PR is trying to introduce for local fixture tests (fail loudly on drift so it's caught in review), but here it fires against a live simulator in CI/interop testing rather than in a local cargo test run, which is a materially worse place for it to surface.

Given the stated goal (harden test parsing without touching the driver), the fix is to give the driver its own non-deny_unknown_fields copies of TestState/Block/TestSignedBlock (or keep those specific nested types field-permissive while still hardening the ones only reachable from cargo test fixture files), rather than sharing the exact same types between the strict fixture-file path and the permissive HTTP-request path.

2. Minor: TestInfo::key_set_digest is a required String, not Option<String>

crates/common/test-fixtures/src/common.rs:307-561 (via the diff) adds key_set_digest as a plain String field on the fixture-wide TestInfo. Since TestInfo::info is embedded in ForkChoiceTest, StateTransitionTest, and VerifySignaturesTest, any fixture generated before this field existed (an older cached fixtures checkout, or a partial/custom fixture) will now fail to parse entirely rather than just missing an assertion. This is likely fine given the stated 07-11 fixture baseline, but making it Option<String> would have been more consistent with how every other "captured but not load-bearing" field in this PR was modeled (post_state_root, rejection_reason, proof_setting, etc., are all Option).

Things that look solid

• reorgDepth/ancestor_set, canonicalEquivocationHeadAmong, and blockAttestations/blockAttestationCount faithfully mirror the described leanSpec semantics, and the walk in ancestor_set (crates/blockchain/tests/forkchoice_spectests.rs:274-285) correctly terminates at the anchor without needing it to be present in the live-chain map.
• Seeding block_registry["genesis"] = anchor_root up front is a nice correctness fix — without it, label-based checks (sourceRootLabel, labelsInStore) would spuriously fail on any fixture referencing the anchor by label.
• The unasserted fields (latestNewAggregatedTargetSlots, storeSnapshot, etc.) are honestly captured with clear TODOs explaining why the offline runner can't assert them yet, rather than silently dropped or fake-asserted.
• Good use of ? propagation for the Store::head()/get_live_chain() Result change from the stacked PR (the last commit here is exactly that fixup, and it's minimal and correct).

---

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