Skip to content

Commit 8476c49

Browse files
committed
test(spec): assert the rejection reason on expected-failure fixtures
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.
1 parent 91eb0a1 commit 8476c49

13 files changed

Lines changed: 492 additions & 81 deletions

File tree

Cargo.lock

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/blockchain/src/spec_test_runner.rs

Lines changed: 122 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -4,19 +4,109 @@
44
//! functions so fixture replay cannot drift between the two entry points.
55
66
use ethlambda_storage::Store;
7-
use ethlambda_test_fixtures::fork_choice::ForkChoiceStep;
7+
use ethlambda_test_fixtures::{RejectionReason, fork_choice::ForkChoiceStep};
88
use ethlambda_types::{
99
attestation::{
1010
AggregationBits, HashedAttestationData, SignedAggregatedAttestation, SignedAttestation,
1111
},
1212
block::{ByteList512KiB, SingleMessageAggregate},
1313
};
1414

15-
use crate::{MILLISECONDS_PER_INTERVAL, MILLISECONDS_PER_SLOT, store};
15+
use crate::{
16+
MILLISECONDS_PER_INTERVAL, MILLISECONDS_PER_SLOT,
17+
store::{self, StoreError},
18+
};
1619

1720
/// Prefix emitted by leanSpec's mocked aggregation prover.
1821
const MOCK_PROOF_PREFIX: &[u8] = b"\x00MOCKED-AGGREGATION-PROOF\x00";
1922

