feat(levm): implement EIP-7906 TXTRACE and EVENTDATACOPY opcodes#6891
feat(levm): implement EIP-7906 TXTRACE and EVENTDATACOPY opcodes#6891edg-l wants to merge 2 commits into
Conversation
Lines of code reportTotal lines added: Detailed view |
🤖 Kimi Code ReviewOverall, this is a solid implementation of EIP-7906 with comprehensive test coverage. The code correctly handles transaction introspection, frame transactions, and EIP-7702 delegation exclusion. Critical IssuesNone found — the implementation correctly handles edge cases (overflows, bounds checking, reverts) and follows Ethereum consensus rules. Performance Concerns
Code Quality & Best Practices
Minor Issues
Security Verification
SummaryThe PR is ready for merge with optional performance optimizations. The repeated computation in Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt |
🤖 Codex Code ReviewFindings
Verification I could not run Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Claude Code ReviewNow I have a complete view of the diff. Let me synthesize the review. PR #6891 —
|
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
|
Greptile SummaryThis PR implements two new EVM opcodes from draft EIP-7906 —
Confidence Score: 4/5Safe to merge as a draft stacked on EIP-8141; no state-corrupting or security-breaking paths were found. The opcode handlers are correctly gated to Hegota, borrow-checker discipline is solid (owned log clones before mutable borrows, immutable DB refs dropped before stack push), and zero-value SSTORE entries are stored explicitly so slot_changes is accurate. The main concerns are quality/future-proofing: the three diff helpers are recomputed from scratch on every opcode invocation with no caching, and the deployed_contracts delegation check silently passes when the code hash is absent from the codes map. Neither causes wrong results today, but they are worth addressing before the EIP is finalised and gas costs are locked in. crates/vm/levm/src/opcode_handlers/tx_trace.rs and test/tests/levm/eip7906_tests.rs deserve a second look.
|
| Filename | Overview |
|---|---|
| crates/vm/levm/src/opcode_handlers/tx_trace.rs | New file implementing TXTRACE and EVENTDATACOPY handlers with helper functions for balance/slot/deploy diffs. Core logic is correct; has O(n) per-call recomputation issue and an implicit invariant in deployed_contracts when code is absent from the codes map. |
| crates/vm/levm/src/gas_cost.rs | Adds TXTRACE gas constant (100) with a clear caveat that it is provisional per the draft EIP. No issues. |
| crates/vm/levm/src/opcode_handlers/frame_tx.rs | Widens visibility of index_to_usize and compute_tx_cost to pub(crate) so the new tx_trace module can reuse them. Minimal, correct change. |
| crates/vm/levm/src/opcodes.rs | Adds TXTRACE=0xB5 and EVENTDATACOPY=0xB6 to the enum and opcode table, gated to build_opcode_table_hegota. The fork-gate test is extended to cover 0xB5/0xB6 on pre-Hegota forks. |
| crates/vm/levm/src/opcode_handlers/mod.rs | Adds pub mod tx_trace. Trivial wiring change. |
| test/tests/levm/eip7906_tests.rs | 1839-line bytecode test suite covering 34 scenarios: counts, sort order, net-zero exclusion, deploys, events, EVENTDATACOPY, gas payer, fork gating. Missing a test for a pre-existing non-zero slot set to zero appearing in slot_changes. |
| test/tests/levm/mod.rs | Registers the new eip7906_tests module. No issues. |
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
crates/vm/levm/src/opcode_handlers/tx_trace.rs:199-261
**O(n) recomputation on every indexed TXTRACE call**
`balance_changes`, `slot_changes`, and `deployed_contracts` are fully recomputed on every opcode invocation — there is no caching across calls. A contract that reads all balance-change addresses (params `0x03`–`0x05` × `n` iterations) incurs O(n) work per call, giving O(n²) total for a full scan, all charged at the fixed 100-gas rate. With a transaction that touches thousands of accounts or slots, this creates an unbounded CPU/time cost for a bounded gas spend. The EIP marks the gas cost TBD and this is draft, but the disparity should be tracked as a known concern before the cost is finalised.
### Issue 2 of 3
crates/vm/levm/src/opcode_handlers/tx_trace.rs:103-112
**Delegation check silently skipped when code is absent from `codes` map**
When `codes.get(&code_hash_after)` returns `None`, the `if let Some(code) = ... && code_has_delegation(...)` guard short-circuits, and the account is included in `deployed_contracts` without the EIP-7702 delegation check. This is safe only under the implicit invariant that every address that reaches this point (newly deployed, `was_empty` guard passed) always has its bytecode present in `codes`. If that invariant is violated — e.g., if a future code path introduces a CREATE that commits the account before storing the code — a delegation designator could silently appear in the deployed list. A `debug_assert!` or a comment explicitly stating the invariant would make the assumption auditable.
### Issue 3 of 3
test/tests/levm/eip7906_tests.rs:912-925
**Missing test: pre-existing non-zero slot SSTORE'd to zero should appear in `slot_changes`**
The suite tests restoring a slot to its original value (excluded) and writing new non-zero slots (included), but there is no test for an account that has a non-zero slot in prestate and explicitly sets it to `0`. The `slot_changes` implementation correctly handles this case (since `update_account_storage` always calls `account.storage.insert(key, new_value)` even for zero), but the gap means a future regression — such as pruning zero-value entries from storage — would go undetected by the EIP-7906 test suite.
Reviews (1): Last reviewed commit: "feat(levm): implement EIP-7906 TXTRACE a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let changes = balance_changes(initial, current); | ||
| let idx = index_to_usize(in2)?; | ||
| let (address, before, after) = | ||
| *changes.get(idx).ok_or(ExceptionalHalt::InvalidOpcode)?; | ||
| match param { | ||
| 0x03 => address_to_u256(address), | ||
| 0x04 => before, | ||
| _ => after, | ||
| } | ||
| } | ||
| // -- storage-slot changes (in2 = index) -- | ||
| 0x06..=0x09 => { | ||
| let changes = slot_changes(initial, current); | ||
| let idx = index_to_usize(in2)?; | ||
| let (address, slot, before, after) = | ||
| *changes.get(idx).ok_or(ExceptionalHalt::InvalidOpcode)?; | ||
| match param { | ||
| 0x06 => address_to_u256(address), | ||
| 0x07 => U256::from_big_endian(slot.as_bytes()), | ||
| 0x08 => before, | ||
| _ => after, | ||
| } | ||
| } | ||
| // -- deployed contracts (in2 = index) -- | ||
| 0x0A | 0x0B => { | ||
| let deployed = deployed_contracts(&vm.db.codes, initial, current)?; | ||
| let idx = index_to_usize(in2)?; | ||
| let (address, code_hash) = | ||
| *deployed.get(idx).ok_or(ExceptionalHalt::InvalidOpcode)?; | ||
| if param == 0x0A { | ||
| address_to_u256(address) | ||
| } else { | ||
| U256::from_big_endian(code_hash.as_bytes()) | ||
| } | ||
| } | ||
| // -- events count (in2 must be 0) -- | ||
| 0x0C => { | ||
| require_zero(in2)?; | ||
| U256::from(ordered_tx_logs(vm).len()) | ||
| } | ||
| // -- event fields (in2 = event index) -- | ||
| 0x0D..=0x13 => { | ||
| let logs = ordered_tx_logs(vm); | ||
| let idx = index_to_usize(in2)?; | ||
| let log = logs.get(idx).ok_or(ExceptionalHalt::InvalidOpcode)?; | ||
| match param { | ||
| 0x0D => address_to_u256(log.address), | ||
| 0x0E => U256::from(log.topics.len()), | ||
| // 0x0F..=0x12 -> topic0..topic3; halt if the topic is absent. | ||
| 0x0F..=0x12 => { | ||
| // Map the param literal to its topic index directly so | ||
| // there is no subtraction to overflow-check. | ||
| let n = match param { | ||
| 0x0F => 0, | ||
| 0x10 => 1, | ||
| 0x11 => 2, | ||
| _ => 3, | ||
| }; | ||
| let topic = topic_at(log, n).ok_or(ExceptionalHalt::InvalidOpcode)?; | ||
| U256::from_big_endian(topic.as_bytes()) | ||
| } | ||
| _ => U256::from(log.data.len()), | ||
| } |
There was a problem hiding this comment.
balance_changes, slot_changes, and deployed_contracts are fully recomputed on every opcode invocation — there is no caching across calls. A contract that reads all balance-change addresses (params 0x03–0x05 × n iterations) incurs O(n) work per call, giving O(n²) total for a full scan, all charged at the fixed 100-gas rate. With a transaction that touches thousands of accounts or slots, this creates an unbounded CPU/time cost for a bounded gas spend. The EIP marks the gas cost TBD and this is draft, but the disparity should be tracked as a known concern before the cost is finalised.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/opcode_handlers/tx_trace.rs
Line: 199-261
Comment:
**O(n) recomputation on every indexed TXTRACE call**
`balance_changes`, `slot_changes`, and `deployed_contracts` are fully recomputed on every opcode invocation — there is no caching across calls. A contract that reads all balance-change addresses (params `0x03`–`0x05` × `n` iterations) incurs O(n) work per call, giving O(n²) total for a full scan, all charged at the fixed 100-gas rate. With a transaction that touches thousands of accounts or slots, this creates an unbounded CPU/time cost for a bounded gas spend. The EIP marks the gas cost TBD and this is draft, but the disparity should be tracked as a known concern before the cost is finalised.
How can I resolve this? If you propose a fix, please make it concise.| if let Some(code) = codes.get(&code_hash_after) | ||
| && code_has_delegation(&code.bytecode)? | ||
| { | ||
| continue; | ||
| } | ||
| deployed.push((*address, code_hash_after)); | ||
| } | ||
| deployed.sort_by(|a, b| a.0.cmp(&b.0)); | ||
| Ok(deployed) | ||
| } |
There was a problem hiding this comment.
codes map
When codes.get(&code_hash_after) returns None, the if let Some(code) = ... && code_has_delegation(...) guard short-circuits, and the account is included in deployed_contracts without the EIP-7702 delegation check. This is safe only under the implicit invariant that every address that reaches this point (newly deployed, was_empty guard passed) always has its bytecode present in codes. If that invariant is violated — e.g., if a future code path introduces a CREATE that commits the account before storing the code — a delegation designator could silently appear in the deployed list. A debug_assert! or a comment explicitly stating the invariant would make the assumption auditable.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/vm/levm/src/opcode_handlers/tx_trace.rs
Line: 103-112
Comment:
**Delegation check silently skipped when code is absent from `codes` map**
When `codes.get(&code_hash_after)` returns `None`, the `if let Some(code) = ... && code_has_delegation(...)` guard short-circuits, and the account is included in `deployed_contracts` without the EIP-7702 delegation check. This is safe only under the implicit invariant that every address that reaches this point (newly deployed, `was_empty` guard passed) always has its bytecode present in `codes`. If that invariant is violated — e.g., if a future code path introduces a CREATE that commits the account before storing the code — a delegation designator could silently appear in the deployed list. A `debug_assert!` or a comment explicitly stating the invariant would make the assumption auditable.
How can I resolve this? If you propose a fix, please make it concise.| // deployed count @0 | ||
| code.extend({ | ||
| let mut c = txtrace(0x02, 0x00); | ||
| c.extend(push1(0)); | ||
| c.push(SSTORE); | ||
| c | ||
| }); | ||
| // deployed_address @ index 0 -> slot 1 | ||
| code.extend({ | ||
| let mut c = txtrace_idx(0x0A, U256::zero()); | ||
| c.extend(push1(1)); | ||
| c.push(SSTORE); | ||
| c | ||
| }); |
There was a problem hiding this comment.
slot_changes
The suite tests restoring a slot to its original value (excluded) and writing new non-zero slots (included), but there is no test for an account that has a non-zero slot in prestate and explicitly sets it to 0. The slot_changes implementation correctly handles this case (since update_account_storage always calls account.storage.insert(key, new_value) even for zero), but the gap means a future regression — such as pruning zero-value entries from storage — would go undetected by the EIP-7906 test suite.
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/tests/levm/eip7906_tests.rs
Line: 912-925
Comment:
**Missing test: pre-existing non-zero slot SSTORE'd to zero should appear in `slot_changes`**
The suite tests restoring a slot to its original value (excluded) and writing new non-zero slots (included), but there is no test for an account that has a non-zero slot in prestate and explicitly sets it to `0`. The `slot_changes` implementation correctly handles this case (since `update_account_storage` always calls `account.storage.insert(key, new_value)` even for zero), but the gap means a future regression — such as pruning zero-value entries from storage — would go undetected by the EIP-7906 test suite.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Adds TXTRACE (0xB5) to enumerate the current transaction's state diff (balance/storage/deploy changes, events, gas pre-charge, gas payer) and EVENTDATACOPY (0xB6) to copy event data into memory. Gated to Hegota alongside EIP-8141. Provisional opcode bytes and gas (EIP marks both TBD).
1 participant
Stacks on EIP-8141 (#6326). Adds two LEVM opcodes from EIP-7906 (Transaction Assertions via State Diff), gated to Hegota.
TXTRACE(0xB5, gas 100): reads the current transaction's state diff at the point of the call: balance changes, storage changes, deployed contracts, events (address, topics, data length), gas pre-charge and gas payer. Takes(param, index)on the stack, like FRAMEPARAM.EVENTDATACOPY(0xB6): copies an event's non-indexed data into memory, with the same gas and semantics as CALLDATACOPY.State diff is built from
initial_accounts_statevscurrent_accounts_state, sorted by address (uint160) then slot (uint256); events stay in emission order viasubstate.extract_logs(). The diff helpers and both handlers live in a new filecrates/vm/levm/src/opcode_handlers/tx_trace.rs; the rest is opcode/table wiring inopcodes.rs, the gas constant ingas_cost.rs, and two helpers exposed aspub(crate)inframe_tx.rs.Tests: 34 bytecode tests in
test/tests/levm/eip7906_tests.rscovering counts, sort order, net-zero exclusion, deploys, events across reverted subcalls, EVENTDATACOPY bounds and halts, the gas-payer subtraction guarantee, the frame-tx payer path, blob-fee pre-charge, STATICCALL, nested calls, and fork gating.Caveats
The EIP is Draft and has no conformance tests. It marks opcode bytes and gas costs as TBD. Chosen here: bytes
0xB5/0xB6(next free after EIP-8141),TXTRACEgas 100 (the EIP's own example value),EVENTDATACOPYgas as CALLDATACOPY.These choices were cross-checked against the author PoC (forshtat/ethrex, branch implement_7906). The gas value, the EVENTDATACOPY formula, the sort order, the event ordering, and the EVENTDATACOPY stack order all match. Three things differ:
contracts_deployed; the PoC includes them. Flagged pending spec clarification, simple to flip.balance_before - balance_after == gas_pre_chargeholds for the payer. The PoC uses max-fee values.gas_payerreturns the sender untilAPPROVE_PAYMENTruns; the spec only defines the final payer and expects assertions in the last frame.Draft because it depends on the unmerged EIP-8141 PR (#6326) and the EIP is not final.