perf(levm): remove Result<OpcodeResult, VMError> to avoid expensive matches#4791
perf(levm): remove Result<OpcodeResult, VMError> to avoid expensive matches#4791
Result<OpcodeResult, VMError> to avoid expensive matches#4791Conversation
github-actions
bot
added
levm
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonNo significant difference was registered for any benchmark run. Detailed ResultsBenchmark Results: BubbleSort
Benchmark Results: ERC20Approval
Benchmark Results: ERC20Mint
Benchmark Results: ERC20Transfer
Benchmark Results: Factorial
Benchmark Results: FactorialRecursive
Benchmark Results: Fibonacci
Benchmark Results: FibonacciRecursive
Benchmark Results: ManyHashes
Benchmark Results: MstoreBench
Benchmark Results: Push
Benchmark Results: SstoreBench_no_opt
|
Benchmark Block Execution Results Comparison Against Main
|
…atches Refactor opcode handlers from impl VM methods to a trait-based pattern (OpcodeHandler trait with struct-per-opcode), replacing method dispatch with a const function pointer table for improved performance. Resolve merge conflicts with main branch: - Integrate EIP-7928 BAL recording into new handler structs - Add Amsterdam fork opcode table (DUPN, SWAPN, EXCHANGE, SLOTNUM) - Keep main's U256 types for chain_id and base_fee_per_gas - Add get_code_length optimization for EXTCODESIZE - Add access_storage_slot and record_storage_slot_to_bal helpers Lint fixes: - Replace all #[allow(clippy::...)] with #[expect(clippy::..., reason)] - Convert .expect() to proper error propagation with map_err - Fix type mismatches from U256 field changes - Remove useless identity conversions
…handlers In the opcode handler refactoring, eip7702_gas_consumed was charged via increase_consumed_gas() before record_bal_call_touch(), reducing gas_remaining before BAL recording could see the original value. This caused BlockAccessListHashMismatch in EIP-7928 tests with EIP-7702 delegation + OOG scenarios. The fix defers the eip7702 gas charge: compute gas_left for the call gas calculation without modifying gas_remaining, then charge gas_cost + eip7702_gas_consumed together after BAL recording. This matches the main branch behavior where get_call_gas_params computed gas_left internally without modifying gas_remaining.
azteca1998
requested review from
ManuelBilbao,
avilagaston9 and
ilitteri
as code owners
🤖 Kimi Code ReviewReview SummaryThis PR refactors LEVM's opcode dispatch mechanism to eliminate expensive Key Changes
Issues Found1. Critical: Potential overflow in blob gas calculations (crates/common/types/block.rs:1075)assert!(fake_exponential(57532635.into(), 3145728.into(), 3338477).is_ok());The 2. Security: Unsafe transmute usage (crates/vm/levm/src/opcode_handlers/block.rs:67-75)#[expect(unsafe_code, reason = "safe")]
vm.current_call_frame.stack.push(unsafe {
let mut bytes = vm.db.store.get_block_hash(block_number)?.0;
bytes.reverse();
mem::transmute_copy::<[u8; 32], [u64; 4]>(&bytes)
})?;This transmute assumes little-endian byte order which may not hold on all platforms. Consider using 3. Performance: Unnecessary bounds checking (crates/vm/levm/src/opcode_handlers/arithmetic.rs:219-230)The // Current complex implementation
if value.bit(8 * x + 7) {
value |= U256::MAX << (8 * (x + 1));
} else if x != 31 {
value &= (U256::one() << (8 * (x + 1))) - 1;
}
// Simpler approach
let mask = (U256::one() << (8 * (x + 1))) - U256::one();
value = if value.bit(8 * x + 7) {
value | !mask
} else {
value & mask
};4. Correctness: Missing overflow check (crates/vm/levm/src/opcode_handlers/system.rs:1138-1144)In 5. Maintainability: Magic numbers (crates/vm/levm/src/opcode_handlers/system.rs:129-135)The constants Positive Aspects
Recommendations
Automated review by Kimi (Moonshot AI) · custom prompt |
🤖 Codex Code Review
The opcode-handler/table refactor itself looks structurally clean and aligned with the performance goal; main concerns are the numeric narrowing changes above. I could not run tests in this environment because Automated review by OpenAI Codex · custom prompt |
Greptile SummaryThis PR implements a significant performance optimization for the LEVM by removing
The refactoring is clean and systematic across all opcode categories. All call sites have been properly updated for the semantic changes. Confidence Score: 5/5
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/vm.rs | Core execution loop refactored to use OnceCell for error handling, avoiding Result matches in hot path; semantic changes to add_accessed_slot/address return values |
| crates/vm/levm/src/opcodes.rs | OpCodeFn refactored to wrap OpcodeHandler trait, enabling error storage in OnceCell instead of Result returns |
| crates/vm/levm/src/opcode_handlers/mod.rs | New OpcodeHandler trait introduced with eval method; provides common interface for all opcode implementations |
| crates/vm/levm/src/opcode_handlers/system.rs | System operations (CALL, CREATE, etc.) converted to handler structs; maintains correct error propagation and gas accounting |
| crates/vm/levm/src/environment.rs | Type optimizations: block_number and timestamp changed from U256 to u64; blob gas fields changed to Option<u64> |
| crates/vm/levm/src/opcode_handlers/stack_memory_storage_flow.rs | Storage operations updated for inverted return value semantics of add_accessed_slot (now returns cold status) |
Last reviewed commit: d5ee807
ElFantasma
left a comment
ElFantasma
left a comment
There was a problem hiding this comment.
The core architectural change — replacing Result<OpcodeResult, VMError> with OnceCell<VMError> + bare OpcodeResult to eliminate Result matching in the interpreter hot loop — is sound, and the trait-based OpcodeHandler pattern with OpCodeFn::new::<T>() const construction is a clean abstraction. The add_accessed_address/add_accessed_slot return semantics inversion (now returns true = cold) is applied consistently across the opcode handlers but creates an issue in one unmodified file (see below). See inline comments for tracing regressions in DELEGATECALL/CALLCODE.
crates/vm/levm/src/db/gen_db.rs:634 — access_storage_slot() still has let storage_slot_was_cold = !self.substate.add_accessed_slot(address, key); with the old ! negation. Since this PR inverts add_accessed_slot to return true = cold, the ! now gives storage_slot_was_cold = false when the slot IS cold. The method has no remaining callers after this PR (SLOAD/SSTORE call add_accessed_slot directly), so it's dead code — but it's pub and would silently give wrong warm/cold gas costs to any future caller. Either delete it or remove the !.
| let data = vm.get_calldata(args_offset, args_len)?; | ||
| vm.tracer.enter( | ||
| CallType::DELEGATECALL, | ||
| vm.current_call_frame.msg_sender, |
There was a problem hiding this comment.
DELEGATECALL tracer parameters changed from old behavior:
- Old:
tracer.enter(DELEGATECALL, callframe.to /* current contract */, code_address /* delegate */, ...) - New:
tracer.enter(DELEGATECALL, msg_sender, callframe.to, ...)
This changes from from the current contract to the original message sender, and to from the delegate's code address to the current contract. The old code had an explicit comment: "In this trace the from is the current contract, we don't want the from to be the EOA that sent the transaction". This will produce different debug_traceTransaction output for DELEGATECALL frames.
| // Trace CALL operation. | ||
| let data = vm.get_calldata(args_offset, args_len)?; | ||
| vm.tracer.enter( | ||
| CallType::CALLCODE, |
There was a problem hiding this comment.
Similar tracer issue: CALLCODE to changed from code_address (the contract whose code is being executed) to callframe.to (the current contract itself). When EIP-7702 delegation is active, these differ — the tracer loses information about which contract's code is actually running.
crates/vm/levm/src/opcodes.rs
Outdated
| opcode_table[Opcode::PUSH6 as usize] = OpCodeFn::new::<OpPushHandler<6>>(); | ||
| opcode_table[Opcode::PUSH7 as usize] = OpCodeFn::new::<OpPushHandler<7>>(); | ||
| opcode_table[Opcode::PUSH8 as usize] = OpCodeFn::new::<OpPushHandler<8>>(); | ||
| opcode_table[Opcode::PUSH8 as usize] = OpCodeFn::new::<OpPushHandler<8>>(); |
There was a problem hiding this comment.
nit: PUSH8 is assigned twice (lines 442-443). This is pre-existing from the old code but worth cleaning up while refactoring.
4 participants
Motivation
Matching the result is slow on tight loops.
Description
Remove the result so that there is no need to match anything.