Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tooling/ef_tests/blockchain/.fixtures_url_zkevm
Original file line number Diff line number Diff line change
@@ -1 +1 @@
https://github.com/ethereum/execution-spec-tests/releases/download/zkevm%40v0.3.0/fixtures_zkevm.tar.gz
https://github.com/ethereum/execution-spec-tests/releases/download/zkevm%40v0.3.3/fixtures_zkevm.tar.gz
4 changes: 2 additions & 2 deletions tooling/ef_tests/blockchain/Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ amsterdam-vectors: $(AMSTERDAM_ARTIFACT) $(SPECTEST_VECTORS_DIR)
$(ZKEVM_ARTIFACT): $(ZKEVM_FIXTURES_FILE)
curl -L -o $(ZKEVM_ARTIFACT) $(ZKEVM_URL)

zkevm-vectors: $(ZKEVM_ARTIFACT) $(SPECTEST_VECTORS_DIR)
tar -xzf $(ZKEVM_ARTIFACT) --strip-components=2 -C $(SPECTEST_VECTORS_DIR) fixtures/blockchain_tests/for_amsterdam/amsterdam/eip8025_optional_proofs
zkevm-vectors: $(ZKEVM_ARTIFACT) $(SPECTEST_VECTORS_DIR) amsterdam-vectors
tar -xzf $(ZKEVM_ARTIFACT) --strip-components=2 -C $(SPECTEST_VECTORS_DIR) fixtures/blockchain_tests/for_amsterdam

help: ## 📚 Show help for each of the Makefile recipes
@grep -E '^[a-zA-Z0-9_-]+:.*?## .*$$' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}'
Expand Down
70 changes: 49 additions & 21 deletions tooling/ef_tests/blockchain/test_runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -551,32 +551,60 @@ async fn run_stateless_from_fixture(
continue;
};

// zkevm fixtures encode the expected stateless outcome in `statelessOutputBytes`
// as `new_payload_request_root (32 bytes) ++ valid (1 byte) ++ trailing padding`.
// When the fixture signals `valid = false` the witness is deliberately incomplete
// and the stateless path must reject it; absent bytes means "expected to succeed".
let expected_valid = block_data
.stateless_output_bytes
.as_deref()
.and_then(parse_expected_valid_flag)
.unwrap_or(true);

let block: CoreBlock = block_data.clone().into();
let block_number = block.header.number;

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expected_valid defaults to true not only when stateless_output_bytes is absent, but also when it is present yet malformed/too short (because parse_expected_valid_flag returns None). That can silently treat corrupted fixtures as “expected to succeed”. Consider returning an explicit error when stateless_output_bytes is Some(_) but the validity byte cannot be parsed, and only default to true when the field is actually missing.

Suggested change
// zkevm fixtures encode the expected stateless outcome in `statelessOutputBytes`
// as `new_payload_request_root (32 bytes) ++ valid (1 byte) ++ trailing padding`.
// When the fixture signals `valid = false` the witness is deliberately incomplete
// and the stateless path must reject it; absent bytes means "expected to succeed".
let expected_valid = block_data
.stateless_output_bytes
.as_deref()
.and_then(parse_expected_valid_flag)
.unwrap_or(true);
let block: CoreBlock = block_data.clone().into();
let block_number = block.header.number;
let block: CoreBlock = block_data.clone().into();
let block_number = block.header.number;
// zkevm fixtures encode the expected stateless outcome in `statelessOutputBytes`
// as `new_payload_request_root (32 bytes) ++ valid (1 byte) ++ trailing padding`.
// When the fixture signals `valid = false` the witness is deliberately incomplete
// and the stateless path must reject it; absent bytes means "expected to succeed".
let expected_valid = match block_data.stateless_output_bytes.as_deref() {
None => true,
Some(stateless_output_bytes) => parse_expected_valid_flag(stateless_output_bytes)
.ok_or_else(|| {
format!(
"Malformed statelessOutputBytes for {test_key} block {block_number}"
)
})?,
};

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5a597e6 — took your suggested shape: match on Some/None, and a present-but-malformed field returns an explicit error rather than defaulting to true.

let rpc_witness: RpcExecutionWitness = serde_json::from_value(witness_json.clone())
.map_err(|e| {
format!("Failed to parse executionWitness for block {block_number}: {e}")
})?;

