feat: Add perf benchmark #1854
feat: Add perf benchmark #1854
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
WalkthroughThis update introduces a comprehensive suite of EVM benchmarks, new test modules, and a fully implemented BLAKE2F precompile. It adds the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant BenchRunner
participant Compiler
participant EVM
participant Precompiles
User->>BenchRunner: Start benchmark (e.g., TenThousandHashes)
BenchRunner->>Compiler: Compile Solidity contract (with optimizer)
Compiler-->>BenchRunner: ABI, bytecode, deployed bytecode
BenchRunner->>EVM: Deploy contract bytecode at test address
BenchRunner->>EVM: Prepare call data (function selector)
BenchRunner->>EVM: Run contract call with gas/context
EVM->>Precompiles: (If precompile called, e.g., BLAKE2F)
Precompiles-->>EVM: Execute precompile, return result
EVM-->>BenchRunner: Return execution status, gas used, output
BenchRunner-->>User: Print benchmark results
Possibly related PRs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (7)
src/evm/vm.zig (1)
299-317: Guard verbose traces behind a compile-time log levelThe newly-added debug statements are valuable while debugging, but unconditional
Log.debugcalls execute unconditionally at runtime (even if they may later be filtered). They introduce a measurable overhead inside contract creation – a hot path that may run in inner loops when a benchmark suite is executed.Consider guarding them behind a
if (comptime std.debug.runtime_safety)or a dedicatedtrace_create_contractcompile-time flag, e.g.:-Log.debug("VM.create_contract_internal: executing init code with {} bytes", .{init_code.len}); +if (constants.ENABLE_CREATE_TRACING) { + Log.debug("VM.create_contract_internal: executing init code with {} bytes", .{init_code.len}); +}or wrap all create-contract traces in a helper
trace()that gets compiled out when-Drelease-fast/-Dno-traceis passed.debug_create_simple.zig (1)
39-40: Reduce noisy byte-array logging
std.log.info("Constructor bytecode: {any}", .{constructor_bytecode});dumps the full byte array on every run, cluttering test output. For repeat invocations (e.g., automated CI) consider loggingconstructor_bytecode.lenor its keccak hash instead.src/evm/precompiles/blake2f.zig (1)
28-37: Consider returningPrecompileError.NotImplementedinstead of genericExecutionFailedDown-stream callers can detect the feature gap explicitly and skip corresponding tests.
test/bench/memory_benchmark.zig (1)
47-52: Avoid per-iteration slice allocation in tight loopAllocating a fresh
[1]u8slice 1 000 times stresses the allocator and skews the benchmark. Usemem.set_byteor a reusable buffer.-for (0..1000) |i| { - const byte_data = [_]u8{@truncate(i)}; - try mem.set_data(i, &byte_data); - const read_byte = try mem.get_byte(i); - std.mem.doNotOptimizeAway(read_byte); -} +var single: [1]u8 = undefined; +for (0..1000) |i| { + single[0] = @truncate(i); + try mem.set_data(i, &single); + const b = try mem.get_byte(i); + std.mem.doNotOptimizeAway(b); +}simple_create_test.zig (1)
54-62: Possible leak ofcreate_result.output
vm.create_contractappears to allocateoutput; in the other test files you free it, but here you don’t. Free it or document ownership.+if (create_result.output) |out| { + defer allocator.free(out); +}bench/evm.zig (1)
124-126: Unused constant
BENCHMARK_SELECTORis defined but never referenced. Remove it or wire it into the execution simulation to avoid dead code warnings.build.zig (1)
574-605: Debug executables are built but never exercised
debug-testanddebug-simple-testare compiled/run via dedicated steps, yet those steps are not chained into any aggregate target (test,test-all, etc.).
If that’s intentional, consider marking themhiddenor documenting how to invoke them; otherwise, adddebug_test_stepanddebug_simple_test_stepas dependencies oftest_step.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.gitmodules(1 hunks)Cargo.toml(1 hunks)bench/evm(1 hunks)bench/evm.zig(1 hunks)bench/evm/memory_bench.zig(0 hunks)build.zig(6 hunks)debug_create_simple.zig(1 hunks)debug_create_test.zig(1 hunks)simple_create_test.zig(1 hunks)src/evm/precompiles/blake2f.zig(1 hunks)src/evm/vm.zig(3 hunks)src/solidity/out/SnailTracer.sol/SnailTracer.json(1 hunks)src/solidity/out/build-info/a4457bac9e7c700a4ad1a2e9aab7cffb.json(1 hunks)test/bench/memory_benchmark.zig(1 hunks)test/evm/compiler_integration_test.zig(1 hunks)test/evm/create_contract_test.zig(1 hunks)test_create_trace.zig(1 hunks)
💤 Files with no reviewable changes (1)
- bench/evm/memory_bench.zig
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: CI Checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
.gitmodules (1)
10-12: Addbench/evmsubmodule
Configuration in.gitmodulesfor the new Zig-based benchmark suite is correct. Ensure that CI and local setups includegit submodule update --init --recursiveto pull in this directory.src/solidity/out/build-info/a4457bac9e7c700a4ad1a2e9aab7cffb.json (1)
1-1: Add build‐info JSON for SnailTracer contract
The new metadata artifact is valid and aligns with theSnailTracer.solcompilation step in the benchmark suite.Cargo.toml (1)
10-12: Excludebench/evmfrom Cargo workspace
Preventing Cargo from picking up the Zig submodule is correct—ensures Rust tooling ignores non-Rust artifacts.bench/evm (1)
1-1: Ensure submodule is fully integrated
Thebench/evmfolder points to commitae7d470fbeb8e8955124d78d4524dc2abc990e7f. Verify that your root.gitmodulescontains a matchingpath = bench/evmandurl = https://github.com/evmts/evm-bench.git, and that CI/CD pipelines rungit submodule update --init --recursive.Would you like a small script to validate the
.gitmodulesentries and CI setup?src/evm/vm.zig (1)
341-346: Clarify lifetime semantics ofoutputslice
CreateResult.outputis set todeployment_code, which points into the VM allocator.
Callers de-allocating the VM before freeing the slice will hold a dangling reference. Either document the expectation (“slice is valid until the VM is de-initialised”) or dupe it into a caller-owned buffer the wayinterpret_with_contextdoes.src/evm/precompiles/blake2f.zig (1)
14-20:calculate_gas_checkeddeviates from EIP-152 specGas must be
rounds * 1whereroundsis parsed from the first four bytes (big-endian) of the input. Returning the stub constant1000can silently mask incorrect round counts during early integration.Until the full implementation lands, at least parse the header and return
0/error.InvalidInputwhenrounds * 1 > gas_limitto surface mis-usage earlier.src/solidity/out/SnailTracer.sol/SnailTracer.json (1)
1-1: Minification looks fine – no functional change.build.zig (4)
108-110: Good call adding the include pathAdding
includetocompiler_modunblocks C imports – looks correct.
651-672: CREATE-contract tests integrated—niceThe new
create_contract_testis wired intotest_step; no issues spotted.
715-742: tevm-runner target lacks dependency on Rust artefacts
tevm_runnerlinks Zig‐only modules but does not depend onrust_steporlibfoundry_wrapper.a, unlike the compiler tests/benchmarks. If the runner executes compiled contracts, it probably needs the Rust static lib too; link consistency now depends on transitive link order and may break on clean builds.
867-868: Nice—CREATE contract tests are included in the main test stepThis keeps coverage complete.
| Log.debug("VM.create_contract_internal: storing {} bytes of deployment code at address {any}", .{ deployment_code.len, new_address }); | ||
| try self.state.set_code(new_address, deployment_code); | ||
|
|
||
| // Verify the code was stored correctly | ||
| const stored_code = self.state.get_code(new_address); | ||
| Log.debug("VM.create_contract_internal: verification - stored code length: {}", .{stored_code.len}); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Turn post-deployment verification into an early error instead of best-effort logging
After persisting the deployment code you only log the stored length:
const stored_code = self.state.get_code(new_address);
Log.debug("stored code length: {}", .{stored_code.len});If the storage backend silently truncated/failed, the caller will still see success = true.
Recommend asserting or converting this into a failure path:
-const stored_code = self.state.get_code(new_address);
-Log.debug("VM.create_contract_internal: verification - stored code length: {}", .{stored_code.len});
+const stored_code = self.state.get_code(new_address);
+if (stored_code.len != deployment_code.len) {
+ Log.err("create_contract_internal: code length mismatch (expected {}, got {})",
+ .{deployment_code.len, stored_code.len});
+ return CreateResult.initFailure(0, null);
+}Fail fast prevents subtle corruption from creeping into higher-level tests.
🤖 Prompt for AI Agents
In src/evm/vm.zig around lines 332 to 338, after storing the deployment code you
currently only log the stored code length without verifying correctness. To fix
this, compare the length and content of the stored code against the original
deployment_code and if they differ, return an error or assert failure
immediately. This early error handling will prevent silent failures and ensure
the caller does not receive a false success indication.
debug_create_test.zig
Outdated
| if (create_result.output) |output| { | ||
| std.debug.print(" Output length: {}\n", .{output.len}); | ||
| // Just show the length for now | ||
| std.debug.print(" Output has {} bytes\n", .{output.len}); | ||
| } else { | ||
| std.debug.print(" Output: null\n", .{}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Free create_result.output to avoid a leak
The output slice is heap-allocated by the VM but never released in this debug utility:
if (create_result.output) |output| {
std.debug.print(" Output length: {}\n", .{output.len});
// missing allocator.free(output);
}Add a defer or explicit allocator.free(output); once you’re done printing.
if (create_result.output) |output| {
std.debug.print(" Output length: {}\n", .{output.len});
- // Just show the length for now
- std.debug.print(" Output has {} bytes\n", .{output.len});
+ std.debug.print(" Output has {} bytes\n", .{output.len});
+ allocator.free(output);
} else {🤖 Prompt for AI Agents
In debug_create_test.zig around lines 50 to 56, the heap-allocated output slice
from create_result.output is printed but never freed, causing a memory leak. Fix
this by adding a defer statement or an explicit call to allocator.free(output)
immediately after printing to release the allocated memory properly.
| const u256_start = std.time.nanoTimestamp(); | ||
| const test_value: u256 = 0xDEADBEEF_CAFEBABE_12345678_9ABCDEF0_11111111_22222222_33333333_44444444; | ||
|
|
There was a problem hiding this comment.
u256 is undefined – compilation will fail
u256 is not a builtin Zig type. Unless something in evm adds it to the top-level namespace via usingnamespace, this will not compile.
-const test_value: u256 = 0xDEADBEEF_CAFEBABE_12345678_9ABCDEF0_11111111_22222222_33333333_44444444;
+const U256 = evm.types.u256; // or whatever re-export your crate provides
+const test_value: U256 = 0xDEADBEEF_CAFEBABE_12345678_9ABCDEF0_11111111_22222222_33333333_44444444;📝 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.
| const u256_start = std.time.nanoTimestamp(); | |
| const test_value: u256 = 0xDEADBEEF_CAFEBABE_12345678_9ABCDEF0_11111111_22222222_33333333_44444444; | |
| const u256_start = std.time.nanoTimestamp(); | |
| const U256 = evm.types.u256; // or whatever re-export your crate provides | |
| const test_value: U256 = 0xDEADBEEF_CAFEBABE_12345678_9ABCDEF0_11111111_22222222_33333333_44444444; |
🤖 Prompt for AI Agents
In test/bench/memory_benchmark.zig around lines 31 to 33, the type u256 is used
but it is not defined or imported, causing compilation failure. To fix this,
either define the u256 type in this file or import it properly from the evm
package or module where it is declared. Ensure that the namespace is correctly
referenced or brought into scope with usingnamespace if needed.
simple_create_test.zig
Outdated
| const creator_address: [20]u8 = [_]u8{0x10} ++ [_]u8{0x00} ** 19; | ||
| try vm.state.set_balance(creator_address, std.math.maxInt(u256)); | ||
|
|
There was a problem hiding this comment.
u256 alias missing
Same issue as above – std.math.maxInt(u256) will not compile. Define/alias the type once at the top of the file.
-const Address = @import("Address");
+const Address = @import("Address");
+const U256 = evm.types.u256; // fix
...
-try vm.state.set_balance(creator_address, std.math.maxInt(u256));
+try vm.state.set_balance(creator_address, std.math.maxInt(U256));📝 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.
| const creator_address: [20]u8 = [_]u8{0x10} ++ [_]u8{0x00} ** 19; | |
| try vm.state.set_balance(creator_address, std.math.maxInt(u256)); | |
| const Address = @import("Address"); | |
| const U256 = evm.types.u256; // fix | |
| // … other top‐level declarations … | |
| const creator_address: [20]u8 = [_]u8{0x10} ++ [_]u8{0x00} ** 19; | |
| try vm.state.set_balance(creator_address, std.math.maxInt(U256)); |
🤖 Prompt for AI Agents
In simple_create_test.zig around lines 21 to 23, the code uses
std.math.maxInt(u256) but the u256 type alias is missing, causing a compilation
error. Define or alias the u256 type at the top of the file before using it, for
example by importing or declaring u256 as a 256-bit unsigned integer type. This
will allow std.math.maxInt(u256) to compile correctly.
| const header_path = "/Users/williamcory/tevm/main/include/foundry_wrapper.h"; | ||
| const file = std.fs.openFileAbsolute(header_path, .{}) catch |err| { | ||
| std.debug.print("Failed to open foundry_wrapper.h: {}\n", .{err}); | ||
| return; | ||
| }; | ||
| defer file.close(); | ||
|
|
||
| std.debug.print("foundry_wrapper.h exists and is accessible\n", .{}); | ||
|
|
||
| // Read the first few lines to see what's in it | ||
| var buffer: [1024]u8 = undefined; | ||
| const bytes_read = file.readAll(&buffer) catch |err| { |
There was a problem hiding this comment.
Hard-coded absolute path – non-portable test
/Users/williamcory/… will never exist on CI or other developers’ machines. Make this path relative to the project root or guard it behind std.fs.cwd() look-ups / env vars.
🤖 Prompt for AI Agents
In test/evm/compiler_integration_test.zig around lines 63 to 74, the file path
to foundry_wrapper.h is hard-coded as an absolute path, which is not portable
across different environments. Replace the absolute path with a relative path
based on the project root or use std.fs.cwd() to construct the path dynamically.
Alternatively, allow the path to be set via an environment variable to ensure
the test can run on any machine or CI environment.
| const file = std.fs.openFileAbsolute(header_path, .{}) catch |err| { | ||
| std.debug.print("Failed to open foundry_wrapper.h: {}\n", .{err}); | ||
| return; | ||
| }; | ||
| defer file.close(); | ||
|
|
||
| std.debug.print("foundry_wrapper.h exists and is accessible\n", .{}); | ||
|
|
||
| // Read the first few lines to see what's in it | ||
| var buffer: [1024]u8 = undefined; | ||
| const bytes_read = file.readAll(&buffer) catch |err| { | ||
| std.debug.print("Failed to read foundry_wrapper.h: {}\n", .{err}); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Early return hides failure
When the header is missing you just return, which still counts as a passing test. Use testing.expect(false) or return err so the test fails visibly.
🤖 Prompt for AI Agents
In test/evm/compiler_integration_test.zig around lines 64 to 77, the code
returns early on file open or read failure, causing the test to pass silently
despite errors. Replace the early returns with calls to testing.expect(false) or
return the error to ensure the test fails visibly when the header file is
missing or unreadable.
| // Benchmark TenThousandHashes contract - actual Solidity compilation and execution | ||
| fn benchmarkTenThousandHashes(allocator: std.mem.Allocator) void { | ||
| compileAndExecuteBenchmark(allocator, "TenThousandHashes", TEN_THOUSAND_HASHES_SOURCE) catch { | ||
| std.debug.print("TenThousandHashes benchmark failed\n", .{}); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Benchmark function signatures likely mismatch zbench’s callback ABI
Each benchmark* function expects an std.mem.Allocator parameter, yet they are registered with bench.add(...) with an empty context object. Double-check zbench’s expected callback type – if zbench calls func(ctx) or func(), compilation will fail.
Fix by conforming to the callback signature or wrap the allocator in the context object.
Also applies to: 82-86, 118-122
🤖 Prompt for AI Agents
In bench/evm.zig around lines 24 to 29, the benchmark functions take an
std.mem.Allocator parameter but are registered with bench.add using an empty
context, causing a signature mismatch with zbench's expected callback ABI. To
fix this, either adjust the benchmark functions to match the callback signature
expected by zbench (likely a single context parameter or no parameters) or wrap
the allocator inside the context object passed to bench.add so the function can
access it correctly. Apply the same fix to the similar functions at lines 82-86
and 118-122.
| // Add EVM Contract benchmark (after rust_step is defined) | ||
| const evm_contract_benchmark = b.addExecutable(.{ | ||
| .name = "evm-contract-benchmark", | ||
| .root_source_file = b.path("bench/evm.zig"), | ||
| .target = target, | ||
| .optimize = .ReleaseFast, | ||
| }); | ||
| evm_contract_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); | ||
| evm_contract_benchmark.root_module.addImport("evm", target_architecture_mod); | ||
| evm_contract_benchmark.root_module.addImport("Address", address_mod); | ||
| evm_contract_benchmark.root_module.addImport("Compiler", compiler_mod); | ||
| evm_contract_benchmark.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| evm_contract_benchmark.root_module.addIncludePath(b.path("include")); | ||
|
|
||
| // Add Rust dependencies to EVM contract benchmark (same as compiler test) | ||
| evm_contract_benchmark.step.dependOn(rust_step); | ||
| evm_contract_benchmark.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| evm_contract_benchmark.linkLibC(); | ||
|
|
||
| // Link system libraries for EVM contract benchmark | ||
| if (target.result.os.tag == .linux) { | ||
| evm_contract_benchmark.linkSystemLibrary("unwind"); | ||
| evm_contract_benchmark.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| evm_contract_benchmark.linkFramework("CoreFoundation"); | ||
| evm_contract_benchmark.linkFramework("Security"); | ||
| } | ||
|
|
||
| const run_evm_contract_benchmark = b.addRunArtifact(evm_contract_benchmark); | ||
|
|
||
| const evm_contract_benchmark_step = b.step("bench-evm-contracts", "Run EVM Contract benchmarks"); | ||
| evm_contract_benchmark_step.dependOn(&run_evm_contract_benchmark.step); | ||
|
|
||
| // Add EVM contract benchmark to the combined benchmark step | ||
| all_benchmark_step.dependOn(&run_evm_contract_benchmark.step); | ||
|
|
There was a problem hiding this comment.
Memory-benchmark dependency drift
all_benchmark_step already depends on the removed memory benchmark (run_evm_memory_benchmark.step earlier in the file). With the new evm_contract_benchmark added, consider deleting obsolete dependencies to avoid build failures and redundant binaries.
🤖 Prompt for AI Agents
In build.zig around lines 804 to 839, the all_benchmark_step still depends on
the removed memory benchmark run_evm_memory_benchmark.step, which is now
obsolete. Remove the dependency on run_evm_memory_benchmark.step from
all_benchmark_step to prevent build failures and avoid building redundant
binaries. Ensure that only current benchmarks like
run_evm_contract_benchmark.step are included as dependencies in
all_benchmark_step.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
test/evm/real_contract_execution_test.zig (1)
124-165: Fallback compilation path still swallows errorsSame pattern here: the outer compilation error and the “simple version” fallback both end in
return;, marking the test as passed regardless of outcome. Propagate the error or add explicit assertions so the test reflects real status.bench/evm.zig (2)
24-29: Benchmark still masks failures
benchmarkTenThousandHashes(and the other two) catch all errors and just print a line, so zbench records a successful run even when compilation/execution failed – the exact issue raised previously. Either let the error bubble or record an explicit failure in the benchmark result.
263-266: Callback signature likely incompatible with zbench
bench.add(..., benchmarkTenThousandHashes, .{})registers a function that expects anAllocator, but zbench callbacks are typicallyfn(ctx: *Context) voidorfn() void. Confirm and adapt—compilation will fail otherwise.build.zig (2)
695-708: Obsolete memory benchmark still built
bench/evm/memory_bench.zigwas deleted, yet the build graph still createsevm-memory-benchmark.zig buildwill now fail with a missing file. Drop this block and the dependency inall_benchmark_step.
710-714:all_benchmark_stepdepends on non-existent stepBecause the memory benchmark above is stale,
all_benchmark_step’s dependencyrun_evm_memory_benchmark.stepbreaks the build. Remove it or gate it behind a file-existence check.
🧹 Nitpick comments (1)
build.zig (1)
574-605: Debug executables clutter the build
debug-testanddebug-simple-testare handy locally but inflate CI time and artefact size. Consider guarding them behind a separate build flag or removing from default graph.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
bench/evm.zig(1 hunks)build.zig(6 hunks)test/evm/real_contract_execution_test.zig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
test/evm/real_contract_execution_test.zig (1)
189-196: VerifyBenchmark()function selectorThe hard-coded selector
{ 0x30, 0x62, 0x7b, 0x7c }should equalbytes4(keccak256("Benchmark()")). Please double-check with a quick script or Web3 tooling; a wrong selector will make later EVM calls no-op or revert.
| var compilation_result = Compiler.Compiler.compile_source( | ||
| allocator, | ||
| "SimpleTest.sol", | ||
| simple_contract, | ||
| settings, | ||
| ) catch |err| { | ||
| std.debug.print("2. Compilation failed: {}\n", .{err}); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
Tests silently succeed on compilation failure
Both early-return blocks (catch |err| { …; return; }) exit the test without signalling failure, so CI will report a green build even when the Solidity source does not compile. Replace the silent return with return err; or try std.testing.expect(false) to make the test fail.
Also applies to: 48-52
🤖 Prompt for AI Agents
In test/evm/real_contract_execution_test.zig around lines 33 to 41, the test
silently returns on compilation failure without signaling an error, causing CI
to report success incorrectly. Replace the silent return with either returning
the error (return err;) or explicitly failing the test using try
std.testing.expect(false) to ensure the test fails visibly on compilation
errors. Apply the same fix to the similar early-return block at lines 48 to 52.
bench/evm.zig
Outdated
| vm.context.gas_limit = 30_000_000; // 30M gas | ||
| vm.context.gas_price = 1; | ||
| vm.context.caller = Address.from_u256(0x2000); | ||
| vm.context.address = contract_address; | ||
| vm.context.value = 0; | ||
| vm.context.data = &call_data; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Slice vs pointer type mismatch
vm.context.data = &call_data passes a *[4]u8; Vm.Context.data almost certainly expects []const u8. Use a slice:
- vm.context.data = &call_data;
+ vm.context.data = call_data[0..];📝 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.
| vm.context.gas_limit = 30_000_000; // 30M gas | |
| vm.context.gas_price = 1; | |
| vm.context.caller = Address.from_u256(0x2000); | |
| vm.context.address = contract_address; | |
| vm.context.value = 0; | |
| vm.context.data = &call_data; | |
| vm.context.gas_limit = 30_000_000; // 30M gas | |
| vm.context.gas_price = 1; | |
| vm.context.caller = Address.from_u256(0x2000); | |
| vm.context.address = contract_address; | |
| vm.context.value = 0; | |
| - vm.context.data = &call_data; | |
| + vm.context.data = call_data[0..]; |
🤖 Prompt for AI Agents
In bench/evm.zig around lines 196 to 202, the assignment vm.context.data =
&call_data incorrectly uses a pointer to a fixed-size array instead of a slice.
Change this to assign a slice of call_data by using the slice syntax to convert
the array to []const u8, ensuring vm.context.data receives the expected slice
type.
| // Add Real Contract Execution tests (for debugging actual contract execution) | ||
| const real_contract_execution_test = b.addTest(.{ | ||
| .name = "real-contract-execution-test", | ||
| .root_source_file = b.path("test/evm/real_contract_execution_test.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| .single_threaded = true, | ||
| }); | ||
| real_contract_execution_test.root_module.stack_check = false; | ||
|
|
||
| // Add module imports to real contract execution test | ||
| real_contract_execution_test.root_module.addImport("evm", target_architecture_mod); | ||
| real_contract_execution_test.root_module.addImport("Compiler", compiler_mod); | ||
| real_contract_execution_test.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| real_contract_execution_test.root_module.addIncludePath(b.path("include")); | ||
|
|
||
| // Add Rust dependencies to real contract execution test | ||
| real_contract_execution_test.step.dependOn(rust_step); | ||
| real_contract_execution_test.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| real_contract_execution_test.linkLibC(); | ||
|
|
||
| // Link system libraries for real contract execution test | ||
| if (target.result.os.tag == .linux) { | ||
| real_contract_execution_test.linkSystemLibrary("unwind"); | ||
| real_contract_execution_test.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| real_contract_execution_test.linkFramework("CoreFoundation"); | ||
| real_contract_execution_test.linkFramework("Security"); | ||
| } | ||
|
|
||
| const run_real_contract_execution_test = b.addRunArtifact(real_contract_execution_test); | ||
|
|
||
| // Add a separate step for testing Real Contract Execution | ||
| const real_contract_execution_test_step = b.step("test-real-contract-execution", "Run Real Contract Execution tests"); | ||
| real_contract_execution_test_step.dependOn(&run_real_contract_execution_test.step); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
real_contract_execution_test_step also omitted from test suite
Same omission for the real-contract execution tests; attach the run step to test_step so regressions are caught.
🤖 Prompt for AI Agents
In build.zig around lines 804 to 839, the real_contract_execution_test_step is
created but not added to the main test suite, so its tests are not run during
regression. Fix this by adding a dependency from the main test_step to
real_contract_execution_test_step, ensuring the real contract execution tests
are included in the overall test suite and executed during test runs.
- Fix critical stack order bug in RETURN, REVERT, JUMPI opcodes - Fix stack order in MSTORE, MSTORE8, SSTORE, TSTORE opcodes - Fix CREATE contract deployment to properly store runtime bytecode - Add comprehensive debugging tools and test cases - Improve test pass rate from 608/681 (89.5%) to 620/681 (91.0%) Key fixes: - JUMPI: Correct dest/condition stack order for proper control flow - RETURN: Fix offset/size stack order for correct bytecode return - CREATE: Full contract deployment pipeline now working correctly - TenThousandHashes contract now deploys 151 bytes of runtime code 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Merged upstream changes including MODEXP and precompile improvements while preserving EVM stack order fixes and CREATE contract deployment functionality.
There was a problem hiding this comment.
Actionable comments posted: 22
♻️ Duplicate comments (3)
src/evm/execution/storage.zig (1)
149-153: Same ordering concern applies to TSTORE
op_tstoremirrors the SSTORE change and shares the same risk. Once you verifypop2_unsafe()semantics, adjust both call-sites together to keep them consistent.test/evm/create_contract_test.zig (1)
1-5:⚠️ Potential issueMissing
u256type aliasThe code uses
u256on lines 21, 83, and 138 without defining it. Add the type alias after the imports:const evm = @import("evm"); const Address = @import("Address"); +const u256 = evm.types.u256;build.zig (1)
808-820: Still references removedbench/evm/memory_bench.zig.
evm_memory_benchmarkbuild target and its step remain, andall_benchmark_stepdepends on it.
The file was deleted in this PR → build will fail.Remove the target & dependency or restore the file.
Also applies to: 824-827
🧹 Nitpick comments (19)
show_bug_bytecode.zig (1)
5-5: Prefer a named constant over the throw-away_binding
_ = "6080604052348015600e575f5ffd5b50609780601a5f395ff3fe";keeps the string alive solely to silence the “unused constant” warning.
Declaring it as aconst(or deleting it if genuinely unused) is clearer and avoids misleading readers into thinking it’s intentionally discarded.- _ = "6080604052348015600e575f5ffd5b50609780601a5f395ff3fe"; + const constructor_bytecode = "6080604052348015600e575f5ffd5b50609780601a5f395ff3fe";src/compilers/compiler.zig (1)
313-344: Micro-optimisations & robustness fordecodeHexBytecode
std.mem.startsWith(u8, hex_string, "0x")ignores an upper-case"0X"prefix – considerstd.ascii.caseInsensitiveStartsWith.- You can avoid the manual loop with
std.fmt.hexToBytesorstd.mem.hexToBytes, which also yields better validation and SIMD paths.- Returning
error.InvalidHexLengthis fine, but you might also want to flagerror.InvalidHexCharacterexplicitly for clarity.Example refactor:
- const binary_data = try allocator.alloc(u8, hex_data.len / 2); - for (0..binary_data.len) |i| { - const hex_pair = hex_data[i * 2..i * 2 + 2]; - binary_data[i] = std.fmt.parseInt(u8, hex_pair, 16) catch |err| { - return err; - }; - } + var binary_data = try allocator.alloc(u8, hex_data.len / 2); + errdefer allocator.free(binary_data); + try std.mem.hexToBytes(binary_data, hex_data);test_function_selector.zig (1)
7-20: Minor: usebytesToHexfor cleaner outputInstead of the manual loop:
for (hash) |byte| { std.debug.print("{x:02}", .{byte}); }you can achieve the same with:
std.debug.print("{s}\n", .{std.fmt.bytesToHex(&hash, .lower)});which is shorter and less error-prone.
debug_create_return_issue.zig (1)
41-47: Free theresult.outputbuffer to avoid leaking from the GPA
vm.create_contractallocates theoutputbuffer with the same allocator you pass to the VM.
Becausegpa.deinit()only frees the allocator after main returns, the buffer itself is leaked until process exit.
Reclaim it explicitly so this debug tool also works when embedded in a long-running harness.if (result.output) |output| { std.log.info(" Output bytes: {any}", .{std.fmt.fmtSliceHexLower(output)}); + allocator.free(output); } else { std.log.info(" Output: null"); }test_stack_bug.zig (1)
60-70: Assertcreate_result.successbefore inspecting outputYou print success but never fail the test when it’s
false.
Add atry testing.expect(create_result.success);to make the check actionable.test/evm/contract_deployment_test.zig (1)
80-92: BypassingCREATEskips important code-pathThe test writes bytecode directly via
set_code, omitting gas charging, constructor validation, and code size checks exercised byCREATE. Consider driving deployment throughvm.create_contractto cover the full path and keep the test meaningful once storage rules change.test/evm/opcodes/control_test.zig (1)
221-233: RETURN argument order comment is now misleadingSpec is
offset (top), size. Your push sequence{4,10}yieldsoffset=10on top,size=4below – correct.
The inline comment sayssize=4, offset=10 (on top)which contradicts the stack order. Fix the comment to avoid future confusion.-try test_frame.pushStack(&[_]u256{ 4, 10 }); // size=4, offset=10 (on top) +try test_frame.pushStack(&[_]u256{ 4, 10 }); // push size then offset; offset is on toptest/evm/jump_dispatch_failure_test.zig (2)
4-8: Test has no assertions – it will always passThe body only prints diagnostics; there is nothing that can fail the test runner, so CI will never catch a regression.
Either convert this into apub fn main()debug program or addtesting.expect(...)/try std.testing.expectEqual(...)assertions that encode the hypothesis you are checking.
5-7: Unused arena allocator
var arena = std.heap.ArenaAllocator.init(testing.allocator);is allocated but never used except to silence the “unused” warning.
If you decide to keep the test, drop the arena or actually allocate something with it to justify the cost.debug_tenhashes_detailed.zig (1)
2-4: Unused importAddress
const Address = @import("Address");is never referenced. Zig treats unused imports as errors; remove the line or use the symbol.debug_tenthousandhashes_test.zig (1)
2-4: Unused importAddressSame as the detailed debug file – drop the import or put the type to work.
test/evm/compiler_hex_decode_test.zig (1)
72-99: Reduce double-scanning & tighten ASCII-hex heuristicThe loop beginning at line 76 counts ASCII hex chars and “valid” opcodes in a single pass—good.
However you later still re-iterate over the slice for other analyses. Consider extracting the ASCII/opcode classification into a small helper so the logic lives in one place and you don’t accidentally drift in thresholds. Also, the> 80 %cut-off is empirical; encode it as aconst HEX_THRESHOLD = 80at the top so future readers know it’s intentional.No functional bug, just readability / maintainability.
test/evm/simple_compilation_test.zig (1)
95-109: Factor out duplicate “hex-string detection” logicThe ASCII-hex percentage check is duplicated across tests. Extract it into a small helper (e.g.
fn isLikelyHexString(b: []const u8) bool) inside a shared util module to avoid divergence in future tweaks.No immediate bug, just DRY.
debug_bytecode_detailed.zig (1)
167-168: Enhance PUSH opcode naming for clarityThe current mapping shows "PUSH" for all PUSH opcodes (0x60-0x7f). Consider showing the specific PUSH variant:
- 0x60...0x7f => "PUSH", + 0x60...0x7f => |opcode| blk: { + const n = opcode - 0x5f; + break :blk std.fmt.allocPrint(allocator, "PUSH{}", .{n}) catch "PUSH"; + },Note: This would require storing the formatted strings and freeing them later.
test/evm/compiler_bytecode_issue_test.zig (1)
157-159: Consider documenting the function selector calculationAdd a comment explaining how the test() selector is calculated for clarity:
- // Look for the test() function selector in the runtime bytecode - const test_selector: [4]u8 = .{ 0xf8, 0xa8, 0xfd, 0x6d }; // bytes4(keccak256("test()")) + // Look for the test() function selector in the runtime bytecode + // test_selector = first 4 bytes of keccak256("test()") = 0xf8a8fd6d + const test_selector: [4]u8 = .{ 0xf8, 0xa8, 0xfd, 0x6d };test/evm/dispatch_failure_reproduction_test.zig (1)
119-136: Simplified gas table may lead to misleading conclusions.The hard-coded costs (
3,8, etc.) ignore many variable-cost opcodes (e.g. CALL, CALLDATACOPY, EXP, memory expansion).
Using this approximation to reason about a 45-gas edge case could send you on a wild-goose chase.
Consider driving the real interpreter under trace mode instead, or import the canonical gas table.test/evm/opcodes/enhanced_arithmetic_edge_cases_test.zig (1)
91-97: Be explicit with 256-bit literals to avoid silent truncation.Literals like
0x7FFFFFFFFF…exceed 64 bits.
Althoughcomptime_intcan hold them, converting tou256later can truncate if a cast slips in.
Prefer@as(u256, 0x7FFFFFFFFF…)(or parse viastd.fmt.parseInt) for every >64-bit constant to make intent unmistakable.test/evm/opcodes/enhanced_calldatacopy_edge_cases_test.zig (1)
136-138: Add a clarifying comment about argument order forCALLDATACOPY.
pushStack(&[_]u256{ size, data_offset, mem_offset })is correct (top-of-stack =mem_offset), but this reverses the human-reading order from the spec.
A brief note will save future readers from a head-scratch.test/evm/execution/bitwise_comprehensive_test.zig (1)
8-11: Verify the import path to the implementation module.
@import("../../../src/evm/execution/bitwise.zig")is brittle—three..segments will break if tests move.
If possible, expose the module viabuild.zigand import by package name (@import("evm").execution.bitwise) for stability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
bench/evm.zig(1 hunks)build.zig(6 hunks)debug_bytecode_analysis.zig(1 hunks)debug_bytecode_detailed.zig(1 hunks)debug_create_return_issue.zig(1 hunks)debug_create_simple_with_logging.zig(1 hunks)debug_gas_issue.zig(1 hunks)debug_return_opcode.zig(1 hunks)debug_tenhashes_detailed.zig(1 hunks)debug_tenthousandhashes_test.zig(1 hunks)show_bug_bytecode.zig(1 hunks)src/compilers/compiler.zig(2 hunks)src/evm/constants/gas_constants.zig(3 hunks)src/evm/execution/control.zig(5 hunks)src/evm/execution/memory.zig(2 hunks)src/evm/execution/storage.zig(2 hunks)src/evm/precompiles/blake2f.zig(1 hunks)test/evm/compiler_bytecode_issue_test.zig(1 hunks)test/evm/compiler_hex_decode_test.zig(1 hunks)test/evm/contract_call_setup_test.zig(1 hunks)test/evm/contract_deployment_test.zig(1 hunks)test/evm/contract_initialization_test.zig(1 hunks)test/evm/create_contract_test.zig(1 hunks)test/evm/dispatch_failure_reproduction_test.zig(1 hunks)test/evm/execution/arithmetic_comprehensive_test.zig(1 hunks)test/evm/execution/bitwise_comprehensive_test.zig(1 hunks)test/evm/function_selector_debug_test.zig(1 hunks)test/evm/gas_execution_pattern_test.zig(1 hunks)test/evm/invalid_execution_point_test.zig(1 hunks)test/evm/jump_dispatch_failure_test.zig(1 hunks)test/evm/minimal_execution_test.zig(1 hunks)test/evm/opcodes/control_test.zig(5 hunks)test/evm/opcodes/enhanced_arithmetic_edge_cases_test.zig(1 hunks)test/evm/opcodes/enhanced_calldatacopy_edge_cases_test.zig(1 hunks)test/evm/opcodes/memory_test.zig(5 hunks)test/evm/opcodes/storage_test.zig(10 hunks)test/evm/real_contract_execution_test.zig(1 hunks)test/evm/simple_compilation_test.zig(1 hunks)test/evm/simple_contract_execution_test.zig(1 hunks)test/evm/tenhashes_specific_test.zig(1 hunks)test_function_selector.zig(1 hunks)test_stack_bug.zig(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/evm/precompiles/blake2f.zig
- src/evm/execution/memory.zig
- test/evm/opcodes/storage_test.zig
🚧 Files skipped from review as they are similar to previous changes (1)
- test/evm/real_contract_execution_test.zig
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: CI Checks
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (14)
src/evm/execution/storage.zig (1)
81-85: Confirmpop2_unsafe()ordering before swapping slot/valueThe new assignment assumes that
pop2_unsafe()returns the second-from-top value in.aand the top value in.b.
Ifpop2_unsafe()instead returns them in push/pop order (.a= top,.b= next), this change will silently re-invertslotandvalue, breaking SSTORE semantics.Please verify the contract of
Stack.pop2_unsafe()(and update the comment / code accordingly) or add a focused unit-test that stores and then loads to catch a regression.test/evm/opcodes/memory_test.zig (1)
133-136: Stack push order fixed – good catchAll updated tests now push offset first, value last, ensuring the value sits on the top of the stack as required by the EVM spec.
This eliminates the previous false-positive behaviour and aligns with opcode implementations.Also applies to: 165-168, 199-201, 233-235, 483-485
debug_return_opcode.zig (1)
46-47: Double-check that the VM really allocatedoutputwithallocatorThe
defer if (create_result.output) …assumes the slice was allocated with the arena.
If the VM returns a view into its own memory arena, freeing it here will UB.
Please verify the ownership contract inVm.create_contractbefore merging.src/evm/execution/control.zig (1)
64-68: Confirmpop2_unsafevalue orderingThe fix swaps the operands (
dest = values.b).
This is correct only ifpop2_unsafe()returns{ a = second-popped, b = first-popped }.
If the helper returns them in push order (a= top-of-stack), the jump condition will still be wrong.Please grep the stack implementation or run the JUMPI opcode tests to be sure the semantic change is correct.
src/evm/constants/gas_constants.zig (1)
392-400: Sub-tract may wrap ifcurrent_cost > new_costafter saturationWhen either cost saturates to
maxInt, you bail out early, 👍.
However, ifcurrent_costis just belowmaxIntandnew_costequalsmaxInt, the early-return fires, but in the (unlikely) case thatnew_cost < current_costdue to rounding,new_cost - current_costwill wrap silently.A safer pattern is:
if (new_cost <= current_cost) return 0; return new_cost - current_cost;after the overflow/saturation checks.
test_stack_bug.zig (1)
58-59: Double-free/ownership ambiguity forcreate_result.outputThe VM may already allocate
outputwith its own allocator.
Freeing it unconditionally risks a double-free if the VM also owns it or a leak if it doesn’t.Clarify ownership in
Vm.create_contract’s API doc & drop the manual free if the VM owns it.test/evm/opcodes/control_test.zig (2)
104-108: Great catch on JUMPI argument orderSwitching to
[destination, condition]aligns the test with the EVM spec and prevents false positives.
289-290: Large offset test could hit undefined behaviour
std.math.maxInt(u256)may overflow addressable memory in helper functions.
Consider using a smaller sentinel (e.g.,u256_from_u64(1 << 128)) to keep the VM within representable bounds while still exercising the error branch.test/evm/function_selector_debug_test.zig (1)
1-160: Well-structured bytecode analysis testThe test provides comprehensive bytecode analysis with clear debugging output, proper error handling, and thorough function selector verification. The dispatch pattern analysis is particularly valuable for debugging EVM execution issues.
test/evm/create_contract_test.zig (1)
23-66: Correct contract deployment testThe constructor correctly returns 1 byte containing the STOP opcode (0x00) from the last byte of a 32-byte word in memory. The test expectations properly verify this behavior.
test/evm/tenhashes_specific_test.zig (1)
1-149: Comprehensive benchmark issue reproduction testExcellent test implementation that systematically reproduces the benchmark issue by testing various gas limits. The detailed logging helps identify the exact failure conditions (45 gas + Invalid status).
test/evm/compiler_bytecode_issue_test.zig (1)
73-109: Excellent hex string detection logicThe approach to detect ASCII hex characters vs valid opcodes is clever and effective. The 80% threshold provides a good heuristic for identifying hex-encoded bytecode.
debug_gas_issue.zig (1)
129-143: Confirm&selectormatches the API’s expected slice type.
Contract.initandVm.interpretusually expect[]const u8calldata.
Passing&selectoryields*const [4]u8; that is not automatically coerced to a slice.
If the functions really need[]const u8, passselector[0..](a slice) instead to avoid implicit-pointer mismatches.test/evm/execution/arithmetic_comprehensive_test.zig (1)
23-24: Verifyu256alias exists.
std.math.maxInt(u256)relies on au256type alias. Ifu256isn’t defined in your codebase Zig will fail to compile.
Confirm thattype u256 = @import("…").u256(or similar) is in scope.
src/evm/execution/control.zig
Outdated
| const offset = values.b; // CORRECTED: First popped (was on top) | ||
| const size = values.a; // CORRECTED: Second popped (was second) | ||
|
|
||
| Log.debug("RETURN: offset={}, size={}", .{ offset, size }); | ||
|
|
||
| if (size == 0) { | ||
| @branchHint(.unlikely); | ||
| Log.debug("RETURN: Size is 0, setting empty return data", .{}); | ||
| try frame.return_data.set(&[_]u8{}); | ||
| } else { |
There was a problem hiding this comment.
Log.debug with {} on u256 may not compile
std.fmt does not implement the default {} formatter for arbitrary-width integers.
Use {d} or {x} explicitly (or convert to usize/[]const u8).
-Log.debug("RETURN: offset={}, size={}", .{ offset, size });
+Log.debug("RETURN: offset={d}, size={d}", .{ offset, size });Same issue a few lines below with Log.debug("RETURN: Size is 0 …").
📝 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.
| const offset = values.b; // CORRECTED: First popped (was on top) | |
| const size = values.a; // CORRECTED: Second popped (was second) | |
| Log.debug("RETURN: offset={}, size={}", .{ offset, size }); | |
| if (size == 0) { | |
| @branchHint(.unlikely); | |
| Log.debug("RETURN: Size is 0, setting empty return data", .{}); | |
| try frame.return_data.set(&[_]u8{}); | |
| } else { | |
| const offset = values.b; // CORRECTED: First popped (was on top) | |
| const size = values.a; // CORRECTED: Second popped (was second) | |
| - Log.debug("RETURN: offset={}, size={}", .{ offset, size }); | |
| + Log.debug("RETURN: offset={d}, size={d}", .{ offset, size }); | |
| if (size == 0) { | |
| @branchHint(.unlikely); | |
| Log.debug("RETURN: Size is 0, setting empty return data", .{}); | |
| try frame.return_data.set(&[_]u8{}); | |
| } else { |
🤖 Prompt for AI Agents
In src/evm/execution/control.zig around lines 127 to 136, the Log.debug calls
use "{}" to format u256 values, which is unsupported by std.fmt. Replace "{}"
with "{d}" or "{x}" to explicitly format the u256 integers as decimal or
hexadecimal, or convert the values to a supported type like usize or a byte
slice before logging. Apply this fix to all Log.debug calls involving u256 in
this section.
| std.debug.print(" Code hash (first 8 bytes): ", .{}); | ||
| for (code_hash[0..8]) |byte| { | ||
| std.debug.print("{x:02}", .{byte}); | ||
| } | ||
| std.debug.print("\n", .{}); |
There was a problem hiding this comment.
Invalid Zig format string – {:02x} is C-style
Zig’s formatter uses {x:0>2} for zero-padded two-digit hex.
The current string does not compile.
- std.debug.print("{x:02}", .{byte});
+ std.debug.print("{x:0>2}", .{byte});Apply the same fix in debug_bytecode_analysis.zig.
📝 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.
| std.debug.print(" Code hash (first 8 bytes): ", .{}); | |
| for (code_hash[0..8]) |byte| { | |
| std.debug.print("{x:02}", .{byte}); | |
| } | |
| std.debug.print("\n", .{}); | |
| std.debug.print(" Code hash (first 8 bytes): ", .{}); | |
| for (code_hash[0..8]) |byte| { | |
| std.debug.print("{x:0>2}", .{byte}); | |
| } | |
| std.debug.print("\n", .{}); |
🤖 Prompt for AI Agents
In test/evm/minimal_execution_test.zig around lines 80 to 84, the format string
"{:02x}" used for printing hex bytes is invalid in Zig. Replace it with the
correct Zig format "{x:0>2}" to print zero-padded two-digit hex values. Also,
apply this same fix to the format strings in debug_bytecode_analysis.zig where
similar hex formatting is used.
debug_bytecode_analysis.zig
Outdated
| const size_byte = bytecode[i-1]; // Size is the last PUSH before CODECOPY | ||
| const offset_byte = bytecode[i-3]; // Offset is typically 2 PUSHes before | ||
| std.debug.print(" Likely copying {} bytes from offset {}\n", .{size_byte, offset_byte}); | ||
|
|
||
| if (offset_byte < bytecode.len and (offset_byte + size_byte) <= bytecode.len) { | ||
| std.debug.print(" That would copy: {any}\n", .{bytecode[offset_byte..offset_byte + size_byte]}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bounds calculation may overflow for large size_byte
offset_byte + size_byte is computed in u8, then implicitly cast to usize in the slice.
If the sum exceeds 255 the wrap-around yields an out-of-bounds slice.
Convert to usize first and check with saturated math:
const start = @as(usize, offset_byte);
const len = @as(usize, size_byte);
if (start +| len <= bytecode.len) { … }🤖 Prompt for AI Agents
In debug_bytecode_analysis.zig around lines 58 to 64, the calculation of
offset_byte + size_byte is done in u8, which can overflow and cause an
out-of-bounds slice. To fix this, cast offset_byte and size_byte to usize before
adding, then use saturated addition to check bounds safely. Replace the current
bounds check with one that converts both values to usize first and verifies that
their sum does not exceed bytecode.len.
debug_bytecode_analysis.zig
Outdated
| for (bytecode, 0..) |byte, i| { | ||
| std.debug.print(" [{:3}] 0x{:02x} ", .{i, byte}); | ||
| switch (byte) { | ||
| 0x60 => std.debug.print("PUSH1"), |
There was a problem hiding this comment.
Format specifiers are not valid Zig syntax
"{:3}" / "{:02x}" are GCC/printf style.
Replace with Zig equivalents:
- std.debug.print(" [{:3}] 0x{:02x} ", .{i, byte});
+ std.debug.print(" [{d:3}] 0x{x:0>2} ", .{i, byte});This occurs in several places in the file.
📝 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.
| for (bytecode, 0..) |byte, i| { | |
| std.debug.print(" [{:3}] 0x{:02x} ", .{i, byte}); | |
| switch (byte) { | |
| 0x60 => std.debug.print("PUSH1"), | |
| for (bytecode, 0..) |byte, i| { | |
| std.debug.print(" [{d:3}] 0x{x:0>2} ", .{i, byte}); | |
| switch (byte) { | |
| 0x60 => std.debug.print("PUSH1"), |
🤖 Prompt for AI Agents
In debug_bytecode_analysis.zig around lines 21 to 24, the format specifiers
"{:3}" and "{:02x}" used in std.debug.print are invalid in Zig as they follow
GCC/printf style. Replace these with Zig's correct format specifiers: use "{d}"
for decimal integers with width control and "{x}" for hexadecimal output,
applying Zig's syntax for padding and width. Update all similar occurrences in
the file accordingly.
debug_gas_issue.zig
Outdated
| // Function selector for getValue(): bytes4(keccak256("getValue()")) | ||
| const GET_VALUE_SELECTOR: [4]u8 = .{ 0x20, 0x96, 0x52, 0x32 }; // This is an approximation | ||
| try testContractExecution(allocator, "Simple", SIMPLE_CONTRACT_SOURCE, GET_VALUE_SELECTOR); | ||
|
|
||
| // Function selector for test(): bytes4(keccak256("test()")) | ||
| const TEST_SELECTOR: [4]u8 = .{ 0xf8, 0xa8, 0xfd, 0x6d }; | ||
| try testContractExecution(allocator, "Minimal", MINIMAL_CONTRACT_SOURCE, TEST_SELECTOR); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Derive selectors programmatically, not “approximations”.
Hard-coding “approximate” selectors is fragile and will silently mis-route calls once the Solidity optimizer or signature changes. Compute them at compile-time instead:
-const GET_VALUE_SELECTOR: [4]u8 = .{ 0x20, 0x96, 0x52, 0x32 }; // This is an approximation
+const GET_VALUE_SELECTOR: [4]u8 = blk: {
+ var h = std.crypto.hash.sha3.Keccak256.init(.{});
+ h.update("getValue()");
+ var tmp: [32]u8 = undefined;
+ h.final(&tmp);
+ break :blk tmp[0..4].*;
+};Same change for TEST_SELECTOR and BENCHMARK_SELECTOR.
📝 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.
| // Function selector for getValue(): bytes4(keccak256("getValue()")) | |
| const GET_VALUE_SELECTOR: [4]u8 = .{ 0x20, 0x96, 0x52, 0x32 }; // This is an approximation | |
| try testContractExecution(allocator, "Simple", SIMPLE_CONTRACT_SOURCE, GET_VALUE_SELECTOR); | |
| // Function selector for test(): bytes4(keccak256("test()")) | |
| const TEST_SELECTOR: [4]u8 = .{ 0xf8, 0xa8, 0xfd, 0x6d }; | |
| try testContractExecution(allocator, "Minimal", MINIMAL_CONTRACT_SOURCE, TEST_SELECTOR); | |
| // Function selector for getValue(): bytes4(keccak256("getValue()")) | |
| const GET_VALUE_SELECTOR: [4]u8 = blk: { | |
| var h = std.crypto.hash.sha3.Keccak256.init(.{}); | |
| h.update("getValue()"); | |
| var tmp: [32]u8 = undefined; | |
| h.final(&tmp); | |
| break :blk tmp[0..4].*; | |
| }; | |
| try testContractExecution(allocator, "Simple", SIMPLE_CONTRACT_SOURCE, GET_VALUE_SELECTOR); | |
| // Function selector for test(): bytes4(keccak256("test()")) | |
| const TEST_SELECTOR: [4]u8 = .{ 0xf8, 0xa8, 0xfd, 0x6d }; | |
| try testContractExecution(allocator, "Minimal", MINIMAL_CONTRACT_SOURCE, TEST_SELECTOR); |
🤖 Prompt for AI Agents
In debug_gas_issue.zig around lines 179 to 185, the function selectors
GET_VALUE_SELECTOR and TEST_SELECTOR are hard-coded as approximate values, which
is fragile and error-prone. Replace these hard-coded byte arrays by computing
the selectors programmatically at compile-time using a keccak256 hash of the
function signature strings, then taking the first 4 bytes. Apply the same
approach to TEST_SELECTOR and any other selectors like BENCHMARK_SELECTOR to
ensure correctness and maintainability.
bench/evm.zig
Outdated
| &call_data, // input data | ||
| false // not static |
There was a problem hiding this comment.
Pass a slice, not a pointer, for call-data.
Contract.init & Vm.interpret expect []const u8; supplying &call_data gives *[4]u8.
- &call_data, // input data
+ call_data[0..], // input dataand
-const result = vm_ptr.interpret(&contract, &call_data) catch |err| {
+const result = vm_ptr.interpret(&contract, call_data[0..]) catch |err| {Compile-time type mismatch otherwise.
Also applies to: 241-244
🤖 Prompt for AI Agents
In bench/evm.zig around lines 233 to 234 and also lines 241 to 244, the
call_data argument is passed as a pointer (&call_data) but the functions
Contract.init and Vm.interpret expect a slice ([]const u8). Replace &call_data
with call_data directly to pass a slice instead of a pointer, resolving the
compile-time type mismatch.
| try bench.add("TenThousandHashes - Keccak256 Performance", benchmarkTenThousandHashes, .{}); | ||
| try bench.add("SnailTracer - Ray Tracing Algorithm", benchmarkSnailTracer, .{}); | ||
| try bench.add("ERC20Transfer - Token Transfer Operations", benchmarkERC20Transfer, .{}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Callback signature likely wrong for zbench.
bench.add(..., benchmarkTenThousandHashes, .{}) registers a function that takes std.mem.Allocator, but zbench’s ABI is fn(ctx: *anyopaque) void (or fn() void).
Either wrap the allocator in the context object or change the function signature; otherwise this will not compile.
🤖 Prompt for AI Agents
In bench/evm.zig around lines 311 to 313, the benchmark functions passed to
bench.add have a signature expecting std.mem.Allocator, but zbench requires a
callback with signature fn(ctx: *anyopaque) void or fn() void. To fix this,
modify the benchmark functions to accept a single context pointer parameter and
extract the allocator from it, or wrap the allocator inside a context struct
passed to the function, ensuring the function signature matches zbench's
expected ABI.
| fn benchmarkTenThousandHashes(allocator: std.mem.Allocator) void { | ||
| compileAndExecuteBenchmark(allocator, "TenThousandHashes", TEN_THOUSAND_HASHES_SOURCE) catch { | ||
| std.debug.print("TenThousandHashes benchmark failed\n", .{}); | ||
| }; | ||
| } |
There was a problem hiding this comment.
Error is swallowed – benchmark still reported as “success”.
Each benchmark* wrapper catches compileAndExecuteBenchmark and only prints, so zbench sees a successful run even when compilation/execution failed.
Propagate the error instead:
-compileAndExecuteBenchmark(allocator, "TenThousandHashes", TEN_THOUSAND_HASHES_SOURCE) catch {
- std.debug.print("TenThousandHashes benchmark failed\n", .{});
-};
+try compileAndExecuteBenchmark(
+ allocator,
+ "TenThousandHashes",
+ TEN_THOUSAND_HASHES_SOURCE,
+);Apply the same change to all benchmark wrappers.
Also applies to: 82-86, 118-122
🤖 Prompt for AI Agents
In bench/evm.zig around lines 25 to 29, the benchmark wrapper function catches
errors from compileAndExecuteBenchmark but only prints a message without
propagating the error, causing the benchmark to be reported as successful even
on failure. Modify the function to propagate the error by returning it instead
of catching it silently. Apply the same fix to the benchmark wrappers at lines
82-86 and 118-122 to ensure all errors are properly propagated and failures are
correctly reported.
| // Add Compiler Integration tests (after rust dependencies are set up) | ||
| const compiler_integration_test = b.addTest(.{ | ||
| .name = "compiler-integration-test", | ||
| .root_source_file = b.path("test/evm/compiler_integration_test.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| .single_threaded = true, | ||
| }); | ||
| compiler_integration_test.root_module.stack_check = false; | ||
|
|
||
| // Add module imports to compiler integration test | ||
| compiler_integration_test.root_module.addImport("evm", target_architecture_mod); | ||
| compiler_integration_test.root_module.addImport("Compiler", compiler_mod); | ||
| compiler_integration_test.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| compiler_integration_test.root_module.addIncludePath(b.path("include")); | ||
|
|
||
| // Add Rust dependencies to compiler integration test (same as compiler test) | ||
| compiler_integration_test.step.dependOn(rust_step); | ||
| compiler_integration_test.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| compiler_integration_test.linkLibC(); | ||
|
|
||
| // Link system libraries for compiler integration test | ||
| if (target.result.os.tag == .linux) { | ||
| compiler_integration_test.linkSystemLibrary("unwind"); | ||
| compiler_integration_test.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| compiler_integration_test.linkFramework("CoreFoundation"); | ||
| compiler_integration_test.linkFramework("Security"); | ||
| } | ||
|
|
||
| const run_compiler_integration_test = b.addRunArtifact(compiler_integration_test); | ||
|
|
||
| // Add a separate step for testing Compiler Integration | ||
| const compiler_integration_test_step = b.step("test-compiler-integration", "Run Compiler Integration tests"); | ||
| compiler_integration_test_step.dependOn(&run_compiler_integration_test.step); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several new test suites are not executed in CI.
compiler_integration_test, real_contract_execution_test, simple_contract_execution_test, compiler_hex_decode_test … are defined but never added to test_step.
Add:
test_step.dependOn(&run_compiler_integration_test.step);
test_step.dependOn(&run_real_contract_execution_test.step);
test_step.dependOn(&run_simple_contract_execution_test.step);
test_step.dependOn(&run_compiler_hex_decode_test.step);(or use a helper to register automatically) to ensure they run in CI.
Also applies to: 947-952, 975-977, 1023-1026
🤖 Prompt for AI Agents
In build.zig around lines 881 to 916, the newly defined test suites such as
compiler_integration_test are not added as dependencies to the main test_step,
so they are not executed in CI. To fix this, add lines to make test_step depend
on the run steps of these new tests, for example, add
test_step.dependOn(&run_compiler_integration_test.step) and similarly for
real_contract_execution_test, simple_contract_execution_test, and
compiler_hex_decode_test. Repeat this pattern for the other test suites defined
around lines 947-952, 975-977, and 1023-1026 to ensure all new tests are
included in the CI test run.
…prove storage debugging - Added 11 comprehensive tests for EIP-2929 and EIP-2930 access list functionality - Test edge cases including zero/max addresses and storage slots - Validate gas cost constants match EIP specifications - Test storage slot collision detection and independence - Test transaction initialization and state clearing - Test pre-warming functionality for addresses and storage slots - Test call cost calculations for cold vs warm addresses - Validate EIP-2929 compliance (always warm tx.origin/coinbase) - Validate EIP-2930 compliance (access list pre-warming) - Test storage key equality and hashing functionality - Add debug logging to SLOAD and SSTORE operations for better traceability 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 21
🔭 Outside diff range comments (1)
test/evm/opcodes/memory_comprehensive_test.zig (1)
191-210:⚠️ Potential issueStack-argument order for MSTORE / MSTORE8 reversed
For 0x52 (MSTORE) and 0x53 (MSTORE8) the EVM expects offset on top of the stack, value underneath – i.e. you must
PUSH valuefirst, thenPUSH offset.
The test cases here push{ offset, value }, leaving value on top, so an implementation that follows the Yellow-Paper will fail these tests even though it is correct.-try test_frame.pushStack(&[_]u256{ 0, value1 }); // offset, value (value on top per EVM spec) +try test_frame.pushStack(&[_]u256{ value1, 0 }); // value, offset (offset ends up on top)The same swap is required for every
pushStackcall marked with~above (lines listed in the range header).Please adjust all occurrences or the tests will produce false-negatives.
Also applies to: 200-210, 208-210, 234-240, 247-249, 260-262, 286-288, 297-299, 305-307, 367-376, 390-392, 420-422, 443-445, 494-496, 503-504, 511-513, 520-522
♻️ Duplicate comments (5)
bench/evm.zig (3)
25-29: Errors are still silently swallowed – thecatch { ... }blocks remain unchanged.
Previously flagged; please propagate the error instead of printing.Also applies to: 82-86, 118-122
311-314: Callback signature still mismatcheszbenchABI
bench.addreceives a fn that expects anAllocator, butzbenchcallbacks arefn(ctx: *anyopaque) void(orfn() void). Compilation will fail.
Previously pointed out, still unresolved.
228-236: Pass slices, not pointers, for call-data
Contract.initandVm.interpretexpect[]const u8; passing&call_datayields*[4]u8.- &call_data, // input data + call_data[0..], // input data ... - const result = vm_ptr.interpret(&contract, &call_data) catch |err| { + const result = vm_ptr.interpret(&contract, call_data[0..]) catch |err| {This was raised earlier but persists. Fix will unblock compilation.
Also applies to: 242-244
build.zig (2)
808-826:⚠️ Potential issueRemove references to deleted memory benchmark to prevent build failure.
The
bench/evm/memory_bench.zigfile was removed according to the PR summary, but the build configuration still references it. This will cause a build failure.Remove these lines to fix the build:
- // Add EVM Memory benchmark - const evm_memory_benchmark = b.addExecutable(.{ - .name = "evm-memory-benchmark", - .root_source_file = b.path("bench/evm/memory_bench.zig"), - .target = target, - .optimize = .ReleaseFast, - }); - evm_memory_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); - evm_memory_benchmark.root_module.addImport("evm", evm_mod); - - const run_evm_memory_benchmark = b.addRunArtifact(evm_memory_benchmark); - - const evm_memory_benchmark_step = b.step("bench-evm-memory", "Run EVM Memory benchmarks"); - evm_memory_benchmark_step.dependOn(&run_evm_memory_benchmark.step); - // Add combined benchmark step (EVM contract benchmark will be added later) const all_benchmark_step = b.step("bench", "Run all benchmarks"); all_benchmark_step.dependOn(&run_memory_benchmark.step); - all_benchmark_step.dependOn(&run_evm_memory_benchmark.step);
1433-1453: 🛠️ Refactor suggestionAdd missing test suites to ensure they run in CI.
Many newly created test suites are not added as dependencies to
test_step, so they won't run duringzig build test.Add the missing test dependencies:
// Define test step for all tests const test_step = b.step("test", "Run unit tests"); test_step.dependOn(&run_lib_unit_tests.step); test_step.dependOn(&run_exe_unit_tests.step); test_step.dependOn(&run_evm_test.step); test_step.dependOn(&run_server_test.step); test_step.dependOn(&run_rlp_test.step); test_step.dependOn(&run_abi_test.step); test_step.dependOn(&run_compiler_test.step); test_step.dependOn(&run_trie_test.step); test_step.dependOn(&run_interpreter_test.step); test_step.dependOn(&run_memory_test.step); test_step.dependOn(&run_stack_test.step); test_step.dependOn(&run_stack_validation_test.step); test_step.dependOn(&run_opcodes_test.step); test_step.dependOn(&run_vm_opcode_test.step); test_step.dependOn(&run_integration_test.step); test_step.dependOn(&run_gas_test.step); test_step.dependOn(&run_static_protection_test.step); test_step.dependOn(&run_create_contract_test.step); test_step.dependOn(&run_compiler_bytecode_issue_test.step); + test_step.dependOn(&run_compiler_integration_test.step); + test_step.dependOn(&run_real_contract_execution_test.step); + test_step.dependOn(&run_simple_contract_execution_test.step); + test_step.dependOn(&run_compiler_hex_decode_test.step); + test_step.dependOn(&run_function_selector_debug_test.step); + test_step.dependOn(&run_contract_deployment_test.step); + test_step.dependOn(&run_contract_call_setup_test.step); + test_step.dependOn(&run_tenhashes_specific_test.step); + test_step.dependOn(&run_simple_compilation_test.step); + test_step.dependOn(&run_minimal_execution_test.step); + test_step.dependOn(&run_contract_initialization_test.step); + test_step.dependOn(&run_gas_execution_pattern_test.step); + test_step.dependOn(&run_invalid_execution_point_test.step); + test_step.dependOn(&run_dispatch_failure_test.step); + test_step.dependOn(&run_jump_dispatch_failure_test.step); + test_step.dependOn(&run_lt_stack_underflow_test.step); + test_step.dependOn(&run_lt_opcode_bug_test.step); + test_step.dependOn(&run_lt_opcode_simple_test.step); + test_step.dependOn(&run_mstore_debug_test.step); + test_step.dependOn(&run_contract_execution_debug_test.step); test_step.dependOn(&run_precompile_test.step);
🧹 Nitpick comments (15)
test_database_simple.zig (2)
178-181: Remove noisystd.debug.printstatements from unit testsExcessive debug printing clutters test output and slows CI. Prefer keeping tests silent unless a failure occurs (use
testing.expectmessages instead).
200-202: Same concern about un-checked error and noisy prints
- Add
tryifvm.state.get_storagecan error.- Consider removing the debug print (see previous remark).
debug_storage_issue.zig (1)
25-28: Trim verbose debug prints in testsThese prints make the test log noisy for routine CI runs. Remove or guard behind a verbosity flag.
src/evm/execution/comparison.zig (1)
18-21: Avoid code repetition for stack-underflow guardSix identical
if (frame.stack.size < N) { … }blocks are duplicated. Consider a tiny helper:inline fn requireStackSize(frame: *Frame, need: usize) ExecutionError.Error!void { if (frame.stack.size < need) return error.StackUnderflow; }and call
try requireStackSize(frame, 2);etc.Also applies to: 43-46, 68-71, 96-99, 124-127, 148-151
test/evm/lt_opcode_bug_test.zig (2)
29-31: Update stale commentaryThe comment still says “EXPECTED: This test should FAIL …”. The bug is now fixed, so the note is misleading.
-// EXPECTED: This test should FAIL because op_lt currently calls unreachable -// AFTER FIX: This test should pass with StackUnderflow error +// Expect StackUnderflow (regression test for underflow bug that was fixed)
15-17: Suppress unnecessary debug prints in automated testsPrinting inside tests is helpful when developing but pollutes CI logs. Remove or behind feature flag.
Also applies to: 21-22, 33-35, 44-46, 60-62
test/evm/lt_opcode_simple_test.zig (1)
4-35: This “test” never asserts anythingThe file only prints debugging text; it cannot fail and therefore does not
guard against regressions. Consider converting this into:
- a proper unit test that executes LT with an empty stack and expects
ExecutionError.Error.StackUnderflow, or- a doc-style example under
docs/so it does not bloat the test suite.test/evm/lt_opcode_stack_underflow_test.zig (1)
9-156: Heavy debug printouts, zero execution – turn into real assertionsThis file is 150 + lines of
std.debug.printwith only two trivial
testing.expectcalls. The test compiles but provides little automated
coverage and significantly increases compile/output size.Recommend:
- Reduce prints – keep only information needed when a check fails.
- Add real execution of the minimal bytecode against the VM and assert the returned error/gas.
This will turn the script from a narrative into a valuable regression test.
test/evm/opcodes/memory_comprehensive_test.zig (1)
291-294: Comment contradicts expectationThe inline comment states the first memory access “should cost 6 gas”
but the assertion accepts any value>= 3. Either fix the expectation
or update the comment – the current mismatch is confusing.test_tenhashes_fixed.zig (2)
82-85: Avoid hard-coding the selector – compute it onceKeeping
benchmark_selectoras a magic constant risks silent breakage if
the Solidity signature changes. Compute it programmatically:-const benchmark_selector: [4]u8 = .{ 0x30, 0x62, 0x7b, 0x7c }; +const benchmark_selector: [4]u8 = blk: { + const sig = "Benchmark()"; + var h = std.crypto.hash.sha3.Keccak256.init(.{}); + h.update(sig); + var full: [32]u8 = undefined; + h.final(&full); + break :blk full[0..4].*; // deref slice to array +};This guarantees consistency with the contract and removes repetitive
maintenance work.
92-104: Repeated allocation / destruction inside the gas-limit loop
Contract.initallocates storage that is destroyed each iteration. For
largegas_limitsarrays this churn is unnecessary. Create one
contract_instanceand only resetgas_leftbetween runs, or reuse the
same instance withContract.reset.
This will cut allocator traffic and make the benchmark more stable.src/evm/access_list/access_list.zig (1)
140-160: Good basic coverage – consider an assertion helperThe repetition of
try testing.expectEqual(COLD_ACCOUNT_ACCESS_COST, cost1);throughout the suite could be extracted into a tiny helper (e.g.
fn expectCold(cost: u64) !void) to keep the signal-to-noise ratio high
and avoid copy-paste errors.test/evm/opcodes/enhanced_stack_edge_cases_test.zig (1)
552-557: Remove unusedfinal_statevariable
var final_state = [4]u256{undefined, undefined, undefined, undefined};is never read and triggers an “unused local constant” warning.
Simply drop it:- const final_state = [4]u256{ undefined, undefined, undefined, undefined };test/evm/opcodes/enhanced_memory_expansion_edge_cases_test.zig (1)
216-229: Comment / assertion inconsistencyCode asserts
size_after_mstore8 == 96, but the comment says “= 128 (4 words)”.
Either update the comment or the expected value to avoid confusion.test/evm/opcodes/enhanced_extcodecopy_edge_cases_test.zig (1)
68-69: Hard-coded 2 500 gas delta is brittle
try testing.expectEqual(2500, …)asserts the current Berlin-schedule cold-vs-warm difference (2600 – 100).
Future forks (e.g., Shanghai) or local tuning will break the test even if functionality is correct.Consider computing the expected delta from constants exposed by the VM instead of hard-coding the magic number.
Also applies to: 459-461
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (32)
bench/evm.zig(1 hunks)build.zig(6 hunks)debug_contract_reality_check.zig(1 hunks)debug_function_selector.zig(1 hunks)debug_gas_issue.zig(1 hunks)debug_storage_issue.zig(1 hunks)smart_merge_strategy.sh(1 hunks)src/compilers/compiler.zig(3 hunks)src/evm/access_list/access_list.zig(1 hunks)src/evm/constants/gas_constants.zig(3 hunks)src/evm/execution/comparison.zig(6 hunks)src/evm/execution/memory.zig(2 hunks)src/evm/execution/storage.zig(2 hunks)test/evm/bytecode_pattern_analysis_test.zig(1 hunks)test/evm/contract_execution_debug_test.zig(1 hunks)test/evm/execution/control_comprehensive_test.zig(1 hunks)test/evm/function_selector_analysis_test.zig(1 hunks)test/evm/gas_issue_resolution_test.zig(1 hunks)test/evm/lt_opcode_bug_test.zig(1 hunks)test/evm/lt_opcode_simple_test.zig(1 hunks)test/evm/lt_opcode_stack_underflow_test.zig(1 hunks)test/evm/opcodes/enhanced_bitwise_edge_cases_test.zig(1 hunks)test/evm/opcodes/enhanced_comparison_edge_cases_test.zig(1 hunks)test/evm/opcodes/enhanced_control_flow_edge_cases_test.zig(1 hunks)test/evm/opcodes/enhanced_extcodecopy_edge_cases_test.zig(1 hunks)test/evm/opcodes/enhanced_memory_expansion_edge_cases_test.zig(1 hunks)test/evm/opcodes/enhanced_stack_edge_cases_test.zig(1 hunks)test/evm/opcodes/memory_comprehensive_test.zig(13 hunks)test/evm/selector_correction_test.zig(1 hunks)test_database_simple.zig(1 hunks)test_tenhashes_fixed.zig(1 hunks)update_pr_branches.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/evm/constants/gas_constants.zig
- src/evm/execution/memory.zig
- src/evm/execution/storage.zig
- debug_gas_issue.zig
- src/compilers/compiler.zig
🧰 Additional context used
🪛 Shellcheck (0.10.0)
update_pr_branches.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 33-33: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 50-50: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 58-58: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 70-70: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
smart_merge_strategy.sh
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
[warning] 12-12: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CI Checks
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (18)
test_database_simple.zig (1)
190-193: Verify whetherstate.get_storagereturns an error-unionEarlier in this file
db_interface.get_storageistry-ed, but herestate.get_storageis not. IfEvmState.get_storageis also an!u256, this will fail to compile.
Please double-check the signature and addtryif required.src/evm/execution/comparison.zig (1)
18-22: Great – unreachable replaced with properStackUnderflowerrorThe new early-return guard makes the opcode safe and testable.
test/evm/opcodes/enhanced_comparison_edge_cases_test.zig (1)
1-485: Comprehensive and accurate – nice workThe comparison-opcode edge-case matrix is exhaustive and compiles cleanly.
No further action required.test/evm/opcodes/enhanced_control_flow_edge_cases_test.zig (10)
1-21: Well-documented and comprehensive test suite setup!The documentation clearly outlines the test coverage areas and the imports are appropriate for the testing needs.
22-82: Excellent coverage of JUMP opcode edge cases!The test thoroughly validates jump destination boundaries, invalid targets, and overflow scenarios. The strategic JUMPDEST placement at positions 0 and 254 effectively tests boundary conditions.
84-151: Thorough testing of JUMPI conditional logic and stack ordering!The test effectively validates the conditional jump behavior, especially the stack order verification (lines 136-143) which confirms that the top stack value is the condition and the second value is the destination.
153-213: Comprehensive PC opcode testing with boundary conditions!Good coverage of edge cases including maximum usize values and sequential PC operations that verify stack accumulation behavior.
215-296: Excellent RETURN opcode testing with memory expansion scenarios!The test effectively validates memory expansion gas costs, overflow protection for both offset and size parameters, and correct data copying. The zero-size return test (lines 234-240) correctly verifies no gas is consumed for memory expansion.
298-377: Well-designed REVERT opcode tests with realistic error scenarios!Good inclusion of typical Solidity error signatures and comprehensive boundary condition testing. The flexible error expectations in the boundary test (lines 373-376) appropriately handle implementation-specific constraints.
379-463: Comprehensive SELFDESTRUCT testing with EIP-2929 compliance!Excellent coverage of access list gas accounting, distinguishing between cold (2600 additional gas) and warm access costs. The static context write protection test ensures proper enforcement of EVM safety rules.
465-519: Correct INVALID opcode testing with gas consumption verification!The test properly validates that INVALID consumes all remaining gas while preserving the stack state, which is the expected behavior for this opcode.
521-597: Excellent table-driven gas cost verification!The structured approach makes it easy to verify gas costs for all control flow operations. The special handling for INVALID (all gas) and SELFDESTRUCT (cold access cost) is correctly implemented.
599-677: Creative complex control flow interaction testing!The test effectively validates realistic scenarios like using PC values for conditional jump decisions and chained conditional jumps. This provides confidence in the control flow implementation's correctness.
build.zig (3)
108-110: Correct addition of include path for C imports.This is necessary for the compiler module to properly handle C imports from the Rust integration.
828-854: Well-configured tevm-runner for benchmarking integration.The executable is properly set up with all necessary module imports and installation steps for the evm-bench integration.
881-1422: Consistent and well-structured test configurations.All new test suites follow a consistent pattern for module imports, Rust integration, and system library linking. This uniformity aids maintainability.
test/evm/opcodes/enhanced_extcodecopy_edge_cases_test.zig (2)
44-49: Confirm EXTCODECOPY argument orderThe stack push uses
{ size, code_offset, mem_offset, address }, yet the EVM spec orders the arguments asaddress, destOffset, offset, length.
Ifhelpers.pushStackdoes not reverse the order internally, these tests are exercising a non-spec call pattern.Please double-check the helper’s behaviour or swap the order to avoid false positives.
Also applies to: 56-60
510-526: Zero-size copy with max offsets may still overflowSome EVM engines treat
mem_offset + sizeoverflow as an immediate error even whensize == 0.
The case withmem_offset = maxInt(usize)assumes only a warm-access 100-gas charge and no error – this may fail on stricter engines.Recommend adding an
expectErrorfallback or guarding the test with implementation-specific feature flags.
test_database_simple.zig
Outdated
| // Verify they're the same | ||
| try testing.expect(std.mem.eql(u8, &contract_address, &contract.address)); | ||
| try testing.expect(std.mem.eql(u8, &contract.address, &frame.contract.address)); |
There was a problem hiding this comment.
std.mem.eql called with array pointers – causes type mismatch
&contract_address is a *const [20]u8, but std.mem.eql expects slices ([]const u8). The code will not compile.
-try testing.expect(std.mem.eql(u8, &contract_address, &contract.address));
-try testing.expect(std.mem.eql(u8, &contract.address, &frame.contract.address));
+try testing.expect(std.mem.eql(u8, contract_address[0..], contract.address[0..]));
+try testing.expect(std.mem.eql(u8, contract.address[0..], frame.contract.address[0..]));📝 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.
| // Verify they're the same | |
| try testing.expect(std.mem.eql(u8, &contract_address, &contract.address)); | |
| try testing.expect(std.mem.eql(u8, &contract.address, &frame.contract.address)); | |
| // Verify they're the same | |
| try testing.expect(std.mem.eql(u8, contract_address[0..], contract.address[0..])); | |
| try testing.expect(std.mem.eql(u8, contract.address[0..], frame.contract.address[0..])); |
🤖 Prompt for AI Agents
In test_database_simple.zig around lines 182 to 184, the calls to std.mem.eql
use pointers to arrays (&contract_address) instead of slices, causing a type
mismatch. Fix this by passing slices of the arrays instead of pointers, for
example by slicing with [:] to convert the array pointer to a slice before
calling std.mem.eql.
debug_storage_issue.zig
Outdated
| try testing.expect(std.mem.eql(u8, &test_helpers.TestAddresses.CONTRACT, &contract.address)); | ||
| try testing.expect(std.mem.eql(u8, &contract.address, &test_frame.frame.contract.address)); |
There was a problem hiding this comment.
Slice conversion needed for std.mem.eql
Same array-vs-slice issue as in test_database_simple.zig.
-try testing.expect(std.mem.eql(u8, &test_helpers.TestAddresses.CONTRACT, &contract.address));
-try testing.expect(std.mem.eql(u8, &contract.address, &test_frame.frame.contract.address));
+try testing.expect(std.mem.eql(u8, test_helpers.TestAddresses.CONTRACT[0..], contract.address[0..]));
+try testing.expect(std.mem.eql(u8, contract.address[0..], test_frame.frame.contract.address[0..]));📝 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.
| try testing.expect(std.mem.eql(u8, &test_helpers.TestAddresses.CONTRACT, &contract.address)); | |
| try testing.expect(std.mem.eql(u8, &contract.address, &test_frame.frame.contract.address)); | |
| try testing.expect(std.mem.eql(u8, test_helpers.TestAddresses.CONTRACT[0..], contract.address[0..])); | |
| try testing.expect(std.mem.eql(u8, contract.address[0..], test_frame.frame.contract.address[0..])); |
🤖 Prompt for AI Agents
In debug_storage_issue.zig around lines 30 to 31, the calls to std.mem.eql are
incorrectly passing pointers to arrays instead of slices. Fix this by converting
the arrays to slices before passing them to std.mem.eql, ensuring both arguments
are slices of u8 for proper comparison.
update_pr_branches.sh
Outdated
| worktree_path=$(git worktree list | grep "\[$branch_name\]" | awk '{print $1}') | ||
|
|
||
| if [ -n "$worktree_path" ]; then | ||
| echo "Found worktree at: $worktree_path" | ||
| cd "$worktree_path" | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cd … || exit 1 and remove hard-coded paths
Un-checked cd can leave the script in an unexpected directory; absolute user paths break portability.
- cd "$worktree_path"
+ cd "$worktree_path" || exit 1
…
- cd "/Users/williamcory/tevm/main"
+ cd "$(git rev-parse --show-toplevel)" || exit 1Replicate for the other cd sites.
Also applies to: 50-50, 58-58, 70-70
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 33-33: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In update_pr_branches.sh around lines 29 to 34, the cd command is used without
checking if it succeeds, which can cause the script to continue in an unexpected
directory. Modify the cd commands to include `|| exit 1` to ensure the script
exits if changing directories fails. Also, remove any hard-coded absolute user
paths to improve portability. Apply these changes similarly to the cd commands
at lines 50, 58, and 70.
update_pr_branches.sh
Outdated
| #\!/bin/bash | ||
|
|
||
| # Array of branches that need updating (branch_name:pr_number) | ||
| branches=( |
There was a problem hiding this comment.
Broken shebang – script won’t execute
The backslash stops the kernel from recognising the interpreter.
-#\!/bin/bash
+#!/usr/bin/env bash
+
+set -euo pipefail🧰 Tools
🪛 Shellcheck (0.10.0)
[error] 1-1: Tips depend on target shell and yours is unknown. Add a shebang or a 'shell' directive.
(SC2148)
🤖 Prompt for AI Agents
In update_pr_branches.sh at lines 1 to 4, the shebang line is incorrect due to a
backslash before the exclamation mark, which prevents the script from executing.
Remove the backslash so the first line reads exactly "#!/bin/bash" to correctly
specify the interpreter.
| std.debug.print("getValue() - Wrong: 0x{x:08}, Correct: 0x{x:08}\n", .{ | ||
| std.mem.readInt(u32, &wrong_getValue_selector, .big), | ||
| std.mem.readInt(u32, &getValue_selector, .big) | ||
| }); | ||
|
|
||
| std.debug.print("test() - Wrong: 0x{x:08}, Correct: 0x{x:08}\n", .{ | ||
| std.mem.readInt(u32, &wrong_test_selector, .big), | ||
| std.mem.readInt(u32, &test_selector, .big) | ||
| }); | ||
|
|
||
| std.debug.print("Benchmark() - Wrong: 0x{x:08}, Correct: 0x{x:08}\n", .{ | ||
| std.mem.readInt(u32, &wrong_benchmark_selector, .big), | ||
| std.mem.readInt(u32, &benchmark_selector, .big) | ||
| }); |
There was a problem hiding this comment.
std.mem.readInt invoked with pointer-to-array instead of slice – compilation fails
std.mem.readInt expects a []const u8 slice. Passing &some_array yields a
*[4]u8 which does not coerce to a slice.
-std.mem.readInt(u32, &wrong_getValue_selector, .big)
+std.mem.readInt(u32, wrong_getValue_selector[0..], .big)Apply the same change to every call that currently uses &{array} (lines 56-69, 94-102).
Without this fix the test file will not compile.
Also applies to: 94-102
🤖 Prompt for AI Agents
In test/evm/selector_correction_test.zig lines 56-69 and 94-102, std.mem.readInt
is incorrectly called with pointers to arrays (e.g., &wrong_getValue_selector)
which causes compilation errors because it expects a slice ([]const u8). Fix
this by passing the array directly without the address-of operator to provide a
slice, for example, replace &wrong_getValue_selector with
wrong_getValue_selector in all such calls.
| test "contract execution debug - gas pattern analysis" { | ||
| const allocator = testing.allocator; | ||
|
|
||
| std.debug.print("\n=== CONTRACT EXECUTION DEBUG - GAS PATTERN ANALYSIS ===\n", .{}); | ||
|
|
||
| // Import modules | ||
| const Compiler = @import("Compiler"); | ||
| const evm_root = @import("evm"); | ||
| const Address = @import("Address"); | ||
| const evm = evm_root.evm; | ||
|
|
||
| // Test 1: Ultra simple contract that returns a constant | ||
| std.debug.print("\n--- Test 1: Ultra Simple Contract ---\n", .{}); | ||
| const ULTRA_SIMPLE_CONTRACT = | ||
| \\// SPDX-License-Identifier: MIT | ||
| \\pragma solidity ^0.8.17; | ||
| \\ | ||
| \\contract UltraSimple { | ||
| \\ function Benchmark() external pure returns (uint256) { | ||
| \\ return 42; | ||
| \\ } | ||
| \\} | ||
| ; | ||
|
|
||
| try testContractExecution(allocator, Compiler, evm, Address, "UltraSimple", ULTRA_SIMPLE_CONTRACT, "Should return 42, use 100-200 gas"); | ||
|
|
||
| // Test 2: Contract with simple arithmetic | ||
| std.debug.print("\n--- Test 2: Simple Arithmetic ---\n", .{}); | ||
| const SIMPLE_ARITHMETIC_CONTRACT = | ||
| \\// SPDX-License-Identifier: MIT | ||
| \\pragma solidity ^0.8.17; | ||
| \\ | ||
| \\contract SimpleArithmetic { | ||
| \\ function Benchmark() external pure returns (uint256) { | ||
| \\ uint256 a = 10; | ||
| \\ uint256 b = 20; | ||
| \\ return a + b; | ||
| \\ } | ||
| \\} | ||
| ; | ||
|
|
||
| try testContractExecution(allocator, Compiler, evm, Address, "SimpleArithmetic", SIMPLE_ARITHMETIC_CONTRACT, "Should return 30, use 200+ gas"); | ||
|
|
||
| // Test 3: Contract with a small loop | ||
| std.debug.print("\n--- Test 3: Small Loop ---\n", .{}); | ||
| const SMALL_LOOP_CONTRACT = | ||
| \\// SPDX-License-Identifier: MIT | ||
| \\pragma solidity ^0.8.17; | ||
| \\ | ||
| \\contract SmallLoop { | ||
| \\ function Benchmark() external pure returns (uint256) { | ||
| \\ uint256 sum = 0; | ||
| \\ for (uint256 i = 0; i < 3; i++) { | ||
| \\ sum = sum + i; | ||
| \\ } | ||
| \\ return sum; // Should return 0+1+2 = 3 | ||
| \\ } | ||
| \\} | ||
| ; | ||
|
|
||
| try testContractExecution(allocator, Compiler, evm, Address, "SmallLoop", SMALL_LOOP_CONTRACT, "Should return 3, use 300+ gas"); | ||
|
|
||
| // Test 4: Contract with single keccak256 | ||
| std.debug.print("\n--- Test 4: Single Keccak256 ---\n", .{}); | ||
| const SINGLE_KECCAK_CONTRACT = | ||
| \\// SPDX-License-Identifier: MIT | ||
| \\pragma solidity ^0.8.17; | ||
| \\ | ||
| \\contract SingleKeccak { | ||
| \\ function Benchmark() external pure returns (bytes32) { | ||
| \\ return keccak256(abi.encodePacked(uint256(42))); | ||
| \\ } | ||
| \\} | ||
| ; | ||
|
|
||
| try testContractExecution(allocator, Compiler, evm, Address, "SingleKeccak", SINGLE_KECCAK_CONTRACT, "Should return hash, use 200+ gas"); | ||
|
|
||
| std.debug.print("\n=== ANALYSIS ===\n", .{}); | ||
| std.debug.print("If all contracts show ~158 gas usage:\n", .{}); | ||
| std.debug.print("1. Function dispatch works (Success status)\n", .{}); | ||
| std.debug.print("2. Contract logic execution has an issue\n", .{}); | ||
| std.debug.print("3. Possible causes: early RETURN, loop issues, or gas accounting bug\n", .{}); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Debug test lacks assertions
Like the bytecode pattern test, this file prints diagnostics but never calls testing.expect*.
Consider converting to an executable (exe) target or add concrete assertions on result.status, gas_used, and output value so that the CI run provides real protection.
🤖 Prompt for AI Agents
In test/evm/contract_execution_debug_test.zig from lines 4 to 86, the test only
prints diagnostic messages without any assertions, which means it does not
verify correctness automatically. To fix this, add concrete assertions using
testing.expect* functions to check key outcomes such as the execution result
status, gas used, and the returned output value for each contract test. This
will ensure the test fails on unexpected behavior and provides real CI
protection. Optionally, convert this test to an executable target if needed to
enable proper test execution.
| // Push beneficiary address | ||
| try test_frame.pushStack(&[_]u256{@as(u256, @intFromPtr(helpers.TestAddresses.ALICE.bytes.ptr))}); | ||
|
|
||
| // Execute SELFDESTRUCT - should end execution with STOP | ||
| try testing.expectError( |
There was a problem hiding this comment.
Wrong value pushed – using a pointer instead of the address
@intFromPtr(helpers.TestAddresses.ALICE.bytes.ptr) pushes a host memory pointer, not the 20-byte address expected by the EVM stack.
Use the actual address as u256, e.g.:
- try test_frame.pushStack(&[_]u256{@as(u256, @intFromPtr(helpers.TestAddresses.ALICE.bytes.ptr))});
+ try test_frame.pushStack(&[_]u256{ helpers.Address.to_u256(helpers.TestAddresses.ALICE) });Also applies to: 838-842
🤖 Prompt for AI Agents
In test/evm/execution/control_comprehensive_test.zig around lines 804 to 808 and
also lines 838 to 842, the code incorrectly pushes a host memory pointer instead
of the actual 20-byte Ethereum address onto the EVM stack. Replace the use of
@intFromPtr(helpers.TestAddresses.ALICE.bytes.ptr) with the actual address value
converted to u256, ensuring the correct 20-byte address is pushed rather than a
pointer. Adjust the pushStack calls accordingly to use the proper address
representation.
| const pattern = 0x5555555555555555555555555555555555555555555555555555555555555555; | ||
| try test_frame.pushStack(&[_]u256{pattern}); | ||
| _ = try helpers.executeOpcode(0x19, test_vm.vm, test_frame.frame); // NOT | ||
| const not_pattern = try test_frame.popStack(); | ||
| const expected_pattern = 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA; | ||
| try testing.expectEqual(expected_pattern, not_pattern); |
There was a problem hiding this comment.
Correct expected_pattern length for NOT test
expected_pattern must be the exact bit-wise complement of the 256-bit pattern.
Current literal is truncated (48 bytes instead of 32 bytes / 64 hex chars), so the test will always fail.
-const expected_pattern = 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA;
+const expected_pattern = 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA;📝 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.
| const pattern = 0x5555555555555555555555555555555555555555555555555555555555555555; | |
| try test_frame.pushStack(&[_]u256{pattern}); | |
| _ = try helpers.executeOpcode(0x19, test_vm.vm, test_frame.frame); // NOT | |
| const not_pattern = try test_frame.popStack(); | |
| const expected_pattern = 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA; | |
| try testing.expectEqual(expected_pattern, not_pattern); | |
| const pattern = 0x5555555555555555555555555555555555555555555555555555555555555555; | |
| try test_frame.pushStack(&[_]u256{pattern}); | |
| _ = try helpers.executeOpcode(0x19, test_vm.vm, test_frame.frame); // NOT | |
| const not_pattern = try test_frame.popStack(); | |
| const expected_pattern = 0xAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA; | |
| try testing.expectEqual(expected_pattern, not_pattern); |
🤖 Prompt for AI Agents
In test/evm/opcodes/enhanced_bitwise_edge_cases_test.zig around lines 355 to
360, the expected_pattern constant for the NOT opcode test is incorrectly sized;
it is truncated and does not represent the full 256-bit complement of the
pattern. Update expected_pattern to a full 256-bit (32-byte, 64 hex characters)
value that is the exact bitwise complement of the pattern constant to ensure the
test correctly verifies the NOT operation.
| const cold_beneficiary = helpers.toU256(helpers.TestAddresses.BOB); | ||
| try test_frame.pushStack(&[_]u256{cold_beneficiary}); | ||
|
|
||
| const cold_gas_before = test_frame.frame.gas_remaining; | ||
| const cold_result = helpers.executeOpcode(0xFF, test_vm.vm, test_frame.frame); | ||
| try testing.expectError(helpers.ExecutionError.Error.STOP, cold_result); | ||
|
|
||
| const cold_gas_used = cold_gas_before - test_frame.frame.gas_remaining; | ||
| // Should consume base SELFDESTRUCT gas (5000) + cold access cost (2600) | ||
| const expected_cold_cost = 5000 + helpers.opcodes.gas_constants.ColdAccountAccessCost; | ||
| try testing.expectEqual(expected_cold_cost, cold_gas_used); |
There was a problem hiding this comment.
Replace undefined toU256 helper
helpers.toU256 is not declared, breaking compilation.
Use the address helper already employed elsewhere:
-const cold_beneficiary = helpers.toU256(helpers.TestAddresses.BOB);
+const cold_beneficiary = helpers.Address.to_u256(helpers.TestAddresses.BOB);Apply the same replacement for warm_beneficiary, static_beneficiary, self_address, and max_address below.
📝 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.
| const cold_beneficiary = helpers.toU256(helpers.TestAddresses.BOB); | |
| try test_frame.pushStack(&[_]u256{cold_beneficiary}); | |
| const cold_gas_before = test_frame.frame.gas_remaining; | |
| const cold_result = helpers.executeOpcode(0xFF, test_vm.vm, test_frame.frame); | |
| try testing.expectError(helpers.ExecutionError.Error.STOP, cold_result); | |
| const cold_gas_used = cold_gas_before - test_frame.frame.gas_remaining; | |
| // Should consume base SELFDESTRUCT gas (5000) + cold access cost (2600) | |
| const expected_cold_cost = 5000 + helpers.opcodes.gas_constants.ColdAccountAccessCost; | |
| try testing.expectEqual(expected_cold_cost, cold_gas_used); | |
| const cold_beneficiary = helpers.Address.to_u256(helpers.TestAddresses.BOB); | |
| try test_frame.pushStack(&[_]u256{cold_beneficiary}); | |
| const cold_gas_before = test_frame.frame.gas_remaining; | |
| const cold_result = helpers.executeOpcode(0xFF, test_vm.vm, test_frame.frame); | |
| try testing.expectError(helpers.ExecutionError.Error.STOP, cold_result); | |
| const cold_gas_used = cold_gas_before - test_frame.frame.gas_remaining; | |
| // Should consume base SELFDESTRUCT gas (5000) + cold access cost (2600) | |
| const expected_cold_cost = 5000 + helpers.opcodes.gas_constants.ColdAccountAccessCost; | |
| try testing.expectEqual(expected_cold_cost, cold_gas_used); |
🤖 Prompt for AI Agents
In test/evm/opcodes/enhanced_control_flow_edge_cases_test.zig around lines 398
to 408, the helper function toU256 is undefined and causes compilation errors.
Replace all instances of helpers.toU256 with the existing address helper used
elsewhere in the file to convert addresses properly. Apply this replacement not
only for cold_beneficiary but also for warm_beneficiary, static_beneficiary,
self_address, and max_address variables further down in the code.
| const std = @import("std"); | ||
| const testing = std.testing; | ||
| const helpers = @import("test_helpers.zig"); | ||
| const evm = @import("evm"); |
There was a problem hiding this comment.
Remove unused evm import – causes compile failure
const evm = @import("evm"); is never referenced.
Zig rejects unused imports, so the whole test suite will not compile until this line is deleted.
-const evm = @import("evm");📝 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.
| const std = @import("std"); | |
| const testing = std.testing; | |
| const helpers = @import("test_helpers.zig"); | |
| const evm = @import("evm"); | |
| const std = @import("std"); | |
| const testing = std.testing; | |
| const helpers = @import("test_helpers.zig"); |
🤖 Prompt for AI Agents
In test/evm/opcodes/enhanced_extcodecopy_edge_cases_test.zig at lines 1 to 4,
the import statement for "evm" is unused and causes a compile failure. Remove
the line `const evm = @import("evm");` entirely to fix the compilation issue.
There was a problem hiding this comment.
Actionable comments posted: 13
🔭 Outside diff range comments (1)
build.zig (1)
804-838:⚠️ Potential issueStale memory-benchmark targets will break the build
memory_benchmark(test/Bench/memory_benchmark.zig) andevm_memory_benchmark(bench/evm/memory_bench.zig) are still referenced, yet the PR description says the old Zig memory bench was removed in favour ofevm-benchsub-module. Unless those files still exist,zig build bench-memory/bench-evm-memory(and any step that depends on them) will hard-fail at compile time.-const memory_benchmark = b.addExecutable(.{ - .name = "memory-benchmark", - .root_source_file = b.path("test/Bench/memory_benchmark.zig"), -... -const evm_memory_benchmark = b.addExecutable(.{ - .name = "evm-memory-benchmark", - .root_source_file = b.path("bench/evm/memory_bench.zig"), +// either delete these blocks entirely, +// or point them at the new evm-bench equivalent, +// e.g. "bench/evm/bench_memory.zig" (if that exists).Same applies to the run / step wiring and to
all_benchmark_step.dependOn(&run_evm_memory_benchmark.step);.
♻️ Duplicate comments (3)
build.zig (3)
840-844:all_benchmark_stepstill depends on the removed memory benchEven after fixing/retargeting the memory benchmark above, the combined
benchstep will fail whenrun_evm_memory_benchmark.stepis absent. Either drop the dependency or re-create the benchmark target.
898-933: New compiler-integration test never runs inzig build test
compiler_integration_test_stepis created but not attached totest_step. This was pointed out in the previous review and is still outstanding.const test_step = b.step("test", "Run unit tests"); ... test_step.dependOn(&run_compiler_test.step); +test_step.dependOn(&run_compiler_integration_test.step);Replicate for the other new suites (real-contract, simple-contract, hex-decode, etc.).
934-969:real_contract_execution_test_steplikewise omitted from test cascadeSame omission for the real-contract execution tests. Attach the run step to
test_stepso regressions are caught.
🧹 Nitpick comments (9)
bench/simple_math_bench.zig (2)
38-41: Remove dead “divide-by-zero” branches
bis a compile-time constant (0x123456789ABCDEF/0x987654321) and is never zero, so the run-timeif (b == 0)checks are dead code and add avoidable branch mis-prediction noise to the benchmark.- const result = if (b == 0) 0 else a / b; + const result = a / b; // b is compile-time non-zeroSame change applies to the modulo benchmark just below.
This keeps the benchmark tight and free from irrelevant branching.
Also applies to: 51-54
142-162: Shift-count casts are narrower than necessary
byte_num * 8can reach 248, yet you cast the shift amount tou8:const x: u256 = @as(u256, 0x80) << @as(u8, @intCast(byte_num * 8));Technically safe today, but casting to
u8is pointless and may trigger UB if someone bumps the range. Cast directly to an integer wide enough for all future cases (e.g.usize).-const x: u256 = @as(u256, 0x80) << @as(u8, @intCast(byte_num * 8)); +const x: u256 = @as(u256, 0x80) << @intCast(byte_num * 8);bench/stack_bench.zig (1)
11-21: Re-use a singleStackinstance per benchmark iterationEvery benchmark allocates a fresh
Stack{}inside the hot loop, which:
- Skews the measurement toward allocator cost rather than the operation under test.
- Produces lots of short-lived allocations (or initialisations) that the optimiser can’t hoist.
Typical micro-bench patterns create the data structure once and then reset it in-place, e.g.:
var stack = Stack{}; for (0..iterations) |_| { stack.reset(); // or clear manually // perform operation(s) on `stack` std.mem.doNotOptimizeAway(&stack); }Doing this across all push/pop/dup/swap benchmarks will yield more representative numbers and remove allocator noise.
Also applies to: 29-39, 47-60, 69-82, 91-104, 113-127, 137-151, 157-176, 184-197, 205-221, 229-242
test/evm/integration/evmone_calls_equivalence_test.zig (1)
52-57: Prefer wider integer type for “stack requirements” constants
u8is sufficient for the literal values but unnecessarily narrow and invites
implicit casts later. Usingusize(oru32) matches typical stack counters.-const call_stack_req: u8 = 7; +const call_stack_req: usize = 7;Repeat for the other
_stack_reqconstants.test/evm/opcodes/geth_memory_comprehensive_test.zig (2)
82-89: Clarify the test case with empty expected field.This test case has
expected = ""with a comment "Will cause memory expansion". Since the test loop skips this case (line 95), consider either:
- Adding a comment explaining why this case is defined but skipped
- Implementing a separate test specifically for very high offset memory expansion
- Removing this case if it's not needed
351-354: Consider adding data verification for CALLDATACOPY tests.While the test verifies memory expansion, it only prints debug messages without verifying the actual copied data. Consider adding assertions to verify that the correct bytes were copied from calldata to memory, especially for partial and out-of-bounds cases.
// Check that some data was copied (detailed verification would require more complex logic) - std.debug.print("CALLDATACOPY test passed: {s}\n", .{test_case.description}); + // Verify the copied data matches expected values + const bytes_to_verify = @min(test_case.length, test_calldata.len -| test_case.src_offset); + if (bytes_to_verify > 0) { + const expected_data = test_calldata[test_case.src_offset..test_case.src_offset + bytes_to_verify]; + const actual_data = test_frame.frame.memory.data[test_case.dest_offset..test_case.dest_offset + bytes_to_verify]; + try testing.expectEqualSlices(u8, expected_data, actual_data); + } + // Verify padding with zeros for out-of-bounds reads + if (test_case.length > bytes_to_verify) { + const padding_start = test_case.dest_offset + bytes_to_verify; + const padding_end = test_case.dest_offset + test_case.length; + for (test_frame.frame.memory.data[padding_start..padding_end]) |byte| { + try testing.expectEqual(@as(u8, 0), byte); + } + }bench/simple_stack_bench.zig (1)
49-53: Inefficient full-array reinitialisation inreset()Re-assigning an entire 1 Ki element array every reset copies 32 KiB each time.
Cleaner & faster:- self.data = [_]u256{0} ** CAPACITY; + std.mem.set(u256, &self.data, 0);
std.mem.setis O(n) but avoids generating a second big constant and copies via memset.src/evm/execution/arithmetic_test.zig (1)
111-114: Comment contradicts codeComment says “b first, then a” but the slice passed is
{ case.a, case.b }.
Please make the code and comment consistent.test/evm/gas/geth_gas_calculation_test.zig (1)
142-157: Gas–cost verification silently ignores mismatchesFor non-memory opcodes a mismatch only logs but does not fail the test.
If the intent is validation against Geth, the test shouldtesting.expectEqualto catch regressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
bench/opcode_bench.zig(1 hunks)bench/simple_math_bench.zig(1 hunks)bench/simple_stack_bench.zig(1 hunks)bench/stack_bench.zig(1 hunks)build.zig(6 hunks)debug_function_selector.zig(1 hunks)src/evm/execution/arithmetic_test.zig(1 hunks)src/evm/execution/bitwise_test.zig(1 hunks)src/evm/execution/comparison_test.zig(1 hunks)src/evm/execution/memory.zig(2 hunks)test/evm/gas/gas_accounting_test.zig(1 hunks)test/evm/gas/geth_gas_calculation_test.zig(1 hunks)test/evm/geth_test_summary.zig(1 hunks)test/evm/integration/evmone_calls_equivalence_test.zig(1 hunks)test/evm/integration/evmone_equivalence_test.zig(1 hunks)test/evm/integration/evmone_memory_equivalence_test.zig(1 hunks)test/evm/integration/evmone_regression_equivalence_test.zig(1 hunks)test/evm/integration/evmone_storage_equivalence_test.zig(1 hunks)test/evm/integration/package.zig(1 hunks)test/evm/opcode_debug.zig(1 hunks)test/evm/opcodes/geth_arithmetic_comprehensive_test.zig(1 hunks)test/evm/opcodes/geth_memory_comprehensive_test.zig(1 hunks)test/evm/opcodes/geth_shift_operations_test.zig(1 hunks)test/evm/opcodes/memory_comprehensive_test.zig(14 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/evm/opcode_debug.zig
- test/evm/geth_test_summary.zig
🚧 Files skipped from review as they are similar to previous changes (2)
- src/evm/execution/memory.zig
- debug_function_selector.zig
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: CI Checks
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (12)
test/evm/integration/package.zig (1)
11-16: Imports & registration look goodThe newly added test modules are correctly imported and referenced in the
test {}block. No functional or stylistic issues spotted here.Also applies to: 25-30
test/evm/integration/evmone_memory_equivalence_test.zig (1)
8-17: Tests read clearly and exercise the right invariantsMemory word-alignment, expansion cost, and bit-wise behaviour mirror the evmone reference nicely. No issues noted.
Also applies to: 19-52, 54-66
test/evm/integration/evmone_regression_equivalence_test.zig (1)
60-76:0xfeis classified as “valid” – verify intentThe helper marks opcode
0xfe(the “INVALID” instruction) as valid:0xfe => true, // INVALIDevmone treats
0xfeas defined but semantically invalid. If the intention ofis_valid_opcodeis “opcode recognised by spec” this is fine; if it’s “opcode that executes without error” then0xfeshould befalse.Please double-check the semantics you need.
bench/opcode_bench.zig (1)
400-417:u256literals exceed 64-bit – ensure host type existsThe constants declared here rely on a
u256builtin/alias.
Ifu256is a custom big-int, fine; otherwise this will not compile on stock Zig.Verify that
u256is defined and that the literal width is accepted.
Otherwise, replace withstd.math.big.intor a custom wrapper.test/evm/gas/gas_accounting_test.zig (1)
53-56: Bounds-check LUT sizeThe loop iterates
0..100but does not guarantee that
MEMORY_EXPANSION_LUTcontains at least 100 elements,
which will panic at runtime if the table is shorter.Guard the loop or derive upper bound from
MEMORY_EXPANSION_LUT.len.-for (0..100) |words| { +for (0..@min(100, gas_constants.MEMORY_EXPANSION_LUT.len)) |words| {test/evm/opcodes/memory_comprehensive_test.zig (2)
191-191: Stack argument order corrections look good!The corrections to push offset first, then value (so value is on top per LIFO stack behavior) are consistent throughout the file and align with the EVM specification for MSTORE/MSTORE8 opcodes.
Also applies to: 200-200, 208-208, 234-234, 239-239, 247-247, 260-260, 286-287, 297-298, 305-306, 367-368, 374-375, 390-391, 420-421, 443-444, 467-468, 494-495, 502-503, 510-511, 519-520, 1085-1086
975-1246: Excellent addition of comprehensive memory edge case tests!These new tests inspired by revm provide thorough coverage of critical edge cases including:
- Word boundary alignment verification
- Sequential memory expansion cost validation
- Large memory operations (1KB, 2KB)
- MCOPY operation edge cases
- Overflow protection with extreme offsets
- Gas exhaustion scenarios
The tests are well-structured with proper resource management and clear assertions.
test/evm/opcodes/geth_arithmetic_comprehensive_test.zig (1)
61-62: Correct stack ordering for two-operand operations.The operands are pushed in reverse order
{ y_bytes, x_bytes }which is correct for LIFO stack behavior, ensuring x is popped first, then y for operations likex op y.src/evm/execution/bitwise_test.zig (1)
1-16: Well-documented test suite with clear references.Excellent documentation explaining the test sources (revm and go-ethereum) and comprehensive coverage structure. This helps maintainers understand the test design and verify against reference implementations.
test/evm/opcodes/geth_shift_operations_test.zig (2)
46-47: Correct stack ordering for shift operations.The operands are pushed as
{ value, shift }which correctly places the shift amount on top of the stack to be popped first, followed by the value to be shifted.
218-275: Excellent power-of-2 pattern tests for real-world scenarios.These tests validate shift operations with common power-of-2 values, which are frequently used in smart contracts for bit manipulation and efficient multiplication/division. The bounds checking prevents test calculation overflow while still covering practical shift ranges.
build.zig (1)
108-111: 👍 Include-path addition unblocks C headers in compiler testsThe extra
addIncludePath("include")on thecompiler_modneatly solves the missing-headers issue that broke the Rust / zabi integration.
bench/opcode_bench.zig
Outdated
| // Benchmark arithmetic operations | ||
| fn benchmarkArithmeticAdd(allocator: std.mem.Allocator) void { | ||
| var frame = create_mock_frame(allocator) catch return; | ||
| defer frame.deinit(); | ||
|
|
||
| // Pre-populate stack with test values | ||
| frame.stack.append(100) catch return; | ||
| frame.stack.append(200) catch return; | ||
|
|
||
| const iterations = 1000; | ||
| for (0..iterations) |_| { | ||
| // Reset stack state | ||
| frame.stack.reset(); | ||
| frame.stack.append(100) catch return; | ||
| frame.stack.append(200) catch return; | ||
|
|
||
| // Execute ADD operation | ||
| const state: *Operation.State = @ptrCast(&frame); | ||
| const interpreter: Operation.Interpreter = undefined; | ||
| _ = @import("evm").execution.arithmetic.op_add(0, &interpreter, state) catch return; | ||
|
|
||
| const result = frame.stack.peek_unsafe().*; | ||
| std.mem.doNotOptimizeAway(result); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Benchmarks silently abort on first allocation error
Every benchmark body ends execution with return whenever an append or opcode call errors.
That exits the benchmark early while reporting success to zbench.
Recommendation:
- Make each benchmark
fn(alloc: Allocator) !voidandtryerror-producing calls, letting zbench capture the failure. - If the signature must stay
void, convert unexpected errors intostd.debug.panic.
🤖 Prompt for AI Agents
In bench/opcode_bench.zig around lines 31 to 55, the benchmark functions
currently return silently on allocation or opcode errors, causing early exit
without failure reporting. To fix this, change the function signature to return
an error union like fn(allocator: std.mem.Allocator) !void and replace all error
handling with try on calls that can fail, allowing zbench to detect and report
errors properly. If the signature must remain void, replace all error returns
with std.debug.panic calls to ensure failures are not silent.
bench/opcode_bench.zig
Outdated
| const state: *Operation.State = @ptrCast(&frame); | ||
| const interpreter: Operation.Interpreter = undefined; | ||
| _ = @import("evm").execution.arithmetic.op_add(0, &interpreter, state) catch return; |
There was a problem hiding this comment.
Fix incorrect @ptrCast syntax – current code will not compile
@ptrCast requires the destination type as its first argument.
All occurrences use the deprecated 1-arg form and will fail on Zig ≥0.11.
-const state: *Operation.State = @ptrCast(&frame);
+const state: *Operation.State = @ptrCast(*Operation.State, &frame);Do this replacement for every benchmark that builds state.
Without it the whole bench/opcode_bench target will not build.
Also applies to: 67-69, 86-88
🤖 Prompt for AI Agents
In bench/opcode_bench.zig at lines 48-50 (and similarly at 67-69 and 86-88), the
@ptrCast usage is incorrect because it uses the deprecated single-argument form.
Fix this by changing @ptrCast to take the destination pointer type as the first
argument, followed by the value to cast. Update all such occurrences to the new
two-argument syntax to ensure the code compiles with Zig version 0.11 and later.
| var storage = std.HashMap(u256, u256, std.hash_map.DefaultContext(u256), std.hash_map.default_max_load_percentage).init(testing.allocator); | ||
| defer storage.deinit(); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use try storage.put(...) to propagate allocation errors
HashMap.put is !void.
A missing try discards errors and hides OOM conditions during tests.
- storage.put(key, value);
+ try storage.put(key, value);Repeat for every put call.
Also applies to: 47-49, 64-66, 76-78, 100-102
🤖 Prompt for AI Agents
In test/evm/integration/evmone_storage_equivalence_test.zig around lines 30 to
32 and also at lines 47-49, 64-66, 76-78, and 100-102, the calls to storage.put
are missing the 'try' keyword, which means allocation errors are not propagated
and can hide out-of-memory conditions during tests. Fix this by prefixing every
storage.put call with 'try' to ensure any allocation errors are properly
propagated and handled.
| const loaded = storage.get(key); | ||
| try testing.expect(loaded != null); | ||
| try testing.expectEqual(value, loaded.?); | ||
| } |
There was a problem hiding this comment.
Dereference HashMap.get result correctly
HashMap.get returns ?*T (pointer) in Zig ≥0.11.
All equality checks pass the pointer itself to expectEqual, producing a type error.
-const loaded = storage.get(key);
-try testing.expect(loaded != null);
-try testing.expectEqual(value, loaded.?);
+const loaded = storage.get(key);
+try testing.expect(loaded != null);
+try testing.expectEqual(value, loaded.?.*);Apply the same .* dereference wherever loaded.? is compared.
Also applies to: 57-60, 70-72, 87-95, 111-113
🤖 Prompt for AI Agents
In test/evm/integration/evmone_storage_equivalence_test.zig around lines 40 to
43, the code uses loaded.? to access the value from storage.get(key), but since
get returns a pointer wrapped in an optional, you need to dereference the
pointer with .* before comparing. Replace loaded.? with loaded.* in the
expectEqual call to fix the type error. Apply the same fix to lines 57-60,
70-72, 87-95, and 111-113 where similar comparisons occur.
| defer frame.memory.deinit(); | ||
|
|
There was a problem hiding this comment.
memory.deinit() is called without allocator
Memory.deinit in the rest of the codebase expects the allocator parameter (memory.deinit(allocator)).
Calling it without will fail to compile.
Also applies to: 146-147, 250-252, 387-389
🤖 Prompt for AI Agents
In src/evm/execution/comparison_test.zig at lines 105-106, the call to
memory.deinit() is missing the required allocator parameter, which causes
compilation errors. Update all memory.deinit() calls in these lines and also at
lines 146-147, 250-252, and 387-389 to include the allocator argument, matching
the usage elsewhere in the codebase.
| // Push values onto stack in reverse order (stack is LIFO) | ||
| for (values) |value| { | ||
| try frame.stack.append(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Stack initialisation order does not match the comment
Comment says “push values onto stack in reverse order (LIFO)”, but the loop pushes values as-is, meaning values[0] ends up below values[1].
Either reverse the iteration or adapt the comment/tests; otherwise operand order for opcodes like LT/GT is wrong. Example fix:
- for (values) |value| {
+ for (values) |value| |idx| {
+ const value = values[values.len - 1 - idx];Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/evm/execution/comparison_test.zig around lines 56 to 59, the code pushes
values onto the stack in the original order, but the comment states they should
be pushed in reverse order to respect LIFO behavior. To fix this, change the
loop to iterate over the values in reverse order before appending them to the
stack, ensuring the top of the stack corresponds to the last value in the
original list and matches the expected operand order for comparison opcodes.
| // Push values onto stack in reverse order (stack is LIFO) | ||
| for (values) |value| { | ||
| try frame.stack.append(value); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same stack-order issue as in comparison tests
createTestFrame pushes operands in forward order but later tests assume reverse (see the “b first, then a” comment).
This will flip operands for ADD/SUB/etc. leading to false positives.
Fix by reversing the iteration or updating every test.
🤖 Prompt for AI Agents
In src/evm/execution/arithmetic_test.zig around lines 67 to 70, the code pushes
values onto the stack in forward order, but the stack is LIFO and tests expect
operands in reverse order. To fix this, reverse the iteration over values when
appending to the stack so that operands are pushed in the correct order,
matching the expected "b first, then a" sequence in the tests.
| if (!test_case.should_overflow) { | ||
| if (actual_cost != test_case.expected_cost) { | ||
| std.debug.print("Memory gas test {} failed: {s}\n", .{ i, test_case.description }); | ||
| std.debug.print("Memory size: {}, Expected: {}, Got: {}\n", .{ test_case.memory_size, test_case.expected_cost, actual_cost }); | ||
| return error.MemoryGasMismatch; | ||
| } | ||
| } |
There was a problem hiding this comment.
Tests return errors from a void block – will not compile
test " blocks return void; return error.Some; is illegal.
Replace with try testing.fail(...) or testing.expect(false, ...).
Example:
- return error.MemoryGasMismatch;
+ try testing.expectEqual(test_case.expected_cost, actual_cost);Apply to all custom error.* returns in this file.
Also applies to: 144-148, 210-214
🤖 Prompt for AI Agents
In test/evm/gas/geth_gas_calculation_test.zig around lines 60 to 66, the test
block returns void, so returning errors like `return error.MemoryGasMismatch;`
is invalid. Replace all such error returns with `try testing.fail("descriptive
message")` or `testing.expect(false, "descriptive message")` to properly fail
the test. Apply this fix also to similar error returns at lines 144-148 and
210-214 in the same file.
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| const run_jump_dispatch_failure_test = b.addRunArtifact(jump_dispatch_failure_test); | ||
| const jump_dispatch_failure_test_step = b.step("test-jump-dispatch-failure", "Run Jump Dispatch Failure tests"); | ||
| jump_dispatch_failure_test_step.dependOn(&run_jump_dispatch_failure_test.step); | ||
|
|
||
| // Add LT Opcode Stack Underflow test (TDD approach) | ||
| const lt_stack_underflow_test = b.addTest(.{ | ||
| .name = "lt-opcode-stack-underflow-test", | ||
| .root_source_file = b.path("test/evm/lt_opcode_stack_underflow_test.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| const run_lt_stack_underflow_test = b.addRunArtifact(lt_stack_underflow_test); | ||
| const lt_stack_underflow_test_step = b.step("test-lt-stack-underflow", "Run LT Opcode Stack Underflow TDD tests"); | ||
| lt_stack_underflow_test_step.dependOn(&run_lt_stack_underflow_test.step); | ||
|
|
||
| // Add LT Opcode Bug test (actual implementation test) | ||
| const lt_opcode_bug_test = b.addTest(.{ | ||
| .name = "lt-opcode-bug-test", | ||
| .root_source_file = b.path("test/evm/lt_opcode_bug_test.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| lt_opcode_bug_test.root_module.addImport("evm", evm_mod); | ||
| const run_lt_opcode_bug_test = b.addRunArtifact(lt_opcode_bug_test); | ||
| const lt_opcode_bug_test_step = b.step("test-lt-opcode-bug", "Run LT Opcode Bug TDD tests"); | ||
| lt_opcode_bug_test_step.dependOn(&run_lt_opcode_bug_test.step); | ||
|
|
||
| // Add simple LT opcode test (documentation only) | ||
| const lt_opcode_simple_test = b.addTest(.{ | ||
| .name = "lt-opcode-simple-test", | ||
| .root_source_file = b.path("test/evm/lt_opcode_simple_test.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| const run_lt_opcode_simple_test = b.addRunArtifact(lt_opcode_simple_test); | ||
| const lt_opcode_simple_test_step = b.step("test-lt-opcode-simple", "Run simple LT Opcode documentation test"); | ||
| lt_opcode_simple_test_step.dependOn(&run_lt_opcode_simple_test.step); | ||
|
|
||
| // Add MSTORE debug test | ||
| const mstore_debug_test = b.addTest(.{ | ||
| .name = "mstore-debug-test", | ||
| .root_source_file = b.path("debug_mstore_params.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| mstore_debug_test.root_module.addImport("Address", address_mod); | ||
| mstore_debug_test.root_module.addImport("Block", block_mod); | ||
| mstore_debug_test.root_module.addImport("evm", evm_mod); | ||
| mstore_debug_test.root_module.addImport("utils", utils_mod); | ||
| mstore_debug_test.root_module.addImport("test_helpers", test_helpers_mod); | ||
| const run_mstore_debug_test = b.addRunArtifact(mstore_debug_test); | ||
| const mstore_debug_test_step = b.step("test-mstore-debug", "Debug MSTORE parameter order"); | ||
| mstore_debug_test_step.dependOn(&run_mstore_debug_test.step); | ||
|
|
||
| // Add EVM Contract benchmark (after rust_step is defined) | ||
| const evm_contract_benchmark = b.addExecutable(.{ | ||
| .name = "evm-contract-benchmark", | ||
| .root_source_file = b.path("bench/evm.zig"), | ||
| .target = target, | ||
| .optimize = .ReleaseFast, | ||
| }); | ||
| evm_contract_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); | ||
| evm_contract_benchmark.root_module.addImport("evm", target_architecture_mod); | ||
| evm_contract_benchmark.root_module.addImport("Address", address_mod); | ||
| evm_contract_benchmark.root_module.addImport("Compiler", compiler_mod); | ||
| evm_contract_benchmark.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| evm_contract_benchmark.root_module.addIncludePath(b.path("include")); | ||
|
|
||
| // Add Rust dependencies to EVM contract benchmark (same as compiler test) | ||
| evm_contract_benchmark.step.dependOn(rust_step); | ||
| evm_contract_benchmark.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| evm_contract_benchmark.linkLibC(); | ||
|
|
||
| // Link system libraries for EVM contract benchmark | ||
| if (target.result.os.tag == .linux) { | ||
| evm_contract_benchmark.linkSystemLibrary("unwind"); | ||
| evm_contract_benchmark.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| evm_contract_benchmark.linkFramework("CoreFoundation"); | ||
| evm_contract_benchmark.linkFramework("Security"); | ||
| } | ||
|
|
||
| const run_evm_contract_benchmark = b.addRunArtifact(evm_contract_benchmark); | ||
|
|
||
| const evm_contract_benchmark_step = b.step("bench-evm-contracts", "Run EVM Contract benchmarks"); | ||
| evm_contract_benchmark_step.dependOn(&run_evm_contract_benchmark.step); | ||
|
|
||
| // Add EVM contract benchmark to the combined benchmark step | ||
| all_benchmark_step.dependOn(&run_evm_contract_benchmark.step); | ||
|
|
||
| // Add EVM Opcode benchmark | ||
| const evm_opcode_benchmark = b.addExecutable(.{ | ||
| .name = "evm-opcode-benchmark", | ||
| .root_source_file = b.path("bench/opcode_bench.zig"), | ||
| .target = target, | ||
| .optimize = .ReleaseFast, | ||
| }); | ||
| evm_opcode_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); | ||
| evm_opcode_benchmark.root_module.addImport("evm", target_architecture_mod); | ||
| evm_opcode_benchmark.root_module.addImport("Address", address_mod); | ||
|
|
||
| const run_evm_opcode_benchmark = b.addRunArtifact(evm_opcode_benchmark); | ||
|
|
||
| const evm_opcode_benchmark_step = b.step("bench-evm-opcodes", "Run EVM Opcode benchmarks"); | ||
| evm_opcode_benchmark_step.dependOn(&run_evm_opcode_benchmark.step); | ||
|
|
||
| // Add EVM opcode benchmark to the combined benchmark step | ||
| all_benchmark_step.dependOn(&run_evm_opcode_benchmark.step); | ||
|
|
||
| // Add Simple Math benchmark | ||
| const simple_math_benchmark = b.addExecutable(.{ | ||
| .name = "simple-math-benchmark", | ||
| .root_source_file = b.path("bench/simple_math_bench.zig"), | ||
| .target = target, | ||
| .optimize = .ReleaseFast, | ||
| }); | ||
| simple_math_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); | ||
|
|
||
| const run_simple_math_benchmark = b.addRunArtifact(simple_math_benchmark); | ||
|
|
||
| const simple_math_benchmark_step = b.step("bench-simple-math", "Run Simple Math benchmarks"); | ||
| simple_math_benchmark_step.dependOn(&run_simple_math_benchmark.step); | ||
|
|
||
| // Add simple math benchmark to the combined benchmark step | ||
| all_benchmark_step.dependOn(&run_simple_math_benchmark.step); | ||
|
|
||
| // Add Stack benchmark | ||
| const stack_benchmark = b.addExecutable(.{ | ||
| .name = "stack-benchmark", | ||
| .root_source_file = b.path("bench/stack_bench.zig"), | ||
| .target = target, | ||
| .optimize = .ReleaseFast, | ||
| }); | ||
| stack_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); | ||
| stack_benchmark.root_module.addImport("evm", target_architecture_mod); | ||
|
|
||
| const run_stack_benchmark = b.addRunArtifact(stack_benchmark); | ||
|
|
||
| const stack_benchmark_step = b.step("bench-stack", "Run Stack benchmarks"); | ||
| stack_benchmark_step.dependOn(&run_stack_benchmark.step); | ||
|
|
||
| // Add stack benchmark to the combined benchmark step | ||
| all_benchmark_step.dependOn(&run_stack_benchmark.step); | ||
|
|
||
| // Add Simple Stack benchmark | ||
| const simple_stack_benchmark = b.addExecutable(.{ | ||
| .name = "simple-stack-benchmark", | ||
| .root_source_file = b.path("bench/simple_stack_bench.zig"), | ||
| .target = target, | ||
| .optimize = .ReleaseFast, | ||
| }); | ||
| simple_stack_benchmark.root_module.addImport("zbench", zbench_dep.module("zbench")); | ||
|
|
||
| const run_simple_stack_benchmark = b.addRunArtifact(simple_stack_benchmark); | ||
|
|
||
| const simple_stack_benchmark_step = b.step("bench-simple-stack", "Run Simple Stack benchmarks"); | ||
| simple_stack_benchmark_step.dependOn(&run_simple_stack_benchmark.step); | ||
|
|
||
| // Add simple stack benchmark to the combined benchmark step | ||
| all_benchmark_step.dependOn(&run_simple_stack_benchmark.step); | ||
|
|
||
| // Add Gas Debugging tool | ||
| const debug_gas_tool = b.addExecutable(.{ | ||
| .name = "debug-gas-issue", | ||
| .root_source_file = b.path("debug_gas_issue.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| debug_gas_tool.root_module.addImport("evm", target_architecture_mod); | ||
| debug_gas_tool.root_module.addImport("Address", address_mod); | ||
| debug_gas_tool.root_module.addImport("Compiler", compiler_mod); | ||
| debug_gas_tool.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| debug_gas_tool.root_module.addIncludePath(b.path("include")); | ||
| debug_gas_tool.step.dependOn(rust_step); | ||
| debug_gas_tool.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| debug_gas_tool.linkLibC(); | ||
| if (target.result.os.tag == .linux) { | ||
| debug_gas_tool.linkSystemLibrary("unwind"); | ||
| debug_gas_tool.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| debug_gas_tool.linkFramework("CoreFoundation"); | ||
| debug_gas_tool.linkFramework("Security"); | ||
| } | ||
| const run_debug_gas_tool = b.addRunArtifact(debug_gas_tool); | ||
| const debug_gas_tool_step = b.step("debug-gas", "Debug EVM gas accounting issues"); | ||
| debug_gas_tool_step.dependOn(&run_debug_gas_tool.step); | ||
|
|
||
| // Add Bytecode Analysis tool | ||
| const debug_bytecode_detailed = b.addExecutable(.{ | ||
| .name = "debug-bytecode-detailed", | ||
| .root_source_file = b.path("debug_bytecode_detailed.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| debug_bytecode_detailed.root_module.addImport("evm", target_architecture_mod); | ||
| debug_bytecode_detailed.root_module.addImport("Address", address_mod); | ||
| debug_bytecode_detailed.root_module.addImport("Compiler", compiler_mod); | ||
| debug_bytecode_detailed.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| debug_bytecode_detailed.root_module.addIncludePath(b.path("include")); | ||
| debug_bytecode_detailed.step.dependOn(rust_step); | ||
| debug_bytecode_detailed.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| debug_bytecode_detailed.linkLibC(); | ||
| if (target.result.os.tag == .linux) { | ||
| debug_bytecode_detailed.linkSystemLibrary("unwind"); | ||
| debug_bytecode_detailed.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| debug_bytecode_detailed.linkFramework("CoreFoundation"); | ||
| debug_bytecode_detailed.linkFramework("Security"); | ||
| } | ||
| const run_debug_bytecode_detailed = b.addRunArtifact(debug_bytecode_detailed); | ||
| const debug_bytecode_detailed_step = b.step("debug-bytecode-detailed", "Analyze EVM bytecode in detail"); | ||
| debug_bytecode_detailed_step.dependOn(&run_debug_bytecode_detailed.step); | ||
|
|
||
| // Add Contract Execution Debug test | ||
| const contract_execution_debug_test = b.addTest(.{ | ||
| .name = "contract-execution-debug-test", | ||
| .root_source_file = b.path("test/evm/contract_execution_debug_test.zig"), | ||
| .target = target, | ||
| .optimize = optimize, | ||
| }); | ||
| contract_execution_debug_test.root_module.addImport("evm", target_architecture_mod); | ||
| contract_execution_debug_test.root_module.addImport("Compiler", compiler_mod); | ||
| contract_execution_debug_test.root_module.addImport("Address", address_mod); | ||
| contract_execution_debug_test.root_module.addImport("zabi", zabi_dep.module("zabi")); | ||
| contract_execution_debug_test.root_module.addIncludePath(b.path("include")); | ||
| contract_execution_debug_test.step.dependOn(rust_step); | ||
| contract_execution_debug_test.addObjectFile(b.path("dist/target/release/libfoundry_wrapper.a")); | ||
| contract_execution_debug_test.linkLibC(); | ||
| if (target.result.os.tag == .linux) { | ||
| contract_execution_debug_test.linkSystemLibrary("unwind"); | ||
| contract_execution_debug_test.linkSystemLibrary("gcc_s"); | ||
| } else if (target.result.os.tag == .macos) { | ||
| contract_execution_debug_test.linkFramework("CoreFoundation"); | ||
| contract_execution_debug_test.linkFramework("Security"); | ||
| } | ||
| const run_contract_execution_debug_test = b.addRunArtifact(contract_execution_debug_test); | ||
| const contract_execution_debug_test_step = b.step("test-contract-execution-debug", "Run Contract Execution Debug tests"); | ||
| contract_execution_debug_test_step.dependOn(&run_contract_execution_debug_test.step); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heavy copy-paste blocks – factor out a helper to cut 300+ lines
The pattern
const X = b.addTest(.{ .name = "...", .root_source_file = b.path("..."), ... });
X.root_module.addImport(...);
X.step.dependOn(rust_step);
X.addObjectFile(...);
X.linkLibC();
...
const run_X = b.addRunArtifact(X);
const X_step = b.step("...", "..."); X_step.dependOn(&run_X.step);
is repeated for ~20 suites. Encapsulate in a small helper:
fn addRustTest(
b: *std.Build,
name: []const u8,
src: []const u8,
extra_imports: anytype,
) (*std.Build.Step, *std.Build.Module) {
const t = b.addTest(.{ .name = name, .root_source_file = b.path(src), .target = target, .optimize = optimize });
// common wiring …
const run = b.addRunArtifact(t);
const step = b.step(name, "Run " ++ name);
step.dependOn(&run.step);
return .{ step, t.root_module };
}This slashes maintenance overhead, prevents drift between suites, and keeps build.zig readable.
🤖 Prompt for AI Agents
In build.zig lines 1270 to 1510, there is a large amount of repeated code for
adding tests and benchmarks with similar patterns of setup, imports,
dependencies, and run steps. To fix this, create a helper function that takes
parameters like the build context, test name, source path, and extra imports,
then encapsulates the common logic of adding the test, setting imports, adding
run artifacts, creating steps, and setting dependencies. Replace the repeated
blocks by calls to this helper, passing the specific parameters for each test or
benchmark. This will reduce code duplication, improve maintainability, and keep
the build.zig file cleaner.
Fixed stack parameter order in control and memory opcode tests to match correct EVM specification after recent implementation fixes. Issues resolved: - JUMPI: Fixed stack order to [condition, destination] with destination on top - MSTORE/MSTORE8: Fixed stack order to [value, offset] with offset on top - Memory gas cost calculations now work correctly with proper stack order - MSTORE8 OutOfOffset test now expects correct parameter order Test results improved from 454/461 to 457/461 passing tests. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
…erations Fixed segmentation fault in evmone_equivalence_test.test.basic_memory_operations by adding required finalize_root() call after Memory.init_default(). The Memory implementation requires finalize_root() to be called after initialization and storage at a stable address to properly set the root pointer. Missing this call caused undefined behavior leading to segfaults. Integration tests now run without crashing: 84/89 tests passing (vs segfault before). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Fix RETURN/REVERT stack parameter order in bytecode generation - Correct JUMPI, MSTORE, MSTORE8 stack parameter ordering for EVM spec compliance - Add required memory.finalize_root() calls to prevent segmentation faults - Fix integration test bytecode offset calculations and gas forwarding expectations - Resolve recursive stack overflow in control flow edge case tests - Improve test pass rate from 454/461 to 777/779 total tests <prompt> The user asked me to continue fixing remaining test failures in the Zig-based EVM implementation. Key issues were: 1. Integration test segmentation faults 2. RETURN/REVERT data handling failures 3. Stack parameter order issues in various opcodes 4. Incorrect test expectations in integration tests 5. Memory safety issues in test framework Fixed all critical test failures systematically, achieving 777/779 tests passing. </prompt> 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (16)
TEST_FAILURES.md (3)
4-4: Convert emphasis to a proper heading for consistency
Emphasis on line 4 reads as a section title—use a heading instead for better structure.- **Status: All major test failures RESOLVED! 🎉** + ### Status: All major test failures RESOLVED! 🎉🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
4-4: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
35-35: Add missing article for grammatical correctness
Insert “The” and lowercase “primary” to fix the missing determiner and adhere to sentence case.- Primary Issue was stack parameter order in opcodes using `pop2_unsafe()`. + The primary issue was the stack parameter order in opcodes using `pop2_unsafe()`.
108-108: Remove trailing colon from heading
Markdown headings should not end with punctuation according to lint rules.- ### Specific Investigation Points: + ### Specific Investigation Points🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
108-108: Trailing punctuation in heading
Punctuation: ':'(MD026, no-trailing-punctuation)
debug_simple_create.zig (2)
15-16: Prefer propagating initialization errors instead ofcatch unreachableUsing
catch unreachablehides legitimate initialization failures and will abort the program instead of surfacing the error. Propagate or handle the error explicitly for better debuggability.- var vm = evm.Vm.init(allocator, db_interface, null, null) catch unreachable; + var vm = try evm.Vm.init(allocator, db_interface, null, null);
47-53: Assert CREATE success to surface deployment failuresThe log prints
success, but the program continues even if deployment failed. Add a guard to fail fast during debugging.- std.log.info("CREATE result: success={}, address={any}", .{ create_result.success, create_result.address }); + if (!create_result.success) { + std.log.err("Contract deployment failed", .{}); + return error.DeploymentFailed; + } + std.log.info("CREATE result: success={}, address={any}", .{ true, create_result.address });debug_jumpi_validation.zig (2)
48-54: Turn “valid” execution into an assertionFor a debug executable intended to validate behaviour, use
std.testing.expect(orstd.debug.assert) so CI fails if the VM misbehaves.- const valid_result = vm.interpret(&valid_contract, &[_]u8{}) catch |err| { - std.log.err("Valid JUMPI failed: {}", .{err}); - return; - }; - - std.log.info("Valid JUMPI result: status={}, gas_left={}", .{ valid_result.status, valid_result.gas_left }); + const valid_result = try vm.interpret(&valid_contract, &[_]u8{}); + std.debug.assert(valid_result.status == .success);
81-87: Fail fast when an invalid JUMPI unexpectedly succeedsCurrently the path only logs an error; the program still exits
mainsuccessfully. Replace with an assertion to avoid silent false-positives.- const invalid_result = vm.interpret(&invalid_contract, &[_]u8{}) catch |err| { - std.log.info("Invalid JUMPI correctly failed with error: {}", .{err}); - return; - }; - - std.log.err("Invalid JUMPI should have failed but succeeded: status={}, gas_left={}", .{ invalid_result.status, invalid_result.gas_left }); + const invalid_result = vm.interpret(&invalid_contract, &[_]u8{}) catch |err| { + std.log.info("Invalid JUMPI correctly failed: {}", .{err}); + return; + }; + std.debug.assert(false, "Invalid JUMPI unexpectedly succeeded: {}", .{invalid_result});debug_return_data.zig (2)
59-66: Add an explicit assertion on returned dataThe goal is to verify return-data handling, but currently the code only logs. Assert to guarantee regressions are caught.
- if (result.output) |output| { - std.log.info("Result output: {} bytes: {any}", .{ output.len, output }); - std.log.info("Result output as string: {s}", .{output}); + if (result.output) |output| { + std.debug.assert(std.mem.eql(u8, output, "Hello"), "Unexpected return data: {s}", .{output});
68-71:{s}expects UTF-8 slice – ensure data is printableIf the returned bytes can be arbitrary,
{s}may mis-render or hit non-UTF-8 sequences. Use{any}or hex formatting for unconditional debugging, or guard withstd.unicode.validateAscii.debug_memory_gas.zig (1)
14-19: Expose discrepancy detection to CIIt’s useful to automatically fail when the computed
expansion_costdiverges from the manual formula.- std.log.info("Expansion gas cost: {}", .{expansion_cost}); + const expected = new_cost - current_cost; + std.debug.assert(expansion_cost == expected, + "memory_gas_cost mismatch: got {}, expected {}", .{ expansion_cost, expected });debug_mstore8.zig (1)
50-55: Verify memory side-effect instead of only loggingThe test currently confirms the VM didn’t error, but doesn’t check that
MSTORE8actually wrote0xFFat offset 0. Reading memory post-execution (via the VM state or frame, if exposed) will make the test meaningful.I can draft a helper that peeks into the VM memory after execution if you point me to the relevant accessor.
src/evm/README.md (1)
275-275: Add comma before 'but' for better readability.-- [ ] 🔴 **WASM Build Integration** - [WASM builds successfully but exports don't match JavaScript interface expectations](./prompts/implement-wasm-build-fix.md) +- [ ] 🔴 **WASM Build Integration** - [WASM builds successfully, but exports don't match JavaScript interface expectations](./prompts/implement-wasm-build-fix.md)🧰 Tools
🪛 LanguageTool
[uncategorized] ~275-~275: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ntegration** - [WASM builds successfully but exports don't match JavaScript interfac...(COMMA_COMPOUND_SENTENCE_2)
bench/call_ops_bench.zig (2)
49-55: Guard against 64-bit addition overflow inconsume_gas
self.gas_used + amount > self.gas_limitcan wrap before the comparison.
Usestd.math.addWithOverflowor reorder the check:if (amount > self.remaining_gas()) return false;
129-137: Surface allocator failures instead of swallowing them
set_account(... ) catch {}ignores allocation errors – benchmarks may silently
proceed with incomplete state. At minimum, log or propagate:try storage.set_account(...);bench/gas_bench.zig (2)
67-89:AccessListlook-ups are O(n); a hash-set gives realistic perfLinear scans distort gas-cost micro-benchmarks once the list grows.
Usingstd.AutoHashMap(u256, void)keeps realism while staying simple.
25-31: Possible overflow inGasTracker.consume
self.consumed + amountmay wrap.
Usestd.math.addWithOverflowor compare first as in:if (amount > self.remaining()) return false;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
TEST_FAILURES.md(1 hunks)bench/call_ops_bench.zig(1 hunks)bench/gas_bench.zig(1 hunks)bench/jumptable_bench.zig(1 hunks)bench/precompile_bench.zig(1 hunks)build.zig(8 hunks)debug_jumpi_validation.zig(1 hunks)debug_memory_gas.zig(1 hunks)debug_mstore8.zig(1 hunks)debug_return_data.zig(1 hunks)debug_simple_create.zig(1 hunks)src/evm/README.md(6 hunks)src/evm/constants/gas_constants.zig(3 hunks)src/evm/execution/control.zig(5 hunks)src/evm/execution/memory.zig(6 hunks)src/evm/execution/storage.zig(2 hunks)src/evm/precompiles/blake2f.zig(1 hunks)src/evm/vm.zig(6 hunks)src/root_wasm_crypto.zig(1 hunks)test/evm/e2e_contract_execution_test.zig(1 hunks)test/evm/e2e_cross_opcode_test.zig(1 hunks)test/evm/e2e_real_world_scenarios_test.zig(1 hunks)test/evm/e2e_state_management_test.zig(1 hunks)test/evm/e2e_transaction_gas_test.zig(1 hunks)test/evm/gas/gas_accounting_test.zig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/evm/constants/gas_constants.zig
🚧 Files skipped from review as they are similar to previous changes (4)
- src/evm/execution/control.zig
- src/evm/execution/storage.zig
- src/evm/vm.zig
- src/evm/execution/memory.zig
🧰 Additional context used
🪛 LanguageTool
src/evm/README.md
[uncategorized] ~275-~275: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ntegration** - [WASM builds successfully but exports don't match JavaScript interfac...
(COMMA_COMPOUND_SENTENCE_2)
TEST_FAILURES.md
[uncategorized] ~34-~34: A determiner appears to be missing. Consider inserting it.
Context: ...ment ### Original Root Cause Analysis Primary Issue was stack parameter order in opco...
(AI_EN_LECTOR_MISSING_DETERMINER)
[style] ~114-~114: To reduce wordiness, try specifying a number or using “many” or “numerous” instead.
Context: ...d memory expansion logic should resolve a large number of the test failures. ## Changes That May...
(LARGE_NUMBER_OF)
🪛 markdownlint-cli2 (0.17.2)
TEST_FAILURES.md
4-4: Emphasis used instead of a heading
null
(MD036, no-emphasis-as-heading)
108-108: Trailing punctuation in heading
Punctuation: ':'
(MD026, no-trailing-punctuation)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: CI Checks
- GitHub Check: WASM Bundle Size Check
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (24)
debug_simple_create.zig (1)
18-21:u256type not imported – may fail to compile
std.math.maxInt(u256)relies onu256being in scope. If the alias isn’t pulled in by@import("evm"), this will not compile. Import the type or replace with the concrete type used by the VM.const u256 = evm.types.u256;(or equivalent)
debug_jumpi_validation.zig (1)
15-17: Shared VM instance across tests can cause state bleed-throughAll four sub-tests reuse the same
vminstance; resulting state (e.g., modified memory, stack overflow counters, return data) could influence later tests. Spawning a fresh VM per case or callingvm.reset()(if available) makes the tests more isolated and deterministic.bench/precompile_bench.zig (5)
1-25: Well-structured precompile result simulation.The
PrecompileResultstruct and helper methods provide a clean interface for simulating precompile execution outcomes in benchmarks.
27-36: Gas constants match Ethereum specifications.The defined gas cost constants accurately reflect the official Ethereum precompile gas costs, ensuring realistic benchmarking scenarios.
40-73: Realistic ECRECOVER workload simulation.The benchmark effectively simulates elliptic curve signature verification workload with appropriate input size (128 bytes) and computational patterns.
75-173: Hash function and identity benchmarks are well-implemented.The benchmarks for SHA256, RIPEMD160, and IDENTITY correctly:
- Calculate gas costs based on word-aligned input sizes
- Simulate appropriate computational workloads
- Use realistic input size variations
175-401: Comprehensive precompile benchmarking suite.The remaining benchmarks effectively cover:
- MODEXP with realistic complexity calculations
- BLAKE2F with proper round variations
- System-level overhead measurements (gas calculation, validation, access patterns)
The main function correctly orchestrates all benchmarks with appropriate progress output.
src/evm/README.md (1)
85-637: Excellent documentation updates providing clear project status.The comprehensive status summary, issue tracking, and developer guidance significantly improve project transparency and onboarding. The breakdown of working/partially working/critical issues is particularly helpful.
🧰 Tools
🪛 LanguageTool
[typographical] ~194-~194: If specifying a range, consider using an en dash instead of a hyphen.
Context: ...[x] PUSH1-PUSH32 (0x60-0x7F) - Push 1-32 bytes onto stack - [x] DUP1-DUP16 (...(HYPHEN_TO_EN)
[uncategorized] ~275-~275: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ntegration** - [WASM builds successfully but exports don't match JavaScript interfac...(COMMA_COMPOUND_SENTENCE_2)
[uncategorized] ~428-~428: When ‘Stack-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...ain Support** - [ ] Optimism - OP Stack specific opcodes and behavior - [ ] **Arbitrum...(SPECIFIC_HYPHEN)
[uncategorized] ~430-~430: When ‘Polygon-specific’ is used as a modifier, it is usually spelled with a hyphen.
Context: ...des and gas model - [ ] Polygon - Polygon specific features - [ ] 🟡 EOF Support - EVM...(SPECIFIC_HYPHEN)
[misspelling] ~450-~450: This word is normally spelled with a hyphen.
Context: ...Configurable execution handlers for pre/post processing #### Production Hardening - [x] **Robu...(EN_COMPOUNDS_POST_PROCESSING)
🪛 markdownlint-cli2 (0.17.2)
495-495: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
src/evm/precompiles/blake2f.zig (4)
5-84: Excellent documentation and correct constant definitions.The implementation correctly follows EIP-152 specifications with:
- Precise input/output format documentation
- Accurate gas cost calculation (1 gas per round)
- Proper constant definitions for sizes and flags
- Helpful legacy compatibility functions
90-153: Correct and secure implementation of BLAKE2F precompile interface.The execute function properly:
- Validates all inputs (size, final flag)
- Handles endianness correctly (big-endian rounds, little-endian data)
- Enforces gas limits to prevent DoS
- Produces correctly formatted output
201-263: Accurate BLAKE2b compression function implementation.The function correctly implements RFC 7693:
- Proper initialization vector (IV) constants
- Correct working vector setup and mixing
- Accurate SIGMA permutation for message scheduling
- Proper column and diagonal G function application
- Correct finalization with XOR operation
155-184: Helper functions and G mixing function are correctly implemented.
- The G function accurately implements the BLAKE2b mixing primitive with correct rotation amounts
- Helper functions provide useful validation and size utilities
- All implementations match the RFC 7693 specification
Also applies to: 276-285
bench/jumptable_bench.zig (3)
1-109: Clean jump table implementation for benchmarking.The implementation provides:
- Efficient O(1) opcode dispatch
- Proper handling of undefined opcodes
- Realistic operation metadata (gas, stack requirements)
- Representative opcode distribution for testing
111-185: Effective dispatch mechanism benchmarks.The benchmarks provide valuable comparisons:
- Direct jump table vs switch performance measurement
- Realistic opcode sequences from actual EVM usage
- Proper validation overhead assessment
- Correct use of optimization barriers
187-377: Comprehensive benchmark coverage for EVM dispatch optimization.The benchmarks effectively measure:
- Realistic bytecode execution patterns
- Cache and memory access effects
- Branch prediction impact
- Metadata access overhead
- Error handling performance
The suite provides valuable insights for optimizing EVM opcode dispatch.
test/evm/gas/gas_accounting_test.zig (3)
165-199: Correct gas overflow and precompile cost tests.The tests properly verify:
- Memory expansion doesn't cause integer overflow
- Precompile gas costs match Ethereum specifications
- RIPEMD160 constants appropriately commented as not yet implemented
201-249: Accurate gas refund and transaction cost tests.The tests correctly verify:
- EIP-3529 refund capping mechanism (gas_used / 5)
- Quadratic growth in memory expansion costs
- Transaction intrinsic gas calculations per Ethereum specs
251-313: Comprehensive edge case coverage for gas accounting.The tests accurately verify:
- CREATE2 gas calculation including salt hashing overhead
- Call stipend mechanics for value transfers
- EIP-2929 cold/warm access cost differentials
- Gas exhaustion boundary conditions
These edge cases strengthen the robustness of gas accounting.
build.zig (6)
108-110: LGTM!Adding the include path for C imports is appropriate for the compiler module's FFI requirements.
192-202: LGTM!The switch to a crypto-focused WASM source aligns well with the addition of BLAKE2F precompile support.
775-795: Good addition of CREATE contract tests.The test suite is properly configured and correctly added to the main test step.
819-839: LGTM!The BLAKE2F precompile test is properly configured and integrated into the test suite, replacing the old RIPEMD160 test as mentioned in the PR summary.
882-908: Excellent addition of tevm-runner for benchmarking.The runner is properly configured with all necessary module imports and uses appropriate optimization settings for benchmarking. The build step is correctly set up.
1364-1537: Well-structured benchmark suite implementation.All benchmarks are properly configured with ReleaseFast optimization and correctly added to the combined benchmark step, ensuring they can be run together with
zig build bench.
| var return_data: [32]u8 = undefined; | ||
| std.mem.writeInt(u256, &return_data, @as(u256, @intCast(i)), .big); | ||
|
|
There was a problem hiding this comment.
Fix writeInt / readInt slice misuse – code will not compile
std.mem.writeInt/readInt expect a slice ([]u8).
Passing &return_data (a *[32]u8) or &address_bytes (*[32]u8) causes a type mismatch.
-std.mem.writeInt(u256, &return_data, value, .big);
+std.mem.writeInt(u256, return_data[0..], value, .big);
...
-std.mem.readInt(u256, &address_bytes, .big);
+std.mem.readInt(u256, address_bytes[0..], .big);Apply the same pattern to every occurrence; otherwise the file fails to build.
Also applies to: 363-365, 378-379
🤖 Prompt for AI Agents
In bench/call_ops_bench.zig at lines 201-203, the code incorrectly passes a
pointer to a fixed-size array (&return_data) to std.mem.writeInt, which expects
a slice ([]u8). To fix this, change the argument to a slice by removing the
address-of operator and passing return_data directly or using return_data[0..]
to create a slice. Apply the same fix to all similar occurrences at lines
363-365 and 378-379 to ensure the code compiles correctly.
| var vm = try Vm.init(allocator); | ||
| defer vm.deinit(); | ||
|
|
||
| var frame = Frame{ | ||
| .stack = Stack{}, | ||
| .memory = try Memory.init(allocator), | ||
| .gas_remaining = gas_limit, | ||
| .contract_address = [_]u8{0x12, 0x34, 0x56, 0x78} ++ [_]u8{0} ** 16, | ||
| .caller = [_]u8{0xAA, 0xBB, 0xCC, 0xDD} ++ [_]u8{0} ** 16, | ||
| .call_value = 0, | ||
| .call_data = &[_]u8{}, | ||
| .return_data = &[_]u8{}, | ||
| .code = bytecode, | ||
| .is_static = false, | ||
| .depth = 0, | ||
| }; | ||
| defer frame.memory.deinit(); | ||
|
|
||
| const initial_gas = frame.gas_remaining; | ||
|
|
||
| const result = vm.interpret(&frame) catch |err| switch (err) { | ||
| error.Revert, error.OutOfGas, error.StackUnderflow, error.StackOverflow => return .{ | ||
| .success = false, | ||
| .gas_used = initial_gas - frame.gas_remaining, | ||
| .return_data = frame.return_data, | ||
| .final_stack_size = frame.stack.size, | ||
| .memory_size = frame.memory.size(), | ||
| }, | ||
| else => return err, | ||
| }; | ||
|
|
||
| return .{ | ||
| .success = true, | ||
| .gas_used = initial_gas - frame.gas_remaining, | ||
| .return_data = result.return_data, | ||
| .final_stack_size = frame.stack.size, | ||
| .memory_size = frame.memory.size(), | ||
| }; | ||
| } |
There was a problem hiding this comment.
Returned return_data becomes dangling after frame.memory.deinit()
frame.memory.deinit() frees the backing buffer before the slice is returned
to the caller, leading to use-after-free in every test.
Allocate a copy before deinit:
-const result = vm.interpret(&frame) ...;
+const result = vm.interpret(&frame) ...;
+const rd_copy = try allocator.dupe(u8, result.return_data);
...
- .return_data = result.return_data,
+ .return_data = rd_copy,Remember to free the copy in the caller.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In test/evm/e2e_cross_opcode_test.zig around lines 28 to 66, the return_data
slice from frame is returned after frame.memory.deinit() is called, causing a
dangling pointer and use-after-free. To fix this, allocate a new copy of the
return_data slice before calling frame.memory.deinit(), then return this copy
instead. Ensure that the caller is responsible for freeing this copied
return_data to avoid memory leaks.
| var frame = Frame{ | ||
| .stack = Stack{}, | ||
| .memory = try Memory.init(allocator), | ||
| .gas_remaining = ctx.gas_limit, | ||
| .contract_address = ctx.to orelse [_]u8{0x12, 0x34, 0x56, 0x78} ++ [_]u8{0} ** 16, | ||
| .caller = ctx.from, | ||
| .call_value = ctx.value, | ||
| .call_data = ctx.data, | ||
| .return_data = &[_]u8{}, | ||
| .code = bytecode, | ||
| .is_static = false, | ||
| .depth = 0, | ||
| }; | ||
| defer frame.memory.deinit(); | ||
|
|
||
| const initial_gas = frame.gas_remaining; | ||
|
|
||
| // Execute the transaction | ||
| const result = vm.interpret(&frame) catch |err| switch (err) { | ||
| error.OutOfGas => return TransactionResult{ | ||
| .success = false, | ||
| .gas_used = initial_gas, // Used all gas | ||
| .gas_refund = 0, | ||
| .return_data = &[_]u8{}, | ||
| .logs = &[_]u8{}, | ||
| .created_address = null, | ||
| .state_changes = 0, | ||
| }, | ||
| error.Revert => return TransactionResult{ | ||
| .success = false, | ||
| .gas_used = initial_gas - frame.gas_remaining, | ||
| .gas_refund = 0, // No refund on revert | ||
| .return_data = frame.return_data, | ||
| .logs = &[_]u8{}, | ||
| .created_address = null, | ||
| .state_changes = 0, | ||
| }, | ||
| else => return err, | ||
| }; | ||
|
|
||
| return TransactionResult{ | ||
| .success = true, | ||
| .gas_used = initial_gas - frame.gas_remaining, | ||
| .gas_refund = 0, // TODO: Implement gas refund tracking | ||
| .return_data = result.return_data, | ||
| .logs = &[_]u8{}, // TODO: Implement log collection | ||
| .created_address = if (ctx.to == null) ctx.to else null, | ||
| .state_changes = 0, // TODO: Track state changes | ||
| }; | ||
| } |
There was a problem hiding this comment.
return_data slice invalid after frame memory is freed
frame.memory.deinit() runs on function exit, so any slice from that memory
escapes dangling. Duplicate the data (or move the deinit after the copy).
🤖 Prompt for AI Agents
In test/evm/e2e_transaction_gas_test.zig around lines 49 to 98, the return_data
slice returned references memory owned by frame.memory, which is deinitialized
immediately after the frame is created, causing a dangling pointer. To fix this,
copy the return_data contents into a new buffer before frame.memory.deinit() is
called, or defer the deinit until after the return_data is safely copied and
returned, ensuring no references to freed memory escape.
| var frame = try createTestFrame(allocator, gas_limit); | ||
| defer frame.memory.deinit(); | ||
|
|
||
| frame.code = bytecode; | ||
|
|
||
| const initial_gas = frame.gas_remaining; | ||
|
|
||
| // Execute the bytecode using the VM | ||
| const result = vm.interpret(&frame) catch |err| switch (err) { | ||
| error.Revert => return .{ | ||
| .success = false, | ||
| .gas_used = initial_gas - frame.gas_remaining, | ||
| .return_data = frame.return_data, | ||
| .logs = &[_]u8{}, | ||
| }, | ||
| else => return err, | ||
| }; | ||
|
|
||
| return .{ | ||
| .success = true, | ||
| .gas_used = initial_gas - frame.gas_remaining, | ||
| .return_data = result.return_data, | ||
| .logs = &[_]u8{}, // TODO: Implement log collection | ||
| }; | ||
| } |
There was a problem hiding this comment.
Use-after-free: returned data points into freed memory
frame.memory.deinit() frees the buffer that backs result.return_data.
Clone the slice or defer the deinit until after the copy.
🤖 Prompt for AI Agents
In test/evm/e2e_contract_execution_test.zig around lines 54 to 78, the code
calls frame.memory.deinit() before returning result.return_data, causing a
use-after-free since return_data points to freed memory. To fix this, either
clone or copy the return_data slice into a new buffer before deinitializing
frame.memory, or defer the call to frame.memory.deinit() until after the
return_data has been safely copied and returned, ensuring no references to freed
memory are returned.
| var vm = try Vm.init(allocator); | ||
| defer vm.deinit(); | ||
|
|
||
| var frame = Frame{ | ||
| .stack = Stack{}, | ||
| .memory = try Memory.init(allocator), | ||
| .gas_remaining = gas_limit, | ||
| .contract_address = address, | ||
| .caller = [_]u8{0xAA} ** 20, | ||
| .call_value = 0, | ||
| .call_data = &[_]u8{}, | ||
| .return_data = &[_]u8{}, | ||
| .code = bytecode, | ||
| .is_static = false, | ||
| .depth = 0, | ||
| }; | ||
| defer frame.memory.deinit(); | ||
|
|
||
| const initial_gas = frame.gas_remaining; | ||
| const initial_changes = state.journal.items.len; | ||
|
|
||
| const result = vm.interpret(&frame) catch |err| switch (err) { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
VM runs without being wired to TestStateManager
executeWithState spins up a fresh Vm but never passes state into it.
Unless Vm internally discovers a global state manager, any SSTORE/SLOAD executed inside vm.interpret will not touch the state.journal, so result.state_changes and the subsequent storage assertions may always be zero / stale.
Expose a hook such as Vm.initWithState(state) or pass callbacks so that contract execution actually mutates the test-state structure.
I’ll gather the definitions of the test, Frame, and Vm to confirm whether state is ever passed into vm.interpret. Running:
🏁 Script executed:
#!/bin/bash
set -e
echo "1) Locating test file"
fd e2e_state_management_test.zig
TEST_FILE=$(fd e2e_state_management_test.zig)
echo -e "\n2) Showing top of test file (state setup & usage)"
sed -n '1,200p' "$TEST_FILE"
echo -e "\n3) Searching for interpret() signature"
rg -n "fn interpret" -g '*.zig'
echo -e "\n4) Finding Frame struct definition"
rg -l "struct Frame" -g '*.zig'
FRAME_FILE=$(rg -l "struct Frame" -g '*.zig' | head -n1)
echo "→ $FRAME_FILE"
sed -n '1,200p' "$FRAME_FILE"
echo -e "\n5) Finding Vm definition & init()"
rg -l "pub const Vm" -g '*.zig'
VM_FILE=$(rg -l "pub const Vm" -g '*.zig' | head -n1)
echo "→ $VM_FILE"
sed -n '1,200p' "$VM_FILE" | rg -n "fn init"Length of output: 8305
I need to inspect the Frame definition and the Vm implementation to see if any state hook exists:
🏁 Script executed:
#!/bin/bash
set -e
# 1) Find where Frame is defined
echo "--- Frame definitions ---"
rg -n "Frame = struct" -g '*.zig' || echo "No matches for 'Frame = struct'"
echo -e "\n--- pub const Frame ---"
rg -n "pub const Frame" -g '*.zig' || echo "No matches for 'pub const Frame'"
# 2) Inspect that file if found
FRAME_FILE=$(rg -l "Frame = struct" -g '*.zig' | head -n1)
if [ -n "$FRAME_FILE" ]; then
echo -e "\n--- Contents of $FRAME_FILE (lines 1–200) ---"
sed -n '1,200p' "$FRAME_FILE"
fi
# 3) Locate the Vm type and its init methods
VM_FILE=$(rg -l "pub const Vm" -g '*.zig' | head -n1)
echo -e "\n--- Vm file: $VM_FILE ---"
rg -n "fn init" "$VM_FILE" || echo "No init methods found in Vm"
# 4) Search for any 'state' mentions in the Vm implementation
echo -e "\n--- 'state' references in $VM_FILE ---"
rg -n "state" "$VM_FILE" || echo "No 'state' usage in Vm"
# 5) Examine interpret implementation to see how storage ops are dispatched
echo -e "\n--- vm.interpret (lines 130–250) ---"
sed -n '130,250p' "$VM_FILE"Length of output: 12337
Link VM to the TestStateManager before interpreting
The current executeWithState helper spins up the VM with only Vm.init(allocator), which defaults to its internal in-memory database. As a result, any SSTORE/SLOAD during vm.interpret hits that DB, not your TestStateManager, so state.journal never sees the storage changes (and state_changes stays zero).
Please update test/evm/e2e_state_management_test.zig around the Vm.init call (lines ~149–170) to pass your state manager’s database interface into the VM. For example:
- var vm = try Vm.init(allocator);
+ // Wire up our TestStateManager so journal entries are recorded
+ const db_interface = state.toDatabaseInterface();
+ var vm = try Vm.init(allocator, db_interface, /*precompiles=*/null, /*chain_rules=*/null);If no two-arg overload exists yet, add a helper such as:
pub fn initWithState(
allocator: std.mem.Allocator,
db_interface: DatabaseInterface,
) !Vm {
return try Vm.init(allocator, db_interface, null, null);
}Then call that so all storage ops mutate state.journal as intended.
🤖 Prompt for AI Agents
In test/evm/e2e_state_management_test.zig around lines 149 to 170, the Vm is
initialized without linking it to the TestStateManager, so storage operations do
not affect the test state journal. Fix this by modifying the Vm initialization
to accept and use the state manager's database interface, either by using an
existing Vm.init overload that takes the state or by adding a new helper
function like initWithState that passes the state manager's database interface
into the Vm. This ensures vm.interpret mutates the test state as expected.
| export fn free(ptr: [*]u8, len: usize) void { | ||
| const slice = ptr[0..len]; | ||
| gpa.free(slice); | ||
| } |
There was a problem hiding this comment.
Unsafe free – length must match original allocation
free reconstructs a slice from an arbitrary (ptr,len) pair and hands it back to the allocator.
If the supplied len differs from the size originally returned by alloc, memory corruption/UB can occur.
Return the length from alloc (or store it in a header) and assert it in free.
🤖 Prompt for AI Agents
In src/root_wasm_crypto.zig around lines 17 to 20, the free function
reconstructs a slice from a pointer and length without verifying that the length
matches the original allocation size, risking memory corruption. Modify the
allocation function to return or store the allocation length, then update free
to retrieve and assert that the length matches before calling gpa.free, ensuring
safe deallocation.
| export fn hexToBytes(hex_ptr: [*]const u8, hex_len: usize, output_ptr: [*]u8) usize { | ||
| // Skip "0x" prefix if present | ||
| var start_idx: usize = 0; | ||
| if (hex_len >= 2 and hex_ptr[0] == '0' and (hex_ptr[1] == 'x' or hex_ptr[1] == 'X')) { | ||
| start_idx = 2; | ||
| } | ||
|
|
||
| // Get the hex string (without prefix) | ||
| const hex_string = hex_ptr[start_idx..hex_len]; | ||
|
|
||
| // Calculate output buffer size (each pair of hex chars = 1 byte) | ||
| const hex_len_adjusted = hex_len - start_idx; | ||
| const out_len = (hex_len_adjusted + 1) / 2; // Handle odd length | ||
|
|
||
| // Create output slice for conversion | ||
| const out_slice = output_ptr[0..out_len]; | ||
|
|
||
| // Manual conversion | ||
| var out_idx: usize = 0; | ||
| var i: usize = 0; | ||
|
|
||
| // Handle odd number of hex digits | ||
| if (hex_len_adjusted % 2 != 0) { | ||
| if (std.fmt.parseInt(u8, hex_string[0..1], 16)) |val| { | ||
| out_slice[out_idx] = val; | ||
| out_idx += 1; | ||
| i += 1; | ||
| } else |_| { | ||
| // Handle error by skipping this char | ||
| } | ||
| } | ||
|
|
||
| // Process the rest of the pairs | ||
| while (i < hex_len_adjusted) : (i += 2) { | ||
| if (i + 1 >= hex_len_adjusted) { | ||
| break; | ||
| } | ||
|
|
||
| if (std.fmt.parseInt(u8, hex_string[i .. i + 2], 16)) |val| { | ||
| out_slice[out_idx] = val; | ||
| out_idx += 1; | ||
| } else |_| { | ||
| // Handle error by skipping this pair | ||
| } | ||
| } | ||
|
|
||
| return out_idx; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
hexToBytes silently drops invalid characters and may overflow caller buffer
parseInterrors are swallowed, producing truncated output without signalling failure.- The function trusts
output_ptris large enough; the caller can pass a too-small buffer.
Add explicit validation & propagate an error when:
- non-hex characters occur, or
out_lenexceeds the supplied buffer capacity.
🤖 Prompt for AI Agents
In src/root_wasm_crypto.zig around lines 54 to 100, the hexToBytes function
currently ignores parseInt errors and assumes the output buffer is large enough,
which can cause silent data loss or buffer overflow. Modify the function to
explicitly check for non-hex characters by handling parseInt errors and return
an error or failure indicator instead of silently skipping invalid input. Also,
add a parameter or mechanism to receive the output buffer capacity and validate
that out_len does not exceed this capacity, returning an error if it does to
prevent buffer overflow.
| export fn keccak256_hex(hex_ptr: [*]const u8, hex_len: usize, output_ptr: [*]u8) usize { | ||
| var binary_buffer: [1024]u8 = undefined; // Buffer for binary data converted from hex | ||
|
|
||
| // Convert hex to bytes | ||
| const binary_len = hexToBytes(hex_ptr, hex_len, &binary_buffer); | ||
|
|
||
| // Compute keccak256 hash | ||
| var hash: [32]u8 = undefined; | ||
| std.crypto.hash.sha3.Keccak256.hash(binary_buffer[0..binary_len], &hash, .{}); | ||
|
|
||
| // Convert hash to hex string | ||
| return bytesToHex(&hash, 32, output_ptr); | ||
| } |
There was a problem hiding this comment.
Possible stack-buffer overflow in keccak256_hex
binary_buffer is a fixed [1024]u8, but hexToBytes can write up to (hex_len+1)/2 bytes without bounds checking.
A user-provided hex string > 2046 chars will overflow the buffer.
Guard with:
if (binary_len > binary_buffer.len)
return 0; // or error.BufferTooSmallor allocate dynamically.
🤖 Prompt for AI Agents
In src/root_wasm_crypto.zig around lines 127 to 139, the fixed-size buffer
binary_buffer of length 1024 can overflow if hexToBytes writes more than 1024
bytes, which happens when the input hex string is longer than 2046 characters.
To fix this, add a check after calling hexToBytes to verify that binary_len does
not exceed binary_buffer.len, and return 0 or an appropriate error if it does.
Alternatively, replace the fixed buffer with a dynamically allocated buffer
sized based on the input length to safely handle larger inputs.
| block_number: u256, | ||
| block_timestamp: u256, | ||
|
|
||
| const AccountInfo = struct { | ||
| balance: u256, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
block_number / block_timestamp are never fed into the VM
Fields are stored on ContractEnvironment but executeContract builds a Frame without them.
Opcodes NUMBER and TIMESTAMP inside the tested bytecode will therefore return whatever defaults the VM uses, breaking realism of the voting / escrow tests.
Extend Frame (and the VM) to accept the desired block context, or inject them via VM setters before calling interpret.
I’ll verify how executeContract, ContractEnvironment, and Frame interact to confirm if block context isn’t propagated.
🏁 Script executed:
#!/bin/bash
set -eo pipefail
echo "1) executeContract occurrences:"
rg -n "executeContract" .
echo
echo "2) ContractEnvironment definition and usage:"
rg -n "ContractEnvironment" .
echo
echo "3) Frame struct definition:"
rg -n "struct Frame" .
echo
echo "4) Frame instantiation or constructor calls:"
rg -n "Frame(" .Length of output: 5385
I’ll pull in the executeContract implementation and look for any Frame construction to see if block context is wired through:
🏁 Script executed:
#!/bin/bash
set -eo pipefail
echo "1) executeContract definition in tests:"
rg -n -A20 -B5 "fn executeContract" test/evm/e2e_real_world_scenarios_test.zig
echo
echo "2) Any Frame instantiation:"
rg -n "Frame{" .
echo
echo "3) Any direct VM setter for block_number or timestamp:"
rg -n "block_number" -A3 -B3 .
rg -n "block_timestamp" -A3 -B3 .Length of output: 1364
Propagate block_number & block_timestamp into the VM frame
The ContractEnvironment in test/evm/e2e_real_world_scenarios_test.zig defines block_number and block_timestamp (lines 26–30) but executeContract (lines 118–130) builds a Frame without them. As a result, EVM opcodes NUMBER and TIMESTAMP will fall back to VM defaults, invalidating real-world scenario tests.
Please update as follows:
• In your VM’s Frame definition (e.g. wherever Frame = struct { … } lives), add:
block_number: u256,
block_timestamp: u256,• In executeContract (test/evm/e2e_real_world_scenarios_test.zig:118–130), inject the fields:
var frame = Frame{
.stack = Stack{},
.memory = try Memory.init(self.allocator),
.gas_remaining = gas_limit,
.contract_address= contract_addr,
.caller = caller,
.call_value = value,
.call_data = call_data,
.return_data = &[_]u8{},
.code = account.code,
.is_static = false,
.depth = 0,
+ .block_number = self.block_number,
+ .block_timestamp = self.block_timestamp,
};• Ensure your interpreter reads these fields when handling NUMBER and TIMESTAMP opcodes.
This change will restore realistic block context in your end-to-end tests.
🤖 Prompt for AI Agents
In test/evm/e2e_real_world_scenarios_test.zig around lines 26 to 30 and 118 to
130, the block_number and block_timestamp fields are defined in
ContractEnvironment but not passed into the VM Frame, causing NUMBER and
TIMESTAMP opcodes to use default values. To fix this, add block_number and
block_timestamp as u256 fields to the Frame struct definition, then update the
executeContract function to initialize these fields in the Frame instance using
the values from ContractEnvironment. Finally, ensure the VM interpreter accesses
these Frame fields when executing the NUMBER and TIMESTAMP opcodes to reflect
the correct block context in tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
build.zig (2)
841-875:⚠️ Potential issueStale memory-benchmark references break the build
test/Bench/memory_benchmark.zigandbench/evm/memory_bench.zigwere deleted in this PR, yet the build script still:
- adds two executables (
memory-benchmark,evm-memory-benchmark)- makes
all_benchmark_stepdepend on their run steps
zig build benchwill now fail with file not found.Either restore the files or delete the corresponding build graph nodes:
-// Remove obsolete memory benchmarks -const memory_benchmark = ... -... -const evm_memory_benchmark = ... -... -all_benchmark_step.dependOn(&run_memory_benchmark.step); -all_benchmark_step.dependOn(&run_evm_memory_benchmark.step);Keeping dead paths around hides real regressions and wastes CI minutes.
Also applies to: 878-882
1643-1665:⚠️ Potential issueNew tests are still missing from the aggregate
teststep
run_compiler_hex_decode_test,run_function_selector_debug_test,run_dispatch_failure_test, … are defined above but not added totest_step.
CI will silently skip them.Add:
test_step.dependOn(&run_compiler_hex_decode_test.step); test_step.dependOn(&run_function_selector_debug_test.step); test_step.dependOn(&run_dispatch_failure_test.step); // … and any other newly-created run stepsFail-fast coverage is the point of these debug suites—ensure they actually run.
♻️ Duplicate comments (1)
test/evm/compiler_bytecode_issue_test.zig (1)
99-101: 🛠️ Refactor suggestionSame divide-by-zero risk as in
compiler_hex_decode_testThe percentage calculation will panic when
runtime_bytecode.len == 0.See the previous comment for a safe pattern; apply the same fix here.
🧹 Nitpick comments (4)
build.zig (1)
573-604: Extreme copy-paste – consider a helper to cut 300+ linesDozens of blocks repeat the same addExecutable → addRunArtifact → step wiring boilerplate.
Maintenance is becoming error-prone (see missingtest_stepdeps above).Factor out a small helper:
fn addDebugExe( b: *std.Build, name: []const u8, src: []const u8, imports: []const struct { name: []const u8, module: *std.Build.Module } ) *std.Build.Step { const exe = b.addExecutable(.{ .name = name, .root_source_file = b.path(src), .target = target, .optimize = optimize }); for (imports) |imp| exe.root_module.addImport(imp.name, imp.module); exe.root_module.stack_check = false; exe.root_module.single_threaded = true; const run = b.addRunArtifact(exe); const step = b.step(name, "Run " ++ name); step.dependOn(&run.step); return step; }and replace the repetitive regions with one-liners.
Also applies to: 605-632
test/evm/function_selector_debug_test.zig (1)
131-154:forloop may read past end whenruntime_bytecode.len < 6The range
0..runtime_bytecode.len - 6underflows whenlen < 6, leading to an empty slice with a very large upper bound.Guard before the loop:
if (runtime_bytecode.len >= 6) { for (0..runtime_bytecode.len - 6) |i| { ... } }Edge-case safety keeps diagnostic tools usable on minimal contracts.
test/evm/dispatch_failure_reproduction_test.zig (2)
109-113: Prefer logical operators for boolean conditions
andworks because it’s a bit-wise operator onbool, but using&&is the idiomatic logical operator and reads better:- while (gas_consumed < 50 and pc < runtime_bytecode.len and step < 25) { + while (gas_consumed < 50 && pc < runtime_bytecode.len && step < 25) {
26-33: Redundantoptimizer_runswhen optimisation disabled
optimizer_enabled = falsemakesoptimizer_runsirrelevant. Either enable optimisation or drop the field to avoid misleading settings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
bench/evm(1 hunks)bench/evm.zig(1 hunks)bench/opcode_bench.zig(1 hunks)bench/stack_bench.zig(1 hunks)build.zig(9 hunks)snailtracer_actual_bytecode.hex(1 hunks)snailtracer_complete_bytecode.hex(1 hunks)test/evm/compiler_bytecode_issue_test.zig(1 hunks)test/evm/compiler_hex_decode_test.zig(1 hunks)test/evm/contract_deployment_test.zig(1 hunks)test/evm/contract_execution_debug_test.zig(1 hunks)test/evm/contract_initialization_test.zig(1 hunks)test/evm/dispatch_failure_reproduction_test.zig(1 hunks)test/evm/function_selector_debug_test.zig(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- snailtracer_complete_bytecode.hex
- snailtracer_actual_bytecode.hex
🚧 Files skipped from review as they are similar to previous changes (7)
- bench/evm
- test/evm/contract_deployment_test.zig
- bench/stack_bench.zig
- test/evm/contract_initialization_test.zig
- bench/opcode_bench.zig
- test/evm/contract_execution_debug_test.zig
- bench/evm.zig
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
test/evm/compiler_hex_decode_test.zig (1)
99-101:❌ Incorrect review comment
Guard against divide-by-zero and potential overflow in percentage math
ascii_percentageandvalid_opcode_percentageblindly divide byruntime_bytecode.len.
If compilation ever yields an empty byte-slice, the test will crash with a division by zero panic before the laterexpect(len > 10)guard runs.-const ascii_percentage = (ascii_hex_count * 100) / runtime_bytecode.len; -const valid_opcode_percentage = (valid_opcode_count * 100) / runtime_bytecode.len; +const ascii_percentage: usize = if (runtime_bytecode.len != 0) + (ascii_hex_count * 100) / runtime_bytecode.len +else + 0; +const valid_opcode_percentage: usize = if (runtime_bytecode.len != 0) + (valid_opcode_count * 100) / runtime_bytecode.len +else + 0;Also consider casting one operand to
u128/usizeafter the division to avoid a theoretical overflow ofascii_hex_count * 100on very large contract artefacts.
🏁 Script executed:
#!/bin/bash # Confirm divide-by-zero guard is already present rg -C3 "ascii_percentage" -n test/evm/compiler_hex_decode_test.zig rg -C3 "valid_opcode_percentage" -n test/evm/compiler_hex_decode_test.zigLength of output: 1593
No changes needed for divide-by-zero or overflow guards
The existing code already uses
if (runtime_bytecode.len > 0) … else 0to protect both
ascii_percentageandvalid_opcode_percentagefrom divide-by-zero, and in practice(ascii_hex_count * 100)can’t overflow ausizefor realistic bytecode lengths. You can safely leave the code as is.Likely an incorrect or invalid review comment.
test/evm/dispatch_failure_reproduction_test.zig (1)
49-54: Guard against empty contract list before indexing
compilation_result.contracts[0]will panic if the compiler returns zero contracts (e.g. on an empty source or unexpected compiler behaviour). Insert an explicit length check and fail early with a descriptive error.- const contract = compilation_result.contracts[0]; + if (compilation_result.contracts.len == 0) { + return error.NoContractsReturned; + } + const contract = compilation_result.contracts[0];
| for (0..runtime_bytecode.len - 3) |i| { | ||
| if (std.mem.eql(u8, runtime_bytecode[i .. i + 4], &benchmark_selector)) { | ||
| found_selector_at = i; | ||
| std.debug.print("Found Benchmark() selector at offset {}\n", .{i}); |
There was a problem hiding this comment.
Fix slice/array type mismatch in selector search
std.mem.eql expects two []const u8 slices, but &benchmark_selector is a pointer to [4]u8, so this code will not compile.
- if (std.mem.eql(u8, runtime_bytecode[i .. i + 4], &benchmark_selector)) {
+ if (std.mem.eql(u8, runtime_bytecode[i .. i + 4], benchmark_selector[0..])) {📝 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.
| for (0..runtime_bytecode.len - 3) |i| { | |
| if (std.mem.eql(u8, runtime_bytecode[i .. i + 4], &benchmark_selector)) { | |
| found_selector_at = i; | |
| std.debug.print("Found Benchmark() selector at offset {}\n", .{i}); | |
| for (0..runtime_bytecode.len - 3) |i| { | |
| if (std.mem.eql(u8, runtime_bytecode[i .. i + 4], benchmark_selector[0..])) { | |
| found_selector_at = i; | |
| std.debug.print("Found Benchmark() selector at offset {}\n", .{i}); |
🤖 Prompt for AI Agents
In test/evm/dispatch_failure_reproduction_test.zig around lines 68 to 71, the
call to std.mem.eql uses &benchmark_selector which is a pointer to a fixed-size
array instead of a slice. To fix this, convert benchmark_selector to a slice by
removing the address operator and using it directly as a slice, ensuring both
arguments to std.mem.eql are []const u8 slices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/evm/README.md (2)
85-87: Refine approximate metric notation for clarity
The tilde (~) before “75%” may be ambiguous in Markdown and isn’t a standard notation for “approximately.”- **Overall Progress**: ~75% of core EVM functionality implemented, but significant stability and integration issues prevent production use. + **Overall Progress**: Approximately 75% of core EVM functionality implemented, but significant stability and integration issues prevent production use.
294-294: Add comma before ‘but’ in compound sentence
A comma improves readability when joining two independent clauses.- - [ ] 🔴 **WASM Build Integration** - [WASM builds successfully but exports don't match JavaScript interface expectations](./prompts/implement-wasm-build-fix.md) + - [ ] 🔴 **WASM Build Integration** - [WASM builds successfully, but exports don't match JavaScript interface expectations](./prompts/implement-wasm-build-fix.md)🧰 Tools
🪛 LanguageTool
[uncategorized] ~294-~294: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ntegration** - [WASM builds successfully but exports don't match JavaScript interfac...(COMMA_COMPOUND_SENTENCE_2)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/evm/README.md(19 hunks)
🧰 Additional context used
🪛 LanguageTool
src/evm/README.md
[uncategorized] ~294-~294: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ntegration** - [WASM builds successfully but exports don't match JavaScript interfac...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: CI Checks
- GitHub Check: WASM Bundle Size Check
- GitHub Check: Analyze (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/evm/README.md (2)
85-87: Clarify approximate percentage phrasingThe tilde before “75%” may be confusing. Consider replacing
**Overall Progress**: ~75% of core EVM functionality implementedwith
**Overall Progress**: Approximately 75% of core EVM functionality implemented
96-100: Consider BLAKE2F stub status in partial worksYou’ve listed ECRECOVER, MODEXP, and KZG as partially working precompiles. Since a stub for BLAKE2F is now included in this PR, you may want to add BLAKE2F here instead of under “Missing Precompiles,” or clarify its stub-only status.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/evm/README.md(19 hunks)
🧰 Additional context used
🪛 LanguageTool
src/evm/README.md
[uncategorized] ~294-~294: Use a comma before ‘but’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...ntegration** - [WASM builds successfully but exports don't match JavaScript interfac...
(COMMA_COMPOUND_SENTENCE_2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: CI Checks
- GitHub Check: WASM Bundle Size Check
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
src/evm/README.md (3)
89-95: Approve summary of working componentsThe “What’s Working” section clearly summarizes completed areas of the implementation. The highlighted Core Architecture, opcode coverage, CREATE operations, and basic precompiles align with the current codebase status.
101-107: Inconsistent precompile statusThe “Missing Precompiles” bullet still lists BLAKE2F, but this PR adds a BLAKE2F stub. Either move BLAKE2F out of the missing list or annotate it as a placeholder stub in this section.
Likely an incorrect or invalid review comment.
108-109: Approve recommendation focusThe recommendation to prioritize test suite stability and CALL operation completion is clear and actionable. It aligns well with the outlined critical issues.
roninjin10
force-pushed
the
main
branch
2 times, most recently
from
a194b7f to
b83518d
Compare
1 participant
Description
Added the evm-bench submodule for benchmarking and integrated it with the build system. This PR:
Testing
The tevm-runner can be built with
zig build tevm-runnerand will be used for standardized benchmarking against other EVM implementations. The BLAKE2F precompile is currently a stub implementation that will be fully implemented in a future PR.Additional Information
Your ENS/address:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Refactor
Tests