Skip to content

fix(eip8130): surface mid-call database errors as fatal, not reverts#3748

Merged
chunter-cb merged 4 commits into
mainfrom
hh/eip-8130-call-db-error-fatal
Jun 24, 2026
Merged

fix(eip8130): surface mid-call database errors as fatal, not reverts#3748
chunter-cb merged 4 commits into
mainfrom
hh/eip-8130-call-db-error-fatal

Conversation

@chunter-cb

@chunter-cb chunter-cb commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Follow-up to #3696. Three fixes to the EIP-8130 phased-call executor, which runs calls directly and bypasses the mainnet single-frame handler — and with it several of the handler's steps.

1. Mid-call DB errors must be fatal, not reverts (consensus-critical)

EIP-8130 calls are dispatched as depth-0 top-level frames via run_call. revm only converts a frame's recorded DB error into an Err when a child frame's outcome is folded into its parent (EthFrame::return_resulttake_error); the root frame is returned directly by run_exec_loop without that check. So a DB failure during a call (e.g. an SLOAD against a missing trie node) halted the interpreter with FatalExternalError and came back as a non-ok InterpreterResult, which execute_calls folded into the phase_reverted path.

This is consensus-unsafe: a node hitting the DB failure would include the tx as reverted (nonce consumed, fee paid), while a healthy node would execute it normally — a fork. Every other tx type is protected via Handler::execution_result's take_error guard; the 8130 path bypasses it.

Fix: call take_error after run_exec_loop in run_call.

2. Warm coinbase + precompiles + set journal spec before calls (EVM equivalence)

The 8130 path skips the handler's load_accounts, so dispatched calls ran against the journal's default spec id with a cold coinbase and an unwarmed precompile set — charging cold access (2600) where a call in a normal tx is charged warm (100). Deterministic, but a gas/EVM-equivalence bug.

Fix: warm_pre_call_accounts mirrors pre_execution::load_accounts (minus access-list handling, which 8130 has no analogue for): sets the journal EVM spec id, warms precompiles, and warms the coinbase (Shanghai+), invoked right before execute_calls.

3. Centralized post-error teardown (defensive)

The execute_calls / settle_fees error paths only reverted the checkpoint and cleared the L1 cost. A DB error inside a nested subcall surfaces while the parent frame is still on the stack, so stale frame/local state could leak into the next tx reusing the same BaseEvm.

Fix: teardown_after_error mirrors the mainnet catch_error cleanup (checkpoint revert + clear L1 cost + local_mut().clear() + frame_stack().clear()), used on both error paths.