let execution_witness = rpc_witness
.into_execution_witness(*chain_config, block_number)
.map_err(|e| format!("Witness conversion failed for block {block_number}: {e}"))?;

let program_input = ProgramInput::new(vec![block], execution_witness);

let execute_result = match backend_type {
BackendType::Exec => ExecBackend::new().execute(program_input),
#[cfg(feature = "sp1")]
BackendType::SP1 => Sp1Backend::new().execute(program_input),
};

if let Err(e) = execute_result {
return Err(format!(
"Stateless execution from fixture failed for {test_key} block {block_number}: {e}"
));
let stateless_outcome: Result<(), String> = (|| {
let rpc_witness: RpcExecutionWitness = serde_json::from_value(witness_json.clone())
.map_err(|e| format!("executionWitness parse: {e}"))?;
let execution_witness = rpc_witness
.into_execution_witness(*chain_config, block_number)
.map_err(|e| format!("witness conversion: {e}"))?;
let program_input = ProgramInput::new(vec![block.clone()], execution_witness);
let res = match backend_type {
BackendType::Exec => ExecBackend::new().execute(program_input),
#[cfg(feature = "sp1")]
BackendType::SP1 => Sp1Backend::new().execute(program_input),
};
res.map(|_| ()).map_err(|e| format!("execution: {e}"))
})();

match (expected_valid, stateless_outcome) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Block on this: the (false, Err(_)) => {} arm is too permissive — it treats any host-side failure as a pass, including executionWitness JSON parse errors, into_execution_witness conversion errors, and program-input setup errors. None exercise the stateless rejection path the fixture is asserting. A regression in RpcExecutionWitness::deserialize would silently pass every valid=false fixture without ever invoking the guest. Split the closure so JSON/conversion errors propagate as real failures regardless of expected_valid. (Copilot flagged.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5a597e6 — IIFE removed. JSON parse and witness conversion always fail the test; only Backend::execute errors satisfy (false, Err(_)).

(true, Ok(())) | (false, Err(_)) => {}
(true, Err(e)) => {

Copilot AI Apr 27, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When expected_valid is false, any error is currently treated as an acceptable pass ((false, Err(_)) => {}). That includes host-side failures like executionWitness JSON parsing and witness conversion, which can mask regressions and also means the guest may never be invoked. Consider distinguishing error stages (parse/conversion vs execution) and only accepting the expected-failure case when the stateless execution step actually runs and rejects the witness.

Copilot uses AI. Check for mistakes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5a597e6 — split the IIFE. serde_json::from_value and into_execution_witness now propagate via ? unconditionally; only backend execution errors are matched against expected_valid.

return Err(format!(
"Stateless execution from fixture failed for {test_key} block {block_number}: {e}"
));
}
(false, Ok(())) => {
return Err(format!(
"Stateless execution from fixture succeeded for {test_key} block \
{block_number} but fixture expected it to fail (invalid executionWitness)"
));
}
}
}

Ok(())
}

/// Extract the `valid` byte from a zkevm-fixture `statelessOutputBytes` hex string.
///
/// The output encoding is `new_payload_request_root (32 bytes) ++ valid (1 byte) ++ padding`,
/// so byte index 32 carries the validity marker.
#[cfg(feature = "stateless")]
fn parse_expected_valid_flag(hex: &str) -> Option<bool> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unwrap_or(true) (at the call site above) collapses three distinct cases — field absent, field present but malformed, field present with valid byte 0x00 — into the same default. Right fix: parse_expected_valid_flag returns Result (or at minimum an Option with the absent case kept distinct from the malformed case), so a bad fixture can't silently flip into the success bucket. With ~2670 new fixtures landing, having one silently misinterpreted is a real risk. (Greptile + Copilot both flagged.)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5a597e6 — parse_expected_valid_flag returns Result<bool, String>, the call site ?s the error, and only 0x00/0x01 are accepted. Absent / malformed / 0x00 / 0x01 are now all distinguishable.

let trimmed = hex.strip_prefix("0x").unwrap_or(hex);
let byte_hex = trimmed.get(64..66)?;
u8::from_str_radix(byte_hex, 16).ok().map(|b| b != 0)
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Silent true fallback on truncated statelessOutputBytes

and_then(parse_expected_valid_flag).unwrap_or(true) treats a present-but-too-short hex string (< 66 chars after stripping 0x) identically to an absent field. If a fixture ships a statelessOutputBytes that is exactly 32 bytes (64 hex chars) — omitting the valid byte — get(64..66) returns None and the test silently runs as "expected to succeed" instead of surfacing the ambiguous fixture. A targeted None-return for the case where bytes are present but the string is too short would make the failure explicit rather than silent.

