fix(levm): keep system-call storage write when coinbase is the called contract

[ElFantasma]

Summary

generic_system_contract_levm snapshots the block coinbase account before a system call (EIP-2935 / 4788 / 7002 / 7251) and restores it afterwards, so the call leaves no fee/value side effect on the fee recipient. That restore was unconditional.

When a block's fee recipient (coinbase) is set to the system contract being called, the restore reverts that contract's account to its pre-call snapshot — dropping the storage write the system call just made (e.g. the EIP-2935 parent-hash slot, or an EIP-7002/7251 request) from the emitted state updates. The result is a post-state root that diverges from other clients for an otherwise-valid block, since feeRecipient is freely chosen by the block producer.

Fix

Skip the coinbase restore when block_header.coinbase == contract_address. In that case the only change to the account is the system call's own storage write, which must persist; other clients commit the system-call diff and never restore the callee. Ordinary fee recipients are unaffected. A single guard covers all four system contracts, since they share this helper.

Test

Adds system_call_coinbase_tests, which drives the real EIP-2935 system call and asserts the history-contract storage write is emitted both for an ordinary fee recipient and when the fee recipient equals the history contract (the regression case). Verified the regression test fails on the pre-fix code.

[github-actions]

⚠️ Known Issues — intentionally skipped tests

Source: docs/known_issues.md

Known Issues

Tests intentionally excluded from CI. Source of truth for the Known
Issues section the L1 workflow appends to each ef-tests job summary
and posts as a sticky PR comment.

EF Tests — Stateless coverage narrowed to EIP-8025 optional-proofs

make -C tooling/ef_tests/blockchain test calls test-stateless-zkevm
instead of test-stateless. The zkevm@v0.3.3 fixtures are filled against
bal@v5.6.1, out of sync with current bal spec; the broad target trips ~549
fixtures. Re-broaden once the zkevm bundle is regenerated.

Why and resolution path

PR #6527 broadened
test-stateless to extract the entire for_amsterdam/ tree from the
zkevm bundle and run all of it under --features stateless; combined with
this branch's bal-devnet-7 semantics that scope produces ~549
GasUsedMismatch / ReceiptsRootMismatch /
BlockAccessListHashMismatch failures.

test-stateless-zkevm filters cargo to the eip8025_optional_proofs
suite, which still validates the stateless harness without the bal-version
mismatch.

Re-broaden by switching test: back to test-stateless in
tooling/ef_tests/blockchain/Makefile once the zkevm bundle is regenerated
against the current bal spec.

[github-actions]

🤖 Kimi Code Review

This PR fixes a consensus-critical bug where system contract storage writes would be incorrectly reverted when the block's fee recipient (coinbase) equals the system contract address.

Summary

The change is correct and well-tested. The logic prevents the post-call coinbase restoration from clobbering legitimate storage mutations when the callee and fee recipient are the same account.

Detailed Review

crates/vm/backends/levm/mod.rs

Lines 2586–2598: The fix correctly identifies that when coinbase == contract_address, the account state changes (specifically storage writes from the system call) must persist rather than be restored from the pre-call backup.

The comment at lines 2586–2593 accurately explains the consensus requirement: other clients commit the system-call diff without restoring the callee, so restoring it here would cause a state root divergence.

Suggestion: Consider adding a brief note about why this specifically affects consensus (state root mismatch) in the function-level documentation, as this is a subtle edge case in Ethereum's state transition rules.

Test Coverage (Lines 3095–3236)

The regression tests are comprehensive:

1. ordinary_coinbase_preserves_history_storage_write: Validates the normal case works correctly
2. history_address_coinbase_preserves_history_storage_write: Validates the edge case where coinbase is the EIP-2935 history contract

Lines 3112–3116: The HISTORY_RUNTIME_CODE constant correctly embeds the EIP-2935 history contract runtime bytecode. Verified against the EIP spec.

Lines 3138–3146: The get_account_code implementation correctly returns the history code for the corresponding hash. This is important because the VM will fetch this code during execution.

Lines 3180–3200: The run_history_update helper properly exercises the full flow through LEVM::process_block_hash_history and LEVM::get_state_transitions, ensuring the fix works at the API boundary, not just in the internal function.

Security/Consensus Considerations

This fix is consensus-critical. The previous behavior would cause a state root mismatch with other clients (Geth, Nethermind, etc.) when processing blocks where coinbase == HISTORY_STORAGE_ADDRESS (or other system contract addresses like EIP-7002/7251 contracts).

The fix correctly handles:

• EIP-2935: Block hash history updates
• EIP-7002: Withdrawal requests
• EIP-7251: Consolidation requests

All system calls that write to storage are protected by this change.

Minor Suggestions

