Skip to content

fix(anvil): stop loadState from corrupting canonical blocks on a fork - #16642

Open
gomesalexandre wants to merge 1 commit into
foundry-rs:masterfrom
gomesalexandre:fix_anvil_loadstate_fork_block_corruption
Open

fix(anvil): stop loadState from corrupting canonical blocks on a fork#16642
gomesalexandre wants to merge 1 commit into
foundry-rs:masterfrom
gomesalexandre:fix_anvil_loadstate_fork_block_corruption

Conversation

@gomesalexandre

Copy link
Copy Markdown
Contributor

anvil_loadState on a forked node (--fork-url + --load-state, a documented flag combination) silently replaces the fork's own canonical blocks below its head with unrelated blocks from the loaded state dump, corrupting the chain.

Root cause

Two spots in crates/anvil/src/eth/backend/:

  • mem/storage.rs's BlockchainStorage::insert_block unconditionally writes both self.blocks[hash] and self.hashes[number], and load_blocks calls it in a loop for every dumped block.
  • mem/mod.rs's Backend::load_state calls storage.load_blocks(blocks) for the whole dump, then afterward corrects only one entry in hashes - the selected head. Every other dumped block at a number ≤ the fork's head keeps its foreign (dump-source) hash.

The read path (block_by_numbermined_block_by_number) checks local storage first, before ever asking the fork RPC, so the foreign block wins for every read at that number. The chain becomes provably discontinuous: block(n).parentHash != block(n-1).hash.

serialized_blocks sorts dumped blocks canonical-last specifically so the last insert_block call wins the hashes map entry (pinned by its own test, serialized_blocks_puts_canonical_block_last) - that machinery exists to make hashes[n] land on the dump's own canonical block when there's no fork involved. Nobody re-points it for the case where the head instead comes from the fork.

The existing test_fork_load_state test forks, dumps, and reloads into a second fork, but only ever asserts block_number and balances - it never asks for an intermediate block number, so it exercises exactly this path without ever catching the corruption.

Fix

BlockchainStorage::load_blocks / load_transactions now take an Option<u64> fork_boundary. It's set only when Backend::load_state resolved the loaded head from the fork rather than from the dump itself. When set, dumped blocks/transactions numbered at or below that boundary no longer claim the canonical number -> hash (or transaction blockNumber) slot for that number - the fork's own real chain keeps it. Block data is still stored by hash (so hash-based lookups keep working for a retained-but-noncanonical dumped block), only the canonical number-keyed associations are skipped.

The non-fork load path is untouched: fork_boundary is None there, so behavior is byte-for-bit identical to before this change.

Why drop transactions rather than just blocks

load_transactions gets the same fork_boundary treatment: a dumped transaction whose blockNumber is ≤ the boundary is dropped entirely rather than inserted, since keeping it would let eth_getTransactionByHash report a blockNumber whose canonical block is a different (foreign) block after this fix.

Known limitation (disclosed, not fixed here)

A dumped block at/below the boundary is still retained by hash in storage.blocks (for eth_getBlockByHash(hash, false) lookups), but since its transactions are now dropped from storage.transactions, eth_getBlockByHash(hash, full=true) and eth_getTransactionByBlockHashAndIndex can't fully materialize it and will return null/fall through to the fork provider, which won't have it either. This only affects deliberately querying a retained-but-noncanonical dump block by its own (foreign) hash, which shouldn't come up in normal use. Flagged by an adversarial Codex review; not fixed in this PR to avoid adding a second, more invasive noncanonical-block-reconstruction path for an edge case with no live-observed impact.

Testing

  • New regression test test_fork_load_state_preserves_fork_blocks_below_head (crates/anvil/tests/it/state.rs): two independent local anvils (origin, what the fork forks from; dump_source, an unrelated node) each mine 5 blocks with distinct transaction values (so their blocks can't coincidentally collide), fork a third node from origin at block 5, load dump_source's dump (best_block_number == 5, exercising the <= boundary exactly at the fork head, not just strictly below it), then assert every intermediate block 1..=5 still has the fork's real hash and correct parentHash chain, that the fork's own transactions are still discoverable by hash, and that the dump's foreign transactions are not.
  • Confirmed real red-before-green: reverted just the source fix (twice, once for the initial 3-block-dump version of the test and once for the extended 5-block/boundary/transaction-filtering version) and confirmed the test fails with exactly the described corruption both times; restored the fix and confirmed green both times.
  • Full state:: test module: 30/30 passing, including the existing test_fork_load_state, test_fork_load_state_keeps_number_opcode_in_sync, and the deliberately-tested best_number == fork_number boundary case in test_load_state_equal_height_fork_keeps_fork_anchor (unaffected).
  • cargo check -p anvil clean. cargo fmt --check -p anvil clean (stable toolchain; some nightly-only rustfmt options unavailable in this environment, same caveat as prior PRs in this series).
  • cargo clippy -p anvil --all-targets -- -D warnings fails on active_monad_context_for_mined_block/active_monad_context_before_mined_transaction in mem/mod.rs (unrelated missing_const_for_fn lint) - confirmed pre-existing on a clean master checkout with the same command, not introduced by this diff.
  • Synchronous Codex adversarial review: no High/Medium findings. Confirmed the <= boundary predicate correct, the explicit head-restoration ordering correct, and skipping the genesis_hash update on the fork path correct (a dumped block 0 could otherwise overwrite genesis_hash with a foreign value). Two Low findings addressed (test now covers the exact boundary and transaction filtering, per above); one Low finding (the noncanonical-block-by-hash limitation above) disclosed rather than fixed.

receipts

No runtime UI - this is a data-integrity fix in anvil's in-memory chain storage; the regression test IS the receipt (it live-drives real local anvil nodes, no external RPC dependency).

anvil_loadState on a forked node let dumped blocks at or below the fork's
head number silently claim their number's slot in the number->hash map,
even though only the head entry got explicitly corrected afterward. Every
other dumped block's foreign hash stayed in place, producing a chain with
mismatched parentHash links between adjacent blocks.

BlockchainStorage::load_blocks/load_transactions now take an optional
fork_boundary: when the loaded head was resolved from the fork rather than
the dump, dumped blocks/transactions numbered at or below that boundary no
longer claim the canonical slot for that number - the fork's own real
chain does. Block data is still stored by hash so hash-based lookups keep
working; only the number->hash and blockNumber->tx associations are
skipped for the fork's own number range. The non-fork load path is
untouched (fork_boundary is None there).
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

✅ Changelog found

The deterministic check will validate the changed entry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant