test(spec): assert the rejection reason on expected-failure fixtures

[MegaRedHand]

Summary

Negative leanSpec fixtures name why their input must be rejected, in a
rejectionReason field. The spec-test runners parsed that field only so
deny_unknown_fields would accept it and asserted nothing more than "some error
came 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

Piece	Where
RejectionReason: typed mirror of leanSpec's RejectionReason StrEnum (36 reasons)	crates/common/test-fixtures/src/rejection.rs
StoreError → reason classifier, exhaustive so new variants must be classified	spec_test_runner::rejection_reason
state_transition::Error → reason, as a total From impl	test-fixtures/src/rejection.rs
apply_fork_choice_step now returns a typed StepError (store rejection vs harness failure) instead of String	spec_test_runner
Shared comparison + failure wording used by all three runners	rejection::check_rejection_reason

Two failure modes are treated as test failures, not passes, since accepting
either restores "any error will do":

• the client error carries no spec counterpart (unclassified), and
• the fixture names a reason this build does not know.

Unknown reasons still deserialize (as Unknown(_)) so the Hive test driver
keeps 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::AggregateVerificationFailed was raised both for an attestation
aggregate (INVALID_SIGNATURE) and for a block's merged multi-message proof
(INVALID_BLOCK_PROOF). Since the spec separates those, block-proof
verification now returns its own BlockProofVerificationFailed variant, which
also makes production logs say which proof failed.

Results

• fork choice: 122/122 pass — all 35 negative steps already failed for
  exactly the reason their fixture names.
• verify signatures: 3/3 pass.
• state transition: 4 fixtures now fail. They passed before for the wrong
  reason; the new assertion is what surfaces them. Per review discussion these
  are left failing rather than allowlisted, so the divergence is visible:

fixture	expects	ethlambda raises	diagnosis
test_block_exceeding_distinct_attestation_data_cap_rejects_block	TOO_MANY_ATTESTATION_DATA	STATE_ROOT_MISMATCH	the MAX_ATTESTATIONS_DATA cap is enforced on block import (store.rs), not in the STF, so replaying the STF alone misses it
test_source_slot_beyond_tracked_range_rejects_block	JUSTIFIED_SLOT_OUT_OF_RANGE	STATE_ROOT_MISMATCH	is_valid_vote treats a source past the tracked justification window as "not justified" and skips the vote, where the spec rejects the block
test_block_with_wrong_slot	BLOCK_SLOT_MISMATCH	STATE_ROOT_MISMATCH	fixture processes the block with slot processing skipped; our runner always calls full state_transition, so process_slots advances the state and the slot check can no longer fire
test_block_at_parent_slot_rejected_when_slot_processing_skipped	BLOCK_OLDER_THAN_LATEST_HEADER	BLOCK_SLOT_NOT_IN_FUTURE	same shape: process_slots rejects first, shadowing the header check the fixture targets

The first two look like real gaps in the STF layer (the node itself still
rejects the capped case at import). The last two are replay limitations: the
fixture schema carries no flag saying slot processing was skipped, so no client
can reproduce that entry point from the fixture alone — worth raising upstream.

CI is therefore red on stf_spectests until those are addressed; everything
else in the workspace passes.

Testing

make fmt && make lint                       # clean
cargo test --workspace --profile release-fast
# stf_spectests: 70 passed / 4 failed (table above); all other suites pass

Three unit tests cover reason parsing, the unknown-reason fallback and JSON
deserialization.

[github-actions]

🤖 Kimi Code Review

This 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

• Introduces RejectionReason enum with wire-spelling synchronization via macro
• Adds StepError to distinguish client rejections from harness failures
• Implements exhaustive error classification for StoreError and StateTransitionError
• Updates fork-choice, STF, and signature test runners to assert specific rejection reasons

Detailed Review

crates/common/test-fixtures/src/rejection.rs (New)

Line 20-55: The rejection_reasons! macro is well-designed—it prevents drift between enum variants and their JSON wire spellings. The Unknown(String) variant gracefully handles forward compatibility with newer spec fixtures.

Line 180-211: The From<&ethlambda_state_transition::Error> implementation is correctly exhaustive. Note the subtle mapping of AggregationBitsOutOfBounds to ValidatorIndexOutOfRange (line 202-204)—the comment explaining this is valuable.

Line 157-168: The check_rejection_reason helper correctly treats unknown expected reasons as failures, preventing silent acceptance of "any error will do."

crates/blockchain/src/spec_test_runner.rs

Line 62-63: When classifying StateTransitionFailed, the code delegates to the From impl in rejection.rs. This maintains single responsibility for error classification.

Line 67-95: The rejection_reason function exhaustively matches StoreError. The comment at lines 70-76 correctly documents the context-dependent nature of InvalidValidatorIndex (gossip vs. block validation paths).

Line 98-102: Explicitly returning None for internal failures (PubkeyDecodingFailed, SignatureAggregationFailed, etc.) is correct—these have no spec counterpart and should not satisfy fixture assertions.

crates/blockchain/src/store.rs

Line 1061-1062: Adding BlockProofVerificationFailed distinct from AggregateVerificationFailed is important for correct rejection reason classification in tests. The change at line 1176 to use this new variant ensures block proof failures map to InvalidBlockProof rather than InvalidSignature.

crates/blockchain/tests/forkchoice_spectests.rs

Line 64-81: The anchor rejection handling correctly validates that the only expressible anchor failure is AnchorStateRootMismatch, failing loudly if the fixture expects other reasons the runner cannot yet assert.