Line 3190: The parent_hash return value in run_history_update is unused in the assertion logic (it's shadowed by the parameter in assert_history_write_emitted). Consider removing it from the return tuple to simplify the signature.

Line 3112: Consider adding a source comment for the bytecode, e.g.:

// EIP-2935 history contract runtime bytecode (see https://eips.ethereum.org/EIPS/eip-2935)

Dependencies

Cargo.toml: Adding hex as a dev-dependency is appropriate since it's only used for test bytecode decoding.

Verdict

Approve. The fix is minimal, correct, and thoroughly tested. The regression test prevents future breakage of this specific consensus edge case.

---

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

[greptile-apps]

Greptile Summary

This PR fixes a state-root divergence bug in generic_system_contract_levm: when a block's coinbase equals a system contract address (EIP-2935/4788/7002/7251), the unconditional post-call restore of the coinbase account was silently dropping the storage writes produced by that system call.

• Fix (crates/vm/backends/levm/mod.rs): Adds a single if block_header.coinbase != contract_address guard around the coinbase restore, preserving the system call's storage writes when the fee recipient equals the callee.
• Tests (mod.rs): Two new tests drive the real EIP-2935 system call and assert the parent-hash slot is both cached in the account state and emitted in get_state_transitions, covering both the ordinary fee-recipient path and the regression case.

Confidence Score: 5/5

Safe to merge — the change is a minimal, well-targeted guard that only affects the specific edge case where the block's fee recipient equals the system contract address.

The fix is a single conditional wrapping the existing coinbase-restore logic, with no changes to the happy path for ordinary fee recipients. The comment explains exactly why the guard is correct, the tests verify both the in-memory cache and the emitted state updates, and the regression test was confirmed to fail on the pre-fix code.

No files require special attention; crates/vm/backends/levm/mod.rs is the only substantive change and it is straightforward.

Important Files Changed

Filename	Overview
crates/vm/backends/levm/mod.rs	Core fix: one-line guard skips coinbase restore when coinbase==contract_address; well-commented with thorough regression tests added in the same file.
crates/vm/Cargo.toml	Adds hex as a dev-dependency for decoding the EIP-2935 runtime bytecode constant in tests.
Cargo.lock	Lock file updated to record the new hex dev-dependency for ethrex-vm.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[generic_system_contract_levm called] --> B[Snapshot coinbase_backup and system_account_backup]
    B --> C[Execute system contract e.g. EIP-2935 storage write]
    C --> D{Execution succeeded?}
    D -- No --> E[Return error]
    D -- Yes --> F[Restore system_address from system_account_backup]
    F --> G{block_header.coinbase == contract_address?}
    G -- Yes coinbase IS the callee --> H[Skip coinbase restore Storage write persists]
    G -- No ordinary fee recipient --> I[Restore coinbase account from coinbase_backup]
    H --> J[Return Ok report]
    I --> J

Loading

_{Reviews (1): Last reviewed commit: "fix(levm): keep system-call storage writ..." | Re-trigger Greptile}

[github-actions]

🤖 Codex Code Review

1. crates/vm/backends/levm/mod.rs:3104 The new test module relies on H256 coming from super::*, but the parent import is feature-gated with #[cfg(all(feature = "rayon", not(feature = "eip-8025")))]. That means this PR’s tests will stop compiling in non-default configurations such as --no-default-features or --features eip-8025. Import ethrex_common::H256 directly inside system_call_coinbase_tests (or gate the test the same way) so the regression test is feature-robust.

No production correctness bug stood out in the actual coinbase == contract_address fix; for the current Prague system-call paths it looks aligned with how get_state_transitions() emits storage-only diffs.

I could not run cargo test in this environment because rustup tries to create temp files under /home/runner/.rustup/tmp, which is read-only here.

---

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

[github-actions]

Lines of code report

Total lines added: 108
Total lines removed: 0
Total lines changed: 108

Detailed view

+---------------------------------------+-------+------+
| File                                  | Lines | Diff |
+---------------------------------------+-------+------+
| ethrex/crates/vm/backends/levm/mod.rs | 2504  | +108 |
+---------------------------------------+-------+------+

[github-actions]

🤖 Claude Code Review

I now have enough context to write the full review.

---

Review: fix(levm): keep system-call storage write when coinbase is the called contract

Summary

The fix is correct, well-scoped, and addresses a real consensus divergence bug. The logic is sound because all four system calls (EIP-2935/4788/7002/7251) share the helper, set gas_price = U256::zero() and value = U256::zero(), meaning storage writes are the only mutation that can occur on contract_address — making the "skip restore" safe in all current cases.

---

Core Fix — crates/vm/backends/levm/mod.rs

Lines 2593–2601 — correct and complete for the stated case.

One edge case worth noting is coinbase == system_address (the privileged call origin, not the destination contract). In that scenario:

• The system_account_backup restore at lines 2578–2584 already reverts the coinbase to its pre-call state.
• Then the coinbase != contract_address guard is true, so the coinbase restore at line 2594 runs a second time on the same address with the same original snapshot.

This is harmless (idempotent double-restore), but it's a latent quirk. A comment noting why the guard only covers coinbase == contract_address and not coinbase == system_address could save a future reader some confusion, since both restore blocks touch the same state key in that scenario.

Another edge case — system_address == contract_address: Not possible today (the system origin is a fixed, well-known address distinct from any deployed contract), but if it ever occurred, the system_account_backup restore at line 2578 would wipe storage writes before the coinbase guard even runs. This is a pre-existing structural issue unrelated to this PR, but the comment at line 2586 implies "other clients never restore the callee" — that's only true when callee ≠ caller ≠ origin. Worth a footnote if the concern ever materialises.

---

Comment quality

The multi-line comment at lines 2586–2592 is accurate and clearly explains the why. No changes needed there.

---

Tests — system_call_coinbase_tests

The test module is solid. A few observations:

history_slot function (line 3178) hardcodes 8191:

H256::from_low_u64_be((block_number - 1) % 8191)

8191 is HISTORY_SERVE_WINDOW from EIP-2935. If there is a named constant defined elsewhere in the codebase (e.g. in ethrex-common or ethrex-levm), using it here would prevent silent test rot if the window were ever adjusted. A quick grep shows no such constant is currently exported, so this is a minor nit — but worth either adding a named constant or a // EIP-2935 HISTORY_SERVE_WINDOW inline comment.

Store::get_block_hash always returns H256::zero() (line 3136):
This is fine. process_block_hash_history passes block_header.parent_hash as calldata directly to the system contract; the contract does not call BLOCKHASH to retrieve it. The stub is correct for this use case.

Test coverage for the other system contracts (EIP-4788/7002/7251):
The fix is in the shared generic_system_contract_levm helper, so EIP-2935 coverage is representative. However, EIP-7002 and EIP-7251 contracts write request data (not just a single hash slot), and their output is consumed differently. A single comment acknowledging "one test covers all four callers via the shared helper" would make the intent explicit to future readers.

No test for coinbase == system_address:
As noted above, that case results in a harmless redundant restore. Not a bug, so no test is strictly required — but a comment in the test module noting this edge case as intentionally untested would be useful.

HISTORY_RUNTIME_CODE constant (lines 3110–3116):
The bytecode is hardcoded as a string constant. If the EIP-2935 canonical bytecode is already available via a constant elsewhere in the codebase (e.g. a deployment fixture), referencing it would be better. If not, the string is fine — just ensure it matches the deployed bytecode (it appears to match the canonical EIP-2935 callcode-based implementation).

---

Cargo.toml / Cargo.lock

Adding hex as a [dev-dependencies] entry is correct and minimal. No concerns.

---

Verdict

The fix is correct and the regression tests pass the critical scenario. The observations above are minor — the double-restore on coinbase == system_address is harmless, and the magic number 8191 is the only thing worth a quick follow-up. Approving with the suggestion to add a comment or named constant for HISTORY_SERVE_WINDOW in the test.

---

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

[JereSalo]

Good news, we don't actually need a backup for any of the accounts. The only change that we have to revert when calling a system contract is the nonce incremental that is done to the SYSTEM_ADDRESS and happens on every transaction independently of its outcome, but there's nothing more that needs to be restored (thanks to gas price being set to zero).
I'd lean towards simplifying things here and forget about the backups entirely and after execution we can just decrement the nonce by one to the sender and that's it. Also, there's no need to remove something from the cache (current_accounts_state) if it wasn't there in the first place, it doesn't produce any harm not to do so, and in fact it may even be beneficial performance-wise (though probably negligible, so it's just a side-effect) if we reuse the same cached accounts for executing multiple blocks because we are not going to be removing accounts from it unnecessarily and reloading them on every block.

