✨ feat(evm): implement production-quality Memory module for EVM #1721
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Skipped Deployment
|
|
## Walkthrough
This change introduces a comprehensive refactor and expansion of the EVM `Memory` module, replacing the previous implementation with a robust, error-checked model. It adds extensive tests, stress tests, and benchmarks for memory operations, integrates new build targets for these tests, and updates package exports for easier access to the new memory functionality.
## Changes
| File(s) | Change Summary |
|------------------------------------------------|--------------------------------------------------------------------------------------------------------|
| build.zig | Normalizes `evm` module path casing; adds new memory-test, memory-stress-test, memory-comparison-test, and memory-benchmark targets with run steps; integrates them into main test step. |
| src/Evm/Memory.zig | Removes old `Memory` struct and all related methods and tests. |
| src/evm/Memory.zig | Introduces a new `Memory` struct with error handling, memory limits, safe/unsafe operations, zero-initialization, snapshot/restore, and extensive functionality replacing prior implementation. |
| src/Evm/evm.zig | Deletes old `Evm` struct with `depth` field. |
| src/evm/evm.zig | Adds new `Evm` struct with `depth` field; publicly re-exports `Memory`, `MemoryError`, and `calculateNumWords` from `Memory.zig`. |
| src/package.zig | New package file exporting `evm` module and re-exporting `Memory` type for external use. |
| test/Bench/memory_benchmark.zig | Adds a comprehensive benchmark suite for `Memory` operations with performance targets and assertions. |
| test/Evm/memory_comparison_test.zig | New test suite verifying `Memory` behavior against revm and evmone implementations for correctness and compatibility. |
| test/Evm/memory_stress_test.zig | New stress test suite covering resizing, sparse writes, large-scale operations, overlapping copies, and snapshot/restore. |
| test/Evm/memory_test.zig | Adds comprehensive unit tests covering initialization, byte/word/u256 operations, slice access, copying, resizing, limits, unsafe ops, snapshots, hex conversion, and edge cases. |
| src/root.zig | Adjusts import statement for `evm` module to lowercase. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant TestRunner
participant Memory
participant Allocator
participant Benchmark
TestRunner->>Memory: init(allocator)
TestRunner->>Memory: set/get/copy/setDataBounded/resize
Memory->>Allocator: allocate/resize/free as needed
TestRunner->>Memory: snapshot()/restore()
TestRunner->>Benchmark: run benchmarks (expansion, word ops, copy, random access)
Benchmark->>Memory: perform memory operations
Benchmark->>TestRunner: report metrics/assertionsPossibly related PRs
Poem
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
test/Evm/memory_stress_test.zig (1)
305-308: Consider verifying or removing the unused gas calculation.The test calculates
expected_gasbut immediately discards it. Either verify the actual gas cost against this expected value or remove the unused calculation to avoid confusion.Option 1 - Remove the unused calculation:
- // Verify gas cost calculation matches EVM spec - // Gas = 3 * words + words² / 512 - const expected_gas = 3 * tc.expected_words + (tc.expected_words * tc.expected_words) / 512; - _ = expected_gas; // Gas calculation verified by new_words countOption 2 - Actually verify the gas cost (if Memory module provides gas calculation):
// Verify gas cost calculation matches EVM spec // Gas = 3 * words + words² / 512 const expected_gas = 3 * tc.expected_words + (tc.expected_words * tc.expected_words) / 512; - _ = expected_gas; // Gas calculation verified by new_words count + const actual_gas = mem.calculateGasCost(); // Assuming this method exists + try testing.expectEqual(expected_gas, actual_gas);test/Bench/memory_benchmark.zig (2)
16-57: Consider making performance targets configurable or environment-aware.The hard-coded performance assertions (e.g.,
avg_ns < 100on line 37,avg_ns < 1000on line 55) may fail on different hardware configurations or under varying system loads. These aggressive targets could cause CI failures on slower machines.Consider using configurable thresholds or converting assertions to warnings:
- std.debug.assert(avg_ns < 100); // Target assertion + if (avg_ns >= 100) { + std.debug.print("WARNING: Small expansion exceeded target ({}ns >= 100ns)\n", .{avg_ns}); + }Alternatively, make the targets configurable via environment variables or build options.
82-85: Fix throughput calculation unit conversion.The throughput calculation divides bytes by nanoseconds but displays the result as GB/s. The correct conversion from bytes/nanosecond to GB/s requires no additional scaling (1 byte/ns = 1 GB/s), so the calculation is actually correct. However, the variable name and calculation could be clearer.
- const throughput_gbps = @as(f64, @floatFromInt(size_bytes)) / @as(f64, @floatFromInt(elapsed_ns)); + // 1 byte/nanosecond = 1 GB/s (10^9 bytes/second) + const throughput_gbps = @as(f64, @floatFromInt(size_bytes)) / @as(f64, @floatFromInt(elapsed_ns));Also applies to: 97-100
src/Evm/Memory.zig (1)
314-314: Avoidcatch unreachablein production code.While
bufPrintis unlikely to fail with the given parameters, usingcatch unreachablecould cause a panic in unexpected scenarios.- _ = std.fmt.bufPrint(hex_str[i * 2 .. (i + 1) * 2], "{x:0>2}", .{byte}) catch unreachable; + _ = try std.fmt.bufPrint(hex_str[i * 2 .. (i + 1) * 2], "{x:0>2}", .{byte});This change would require updating the function signature to return an error union, but provides better error handling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
build.zig(2 hunks)src/Evm/Memory.zig(1 hunks)src/Evm/evm.zig(1 hunks)src/package.zig(1 hunks)test/Bench/memory_benchmark.zig(1 hunks)test/Evm/memory_comparison_test.zig(1 hunks)test/Evm/memory_stress_test.zig(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Nx Cloud Agents (5)
- GitHub Check: Nx Cloud Agents (3)
- GitHub Check: Nx Cloud Agents (2)
- GitHub Check: Nx Cloud Agents (6)
- GitHub Check: Nx Cloud Agents (4)
- GitHub Check: Nx Cloud Agents (1)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (6)
src/package.zig (1)
1-8: Clean package structure!The package exports are well-organized and properly documented. The re-export of the Memory type provides convenient access to the enhanced EVM memory functionality.
src/Evm/evm.zig (1)
1-6: Well-structured module exports!The re-exports of Memory, MemoryError, and calculateNumWords provide a clean API surface for the EVM module consumers. This aligns perfectly with the PR's goal of exposing the enhanced Memory functionality.
build.zig (1)
381-436: Comprehensive test infrastructure!The build configuration properly integrates all Memory module tests:
- Unit tests for core functionality
- Stress tests for robustness
- Comparison tests for compatibility
- Benchmarks with appropriate ReleaseFast optimization
All test targets are correctly configured and integrated into the main test step.
Also applies to: 475-477
test/Evm/memory_comparison_test.zig (1)
1-229: Excellent comprehensive test coverage!This test suite thoroughly validates the Memory implementation against revm and evmone behaviors, covering:
- Initial state and capacity
- Word-aligned operations
- Bounded data operations with edge cases
- Memory expansion and growth strategy
- Copy semantics with overlapping regions
- Gas cost calculations
- Memory limits
- Unsafe operations
- Snapshot/restore functionality
The tests are well-structured with clear naming and good coverage of edge cases.
src/Evm/Memory.zig (2)
10-332: Excellent production-quality Memory implementation!This is a well-designed, robust implementation with:
- Comprehensive error handling replacing all panics
- Smart 2x growth strategy with configurable memory limits
- Optimized word-aligned operations for EVM patterns
- Safe bounds checking with overflow protection
- Performance-oriented unsafe operations when needed
- Complete test coverage
The implementation successfully achieves the PR objectives of creating a production-quality Memory module.
186-192:⚠️ Potential issueFix potential integer underflow in U256 conversion.
The loop decrements
ibefore checking if it's zero, which will cause an underflow whenireaches 0 sinceusizeis unsigned.- var i: usize = 31; - while (v > 0) : (i -%= 1) { - word[i] = @truncate(v & 0xFF); - v >>= 8; - if (i == 0) break; - } + var i: usize = 32; + while (v > 0 and i > 0) { + i -= 1; + word[i] = @truncate(v & 0xFF); + v >>= 8; + }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/evm/Memory.zig (1)
296-306: Consider safer error handling in hex conversion.The
unreachablestatement assumesbufPrintwill never fail, which should be true given the pre-allocated buffer size, but could be made more robust.Consider using a more defensive approach:
- _ = std.fmt.bufPrint(hex_str[i * 2 .. (i + 1) * 2], "{x:0>2}", .{byte}) catch unreachable; + _ = std.fmt.bufPrint(hex_str[i * 2 .. (i + 1) * 2], "{x:0>2}", .{byte}) catch |err| { + std.debug.panic("Unexpected bufPrint error: {}", .{err}); + };This provides better debugging information if the assumption is ever violated.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
build.zig(11 hunks)src/evm/Memory.zig(1 hunks)src/evm/evm.zig(1 hunks)src/package.zig(1 hunks)src/root.zig(1 hunks)test/Evm/memory_comparison_test.zig(1 hunks)test/Evm/memory_stress_test.zig(1 hunks)test/Evm/memory_test.zig(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/root.zig
🚧 Files skipped from review as they are similar to previous changes (3)
- src/package.zig
- test/Evm/memory_comparison_test.zig
- build.zig
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Nx Cloud Agents (4)
- GitHub Check: Nx Cloud Agents (3)
- GitHub Check: Nx Cloud Agents (5)
- GitHub Check: Nx Cloud Agents (2)
- GitHub Check: Nx Cloud Agents (6)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Nx Cloud Agents (1)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
src/evm/evm.zig (1)
1-9: LGTM! Clean module structure and API design.The module provides a clean interface by re-exporting the Memory functionality and includes a placeholder EVM context struct. The structure follows good practices for API organization.
test/Evm/memory_stress_test.zig (1)
1-310: Excellent comprehensive stress test suite!The stress test suite provides excellent coverage of edge cases and performance scenarios:
- Rapid expansion/contraction cycles with data integrity verification
- Simulated concurrent access patterns across memory regions
- Memory pressure testing with graceful error handling
- Pathological sparse access patterns
- Large-scale memory operations
- Overlapping copy stress testing
- Boundary condition testing for
setDataBounded- Snapshot/restore cycle validation
- Gas cost calculation accuracy verification
All tests properly handle resource cleanup and error cases. The test scenarios are realistic and would effectively catch regressions or performance issues.
test/Evm/memory_test.zig (1)
11-380: Excellent test coverage of all Memory functionality!The test suite provides comprehensive coverage including:
- Initialization with various configurations
- All read/write operations (bytes, words, U256, slices)
- Memory expansion and gas calculation
- Copy operations including overlapping regions
- Memory limits and error handling
- Unsafe operations
- Snapshot/restore functionality
- Edge cases and boundary conditions
- Growth strategy validation
The tests are well-structured with proper setup/teardown and cover both positive and negative test cases.
src/evm/Memory.zig (2)
10-325: Outstanding EVM Memory implementation!This is a production-quality implementation with excellent design choices:
- ✅ 4KB initial capacity optimized for OS page size
- ✅ 2x growth strategy for efficient memory expansion
- ✅ Comprehensive error handling with proper error types
- ✅ Memory limit enforcement for resource control
- ✅ Safe overlapping copy operations using directional algorithms
- ✅ Zero-initialization on expansion for security
- ✅ Both safe and unsafe operation variants
- ✅ Snapshot/restore functionality for EVM state management
- ✅ Integer overflow protection throughout
- ✅ EVM-specific word-aligned operations
The implementation follows established patterns from revm and evmone while providing robust error handling and safety guarantees. The API design is clean and well-documented.
182-195:✅ Verification successful
Verify U256 to big-endian conversion correctness.
The U256 serialization logic uses wrapping subtraction and a specific loop structure. While it appears correct, this is critical for EVM compatibility.
Let me verify the U256 conversion logic works correctly for edge cases:
🏁 Script executed:
#!/bin/bash # Verify U256 conversion logic by checking some test cases # Search for any existing U256 conversion tests rg -A 10 -B 2 "setU256.*test|getU256.*test" # Look for U256 test cases in the test files ast-grep --pattern 'test $_ { $$$ setU256($_, $_) $$$ }'Length of output: 1042
Conversion logic already covered by existing tests
The U256→big-endian serialization is exercised in test/Evm/memory_comparison_test.zig, where an arbitrary non-zero value is written with setU256 and read back with getU256, and the round-trip equality is asserted. No further changes are needed here.
| pub fn calculateNumWords(len: usize) usize { | ||
| return (len + 31) / 32; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate utility function.
The calculateNumWords function is already defined in the Memory.zig module (lines 322-325) and is re-exported via Evm.calculateNumWords. This duplicate definition in the test file is unnecessary.
Remove the duplicate function and use the imported one:
-pub fn calculateNumWords(len: usize) usize {
- return (len + 31) / 32;
-}Then update line 305 to use the imported function:
- try testing.expectEqual(@as(usize, 0), calculateNumWords(0));
+ try testing.expectEqual(@as(usize, 0), Evm.calculateNumWords(0));And similarly for other calls to calculateNumWords in this test 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.
| pub fn calculateNumWords(len: usize) usize { | |
| return (len + 31) / 32; | |
| } | |
| // --- test/Evm/memory_test.zig --- | |
| // 1) Remove the duplicate helper entirely: | |
| - pub fn calculateNumWords(len: usize) usize { | |
| - return (len + 31) / 32; | |
| - } | |
| // …later, when invoking it in your tests… | |
| // Before: | |
| - try testing.expectEqual(@as(usize, 0), calculateNumWords(0)); | |
| // After: | |
| + try testing.expectEqual(@as(usize, 0), Evm.calculateNumWords(0)); | |
| // Repeat the same replacement for every other call to `calculateNumWords` in this file. |
🤖 Prompt for AI Agents
In test/Evm/memory_test.zig around lines 7 to 9, remove the duplicate definition
of the calculateNumWords function since it is already defined and re-exported
from Memory.zig. Replace all calls to calculateNumWords in this test file with
the imported version from Evm.calculateNumWords to avoid redundancy and maintain
consistency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/Evm/memory_test.zig (1)
7-9: Remove duplicate utility function.The
calculateNumWordsfunction is already imported from the evm module on line 5. This duplicate definition creates unnecessary code duplication and shadows the imported function.
🧹 Nitpick comments (2)
src/evm/Memory.zig (2)
76-76: Consider explicit error mapping for ArrayList operations.The
try self.buffer.resize(new_size)call can returnOutOfMemoryfrom ArrayList, but this function is declared to returnMemoryError. While Zig's error union coercion handles this automatically, consider explicitly mapping for clarity.- try self.buffer.resize(new_size); + self.buffer.resize(new_size) catch |err| switch (err) { + error.OutOfMemory => return MemoryError.OutOfMemory, + };
302-302: Consider safer error handling in hex formatting.The
unreachablecatch assumes the buffer is always correctly sized, but this could panic if there's a logic error. Consider using a more defensive approach.- _ = std.fmt.bufPrint(hex_str[i * 2 .. (i + 1) * 2], "{x:0>2}", .{byte}) catch unreachable; + _ = std.fmt.bufPrint(hex_str[i * 2 .. (i + 1) * 2], "{x:0>2}", .{byte}) catch |err| { + std.debug.panic("hex formatting failed: {}", .{err}); + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
build.zig(11 hunks)src/Evm/Memory.zig(0 hunks)src/Evm/evm.zig(0 hunks)src/evm/Memory.zig(1 hunks)src/evm/evm.zig(1 hunks)src/package.zig(1 hunks)src/root.zig(1 hunks)test/Bench/memory_benchmark.zig(1 hunks)test/Evm/memory_comparison_test.zig(1 hunks)test/Evm/memory_stress_test.zig(1 hunks)test/Evm/memory_test.zig(1 hunks)
💤 Files with no reviewable changes (2)
- src/Evm/evm.zig
- src/Evm/Memory.zig
🚧 Files skipped from review as they are similar to previous changes (6)
- src/root.zig
- src/package.zig
- src/evm/evm.zig
- test/Evm/memory_stress_test.zig
- test/Bench/memory_benchmark.zig
- build.zig
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Nx Cloud - Main Job
- GitHub Check: Nx Cloud Agents (3)
- GitHub Check: Nx Cloud Agents (5)
- GitHub Check: Nx Cloud Agents (2)
- GitHub Check: Nx Cloud Agents (6)
- GitHub Check: Nx Cloud Agents (1)
- GitHub Check: Nx Cloud Agents (4)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (8)
test/Evm/memory_test.zig (1)
12-380: Comprehensive test suite with excellent coverage.The test suite provides thorough coverage of the Memory module functionality including:
- Initialization variants (default, with capacity, with limits)
- All data type operations (bytes, words, U256, slices)
- Memory expansion and gas calculations
- Copy operations including overlapping scenarios
- Error conditions and edge cases
- Unsafe operations
- Snapshot/restore functionality
- Growth strategy validation
The tests are well-structured, properly clean up resources, and test both normal and boundary conditions effectively.
test/Evm/memory_comparison_test.zig (1)
1-230: Excellent compatibility test suite ensuring implementation alignment.This test suite effectively validates that the Memory implementation behaves consistently with established EVM implementations (revm and evmone). Key strengths include:
- Proper imports: Uses the centralized
calculateNumWordsfunction without duplication- Comprehensive coverage: Tests initial state, word operations, bounded data setting, memory expansion, copy semantics, gas calculations, and context management
- Clear documentation: Each test clearly states what behavior it's comparing against
- Realistic scenarios: Tests real-world EVM operations like CALLDATACOPY behavior and overlapping memory copies
The gas cost calculation test (lines 120-146) is particularly valuable as it validates the EVM specification formula:
3 * words + words² / 512.src/evm/Memory.zig (6)
1-8: Well-defined error types for comprehensive error handling.The error set covers all necessary scenarios for EVM memory operations including bounds checking, size validation, and memory limit enforcement.
10-22: Excellent documentation explaining design decisions.The comprehensive documentation clearly explains the EVM memory model and design choices, referencing established implementations (revm, evmone) for validation.
188-192: Verify integer overflow safety in U256 conversion loop.The wrapping subtraction
i -%= 1with the break conditionif (i == 0) breakcorrectly handles the underflow case. This ensures the loop terminates safely wheniwraps from 0 tomaxInt(usize).
265-269: Excellent handling of overlapping memory copies.The implementation correctly uses
copyForwardsandcopyBackwardsbased on the relative positions of source and destination, ensuring proper behavior for overlapping regions as required by the EVM MCOPY instruction.
323-325: Standard and correct EVM word calculation.The
(len + 31) / 32formula correctly implements ceiling division for EVM word boundaries, matching the specification used by other implementations.
68-92: Robust memory expansion with security considerations.The implementation includes several important features:
- Memory limit enforcement for security
- Doubling growth strategy for efficiency
- Zero-initialization of new memory regions for security
- Proper handling of both growth and shrinkage scenarios
This aligns well with EVM security requirements and performance best practices.
- 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
Description
Completely rewrote the EVM Memory implementation with significant performance optimizations and improved API. The new implementation follows design patterns from revm and evmone with:
Added comprehensive test suite including:
Testing
Additional Information
Your ENS/address:
Summary by CodeRabbit
New Features
Bug Fixes
Tests