Conversation
Add ADD opcode (0x01) using peek-peek-shrink-overwrite pattern with native wrapping arithmetic (+%). Add continue_ and stack_underflow to InstructionResult. Benchmark uses div0 methodology (pre-fill 900, batch 400 ops) and is hardcoded to ReleaseFast. Achieves ~2 ns/op, ~1200 MGas/sec. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
daniellehrner
force-pushed
the
feat/arithmetic_basic_opcodes
branch
from
49f4144 to
8d59c11
Compare
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
|
What do you think about the structure and layout? Were you anticipating implementing InterpreterType field for other evm contexts, like L2s which might have different instruction implementations? This PR has a concrete interpreter implementation, much like the stack and memory implementations that already exist. Would you like to break this into smaller reviewable PRs chunks, or is a 6k+ line diff ok ? |
Yeah, my general rule of thumb is try to keep PRs less than 1000 LOC if possible. however, this looks like a structural refactor with lots of comprehensive tests. Love this layout, much more ergonomic for the human eye. I'll comb through it more thoroughly in the coming days. |
10d9e
left a comment
10d9e
left a comment
There was a problem hiding this comment.
Excellent PR 🙌 , some minor knits/questions around duplicate vars, memory idioms, etc.
| } | ||
|
|
||
| /// Build instruction table for a specific hardfork | ||
| pub fn makeInstructionTable(spec: primitives.SpecId) InstructionTable { |
There was a problem hiding this comment.
The instruction table is created (makeInstructionTable) and stored in the handler (Instructions struct), but I don't see it being used for actual opcode dispatch in the interpreter loop.
Is there a dispatch mechanism that uses the instruction table, or is it only for gas cost lookup? If it's only for gas costs maybe we could consider renaming to GasCostTable for clarity?
|
|
||
| // Expand memory if needed | ||
| if (new_size > memory.size()) { | ||
| memory.buffer.resize(std.heap.c_allocator, new_size) catch return .memory_limit_oog; |
There was a problem hiding this comment.
Minor knit, perhaps more of a zig idiom than anything: it looks like the Memory struct manages it's own memory, which is fine, maybe we could consider if allocator should be passed through the interpreter context, "the zig way"? Not sure though.
There was a problem hiding this comment.
We will definitely have to revisit this comprehensively soon, since the allocator we want to use in zkvm is not std
| // Base gas costs | ||
| pub const G_ZERO = 0; | ||
| pub const G_BASE = 2; | ||
| pub const G_VERYLOW = 3; |
There was a problem hiding this comment.
Gas cost constants are defined in multiple places:
src/interpreter/gas_costs.zig(G_VERYLOW, G_LOW, etc.)src/interpreter/opcodes/arithmetic.zig(GAS_VERYLOW, GAS_LOW, etc.)src/interpreter/opcodes/memory.zig(GAS_VERYLOW, GAS_BASE, etc.)
Maybe we could import from gas_costs.zig instead of redefining?
There was a problem hiding this comment.
refactored this in subsequent PR
|
we don't want to close this one :) |
* consistently handle log topics and data deinit() throughout. fixes some broken spec tests in zevm-stateless Signed-off-by: garyschulte <garyschulte@gmail.com>
3 participants
Add mainnet/default evm construction
This is a pretty unwieldy pr, we might consider breaking it up into chunks for review, perhaps along those lines