🤖 Kimi Code Review

This PR fixes a consensus-critical bug in the LEVM implementation regarding EIP-7702 authority account state handling during transaction reverts.

Summary of Changes

The fix ensures that the exists flag on accounts is properly backed up before any modification (which might flip exists to true) and restored during cache reversion. This prevents "pollution" of account existence state across reverted transactions.

Detailed Review

1. crates/vm/levm/src/db/gen_db.rs (lines 534-543)

Correctness: ✅ Good

The split into immutable then mutable lookup is the correct fix:

// Backup must be taken before mark_modified flips `exists` to true.
let account = self.db.get_account(address)?;  // Capture pre-modification state
// ... backup ...
let account = self.db.get_account_mut(address)?;  // Now modify

Note: This pattern introduces a double lookup (two hashmap lookups in the underlying DB), but this is acceptable for correctness in consensus-critical code. If performance becomes a concern, consider a get_account_mut_with_snapshot method, but the current approach is clearer and safer.

2. crates/vm/levm/src/hooks/default_hook.rs (lines 362-372)

Correctness: ✅ Good

Same pattern as above in delete_self_destruct_accounts. The backup captures the pre-destruction state before mark_destroyed() is called.

3. crates/vm/levm/src/utils.rs (line 85)

Critical Fix: ✅ Essential

current_account.exists = account.exists;

This is the missing piece that caused the consensus bug. Without restoring the exists flag:

• A reverted transaction that touched an authority address would leave exists = true
• Subsequent EIP-7702 transactions would incorrectly treat the account as warm/existing
• Gas accounting would differ from other clients, causing a consensus split

4. test/tests/blockchain/eip7702_revert_authority_tests.rs

Test Coverage: ✅ Comprehensive

The test correctly validates that:

1. A reverted transaction touching an authority doesn't affect subsequent EIP-7702 authorization gas costs
2. The exists flag is properly isolated across transaction boundaries

Minor suggestions:

1. Line 45: Consider using a random or derived key instead of [0x42; 32] to avoid potential key collisions if tests run in parallel or if the authority address has special properties:

   // Could use: SecretKey::from_slice(&rand::random::<[u8; 32]>())

2. Line 81: The path fixtures/genesis/execution-api.json assumes a specific working directory. Consider using a more robust path resolution or documenting the requirement that tests must run from the workspace root.

3. Lines 265, 280: The gas calculation logic is correct but subtle. Consider adding a comment explaining that both branches return the gas used by the EIP-7702 transaction specifically (not cumulative).

Security Assessment

Severity: Critical Consensus Bug

This fix prevents a consensus split between ethrex and other Ethereum clients. The bug would have caused:

• Different state roots after processing blocks with reverted EIP-7702 authorizations
• Potential fund loss or invalid execution in L2 contexts using ethrex as the execution client

Recommendation

Approve with minor comments. The fix is correct, minimal, and well-tested. The double-lookup pattern is a clean solution that prioritizes correctness over micro-optimizations.

Consider adding a regression test comment referencing the specific EIP-7702 consensus rule being tested:

// Tests that authority account warming is properly reverted per EIP-7702:
// "If the transaction is reverted, the nonce increase and any authority 
//  account creation must be reverted."

Automated review by Kimi (Moonshot AI) · kimi-k2.5 · custom prompt

Lines of code report

Total lines added: 3
Total lines removed: 0
Total lines changed: 3

+-------------------------------------------------+-------+------+
| File                                            | Lines | Diff |
+-------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/db/gen_db.rs          | 561   | +1   |
+-------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/hooks/default_hook.rs | 459   | +1   |
+-------------------------------------------------+-------+------+
| ethrex/crates/vm/levm/src/utils.rs              | 461   | +1   |
+-------------------------------------------------+-------+------+

Greptile Summary

This PR fixes a bug where a reverted account creation incorrectly left the exists flag set to true on the in-memory LevmAccount, causing EIP-7702 authority gas calculations to be wrong in subsequent transactions. The root cause was that exists was not included in the callframe backup/restore cycle.

• restore_cache_state now restores the exists field alongside info, status, and has_storage, so a revert properly unmarks a newly-created account.
• VM::get_account_mut and the SELFDESTRUCT path in default_hook.rs now capture the backup via the immutable get_account call before get_account_mut is invoked (which internally calls mark_modified, setting exists = true), ensuring the backed-up snapshot holds the pre-modification exists value.
• A regression test (eip7702_revert_authority_tests.rs) validates that a reverted transfer to an authority address does not pollute the exists flag used for EIP-7702 gas accounting.

Confidence Score: 5/5

Safe to merge. The change is minimal and targeted — only the callframe backup/restore path for the exists field is touched, and a regression test directly verifies the fix.

The change is small and well-understood: one extra field restored on revert, and two sites where the backup snapshot is now taken before mark_modified flips exists. The regression test faithfully reproduces the original mis-accounting and confirms it is resolved. No unrelated logic was touched.

No files require special attention beyond the five changed here.

_{Reviews (1): Last reviewed commit: "fix(levm): restore exists flag on transa..." | Re-trigger Greptile}

🤖 Claude Code Review

Now I have a thorough understanding of the changes. Let me write the review.

PR Review: fix(levm): revert doesn't unmark the account as existing

Overview

This PR fixes a two-part bug in LEVM's EIP-7702 gas accounting during reverts:

• exists was not included in the fields restored by restore_cache_state, so a reverted account creation left exists=true permanently
• The backup in get_account_mut (and the SELFDESTRUCT path in default_hook.rs) was taken after db.get_account_mut was called, which internally calls mark_modified() — flipping exists to true before the snapshot was captured