The fix I propose is simply removing the backup of both coinbase and system account and just add these lines at the end to decrease the nonce of the sender, that's it:

if let Some(account) = db.current_accounts_state.get_mut(&system_address) {
    account.info.nonce = account.info.nonce.saturating_sub(1);
}

I made the change in another branch in case you want to check it out. Feel free to incorporate it if you agree on the approach:
fix/syscall-coinbase-collision...refactor/syscall-nonce-decrement

[ElFantasma]

Thanks Jere — great call, this is a much cleaner fix than my guard. Incorporated your commit as-is (kept your authorship): cherry-picked it on top after merging main, so the branch now drops both backups and just decrements the SYSTEM_ADDRESS nonce.

I confirmed it against the regression test I'd added (system_call_coinbase_tests) and it stays green — the EIP-2935 history write survives in both the ordinary-coinbase and coinbase == history contract cases. And I double-checked the reasoning holds: with gas_price = 0 there's no coinbase fee and no sender charge, so the nonce bump is the only residue; and get_state_transitions already filters empty/unchanged accounts (the EIP-161 handling in gen_db.rs), so the old "remove if not cached" branches were redundant — leaving an empty SYSTEM_ADDRESS or a warmed coinbase in the cache produces no trie change.

One small thing I added on top (425e09466): a debug_assert! right after the env is built, asserting gas_price/base_fee_per_gas are zero, with a comment that the post-call cleanup depends on it. Since we're now leaning on the zero-price assumption (the backups used to tolerate any value), I figured it's worth making that invariant explicit so it can't silently regress. Happy to drop it if you think it's overkill.

[JereSalo]

Great, perhaps in the future we could drop the tests added here in case tests with same behavior are added to the execution-specs test suite. In the meantime it might be okay to keep them here.

[MegaRedHand]

LGTM

[MegaRedHand]

Tests make this redundant, I think