Conversation
WalkthroughThis PR introduces a new zk-evm crate for zero-knowledge EVM test execution alongside enhancements to the evm-tests public API. The zk-evm crate integrates RISC0 proving infrastructure with a guest binary for executing EVM state tests. The evm-tests crate is refactored to expose previously internal modules, types, and functions while disabling KZG precompile support. Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as zk-evm CLI
participant Prover as RISC0 Prover
participant Guest as Guest Executor
participant Output
User->>CLI: Execute with fixture path
CLI->>CLI: Parse & validate JSON test file
CLI->>Prover: Build ExecutorEnv with test suite
Prover->>Guest: Execute ZK_EVM_ELF with environment
Guest->>Guest: Parse test suites from input
Guest->>Guest: Execute each test transaction<br/>(gas, precompiles, state changes)
Guest->>Guest: Validate exit reasons & compute hashes
Guest->>Output: Commit JSON results to journal
Prover->>Prover: Generate receipt with proof
CLI->>CLI: Verify receipt against ZK_EVM_ID
CLI->>User: Return verified results
sequenceDiagram
participant Guest as Guest Executor
participant Parser as JSON Parser
participant Config as Test Config
participant Executor as Stack Executor
participant Backend as Memory Backend
participant Results as Result Vector
Guest->>Parser: Read test suite from stdin
Parser->>Config: Initialize TestConfig::default()
loop For each test case
Config->>Config: Get gasometer config by spec
Config->>Backend: Create MemoryBackend
Backend->>Executor: Create StackExecutor with precompiles
Executor->>Executor: Validate transaction (gas, nonce, etc.)
Executor->>Executor: Execute call or create based on EIP-3607/7702
Executor->>Executor: Compute miner rewards & update balances
Executor->>Results: Collect ExecutionResult<br/>(gas_used, valid_hash, actual_hash)
end
Results->>Guest: Serialize to JSON
Guest->>Guest: Commit results to journal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@zk-evm/methods/guest/src/main.rs`:
- Around line 62-78: The code calls states.first().unwrap() when handling a
vicinity error, which will panic if states is empty; replace the unwrap with a
clear expect message (e.g., let h = states.first().expect("post_states should
not be empty").hash;) or otherwise defensively handle the empty-case before
calling original_state.check_valid_hash; update the occurrence in the vicinity
error branch near assert_vicinity_validation and ExecutionResult so the failure
is explicit and debuggable.
- Line 43: Fix the typo in the inline comment "Geet gasometer config for the
current spec" by replacing "Geet" with "Get" so the comment reads "Get gasometer
config for the current spec" (update the comment in main.rs where that comment
appears).
🧹 Nitpick comments (7)
zk-evm/Makefile (1)
1-3: Consider adding.PHONYdeclaration and documenting fixture path requirements.A few observations for this PoC Makefile:
- The
runtarget should be declared as.PHONYsince it doesn't produce a file output.- The hardcoded path to external fixtures (
../../../ethereum/fixtures_static-v4.5.0/...) assumes a specific directory structure outside the repository, which may not exist on other developer machines.Consider making the fixture path configurable via a variable or documenting the expected setup in a README.
♻️ Suggested improvement
+.PHONY: run + +# Path to ethereum test fixtures (adjust to your local setup) +FIXTURE_PATH ?= ../../../ethereum/fixtures_static-v4.5.0/fixtures/state_tests/static/state_tests/stStaticCall/static_log_Caller.json + run: - `@cargo` run --release -- ../../../ethereum/fixtures_static-v4.5.0/fixtures/state_tests/static/state_tests/stStaticCall/static_log_Caller.json + `@cargo` run --release -- $(FIXTURE_PATH) # `@cargo` run --release -- ../../../ethereum/ethtests-17.0/GeneralStateTests/stInitCodeTest/StackUnderFlowContractCreation.jsonThis allows overriding the path:
make run FIXTURE_PATH=/path/to/other/fixture.jsonzk-evm/src/main.rs (1)
44-52: Consider reading the file only once.The file is read twice: first via
BufReaderfor JSON validation, then again viaread_to_string. For a PoC this is acceptable, but could be optimized.♻️ Suggested optimization
fn get_tests_suite(file_name: &Path) -> String { - let file = File::open(file_name).expect("Open file failed"); - let reader = BufReader::new(file); - - // We parse the JSON test cases to verify is it deserialized correctly - let _ = serde_json::from_reader::<_, HashMap<String, StateTestCase>>(reader) - .expect("Parse test cases failed"); - std::fs::read_to_string(file_name).unwrap() + let content = std::fs::read_to_string(file_name).expect("Open file failed"); + // We parse the JSON test cases to verify it deserializes correctly + let _ = serde_json::from_str::<HashMap<String, StateTestCase>>(&content) + .expect("Parse test cases failed"); + content }evm-tests/src/precompiles.rs (1)
1-3: Consider removing commented-out KZG code if not needed.Commented-out code (module import and
usestatement) adds noise. If KZG support is intentionally disabled for the zk-EVM PoC and won't be re-enabled soon, consider removing these lines entirely. If this is temporary, a TODO comment explaining when it will be restored would be helpful.evm-tests/Cargo.toml (1)
22-22: Consider removing the commented dependency.Similar to the precompiles code, if
c-kzgis intentionally disabled for the zk-EVM PoC, removing the commented line keeps the manifest cleaner. Alternatively, add a comment explaining when/why it would be restored.zk-evm/methods/guest/src/main.rs (3)
1-1: Consider narrowing the allow attributes.
#![allow(unused_imports, dead_code)]is quite broad. For a PoC this is acceptable, but consider removing unused imports and dead code before promoting to production, or using more targeted#[allow(...)]on specific items.
232-241: Hardcoded test name introduces maintenance burden.The magic string
"failed_tx_xcf416c53"couples this code to a specific test case name. While the comment explains the edge case well, consider extracting this to a constant or configuration for better maintainability.Suggested refactor
+const PRECOMPILE_EDGE_CASE_TEST: &str = "failed_tx_xcf416c53"; + fn main() { // ... - if *spec <= Spec::London && test_config.name == "failed_tx_xcf416c53" { + if *spec <= Spec::London && test_config.name == PRECOMPILE_EDGE_CASE_TEST {
91-250: Complex loop body could benefit from extraction.The inner loop body (lines 91-250) handles multiple concerns: transaction validation, execution branching (call vs create), gas fee calculation, and state verification. For a PoC this is acceptable, but consider extracting logical units into helper functions if this code evolves beyond PoC.
|
|
||
| for (_, test) in test_suite { | ||
| for (spec, states) in &test.post_states { | ||
| // Geet gasometer config for the current spec |
There was a problem hiding this comment.
Typo in comment.
"Geet" should be "Get".
Proposed fix
- // Geet gasometer config for the current spec
+ // Get gasometer config for the current spec📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Geet gasometer config for the current spec | |
| // Get gasometer config for the current spec |
🤖 Prompt for AI Agents
In `@zk-evm/methods/guest/src/main.rs` at line 43, Fix the typo in the inline
comment "Geet gasometer config for the current spec" by replacing "Geet" with
"Get" so the comment reads "Get gasometer config for the current spec" (update
the comment in main.rs where that comment appears).
| if let Err(tx_err) = vicinity { | ||
| let h = states.first().unwrap().hash; | ||
| // if vicinity could not be computed, then the transaction was invalid, so we simply | ||
| // check the original state and move on | ||
| let (is_valid_hash, actual_hash) = original_state.check_valid_hash(&h); | ||
| if !is_valid_hash { | ||
| results.push(ExecutionResult { | ||
| used_gas: 0, | ||
| is_valid_hash, | ||
| actual_hash: actual_hash.to_string(), | ||
| }); | ||
|
|
||
| continue; | ||
| } | ||
| assert_vicinity_validation(&tx_err, states, spec, &test_config); | ||
| // As it's an expected validation error-skip the test run | ||
| continue; |
There was a problem hiding this comment.
Potential panic if states is empty.
states.first().unwrap() will panic if the states vector is empty. While test JSON files typically guarantee non-empty post-states, consider defensive handling or documenting this assumption.
Proposed fix using if-let or pattern match
if let Err(tx_err) = vicinity {
- let h = states.first().unwrap().hash;
+ let Some(first_state) = states.first() else {
+ continue;
+ };
+ let h = first_state.hash;
// if vicinity could not be computed, then the transaction was invalid, so we simply
// check the original state and move onBased on learnings, explicit panics are preferred in test code for debugging. If empty states should never occur, an expect() with a descriptive message would be clearer:
let h = states.first().expect("post_states should not be empty").hash;🤖 Prompt for AI Agents
In `@zk-evm/methods/guest/src/main.rs` around lines 62 - 78, The code calls
states.first().unwrap() when handling a vicinity error, which will panic if
states is empty; replace the unwrap with a clear expect message (e.g., let h =
states.first().expect("post_states should not be empty").hash;) or otherwise
defensively handle the empty-case before calling
original_state.check_valid_hash; update the occurrence in the vicinity error
branch near assert_vicinity_validation and ExecutionResult so the failure is
explicit and debuggable.
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.