✨ feat(evm): implement production-quality Frame module for execution context management #1736
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughA new EVM execution context module ( Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Test Suite
participant Frame as Frame
participant Stack as Stack
participant Memory as Memory
participant Gas as Gas
participant Contract as Contract
TestSuite->>Frame: init(contract, params)
Frame->>Stack: initialize
Frame->>Memory: initialize
Frame->>Gas: initialize
TestSuite->>Frame: push(value)
Frame->>Stack: push(value)
TestSuite->>Frame: pop()
Frame->>Stack: pop()
TestSuite->>Frame: consumeGas(amount)
Frame->>Gas: consume(amount)
TestSuite->>Frame: ensureMemory(size)
Frame->>Memory: expand(size)
Frame->>Gas: memoryGasCost(size)
TestSuite->>Frame: setReturnData(data)
Frame->>Memory: allocate(data)
TestSuite->>Frame: halt(reason)
Frame-->>TestSuite: halt_reason set
TestSuite->>Frame: deinit()
Frame->>Memory: cleanup
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 (
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
…context management Adds a comprehensive execution frame module that manages: - Stack and memory delegation with efficient operations - Program counter and execution control (jumps, halts) - Gas metering with memory expansion cost calculation - Return data management and buffer handling - Static mode enforcement for read-only calls - Call depth tracking for recursion limits - Debug support with conditional logging The implementation follows patterns from revm, evmone, and go-ethereum with optimizations for zero-allocation paths and inline hot functions. Includes extensive test coverage for all functionality including edge cases, error conditions, and integration scenarios. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/Evm/frame_test.zig (1)
24-44: Consider extracting test bytecode pattern for better maintainability.The test bytecode is currently hardcoded inline. While acceptable for tests, consider extracting it as a constant for better readability and reusability across tests.
+const TEST_BYTECODE = [_]u8{ + constants.PUSH1, 0x42, + constants.JUMPDEST, + constants.PUSH1, 0x00, + constants.JUMP, + constants.JUMPDEST, + constants.STOP, + 0x00, 0x00 +}; + fn createTestContract(allocator: std.mem.Allocator) !*Contract { - const code = try allocator.alloc(u8, 10); - @memcpy(code, &[_]u8{ constants.PUSH1, 0x42, constants.JUMPDEST, constants.PUSH1, 0x00, constants.JUMP, constants.JUMPDEST, constants.STOP, 0x00, 0x00 }); + const code = try allocator.alloc(u8, TEST_BYTECODE.len); + @memcpy(code, &TEST_BYTECODE);src/evm/Frame.zig (1)
170-190: Consider validating depth parameter during initialization.The frame accepts any depth value but has a limit of 1024. Consider validating this during initialization to fail fast.
Add depth validation:
pub fn init( allocator: std.mem.Allocator, contract: *Contract, gas: *Gas, is_static: bool, depth: u32, ) !Frame { + if (depth >= 1024) { + return error.DepthLimit; + } return Frame{ .stack = Stack{}, .memory = try Memory.init(allocator), .contract = contract, .gas = gas, .pc = 0, .return_data = null, .halt_reason = .None, .is_static = is_static, .depth = depth, .allocator = allocator, }; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build.zig(2 hunks)src/evm/Frame.zig(1 hunks)src/evm/evm.zig(1 hunks)test/Evm/frame_test.zig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: CI Checks
🔇 Additional comments (10)
src/evm/evm.zig (1)
34-39: LGTM! Clean module integration.The Frame module is properly imported and its key types are re-exported, making them accessible through the main evm module. This follows the established pattern used for other submodules in this file.
build.zig (2)
287-314: Test targets properly configured.The Frame test targets are correctly set up with appropriate module imports. Both
frame-testandframe-minimal-testfollow the established patterns for test configuration in this build file.
609-609: Frame tests correctly integrated into test suite.The frame test is properly added as a dependency to the main test step, ensuring it runs as part of the overall test suite.
test/Evm/frame_test.zig (2)
48-568: Excellent comprehensive test coverage!The test suite thoroughly covers all aspects of the Frame module:
- ✅ Proper resource management with defer statements
- ✅ Edge case testing (stack overflow/underflow, out of gas, invalid jumps)
- ✅ Gas metering accuracy verification
- ✅ Memory operation and expansion testing
- ✅ Integration scenarios simulating real execution
- ✅ Debug functionality validation
The tests are well-structured, follow consistent patterns, and provide production-quality coverage for the Frame module.
14-14:❓ Verification inconclusive
Verify the Address import path is correct.
The Address type is imported from a separate "address" module rather than from the evm module. Please confirm this is the intended import path and aligns with the project's module structure.
🏁 Script executed:
#!/bin/bash # Description: Verify if Address is also exported from evm module or if the separate import is required # Check if Address is exported from evm module rg -A 5 "pub const Address" src/evm/evm.zig # Check for other files importing Address to see the pattern rg "import.*[Aa]ddress" --type zigLength of output: 290
EVM module doesn’t export Address; test import path is required
We checked
src/evm/evm.zigand found nopub const Address, so pulling inAddressfrom its own module is correct. Other usages in the repo include:
src/Signature/eip712.zig→@import("../Address/address.zig").Addresssrc/Block/block.zig→@import("Address")test/Evm/frame_test.zig→@import("address").AddressPlease confirm that
@import("address")in your test resolves to the samesrc/Address/address.zigfile and aligns with your Zig module-naming conventions. If not, consider normalizing the import path (casing and/or relative path) for consistency.src/evm/Frame.zig (5)
1-41: Well-structured module setup with comprehensive documentation.The file header provides excellent context with performance characteristics and reference implementations. The error types comprehensively cover all frame operation failures.
42-62: Comprehensive halt reason enumeration.The
HaltReasonenum covers all EVM execution termination scenarios, including normal exits, errors, and edge cases specific to contract creation.
360-386: Excellent return data management.The implementation properly handles memory allocation and cleanup, with null safety for empty return data.
460-500: Well-designed debug support with zero runtime cost.The debug functions are properly gated by compile-time checks, ensuring no performance impact in release builds. The logging provides comprehensive frame state information.
318-342:⚠️ Potential issueFix endianness for EVM memory word operations.
EVM uses little-endian byte ordering for memory storage. The current implementation uses big-endian, which will cause incorrect behavior with memory-related opcodes.
Apply these fixes:
/// Read word from memory pub fn readMemoryWord(self: *Frame, offset: usize) !u256 { try self.ensureMemory(offset, 32); const word = try self.memory.getWord(offset); var result: u256 = 0; var i: usize = 0; while (i < 32) : (i += 1) { - result = (result << 8) | word[i]; + result |= @as(u256, word[i]) << @intCast(i * 8); } return result; } /// Write word to memory pub fn writeMemoryWord(self: *Frame, offset: usize, value: u256) !void { try self.ensureMemory(offset, 32); var word: [32]u8 = undefined; var val = value; - var i: usize = 32; - while (i > 0) { - i -= 1; + var i: usize = 0; + while (i < 32) : (i += 1) { word[i] = @truncate(val); val >>= 8; } try self.memory.setWord(offset, word); }Likely an incorrect or invalid review comment.
| /// Add gas refund | ||
| pub fn refund(self: *Gas, amount: u64) void { | ||
| self.refunded = @min(self.refunded + amount, std.math.maxInt(u64) - amount); | ||
| } |
There was a problem hiding this comment.
Fix the overflow protection logic in the refund function.
The current logic limits refunds incorrectly. The second parameter to @min should ensure the total doesn't exceed maxInt(u64).
Apply this fix:
- pub fn refund(self: *Gas, amount: u64) void {
- self.refunded = @min(self.refunded + amount, std.math.maxInt(u64) - amount);
- }
+ pub fn refund(self: *Gas, amount: u64) void {
+ const max_refund = std.math.maxInt(u64) - self.refunded;
+ self.refunded += @min(amount, max_refund);
+ }📝 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.
| /// Add gas refund | |
| pub fn refund(self: *Gas, amount: u64) void { | |
| self.refunded = @min(self.refunded + amount, std.math.maxInt(u64) - amount); | |
| } | |
| /// Add gas refund | |
| pub fn refund(self: *Gas, amount: u64) void { | |
| const max_refund = std.math.maxInt(u64) - self.refunded; | |
| self.refunded += @min(amount, max_refund); | |
| } |
🤖 Prompt for AI Agents
In src/evm/Frame.zig around lines 97 to 100, the refund function's overflow
protection is incorrect because it subtracts the amount from maxInt(u64) instead
of ensuring the sum of refunded and amount does not exceed maxInt(u64). Fix this
by changing the second parameter of @min to maxInt(u64) directly, so the
refunded value is capped at maxInt(u64) without subtracting amount.
| /// Check if valid jump destination | ||
| pub fn isValidJump(self: *const Frame, dest: usize) bool { | ||
| if (dest >= self.contract.code.len) return false; | ||
| if (self.contract.code[dest] != constants.JUMPDEST) return false; | ||
| // If contract has advanced analysis, use it | ||
| if (self.contract.analysis) |analysis| { | ||
| // Check if the jumpdest is in the valid positions | ||
| for (analysis.jumpdest_positions) |pos| { | ||
| if (pos == dest) return true; | ||
| } | ||
| return false; | ||
| } | ||
| // For simple contracts without analysis, JUMPDEST opcode check is sufficient | ||
| return true; | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security issue: Invalid jump destination validation without analysis.
The current implementation can incorrectly validate jumps to PUSH data that happens to contain the JUMPDEST byte (0x5B). Without contract analysis, you cannot determine if a JUMPDEST byte is an actual opcode or data.
The safe approach is to require contract analysis for jump validation:
/// Check if valid jump destination
pub fn isValidJump(self: *const Frame, dest: usize) bool {
if (dest >= self.contract.code.len) return false;
if (self.contract.code[dest] != constants.JUMPDEST) return false;
// If contract has advanced analysis, use it
if (self.contract.analysis) |analysis| {
// Check if the jumpdest is in the valid positions
for (analysis.jumpdest_positions) |pos| {
if (pos == dest) return true;
}
return false;
}
- // For simple contracts without analysis, JUMPDEST opcode check is sufficient
- return true;
+ // Without analysis, we cannot safely validate jump destinations
+ return false;
}To verify this concern, let's check if contract analysis is always performed before execution:
🏁 Script executed:
#!/bin/bash
# Search for contract initialization and analysis patterns
rg -A 10 "Contract.*init|analyze.*contract" --type zigLength of output: 21215
I want to confirm how Contract.analysis is populated. Let’s inspect the Contract definition and its init method:
🏁 Script executed:
#!/bin/bash
# Locate Contract struct and init function to see how 'analysis' is set
rg "pub const Contract" -A 20 --type zig
rg "pub fn init" -A 20 --type zigLength of output: 43293
Let’s inspect the validJumpdest implementation to see if and how it populates Contract.analysis before validation:
🏁 Script executed:
#!/bin/bash
# Find and show the validJumpdest method
rg "pub fn validJumpdest" -A20 --type zigLength of output: 1117
Let’s inspect the ensureAnalysis implementation in src/evm/Contract.zig to see how—and when—it populates self.analysis:
🏁 Script executed:
#!/bin/bash
# Locate and display the ensureAnalysis method
rg "fn ensureAnalysis" -A20 --type zig src/evm/Contract.zigLength of output: 761
Ensure analysis before validating JUMPDEST in Frame.isValidJump
Frame.isValidJump currently bypasses full analysis whenever contract.analysis is null, accepting any 0x5B byte as a valid jump destination (even if it’s PUSH data). Since contract.analysis only gets populated by calling Contract.ensureAnalysis(), the first JUMP into data still succeeds. We must always invoke analysis before approving a jump.
Locations to update:
- src/evm/Frame.zig (around lines 420–434)
Proposed change:
pub fn isValidJump(self: *const Frame, dest: usize) bool {
if (dest >= self.contract.code.len) return false;
if (self.contract.code[dest] != constants.JUMPDEST) return false;
+ // Populate valid jump positions before checking
+ self.contract.ensureAnalysis();
if (self.contract.analysis) |analysis| {
for (analysis.jumpdest_positions) |pos| {
if (pos == dest) return true;
}
return false;
}
- // For simple contracts without analysis, JUMPDEST opcode check is sufficient
- return true;
+ // No analysis available ⇒ disallow jump
+ return false;
}📝 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.
| /// Check if valid jump destination | |
| pub fn isValidJump(self: *const Frame, dest: usize) bool { | |
| if (dest >= self.contract.code.len) return false; | |
| if (self.contract.code[dest] != constants.JUMPDEST) return false; | |
| // If contract has advanced analysis, use it | |
| if (self.contract.analysis) |analysis| { | |
| // Check if the jumpdest is in the valid positions | |
| for (analysis.jumpdest_positions) |pos| { | |
| if (pos == dest) return true; | |
| } | |
| return false; | |
| } | |
| // For simple contracts without analysis, JUMPDEST opcode check is sufficient | |
| return true; | |
| } | |
| /// Check if valid jump destination | |
| pub fn isValidJump(self: *const Frame, dest: usize) bool { | |
| if (dest >= self.contract.code.len) return false; | |
| if (self.contract.code[dest] != constants.JUMPDEST) return false; | |
| // Populate valid jump positions before checking | |
| self.contract.ensureAnalysis(); | |
| if (self.contract.analysis) |analysis| { | |
| // Check if the jumpdest is in the valid positions | |
| for (analysis.jumpdest_positions) |pos| { | |
| if (pos == dest) return true; | |
| } | |
| return false; | |
| } | |
| // No analysis available ⇒ disallow jump | |
| return false; | |
| } |
🤖 Prompt for AI Agents
In src/evm/Frame.zig around lines 420 to 434, the isValidJump function currently
returns true if the byte at the destination is JUMPDEST without requiring
contract analysis, which can incorrectly validate jumps into PUSH data. To fix
this, modify isValidJump to always require that contract.analysis is present by
calling Contract.ensureAnalysis() if analysis is null before checking
jumpdest_positions. This ensures that jump validation only succeeds when full
contract analysis is available, preventing invalid jump destinations.
Description
Added a new
Framemodule to the EVM implementation, which manages execution context for a single call frame. The Frame module handles stack, memory, program counter, gas tracking, and return data management with zero-allocation paths for common operations.Added comprehensive test coverage for the Frame module, including basic initialization, execution control, stack operations, gas metering, memory operations, return data management, and context accessors.
Updated the build system to include frame tests in the test suite, with dedicated test steps for both standard and minimal frame tests.
Testing
Additional Information
Your ENS/address:
Summary by CodeRabbit
New Features
Tests