✨ feat(evm): implement production-quality Stack module for EVM #1724
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
WalkthroughThe changes introduce a comprehensive refactor and extension of the EVM stack implementation. The Stack module is rewritten for clarity, safety, and performance, adding generic swap/dup operations, EIP-663 support, utilities, and extensive tests and benchmarks. Public exports are updated, and new benchmark and test suites are provided. Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Stack Test/Benchmark
participant Stack as Stack
participant EVM as evm
participant Package as package
TestSuite->>Stack: push(value)
Stack-->>TestSuite: Result/Error
TestSuite->>Stack: pop()
Stack-->>TestSuite: value/Error
TestSuite->>Stack: swap(n) / dup(n) / popn(N)
Stack-->>TestSuite: Result/Error
EVM->>Stack: (re-export Stack, StackError, etc.)
Package->>EVM: (type alias Stack = evm.Stack)
TestSuite->>Package: use Stack for tests/benchmarks
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. |
- Add Memory struct with word-aligned operations and efficient resizing - Implement 4KB initial capacity with 2x growth strategy (matching evmone) - Add comprehensive read/write operations (getByte, getWord, getU256, setData) - Implement setDataBounded for CALLDATACOPY/CODECOPY with zero-fill support - Add memory copy with overlapping region support (MCOPY instruction) - Include memory limit enforcement and gas cost calculation support - Implement unsafe operations for performance-critical paths - Add snapshot/restore functionality for state management - Zero-initialize all expanded memory for security - Export Memory, MemoryError, and calculateNumWords from Evm module The implementation follows design patterns from revm and evmone: - Word-aligned (32-byte) operations for EVM compatibility - Efficient memory expansion with gas cost tracking - Page-aligned allocations for better OS memory management - No debug logging in production code
Implements a high-performance, EVM-compatible stack with 1024 u256 capacity: **Core Features**: - Fixed-size stack with 1024 u256 elements (32KB total) - 32-byte alignment for optimal cache performance - Zero heap allocations - purely stack-based - Dual safe/unsafe API pattern following revm design **Operations Implemented**: - Basic: push, pop, peek with bounds checking - Stack-relative: back, peek_n for indexed access - Swap operations: swap1-swap16 with manual optimization - Dup operations: dup with 1-16 range validation - Bulk operations: popn, popn_top for multi-value ops - EIP-663: dupn, swapn, exchange with dynamic indices - Utilities: clear, toSlice, checkRequirements **Performance Optimizations**: - All operations marked inline for cross-function optimization - Manual swap implementation avoiding std.mem.swap overhead - @setRuntimeSafety(false) in unsafe variants - Comptime swap/dup generation reduces code size - Direct indexing instead of pointer arithmetic - Security clearing only in safe variants **Testing**: - Comprehensive test suite covering all operations - Edge cases and error conditions tested - Performance benchmarks comparing with revm/evmone - Memory safety verification Targets: >500M push/pop ops/sec, >400M swap1 ops/sec
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
test/Bench/stack_benchmark.zig (1)
266-266: Use the imported STACK_LIMIT constant for consistency.The file imports
STACK_LIMITbut usesStack.capacityhere. While they likely have the same value, using the imported constant would be more consistent with the test file which usesSTACK_LIMIT.- for (0..Stack.capacity) |i| { + for (0..STACK_LIMIT) |i| {src/evm/Stack.zig (3)
174-190: Consider reducing code duplication in individual swap functions.While having individual swap functions can improve optimization, they all delegate to
swapN. Consider whether these are necessary given the generic implementation.You could simplify by removing these individual functions and using
swapNdirectly, or use themakeSwapNhelper consistently:- pub inline fn swap1(self: *Stack) StackError!void { return self.swapN(1); } - pub inline fn swap2(self: *Stack) StackError!void { return self.swapN(2); } - // ... etc + pub const swap1 = makeSwapN(1); + pub const swap2 = makeSwapN(2); + // ... etc
296-297: Clarify the overlap check in exchange operation.The comment "No overlap allowed" for
m == 0check needs clarification. According to EIP-663, this prevents exchanging an element with itself.- if (m == 0) return StackError.InvalidPosition; // No overlap allowed + if (m == 0) return StackError.InvalidPosition; // EIP-663: Cannot exchange element with itself
268-305: Add tests for EIP-663 operations.The EIP-663 operations (dupn, swapn, exchange) are implemented but lack test coverage. These dynamic operations are critical for EVM compatibility.
Would you like me to generate comprehensive tests for the EIP-663 operations to ensure they work correctly with dynamic indices?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
build.zig(0 hunks)src/evm/Stack.zig(3 hunks)src/evm/evm.zig(1 hunks)src/package.zig(1 hunks)test/Bench/stack_benchmark.zig(1 hunks)test/Evm/stack_test.zig(1 hunks)
💤 Files with no reviewable changes (1)
- build.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 (9)
src/package.zig (1)
10-11: LGTM!The Stack export follows the same pattern as the Memory export and is properly documented.
src/evm/evm.zig (1)
7-11: LGTM!The Stack module imports and exports follow the established pattern and expose the appropriate public API surface.
test/Evm/stack_test.zig (1)
1-528: Excellent test coverage!This comprehensive test suite thoroughly covers all Stack operations including:
- Basic operations (push, pop, peek)
- All swap variants (swap1-swap16, swapN, swapn)
- Duplication operations
- Bulk operations (popn, push_slice)
- EIP-663 operations (dupn, swapn, exchange)
- Edge cases and error conditions
- Memory safety verification
The tests are well-organized and provide confidence in the Stack implementation's correctness.
src/evm/Stack.zig (6)
3-7: LGTM! Good error type refactoring.Replacing
OutOfMemorywithInvalidPositionis appropriate for a statically allocated stack implementation.
27-33: Excellent performance optimizations!The 32-byte alignment and static allocation are well-suited for EVM stack operations. The documentation clearly explains the design rationale and comparisons with other implementations.
57-60: Good security practice with value clearing.Clearing popped values in the safe variant helps prevent potential data leakage. This is a thoughtful security enhancement.
122-134: Well-documented workaround for compiler optimization issues.The manual swap implementation with clear reference to evmone's approach for clang issue #59116 shows good attention to cross-compiler compatibility.
328-341:❌ Incorrect review comment
Consider removing redundant helper function.
The
makeSwapNfunction appears redundant since you already haveswapNmethods that provide the same functionality with better type safety.Let me verify if this helper is actually used elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Description: Check if makeSwapN is used outside of this file # Expected: No usage found, confirming it's redundant rg -A 2 "makeSwapN" --glob '!src/evm/Stack.zig'Length of output: 424
Ignore the removal suggestion—
makeSwapNis used in tests and re-exported
The helper is referenced in:
test/Evm/stack_test.zig(dynamically generating swap functions for testing)src/evm/evm.zig(publicly re-exported)Since it’s actively used, the helper is not redundant.
Likely an incorrect or invalid review comment.
27-28:❓ Verification inconclusive
Verify the alignment directive syntax for the target platform.
The 32-byte alignment is crucial for performance. Let's verify this syntax is correct for all target platforms.
🏁 Script executed:
#!/bin/bash # Description: Check if similar alignment syntax is used elsewhere in the codebase # Expected: Find other examples to confirm this is the standard pattern ast-grep --pattern 'align($_)'Length of output: 32
🏁 Script executed:
#!/bin/bash # Search for any usage of align(...) in Zig files to confirm the syntax is consistent grep -R --include="*.zig" "align(" -n .Length of output: 131
Verify Zig
align(32)Directive Cross-PlatformPlease confirm that the
align(32)annotation is correctly recognized and produces a 32-byte alignment on all target architectures by testing with the Zig compiler and reviewing platform ABI docs.
- Location:
src/evm/Stack.zigline 28- Code under review:
data: [STACK_LIMIT]u256 align(32) = [_]u256{0} ** STACK_LIMIT,
| pub fn push_slice(self: *Stack, values: []const u256) StackError!void { | ||
| if (self.size + values.len > capacity) { | ||
| return StackError.StackOverflow; | ||
| } | ||
|
|
||
| for (values) |value| { | ||
| self.data[self.size] = value; | ||
| self.size += 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using the push method for consistency.
The current implementation bypasses the normal push logic. While it includes overflow checking, using the existing push method would maintain consistency.
pub fn push_slice(self: *Stack, values: []const u256) StackError!void {
if (self.size + values.len > capacity) {
return StackError
8000
.StackOverflow;
}
for (values) |value| {
- self.data[self.size] = value;
- self.size += 1;
+ self.push_unsafe(value);
}
}📝 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.
| pub fn push_slice(self: *Stack, values: []const u256) StackError!void { | |
| if (self.size + values.len > capacity) { | |
| return StackError.StackOverflow; | |
| } | |
| for (values) |value| { | |
| self.data[self.size] = value; | |
| self.size += 1; | |
| } | |
| } | |
| pub fn push_slice(self: *Stack, values: []const u256) StackError!void { | |
| if (self.size + values.len > capacity) { | |
| return StackError.StackOverflow; | |
| } | |
| for (values) |value| { | |
| self.push_unsafe(value); | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/evm/Stack.zig around lines 257 to 266, the push_slice function manually
adds values to the stack, bypassing the existing push method. To maintain
consistency and reuse overflow checks, refactor push_slice to iterate over
values and call the push method for each value instead of directly modifying
data and size.
Description
Optimized the EVM stack implementation for better performance and cache efficiency. Key improvements include:
popnandpopn_topfor efficient multi-value handlingOutOfMemoryerror withInvalidPositionfor better semanticsAdded extensive test coverage and benchmarking to verify performance improvements against revm/evmone targets.
Testing
test/Evm/stack_test.zigcovering all operationstest/Bench/stack_benchmark.zigto measure performanceAdditional Information
Your ENS/address:
Summary by CodeRabbit