Line 173-196: The assert_step_outcome function correctly handles the transition from "any failure passes" to "specific reason required." The match arm at line 190 handles legacy fixtures without rejectionReason.

crates/net/rpc/src/test_driver.rs

Line 234: Converting to String here loses structured error information, but this is acceptable for the Hive test driver interface which expects string errors.

Minor Suggestions

1. crates/blockchain/src/spec_test_runner.rs:58: Consider making rejection_reason a method on StoreError directly (via impl StoreError), rather than a free function, for better discoverability.

2. crates/common/test-fixtures/src/rejection.rs:215: The unit tests are good. Consider adding a test for the Unknown round-trip case to ensure serialization/deserialization symmetry.

3. Documentation: The PR description mentions "3SF-mini" fork choice—ensure the rejection reasons cover 3SF-specific failures (e.g., sync committee-related) if applicable in future work.

Consensus Safety

No concerns. This PR only affects test infrastructure and error classification. No changes to:

• State transition logic
• Signature verification algorithms
• Fork choice weight calculations
• SSZ encoding/decoding

The production code change (new BlockProofVerificationFailed variant) is purely additive for error specificity.

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]

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

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.

[greptile-apps]

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.

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-apps]

Greptile Summary

This PR adds typed rejection-reason assertions across the offline spec runners.

• Introduces a shared rejection-reason vocabulary and classifiers for store and state-transition errors.
• Separates block-proof verification failures from attestation aggregate failures.
• Distinguishes fork-choice harness errors from store rejections.
• Updates fork-choice, STF, and signature runners to compare expected and actual rejection reasons.

Confidence Score: 4/5

This 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

Important Files Changed

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"]

Loading

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}

[github-actions]

🤖 Claude Code Review

Review: PR #547 — assert rejection reason on expected-failure fixtures

Overall: Solid, well-scoped test-infrastructure change. The core idea — mirroring leanSpec's RejectionReason vocabulary and asserting it instead of "any error will do" — closes a real gap (the four now-failing stf_spectests fixtures prove it). Two things stood out as correctness safeguards done right:

1. Both classifier functions (spec_test_runner::rejection_reason over StoreError, and RejectionReason::from(&state_transition::Error)) are exhaustive matches with no wildcard arm, so the compiler forces a decision whenever a new error variant is added — exactly the guarantee the doc comments claim. I checked both against the current variant lists in store.rs and state_transition/src/lib.rs and they're complete and correctly mapped.
2. Splitting AggregateVerificationFailed into AggregateVerificationFailed (attestation aggregate → INVALID_SIGNATURE) and BlockProofVerificationFailed (block merged proof → INVALID_BLOCK_PROOF) is a genuine production bug fix, not just a test change — the two call sites (store.rs:534 and store.rs:1176) were verified to map to the right paths, and no other code exhaustively matches StoreError in a way this would break.

Minor observations (not blocking):

• crates/common/test-fixtures/src/state_transition.rs (the StateTransitionRunRequest used by the online Hive test-driver path, untouched by this PR) still carries expect_exception: Option<String> — a stringly-typed, unvalidated sibling of the newly-typed rejection_reason used by the offline runners. That's consistent with the PR's stated scope (only the three offline runners), but it means the same concept now has two different representations in the codebase. Worth a tracking note/follow-up if there's appetite to unify, but not a defect in this PR.
• The four newly-red stf_spectests fixtures are left failing by design (per the PR description) rather than allowlisted, which is the right call for visibility — just flagging that CI will show red on stf_spectests until the underlying STF gaps (attestation-data cap enforcement, justified-slot-range handling) are fixed, so reviewers/CI dashboards should expect that rather than treat it as this PR's own regression.
• RejectionReason::Unknown(String) correctly stays deserializable (satisfying deny_unknown_fields on all three fixture structs) while still failing offline-runner assertions via check_rejection_reason — this dual behavior (permissive for Hive, strict for spec tests) is a subtle but correctly-implemented distinction, worth keeping in mind if this code is touched again.

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 StoreError variant split.

---

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

[github-actions]

🤖 Codex Code Review

Findings

1. InvalidValidatorIndex is classified incorrectly for the block-signature path. The shared mapper in spec_test_runner.rs always turns StoreError::InvalidValidatorIndex into VALIDATOR_NOT_IN_STATE, but verify_block_signatures emits that same store error for out-of-range block participants and proposer indices in store.rs. The new verify_signatures runner then consumes that generic mapping in signature_spectests.rs. Result: a fixture expecting VALIDATOR_INDEX_OUT_OF_RANGE will now fail for the wrong reason. This needs either separate store variants or a path-aware classifier.

2. DECODE_ERROR is added to the fixture vocabulary, but the offline runners still cannot produce it. verify_signatures eagerly converts signedBlock with From<TestSignedBlock> in signature_spectests.rs, and that conversion still panics on bad hex / oversized proof bytes in verify_signatures.rs. Fork-choice aggregated proof decoding also still expects on invalid hex in fork_choice.rs and reports oversized proof blobs as Harness errors in spec_test_runner.rs. So malformed-wire negative fixtures will crash or be treated as harness failures instead of matching DECODE_ERROR.

3. The backward-compatibility arm in forkchoice_spectests.rs still accepts any Err(_) for valid: false steps that do not name a rejectionReason. That includes StepError::Harness, which this PR introduced specifically to mean “runner cannot replay this fixture step.” In practice, an unsupported or malformed step can still pass as a valid negative fixture, which preserves the same false-positive class this change is supposed to remove. That arm should only accept StepError::Store(_).

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