Both fixes together are necessary: without the ordering fix, the backup would capture exists=true even for a non-existent account, and without the restore fix, the backup value would simply be ignored.

Correctness

Fix 1 — utils.rs:85: Adding current_account.exists = account.exists; to restore_cache_state is the primary fix. The field was already being saved in backup_account_info, but silently ignored during restore.

Fix 2 — gen_db.rs:537-544 and default_hook.rs:363-373: Taking the backup from get_account (immutable, no side effects) before calling get_account_mut (which triggers mark_modified() → exists = true) is correct and ensures the snapshot captures the pre-modification state.

Both fixes are necessary and complementary. The logic is sound.

Potential Concern: l2_hook.rs Has the Same Pattern

The subagent found a third direct call to vm.db.get_account_mut that bypasses the backup-before-modify ordering:

crates/vm/levm/src/hooks/l2_hook.rs (transfer_fee_token function)
vm.db.get_account_mut(fee_token)?.storage = new_storage;

Unlike the two call sites fixed in this PR, there is no preceding backup_account_info call for the fee_token account info — only storage slots are backed up. In practice this is unlikely to cause a bug today because the fee token is a genesis-deployed contract (exists is always true), but it's an inconsistency with the pattern now being enforced elsewhere and could become a latent bug if the invariant changes.

Worth either documenting the assumption or applying the same fix for consistency.

Minor Points

• Double lookup overhead (get_account + get_account_mut): The extra read adds one cache lookup per get_account_mut call. Given LEVM keeps a current_accounts_state HashMap, this is a HashMap lookup (O(1)) with no disk I/O. The cost is negligible.

• Test structure — run_scenario duplication: The block-building and receipt-fetching logic is duplicated between the if precede_with_revert_touch branch (early return) and the fall-through path. This is readable enough for a test, but the duplication is a slight maintainability concern. Extracting a run_block_and_get_7702_gas helper would clean it up without affecting correctness.

• sign_auth_tuple — Into::<i32>::into(recovery_id) as u64: The double conversion is safe but verbose. recovery_id as u8 as u64 or u64::from(recovery_id as u8) would be cleaner. Minor test-code nit.

• REVERT_HELPER_BYTECODE correctness: The bytecode is correct. Stack state before CALL is (top→bottom): gas=0xFFFF, addr (from CALLDATALOAD), value=1, args_offset=0, args_size=0, ret_offset=0, ret_size=0 — matching the CALL argument order exactly. The 12-byte zero-pad + 20-byte address construction in create_revert_touch_tx correctly aligns with CALLDATALOAD at offset 0.

Summary

The fix is correct, minimal in scope, and the regression test accurately captures the bug scenario. The l2_hook.rs inconsistency is the only non-trivial concern worth addressing before or after merge. Everything else is minor style. Approve with the l2_hook.rs note for the author's awareness.

Automated review by Claude (Anthropic) · sonnet · custom prompt

🤖 Codex Code Review

No findings.

The fix is coherent with the underlying bug: mark_modified() forces exists = true, so taking the backup before mutable access in gen_db.rs and in the selfdestruct cleanup path in default_hook.rs is the right correction. Restoring exists in utils.rs also matches the EIP-7702 refund logic that reads authority_exists in the auth validation path at utils.rs.

Residual risk: the new regression test in eip7702_revert_authority_tests.rs covers the value-transfer/revert case well, but it does not directly exercise the separate selfdestruct cleanup fix in default_hook.rs. I couldn’t run the test here because the sandbox blocks writable cargo/rustup cache state and dependency fetching.

Automated review by OpenAI Codex · gpt-5.4 · custom prompt

Benchmark Results Comparison

No significant difference was registered for any benchmark run.

Reviewed the patch and ran a merge-tree test against a long-lived branch (no conflicts). Fix is clean and the test exercises the exact path. Two follow-up observations:

1. One more raw db.get_account_mut callsite that has the same antipattern

Grepping the levm tree turns up a third caller that bypasses VM::get_account_mut:

// crates/vm/levm/src/hooks/l2_hook.rs:780
vm.db.get_account_mut(fee_token)?.storage = new_storage;

transfer_fee_token backs up storage slots via vm.backup_storage_slot(...) (lines 773-779) but never calls backup_account_info(fee_token, ...). So mark_modified flips exists=true for the fee-token account with no pre-flip backup, and on revert restore_cache_state (now with the new line) would restore an exists that was already true — practically a no-op since fee-token contracts are deployed at genesis and always pre-exist, but it's the same code smell. Worth either:

• applying the same backup-before-flip pattern there, or
• adding a comment justifying why the fee-token path doesn't need it.

Either way it keeps the antipattern from proliferating to a future site where the account isn't guaranteed to pre-exist.

2. Worth auditing the storage-slot side separately

The exists bug exists because mark_modified is called inside db.get_account_mut before any backup. Storage slots have an analogous risk surface: if anyone reads/writes db.get_account_mut(addr)?.storage without calling backup_storage_slot first, a revert could leak the storage write. Out of scope for this PR (which is just exists), but a quick grep for that idiom in a follow-up would be cheap insurance.

Otherwise the fix looks correct:

• backup pre-flip + restore on revert is the right two-sided fix
• LevmAccount now has all 5 fields (info, storage, has_storage, status, exists) consistently captured and restored
• the regression test correctly fails without commit 2 and passes with it (verified by tracing the path through restore_cache_state)
• one extra db.get_account lookup per VM::get_account_mut call is O(1) on FxHashMap, no perf concern