23+
/// Why a fork-choice fixture step failed.
24+
///
25+
/// Distinguishes a client rejection, which negative fixtures assert against
26+
/// their `rejectionReason`, from a harness failure, which means the fixture
27+
/// asked for something this runner cannot replay.
28+
#[derive(Debug, thiserror::Error)]
29+
pub enum StepError {
30+
/// The store rejected the step.
31+
#[error(transparent)]
32+
Store(#[from] StoreError),
33+
34+
/// The step is malformed or names something the runner cannot replay. Never
35+
/// a client rejection, so it never satisfies an expected `rejectionReason`.
36+
#[error("{0}")]
37+
Harness(String),
38+
}
39+
40+
impl StepError {
41+
/// The leanSpec rejection reason this failure corresponds to, if any.
42+
pub fn rejection_reason(&self) -> Option<RejectionReason> {
43+
match self {
44+
Self::Store(err) => rejection_reason(err),
45+
Self::Harness(_) => None,
46+
}
47+
}
48+
}
49+
50+
/// Classify a store rejection into the reason leanSpec would report for it,
51+
/// mirroring the spec's `classify_rejection`.
52+
///
53+
/// `None` means the variant has no spec counterpart, which the spec-test runners
54+
/// report as an unclassified rejection rather than accepting silently. The match
55+
/// is exhaustive so a new [`StoreError`] variant forces that decision here.
56+
///
57+
/// Two variants are context-dependent and classified for the gossip path they
58+
/// are reached through in fixtures:
59+
///
60+
/// * `InvalidValidatorIndex` is raised both for a gossip attestation from an
61+
/// unregistered validator (`VALIDATOR_NOT_IN_STATE`) and for a block-level
62+
/// participant bounds check (`VALIDATOR_INDEX_OUT_OF_RANGE`, spec
63+
/// `signatures.py`). Only the former has fixtures today.
64+
/// * `StateTransitionFailed` defers to the state-transition classification,
65+
/// which the STF runner asserts directly.
66+
pub fn rejection_reason(err: &StoreError) -> Option<RejectionReason> {
67+
let reason = match err {
68+
StoreError::MissingParentState { .. } => RejectionReason::UnknownParentBlock,
69+
StoreError::InvalidValidatorIndex => RejectionReason::ValidatorNotInState,
70+
StoreError::SignatureDecodingFailed | StoreError::SignatureVerificationFailed => {
71+
RejectionReason::InvalidSignature
72+
}
73+
StoreError::StateTransitionFailed(err) => err.into(),
74+
StoreError::UnknownSourceBlock(_) => RejectionReason::UnknownSourceBlock,
75+
StoreError::UnknownTargetBlock(_) => RejectionReason::UnknownTargetBlock,
76+
StoreError::UnknownHeadBlock(_) => RejectionReason::UnknownHeadBlock,
77+
StoreError::SourceExceedsTarget => RejectionReason::SourceAfterTarget,
78+
StoreError::HeadOlderThanTarget { .. } => RejectionReason::HeadOlderThanTarget,
79+
StoreError::SourceSlotMismatch { .. } => RejectionReason::SourceSlotMismatch,
80+
StoreError::TargetSlotMismatch { .. } => RejectionReason::TargetSlotMismatch,
81+
StoreError::HeadSlotMismatch { .. } => RejectionReason::HeadSlotMismatch,
82+
StoreError::SourceNotAncestorOfTarget => RejectionReason::SourceNotAncestorOfTarget,
83+
StoreError::TargetNotAncestorOfHead => RejectionReason::TargetNotAncestorOfHead,
84+
StoreError::HeadNotDescendantOfFinalized { .. } => {
85+
RejectionReason::HeadNotDescendantOfFinalized
86+
}
87+
StoreError::AttestationSlotBeforeHead { .. } => RejectionReason::AttestationSlotBeforeHead,
88+
StoreError::AttestationTooFarInFuture { .. } => RejectionReason::AttestationTooFarInFuture,
89+
StoreError::AggregateVerificationFailed(_) => RejectionReason::InvalidSignature,
90+
StoreError::BlockProofVerificationFailed(_) => RejectionReason::InvalidBlockProof,
91+
StoreError::EmptyAggregationBits => RejectionReason::EmptyAggregationBits,
92+
StoreError::NotProposer { .. } => RejectionReason::WrongProposer,
93+
StoreError::DuplicateAttestationData { .. } => RejectionReason::DuplicateAttestationData,
94+
StoreError::TooManyAttestationData { .. } => RejectionReason::TooManyAttestationData,
95+
StoreError::BlockSlotGapTooLarge { .. } => RejectionReason::BlockSlotGapTooLarge,
96+
StoreError::BlockTooFarInFuture { .. } => RejectionReason::BlockTooFarInFuture,
97+
98+
// Internal failures with no spec counterpart: the spec has no undecodable
99+
// registry pubkey, no aggregation step inside validation, no state that
100+
// can go missing behind a known block, and no slot width limit (its
101+
// slots are unbounded where ours narrow to the XMSS epoch's u32).
102+
StoreError::PubkeyDecodingFailed(_)
103+
| StoreError::SignatureAggregationFailed(_)
104+
| StoreError::MissingTargetState(_)
105+
| StoreError::SlotOutOfRange(_) => return None,
106+
};
107+
Some(reason)
108+
}
109+
20110
/// Apply one fork-choice fixture step.
21111
///
22112
/// `proofs_are_mocked` is supplied by complete offline vectors through their
@@ -26,7 +116,7 @@ pub fn apply_fork_choice_step(
26116
store: &mut Store,
27117
step: &ForkChoiceStep,
28118
proofs_are_mocked: Option<bool>,
29-
) -> Result<(), String> {
119+
) -> Result<(), StepError> {
30120
match step.step_type.as_str() {
31121
"tick" => {
32122
let genesis_time = store.config().expect("config exists").genesis_time;
@@ -35,7 +125,11 @@ pub fn apply_fork_choice_step(
35125
(None, Some(interval)) => {
36126
genesis_time * 1000 + interval * MILLISECONDS_PER_INTERVAL
37127
}
38-
(None, None) => return Err("tick step missing time and interval".to_string()),
128+
(None, None) => {
129+
return Err(StepError::Harness(
130+
"tick step missing time and interval".to_string(),
131+
));
132+
}
39133
};
40134
store::on_tick(store, timestamp_ms, step.has_proposal.unwrap_or(false));
41135
Ok(())
@@ -44,14 +138,14 @@ pub fn apply_fork_choice_step(
44138
let block_data = step
45139
.block
46140
.as_ref()
47-
.ok_or_else(|| "block step missing block data".to_string())?;
141+
.ok_or_else(|| StepError::Harness("block step missing block data".to_string()))?;
48142
let signed_block = block_data.to_blank_signed_block();
49143
if step.tick_to_slot {
50144
let block_time_ms = store.config().expect("config exists").genesis_time * 1000
51145
+ signed_block.message.slot * MILLISECONDS_PER_SLOT;
52146
store::on_tick(store, block_time_ms, true);
53147
}
54-
store::on_block_without_verification(store, signed_block).map_err(|e| e.to_string())?;
148+
store::on_block_without_verification(store, signed_block)?;
55149

56150
let block = block_data.to_block();
57151
let entries = block.body.attestations.iter().map(|att| {
@@ -68,48 +162,45 @@ pub fn apply_fork_choice_step(
68162
let att = step
69163
.attestation
70164
.as_ref()
71-
.ok_or_else(|| "attestation step missing data".to_string())?;
165+
.ok_or_else(|| StepError::Harness("attestation step missing data".to_string()))?;
72166
let signed = SignedAttestation {
73-
validator_id: att
74-
.validator_id
75-
.ok_or_else(|| "attestation step missing validatorId".to_string())?,
167+
validator_id: att.validator_id.ok_or_else(|| {
168+
StepError::Harness("attestation step missing validatorId".to_string())
169+
})?,
76170
data: att.data.clone().into(),
77-
signature: att
78-
.signature
79-
.clone()
80-
.ok_or_else(|| "attestation step missing signature".to_string())?,
171+
signature: att.signature.clone().ok_or_else(|| {
172+
StepError::Harness("attestation step missing signature".to_string())
173+
})?,
81174
};
82-
store::on_gossip_attestation(store, &signed, step.is_aggregator.unwrap_or(false))
83-
.map_err(|e| e.to_string())
175+
store::on_gossip_attestation(store, &signed, step.is_aggregator.unwrap_or(false))?;
176+
Ok(())
84177
}
85178
"gossipAggregatedAttestation" => {
86-
let att = step
87-
.attestation
88-
.as_ref()
89-
.ok_or_else(|| "gossipAggregatedAttestation step missing data".to_string())?;
90-
let proof = att
91-
.proof
92-
.as_ref()
93-
.ok_or_else(|| "gossipAggregatedAttestation step missing proof".to_string())?;
179+
let att = step.attestation.as_ref().ok_or_else(|| {
180+
StepError::Harness("gossipAggregatedAttestation step missing data".to_string())
181+
})?;
182+
let proof = att.proof.as_ref().ok_or_else(|| {
183+
StepError::Harness("gossipAggregatedAttestation step missing proof".to_string())
184+
})?;
94185
let participants: AggregationBits = proof.participants.clone().into();
95186
let proof_bytes: Vec<u8> = proof.proof.clone().into();
96187
let is_mocked =
97188
proofs_are_mocked.unwrap_or_else(|| proof_bytes.starts_with(MOCK_PROOF_PREFIX));
98-
let proof_data = ByteList512KiB::try_from(proof_bytes)
99-
.map_err(|err| format!("aggregated proof data too large: {err:?}"))?;
189+
let proof_data = ByteList512KiB::try_from(proof_bytes).map_err(|err| {
190+
StepError::Harness(format!("aggregated proof data too large: {err:?}"))
191+
})?;
100192
let aggregated = SignedAggregatedAttestation {
101193
proof: SingleMessageAggregate::new(participants, proof_data),
102194
data: att.data.clone().into(),
103195
};
104196
if is_mocked {
105-
store::on_gossip_aggregated_attestation_without_verification(store, aggregated)
106-
.map_err(|e| e.to_string())
197+
store::on_gossip_aggregated_attestation_without_verification(store, aggregated)?;
107198
} else {
108-
store::on_gossip_aggregated_attestation(store, aggregated)
109-
.map_err(|e| e.to_string())
199+
store::on_gossip_aggregated_attestation(store, aggregated)?;
110200
}
201+
Ok(())
111202
}
112203
"checks" => Ok(()),
113-
other => Err(format!("unknown step type: {other}")),
204+
other => Err(StepError::Harness(format!("unknown step type: {other}"))),
114205
}
115206
}

crates/blockchain/src/store.rs

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1058,6 +1058,9 @@ pub enum StoreError {
10581058
#[error("Aggregated signature verification failed: {0}")]
10591059
AggregateVerificationFailed(ethlambda_crypto::VerificationError),
10601060

1061+
#[error("Block proof verification failed: {0}")]
1062+
BlockProofVerificationFailed(ethlambda_crypto::VerificationError),
1063+
10611064
#[error("Signature aggregation failed: {0}")]
10621065
SignatureAggregationFailed(ethlambda_crypto::AggregationError),
10631066

@@ -1170,7 +1173,7 @@ pub fn verify_block_signatures(
11701173
pubkeys_per_component,
11711174
&expected_bindings,
11721175
)
1173-
.map_err(StoreError::AggregateVerificationFailed)?;
1176+
.map_err(StoreError::BlockProofVerificationFailed)?;
11741177
let crypto_elapsed = crypto_start.elapsed();
11751178

11761179
let total_elapsed = total_start.elapsed();

crates/blockchain/state_transition/tests/stf_spectests.rs

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ use std::collections::HashMap;
22
use std::path::Path;
33

44
use ethlambda_state_transition::state_transition;
5+
use ethlambda_test_fixtures::{RejectionReason, rejection::check_rejection_reason};
56
use ethlambda_types::{
67
block::Block,
78
primitives::{H256, HashTreeRoot as _},
@@ -69,12 +70,24 @@ fn run(path: &Path) -> datatest_stable::Result<()> {
6970
}
7071
}
7172
(Ok(_), None) => {
72-
return Err(
73-
format!("Test '{name}' failed: expected failure but got success").into(),
74-
);
73+
let expected = test
74+
.rejection_reason
75+
.as_ref()
76+
.map(|reason| format!(" ({reason})"))
77+
.unwrap_or_default();
78+
return Err(format!(
79+
"Test '{name}' failed: expected failure{expected} but got success"
80+
)
81+
.into());
7582
}
76-
(Err(_), None) => {
77-
// Expected failure
83+
// Expected failure. When the fixture names why, the transition must
84+
// have failed for that reason: a state-root mismatch standing in for
85+
// the rule under test is a pass for the wrong reason.
86+
(Err(err), None) => {
87+
if let Some(expected) = test.rejection_reason.as_ref() {
88+
let actual = RejectionReason::from(&err);
89+
check_rejection_reason(&name, expected, Some(&actual), &err)?;
90+
}
7891
}
7992
(Err(err), Some(_)) => {
8093
return Err(format!(

crates/blockchain/state_transition/tests/types.rs

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,10 @@ pub struct StateTransitionTest {
3838
/// any state field those checks don't enumerate is still pinned.
3939
#[serde(rename = "postStateRoot")]
4040
pub post_state_root: Option<H256>,
41-
/// Expected rejection reason for negative cases. Captured only so
42-
/// `deny_unknown_fields` accepts it; failure is asserted via a missing
43-
/// `post`.
41+
/// Expected rejection reason for negative cases. A missing `post` asserts
42+
/// that the transition failed; this pins *why* it had to fail.
4443
#[serde(rename = "rejectionReason")]
45-
#[allow(dead_code)]
46-
pub rejection_reason: Option<String>,
44+
pub rejection_reason: Option<RejectionReason>,
4745
/// Aggregation proof regime (unused by the STF runner). Captured only so
4846
/// `deny_unknown_fields` accepts it.
4947
#[serde(rename = "proofSetting")]

0 commit comments

Comments
 (0)