The existing code already handles this correctly at the call site (unwrap_or(true)) for the absent-field case; the concern is that a malformed fixture with a truncated byte string would be silently treated as "expected valid" rather than triggering a parse warning.

Prompt To Fix With AI
This is a comment left during a code review.
Path: tooling/ef_tests/blockchain/test_runner.rs
Line: 606-610

Comment:
**Silent `true` fallback on truncated `statelessOutputBytes`**

`and_then(parse_expected_valid_flag).unwrap_or(true)` treats a present-but-too-short hex string (< 66 chars after stripping `0x`) identically to an absent field. If a fixture ships a `statelessOutputBytes` that is exactly 32 bytes (64 hex chars) — omitting the `valid` byte — `get(64..66)` returns `None` and the test silently runs as "expected to succeed" instead of surfacing the ambiguous fixture. A targeted `None`-return for the case where bytes are present but the string is too short would make the failure explicit rather than silent.

The existing code already handles this correctly at the call site (`unwrap_or(true)`) for the absent-field case; the concern is that a malformed fixture with a truncated byte string would be silently treated as "expected valid" rather than triggering a parse warning.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 5a597e6 — parse_expected_valid_flag now returns Result<bool, String>. Truncated input, non-hex bytes, and validity bytes other than 0x00/0x01 all become hard errors at the call site.

23 changes: 22 additions & 1 deletion tooling/ef_tests/blockchain/tests/all.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,28 @@ const EXTRA_SKIPS: &[&str] = &[
"Return50000",
"static_Call1MB1024Calldepth",
];
#[cfg(not(feature = "sp1"))]
#[cfg(feature = "stateless")]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cfg(feature = "sp1") and cfg(feature = "stateless") aren't mutually exclusive at the cfg level — if both are enabled together, this file fails to compile (two definitions of EXTRA_SKIPS). Either gate the third arm with cfg(all(not(feature="sp1"), not(feature="stateless"))) (which it already does via not(any(...))) AND add a compile_error! in cfg(all(feature="sp1", feature="stateless")) to make the exclusivity explicit. Currently those features may be exclusive in practice, but the constraint isn't documented anywhere.

@avilagaston9 avilagaston9 May 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compile_error! is at lines 6–7. In ad031d5 I gated the sp1 arms with not(feature = "stateless") so the both-on combo now fails with only that diagnostic (previously two duplicate-definition errors rode along with it).

const EXTRA_SKIPS: &[&str] = &[
// zkevm@v0.3.3 tolerance tests: the fixture's `statelessOutputBytes` declares `valid = 1`
// because the executed path does not actually consume the malformed/extra/missing witness
// entry, but our RpcExecutionWitness conversion eagerly validates the full witness and
// rejects it. Re-enable once the witness conversion is lazy per EIP-8025 §Tolerance.
"validation_headers_malformed_rlp_header",
"validation_headers_missing_oldest_blockhash_ancestor",
"validation_headers_missing_parent_header",
"validation_state_extra_unused_trie_node",
// zkevm@v0.3.3 rejection tests: `statelessOutputBytes` declares `valid = 0` so the guest
// program must reject the deliberately-incomplete witness, but our stateless path runs
// to completion instead of detecting the missing entry. Re-enable once the witness
// completeness checks land (missing delegation/external-code bytecodes, non-contiguous
// header chain detection).
"validation_codes_missing_delegated_code_on_insufficient_balance_call",
"validation_codes_missing_external_code_read_target",
"validation_codes_missing_redelegation_old_marker",
"validation_codes_missing_sender_delegation_marker",
"validation_headers_non_contiguous_chain",
];
#[cfg(not(any(feature = "sp1", feature = "stateless")))]
const EXTRA_SKIPS: &[&str] = &[];
Comment on lines -31 to 60

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if sp1 is set but stateless isn't this won't compile, and if sp1 implies stateless then why specify both?

@avilagaston9 avilagaston9 May 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sp1 alone compiles (verified). The sp1 + stateless combo is rejected by the compile_error! at lines 6–7. They are not redundant: stateless uses the in-process Exec backend; sp1 runs the guest ELF inside the SP1 zkVM executor.


// Select backend
Expand Down
Loading