Tests

  • db_failure_during_call_propagates_as_error_not_revert — a call whose SLOAD is refused by a wrapping DB must fail with EVMError::Database, not be included as a revert. (Verified it fails without fix RPC Placeholders for flashblocks #1.)
  • call_warms_coinbase_per_eip3651BALANCE(coinbase) must be strictly cheaper than a byte-identical BALANCE of an untouched address, which only holds when the coinbase is pre-warmed. (Verified it fails without fix Setup CI #2.)

Test plan

  • cargo test -p base-common-evm --features std eip8130 (13 passed)
  • cargo clippy -p base-common-evm --features std --all-targets clean
  • cargo check -p base-common-evm --no-default-features (no_std) compiles

EIP-8130 calls are dispatched as depth-0 top-level EVM frames via
`run_call`. revm only converts a frame's recorded database error into an
`Err` when a child frame's outcome is folded into its parent
(`EthFrame::return_result` -> `take_error`); the root frame is returned
directly by `run_exec_loop` without that check. As a result, a node-local
DB failure raised mid-call (e.g. an `SLOAD` against a missing trie node)
halted the interpreter with `FatalExternalError` and came back as a
non-ok `InterpreterResult`, which `execute_calls` then folded into the
deterministic "phase reverted" path.

That is consensus-unsafe: a node hitting the DB failure would include the
transaction as reverted (nonce consumed, fee paid) while a healthy node
would execute it normally, forking the chain.

Mirror the mainnet `Handler::execution_result` guard by calling
`take_error` after `run_exec_loop` in `run_call`, so a database error
propagates as a fatal `EVMError::Database` instead of a call revert.

Add `db_failure_during_call_propagates_as_error_not_revert`, which runs a
call whose `SLOAD` is refused by a wrapping database and asserts the
transaction fails with `EVMError::Database` rather than being included as
a revert.
@cb-heimdall

cb-heimdall commented Jun 24, 2026

Copy link
Copy Markdown
Collaborator

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

…ror teardown

Two EVM-equivalence / robustness fixes to the phased-call executor, which
bypasses the mainnet single-frame handler and so misses two of its steps.

1. Pre-call account warming (EIP-3651 / EIP-2929 equivalence). The 8130
   path skips the handler's `load_accounts`, so dispatched calls ran
   against the journal's default spec id with a cold coinbase and an
   unwarmed precompile set — charging cold access (2600) where a call in a
   normal transaction is charged warm (100). Add `warm_pre_call_accounts`,
   mirroring `pre_execution::load_accounts` (minus access-list handling,
   which 8130 has no analogue for): set the journal EVM spec id, warm the
   precompile addresses, and warm the coinbase from Shanghai onward,
   invoked right before `execute_calls`.

2. Centralized post-error teardown. The `execute_calls` / `settle_fees`
   error paths previously only reverted the checkpoint and cleared the L1
   cost. A database error inside a nested subcall surfaces while the
   parent frame is still on the stack, so without draining it stale
   frame/local state could leak into the next transaction reusing the same
   `BaseEvm`. Add `teardown_after_error`, mirroring the mainnet
   `catch_error` cleanup (checkpoint revert + clear L1 cost +
   `local_mut().clear()` + `frame_stack().clear()`), and use it on both
   error paths.

Add `call_warms_coinbase_per_eip3651`: a `BALANCE(coinbase)` call must be
strictly cheaper than a byte-identical `BALANCE` of an untouched address,
which only holds when the coinbase is pre-warmed.
/// a cold coinbase / precompile set — charging cold-access gas (2600) where a
/// call in a normal transaction is charged warm (100) and otherwise drifting
/// from EVM equivalence on the same chain.
fn warm_pre_call_accounts<DB, I, P>(evm: &mut BaseEvm<DB, I, P>)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may also add the account config contract here.

@github-actions

Copy link
Copy Markdown
Contributor

Review Summary

This PR fixes three correctness issues in the EIP-8130 phased-call executor. I reviewed the diff thoroughly against the mainnet handler patterns it mirrors.

Assessment: No blocking issues found

Fix 1 — take_error after run_exec_loop (consensus-critical): Correctly mirrors the mainnet Handler::execution_result guard. The root frame's DB error is stashed on the context (not surfaced by run_exec_loop), so without this check a FatalExternalError would be silently folded into the revert path. The placement after run_exec_loop and before returning the frame is the right spot.

Fix 2 — warm_pre_call_accounts: Correctly placed after the outer transaction checkpoint (line 215) but before execute_calls, so the warming persists across per-phase checkpoint reverts (phase checkpoints are nested children) while still being reverted by teardown_after_error when the whole transaction is abandoned. The scope (spec ID + precompiles + coinbase) matches what the mainnet load_accounts does beyond what the call frame setup already handles naturally. Access-list omission is correctly noted as inapplicable to EIP-8130.

Fix 3 — teardown_after_error: Mirrors the mainnet catch_error cleanup (handler.rs:390-393) — checkpoint_revert + clear_tx_l1_cost + local_mut().clear() + frame_stack().clear(). This is applied on both error paths (execute_calls and settle_fees), which is correct since either can surface a DB error from a nested subcall. The pre-call error paths (authorize_and_apply, prepay) correctly omit the local/frame_stack cleanup since no EVM frames have been created at that point.

Tests: Both new tests are well-designed — they verify the fix by testing the exact failure mode (DB error surfacing as EVMError::Database, coinbase warm-access gas strictly cheaper than cold) and document that they fail without the corresponding fix.

@github-actions

Copy link
Copy Markdown
Contributor

✅ base-std fork tests: all 616 passed

base/base is fully in sync with the base-std spec.

Dependency Ref Commit
base-std main 4658f1b7
base-anvil 0092692587d8d064dd2c6923ce26a682c58f3694 00926925

@chunter-cb chunter-cb added this pull request to the merge queue Jun 24, 2026
Merged via the queue into main with commit c387402 Jun 24, 2026
24 checks passed
@chunter-cb chunter-cb deleted the hh/eip-8130-call-db-error-fatal branch June 24, 2026 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants