test(spec): assert the rejection reason on expected-failure fixtures - #547
Conversation
Negative leanSpec fixtures name *why* their input must be rejected in a `rejectionReason` field, but the runners only asserted that some error came back. A fixture could therefore pass on a failure unrelated to the rule it exercises, which is what four state-transition fixtures were doing: a late state-root mismatch stood in for the attestation-data cap, the justification window and the block-slot check. Mirror leanSpec's `RejectionReason` vocabulary as a typed enum, classify client errors into it, and compare the two on every expected failure. An unclassified error and a reason string this build does not know both fail the fixture: accepting either would restore "any error will do". Unknown reasons stay deserializable as `Unknown(_)` so the Hive test driver still answers such a step over HTTP instead of rejecting the request; only the offline runners treat them as failures. `AggregateVerificationFailed` covered both an attestation aggregate and a block's merged proof, which the spec separates into `INVALID_SIGNATURE` and `INVALID_BLOCK_PROOF`, so block-proof verification now has its own variant.
🤖 Kimi Code ReviewThis PR significantly improves the spec-test infrastructure by introducing structured rejection reasons, ensuring tests fail for the correct reasons rather than any error. The implementation is sound and follows Rust best practices. Summary of Changes
Detailed Reviewcrates/common/test-fixtures/src/rejection.rs (New)Line 20-55: The Line 180-211: The Line 157-168: The crates/blockchain/src/spec_test_runner.rsLine 62-63: When classifying Line 67-95: The Line 98-102: Explicitly returning crates/blockchain/src/store.rsLine 1061-1062: Adding crates/blockchain/tests/forkchoice_spectests.rsLine 64-81: The anchor rejection handling correctly validates that the only expressible anchor failure is Line 173-196: The crates/net/rpc/src/test_driver.rsLine 234: Converting to Minor Suggestions
Consensus SafetyNo concerns. This PR only affects test infrastructure and error classification. No changes to:
The production code change (new Verdict: Approve. The PR is well-structured, exhaustively handles error variants, and prevents the test suite from accepting false positives. Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
| (Err(_), None) => { | ||
| // Expected failure | ||
| // Expected failure. When the fixture names why, the transition must | ||
| // have failed for that reason: a state-root mismatch standing in for |
There was a problem hiding this comment.
When the existing negative fixtures exercise attestation limits, justification ranges, or skipped slot processing, this unconditional reason comparison receives a different transition error and fails four stf_spectests, leaving the required CI target red.
Knowledge Base Used: Blockchain core: fork choice, state transition, block building, sync
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/blockchain/state_transition/tests/stf_spectests.rs
Line: 82-84
Comment:
**STF fixtures now fail deterministically**
When the existing negative fixtures exercise attestation limits, justification ranges, or skipped slot processing, this unconditional reason comparison receives a different transition error and fails four `stf_spectests`, leaving the required CI target red.
**Knowledge Base Used:** [Blockchain core: fork choice, state transition, block building, sync](https://app.greptile.com/lambdaclass/-/custom-context/knowledge-base/lambdaclass/ethlambda/-/docs/blockchain-core.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| version.workspace = true | ||
|
|
||
| [dependencies] | ||
| ethlambda-state-transition.workspace = true |
There was a problem hiding this comment.
Adding ethlambda-state-transition as a regular dependency couples the common fixture model to a blockchain implementation crate, violating the repository's documented dependency direction and expanding the build surface for every fixture consumer.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/common/test-fixtures/Cargo.toml
Line: 13
Comment:
**Common crate crosses dependency layers**
Adding `ethlambda-state-transition` as a regular dependency couples the common fixture model to a blockchain implementation crate, violating the repository's documented dependency direction and expanding the build surface for every fixture consumer.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Greptile SummaryThis PR adds typed rejection-reason assertions across the offline spec runners.
Confidence Score: 4/5This PR should not merge until the four deterministic STF spec-test failures are resolved or handled without weakening the new reason assertions. The new comparison correctly exposes rejection mismatches, but it is enabled unconditionally for fixtures the current STF replay path cannot satisfy, leaving a required test target red; the added common-to-blockchain dependency is also an actionable layering regression. Files Needing Attention: crates/blockchain/state_transition/tests/stf_spectests.rs and crates/common/test-fixtures/Cargo.toml
|
| Filename | Overview |
|---|---|
| crates/common/test-fixtures/src/rejection.rs | Adds the shared rejection-reason enum, comparison helper, and state-transition error classifier. |
| crates/blockchain/src/spec_test_runner.rs | Adds typed fork-choice step errors and classifies store failures for fixture assertions. |
| crates/blockchain/state_transition/tests/stf_spectests.rs | Enforces rejection-reason equality but deterministically leaves four existing fixtures failing. |
| crates/common/test-fixtures/Cargo.toml | Adds a higher-layer state-transition dependency to a common crate, contrary to the documented dependency direction. |
| crates/blockchain/src/store.rs | Splits block-proof verification failures into a dedicated StoreError variant. |
| crates/blockchain/tests/forkchoice_spectests.rs | Validates negative fork-choice steps against their expected typed rejection reasons. |
| crates/blockchain/tests/signature_spectests.rs | Validates signature-test failures against their expected rejection reasons. |
| crates/net/rpc/src/test_driver.rs | Converts typed StepError values to strings while preserving the existing HTTP response shape. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Fixture["Negative fixture<br/>rejectionReason"] --> Runner["Offline spec runner"]
Runner --> Operation["Fork choice / STF / signature verification"]
Operation --> Error["Typed client error"]
Error --> Classifier["RejectionReason classifier"]
Fixture --> Compare["check_rejection_reason"]
Classifier --> Compare
Compare -->|match| Pass["Fixture passes"]
Compare -->|mismatch / unknown| Fail["Fixture fails"]
Prompt To Fix All With AI
### Issue 1
crates/blockchain/state_transition/tests/stf_spectests.rs:82-84
**STF fixtures now fail deterministically**
When the existing negative fixtures exercise attestation limits, justification ranges, or skipped slot processing, this unconditional reason comparison receives a different transition error and fails four `stf_spectests`, leaving the required CI target red.
### Issue 2
crates/common/test-fixtures/Cargo.toml:13
**Common crate crosses dependency layers**
Adding `ethlambda-state-transition` as a regular dependency couples the common fixture model to a blockchain implementation crate, violating the repository's documented dependency direction and expanding the build surface for every fixture consumer.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "test(spec): assert the rejection reason ..." | Re-trigger Greptile
🤖 Claude Code ReviewReview: PR #547 — assert rejection reason on expected-failure fixturesOverall: Solid, well-scoped test-infrastructure change. The core idea — mirroring leanSpec's
Minor observations (not blocking):
No security, memory-safety, or fork-choice/state-transition-logic issues found — this PR only changes how test failures are classified and reported, plus the one legitimate Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewFindings
Verification Static review only. I could not run Cargo checks in this environment because dependency resolution needs network access and a writable Cargo home. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
Both are authored with leanSpec's `BlockSpec.skip_slot_processing`, which makes the filler call `process_block` alone, but the flag never reaches the emitted fixture: `StateTransitionFixture` carries only pre, blocks, post, postStateRoot and rejectionReason. Replaying `state_transition()` as the fixture format prescribes therefore runs `process_slots` first and dies on the placeholder zero state root instead of the rule under test, and nothing in the JSON marks the intended entry point. ream, zeam, gean, lantern and grandine all "pass" these two on that same state-root mismatch because they assert only that some error occurred; grandine's negative path even counts a state-root mismatch as the expected failure outright. Skipping states the limitation instead of pretending the assertion holds. The two remaining state-transition failures are real divergences, not replay artifacts, and stay red.
The macro saved one copy of the 36-entry table at the cost of making the enum invisible to readers, rustdoc and go-to-definition. Write the enum, `as_str` and `From<&str>` out directly. Drift between the two tables cannot pass silently: `as_str` is exhaustive, so a new variant fails to compile until it has a wire spelling, and a missing `From` arm lands the reason in `Unknown`, which every runner reports as a failure naming the string to add.
pablodeymo
left a comment
pablodeymo
left a comment
There was a problem hiding this comment.
Reviewed the branch at cb9bd7d and reproduced the results locally in a worktree off the PR head.
Verification
| Suite | Claimed | Measured |
|---|---|---|
forkchoice_spectests |
122/122 | 122 passed ✅ |
signature_spectests |
3/3 | 3 passed ✅ |
stf_spectests |
72 pass / 2 fail / 2 skip | 72 passed, 2 failed, both skips confirmed firing ✅ |
test-fixtures unit tests |
3 | 3 passed ✅ |
CI on the PR: Lint pass, Verify License pass, Test fail (the two STF fixtures).
The core design is good. Both classifiers (spec_test_runner::rejection_reason over StoreError, From<&Error> over the STF error) are wildcard-free, so a new error variant is a compile error until someone classifies it; Unknown(_) still deserializes so the Hive driver answers a step instead of 422-ing the request; and splitting BlockProofVerificationFailed out of AggregateVerificationFailed is a genuine fix that also makes production logs say which proof failed. Because negative fixtures ship a zero placeholder stateRoot, any unenforced rule surfaces as STATE_ROOT_MISMATCH — that is what makes the new assertion sharp.
Findings
1. CI is red by design, and the two known-bad classes are handled inconsistently. Two STF fixtures fail; two others are silently skipped. Both groups are "known non-conformance", handled two different ways, and the red one makes make test unusable as a gate. Suggest a single mechanism: an expected-divergence table (fixture → reason the fixture names, reason we actually produce) that fails if the mismatch changes or disappears. That keeps the divergence visible, stops it rotting, and self-clears when it's fixed. Plus tracking issues for both.
2. InvalidValidatorIndex is knowingly misclassified for the block path (crates/blockchain/src/spec_test_runner.rs:69). It maps unconditionally to VALIDATOR_NOT_IN_STATE, but verify_block_signatures raises the same variant for out-of-range attestation participants (store.rs:1122, store.rs:1147 → spec VALIDATOR_INDEX_OUT_OF_RANGE) and for an out-of-range proposer_index (store.rs:1127, store.rs:1160 → PROPOSER_INDEX_OUT_OF_RANGE). Consequence: both of those reasons are unreachable from the store classifier, so the moment leanSpec adds a verify_signatures fixture for either, it fails for the wrong reason. The doc comment acknowledges the ambiguity, but the fix is the same shape this PR already applies to the block proof — split the variant. Worth doing here, since "one error ⇒ one reason" is the PR's thesis.
3. A harness error can satisfy an expected rejection. In crates/blockchain/tests/forkchoice_spectests.rs, the arm (Err(_), false) if expected_reason.is_none() => Ok(()) accepts a StepError::Harness — e.g. "unknown step type: X" on a valid: false step passes. That contradicts StepError's documented invariant that a harness failure "never satisfies an expected rejectionReason". I checked the pinned fixtures: all 34 invalid fork-choice steps carry a reason, so the arm is unreachable today — either drop it (strictly stronger) or match Harness first and always fail.
Nits
crates/common/test-fixtures/src/verify_signatures.rs:50keepsalias = "expectException"but now parses it as aRejectionReason. A legacy value like"AssertionError"becomesUnknown(_)and fails with "add it toRejectionReasonand classify the error that must produce it" — misleading advice for a field that never carried a spec reason. Either drop the alias or keep the old field as presence-only.- Commit 3 inlined the macro, so
as_strandFrom<&str>are now two parallel 37-arm tables. Drift fails loudly (Unknown) rather than silently, but aconst ALL: &[RejectionReason]round-trip test pins it cheaply; the current tests cover one variant per direction. crates/common/test-fixtures/src/state_transition.rs:22still spells the fieldexpectExceptionasOption<String>. That's fine — the Hive driver only checksis_some()— but it leaves three spellings in-tree; one line of doc would settle why.SKIP_TESTSmatches by substring with no upstream link. Linking the leanSpec issue would give the skip an expiry path. Alternative worth considering instead of skipping: when a negative fixture'sstate_transition()yieldsSTATE_ROOT_MISMATCHagainst a different expected reason, retry withprocess_blockalone.test_block_at_parent_slot_rejected_when_slot_processing_skippedcarries two blocks at slot 1 with pre-state slot 1, so that path would actually reachBLOCK_OLDER_THAN_LATEST_HEADER— recovering the coverage rather than dropping it.
The StoreError classifier living in blockchain/src/ while the STF one is a From impl in test-fixtures is forced by the dependency direction (blockchain → test-fixtures), not sloppiness — no action needed.
…ion (#555) ## What Enforce the per-block cap on distinct `AttestationData` inside `process_attestations`, where leanSpec has it, in addition to the existing check at the import boundary in `on_block`. ## Why leanSpec puts the bound in the transition (`state_transition.process_attestations`) and `fork_choice.on_block` defers to it explicitly: > The transition itself bounds the distinct-data count. Only the wire-level duplicate prohibition lives here. We only had it in `on_block` (`store.rs`), so `state_transition()` accepted an over-cap block and then failed on the state root instead. Two callers reach the transition without passing through `on_block`: | caller | before | after | |---|---|---| | `build_block` -> `process_block` | unbounded (the proposer-side clamp is the only guard) | fails loudly instead of publishing an unimportable block | | spec-fixture replay / Hive `state_transition/run` | over-cap block accepted, then `STATE_ROOT_MISMATCH` | rejected with the cap error | The check goes first in `process_attestations`, ahead of the justification-bookkeeping guards, matching the spec's order when a block violates two rules at once. ## The duplicate check in `on_block` stays Deliberately, for now: it runs before `verify_block_signatures`, so an over-cap block is still rejected without paying for proof verification. Whether we collapse the two sites into one is a follow-up decision. ## Testing - `cargo test --workspace --profile release-fast`: green, no new tests added here. - The behavior is covered by the existing fixture `test_block_exceeding_distinct_attestation_data_cap_rejects_block`, which asserts failure but not yet the reason. Cross-checked against #547, which adds the reason assertion to the fixture runners: with both branches applied, that fixture passes for the right reason (`TOO_MANY_ATTESTATION_DATA`, previously `STATE_ROOT_MISMATCH`). Whichever of the two lands first, the other's exhaustive `From<&Error> for RejectionReason` match makes the missing mapping arm a compile error, so the reason cannot be silently dropped. (leanSpec #536)
Merging main brought two state-transition error variants the reason classifier does not know, and its match is exhaustive on purpose, so the merge itself does not build: `JustifiedSlotOutOfRange` (#549) and `TooManyAttestationData` (#555). Both have a spec counterpart, so they map straight through. That also closes the two divergences this branch documented as failing: the fixtures expecting JUSTIFIED_SLOT_OUT_OF_RANGE and TOO_MANY_ATTESTATION_DATA now fail for exactly those reasons. Pin the two parallel spelling tables while here, with `RejectionReason::ALL` and a round-trip test over it: an arm present on only one side, or a spelling claimed by two variants, now fails in the crate's own tests instead of silently making a fixture unmatchable.
One `StoreError` stood for three distinct spec rejections: VALIDATOR_NOT_IN_STATE for a gossiped vote naming a validator the target state never knew (`fork_choice.py`), and VALIDATOR_INDEX_OUT_OF_RANGE or PROPOSER_INDEX_OUT_OF_RANGE for the block-proof bounds checks (`signatures.py`). The classifier had to pick one, so the other two reasons were unreachable from a store rejection and a fixture naming either would have failed for the wrong reason. leanSpec already carries such vectors (`verify_signatures/test_index_out_of_range.py`, `test_proposer_index_bounds.py`); they are only absent from the released bundle. Split the variant three ways, mirroring the spec's own split, and carry the offending index and registry size so production logs name them too.
`StepError::Harness` means the runner never replayed the step at all, yet the arm accepting a reasonless rejection took any error for a `valid: false` step, so an unknown step type could satisfy an expected rejection. Reject harness failures before matching on what the fixture expected, which is the invariant `StepError`'s own docs already claimed. Unreachable with the pinned fixtures, where all 34 invalid fork-choice steps name a reason, so this guards the next fixture bump rather than fixing a live hole.
Two STF fixtures are authored with leanSpec's `BlockSpec.skip_slot_processing` (PR #161), which makes the filler call `process_block` alone and write the failing block with a placeholder zero `stateRoot`. That entry point never reaches the emitted fixture, so replaying `state_transition()` runs `process_slots` first and the run dies on the state root, or on the slot check, before reaching the rule under test. Both were skipped for that reason, asserting nothing. Supply the entry point instead, since it is the one piece of information the JSON omits: `process_block` alone does enforce both rules and reports exactly the reason each fixture names, BLOCK_SLOT_MISMATCH and BLOCK_OLDER_THAN_LATEST_HEADER. Scoped to the two names rather than a general "retry under another entry point" fallback, which would let any negative fixture pass on whichever path happened to produce the expected reason. A listed fixture that carries a `post` is an error, because skipping slot processing also skips the post-state root check.
`expectException` is gone from leanSpec: zero occurrences in the spec repo
and in all 561 fixtures of the latest bundle, which name `rejectionReason`
instead. Drop the compatibility alias rather than let a Python exception
name ("AssertionError") parse as a spec reason and ask for that name to be
added to `RejectionReason`, and rename the Hive driver's field, which still
declared the retired spelling as its primary name with `rejectionReason`
merely aliased onto it.
Document the rule at the crate root: these types mirror the format the
released bundle carries and nothing older, so a retired field is removed
rather than aliased and a pinned old bundle fails loudly instead of
half-parsing. `RejectionReason::Unknown` stays the deliberate exception,
since the Hive driver must keep answering a step when leanSpec adds a
reason name.
…proof The proposer signature moved out of the attestation aggregate but stayed inside `BlockProof`, so a failure there is still the spec's INVALID_BLOCK_PROOF rejection, not INVALID_SIGNATURE. Reusing the attestation-signature variants classified it as the latter, which the `test_corrupt_proof_rejected` and `test_proof_reused_under_different_message_rejected` fixtures reject now that the runner asserts the reason (#547). Separate variants keep the gossip attestation path on INVALID_SIGNATURE.
2 participants
Summary
Negative leanSpec fixtures name why their input must be rejected, in a
rejectionReasonfield. The spec-test runners parsed that field only sodeny_unknown_fieldswould accept it and asserted nothing more than "some errorcame back", so a fixture could pass on a failure unrelated to the rule it
exercises.
This wires the reason into the assertion for all three offline runners
(fork choice, state transition, verify signatures).
What changed
RejectionReason: typed mirror of leanSpec'sRejectionReasonStrEnum (36 reasons)crates/common/test-fixtures/src/rejection.rsStoreError→ reason classifier, exhaustive so new variants must be classifiedspec_test_runner::rejection_reasonstate_transition::Error→ reason, as a totalFromimpltest-fixtures/src/rejection.rsapply_fork_choice_stepnow returns a typedStepError(store rejection vs harness failure) instead ofStringspec_test_runnerrejection::check_rejection_reasonTwo failure modes are treated as test failures, not passes, since accepting
either restores "any error will do":
Unknown reasons still deserialize (as
Unknown(_)) so the Hive test driverkeeps answering a step over HTTP rather than 422-ing the request when leanSpec
adds a reason name; only the offline runners fail on them.
StoreError::AggregateVerificationFailedwas raised both for an attestationaggregate (
INVALID_SIGNATURE) and for a block's merged multi-message proof(
INVALID_BLOCK_PROOF). Since the spec separates those, block-proofverification now returns its own
BlockProofVerificationFailedvariant, whichalso makes production logs say which proof failed.
Results
exactly the reason their fixture names.
The 2 remaining failures are real divergences
They passed before for the wrong reason; the new assertion is what surfaces
them. Left failing rather than allowlisted, so the divergence stays visible:
test_block_exceeding_distinct_attestation_data_cap_rejects_blockTOO_MANY_ATTESTATION_DATASTATE_ROOT_MISMATCHMAX_ATTESTATIONS_DATAcap is enforced on block import (store.rs), not in the STF, so replaying the STF alone misses ittest_source_slot_beyond_tracked_range_rejects_blockJUSTIFIED_SLOT_OUT_OF_RANGESTATE_ROOT_MISMATCHis_valid_votetreats a source past the tracked justification window as "not justified" and skips the vote, where the spec rejects the blockThe first is a layering gap rather than live exposure: a node still rejects the
capped block at import. Both want follow-up PRs.
The 2 skips are unreplayable fixtures, not client bugs
test_block_with_wrong_slotandtest_block_at_parent_slot_rejected_when_slot_processing_skippedare authoredwith leanSpec's
BlockSpec.skip_slot_processing(block_spec.py:71), whichmakes the filler call
process_blockalone. That flag never reaches the emittedfixture —
StateTransitionFixturecarries onlypre,blocks,post,postStateRoot,rejectionReason— and the failing block is written with aplaceholder zero
stateRoot. Replayingstate_transition(), which is what thefixture format's own description prescribes, therefore advances the slot first
and then dies on the state root instead of reaching the rule under test.
Nothing in the JSON marks the entry point, so no client can reproduce these
two. Verified against fresh clones: ream, zeam, gean, lantern and grandine all
"pass" them on the same state-root mismatch because they assert only that some
error occurred (grandine's negative path counts a state-root mismatch as the
expected failure outright). qlean's 2-month-old vendored copy has the identical
shape under the older
expectException: AssertionErrorspelling, so this is nota regression: sharpening the fixture's claim to a specific reason is what made
it checkable, and therefore visibly unsatisfiable.
Upstream fix would be to emit the entry point per block (
skipSlotProcessing,or
entryPoint: state_transition | process_block), or to drop these two vectorssince they pin a Python-level API contract rather than cross-client behaviour.
Testing
Three unit tests cover reason parsing, the unknown-reason fallback and JSON
deserialization.