Repository navigation
fix(l1): seed the hashers of every map keyed by contract- or block-chosen values - #7408
Conversation
…he block producer, as #7373 did for transient storage. rustc-hash 2.x is unseeded and hashes a key of 17 to 32 bytes as multiply_mix(SEED2 ^ word(bytes[len-16..len-8]), ..) ^ len, with SEED2 public and multiply_mix(0, _) == 0, so storage slots, touched addresses, precompile calldata and (address, slot) tuples can all be crafted to share one hashbrown bucket: every lookup becomes a scan and a block becomes O(n^2) work at flat opcode gas. Now on foldhash::HashMap / HashSet (seeded per process and per map): in LEVM, LevmAccount.storage, CacheDB, both CallFrameBackup maps, the Substate accessed-address and accessed-slot sets (new aliases AccessedAddresses and AccessedStorageSlots), PrepareRegionBackupMarker, VM.storage_original_values, GeneralizedDatabase.accessed_accounts, the CachingDatabase account and storage caches, and the per-precompile calldata map of the precompile cache; in ethrex-common, AccountUpdate.added_storage, BalSynthesisItem.added_storage with synthesize_bal_updates, the addr_to_idx and slot_idx_by_account maps of BalAddressIndex, and the recorder's tx_initial_storage; in the LEVM backend, the BAL validation sets (unread_storage_reads, unaccessed_pure_accounts, pure_reads, canonical) and the validate_tx_execution parameters; in ethrex-blockchain, the merkleizer's per-account storage and BalSynthesisItem maps, the witness accumulator and hashed-address cache, the prewarmer's walked-account and walked-slot sets, StoreVmDatabase's account-state cache and ReplayedVmDatabase's updates. Maps keyed by keccak outputs (code hashes, created and self-destructed contract addresses, EIP-7702 authorities, transaction hashes and senders), by block access indices, or by hashed trie keys stay on FxHash: nobody can steer those keys into a bucket. No iteration over a seeded map reaches consensus-visible bytes: each one feeds a trie, a BTreeMap, the sorted and deduplicated witness, a validation check whose verdict is order-free, or the choice of which BAL error to report first. The L2 store serialises Vec<AccountUpdate> with bincode only to decode it again. Guests keep foldhash's own RandomState, as #7373 left it. test/tests/levm/seeded_hashers_tests.rs builds, for every key shape, keys that provably collide under FxHash and checks that the real types hash them apart; the private maps get the same check as unit tests next to them, so reverting any hasher fails a test. The lockfiles of every workspace that builds the changed crates gain only the foldhash dependency edge; tooling's state_v2 runner compares storage through the new type and drops its now-unused rustc-hash dependency.
… of foldhash's HashMap directly, and feed fixed-size keys to foldhash as words. The previous commit cost the SSTORE microbenchmark 21% (39.9 to 48.2 ms over 5433 warm SSTOREs x 200 runs): foldhash's byte path spends four folded multiplies on a 32-byte key where FxHash spends three, and a warm SSTORE hashes several H256 and Address keys. SeededState wraps foldhash's RandomState and its hasher turns a 32-byte write into two u128 writes and a 20-byte write into a u128 plus a u32, which foldhash folds with two multiplies; every other write delegates unchanged. The attack resistance is the same, since each multiply still mixes in a seed the key does not know. With it the same benchmark reads 40.1 vs 41.2 ms, inside its own run-to-run spread, and the other LEVM benchmarks move by at most 1.6% either way. TransientStorage moves to the same type, so every seeded map in the tree hashes the same way, and foldhash is now a dependency of ethrex-common alone: the levm, vm, blockchain, test and tooling state_v2 manifests drop it, and the lockfiles follow. The module carries unit tests for H256, U256, Address and byte-string keys that collide under FxHash and hash apart here, plus one that equal keys hash equal on both the word and the byte path.
…the backend's. Every use sits in rayon-only functions, so a build without the feature flagged the import as unused under -D warnings.
|
🤖 Claude Code ReviewReview: PR 7408 — seeded hasher for attacker-chosen map keysI reviewed the diff only. I did not build it or run the tests. The approach is sound. Replacing FxHash with a per-process, per-map seeded foldhash for maps whose keys contracts or block producers choose closes a real hash-flooding gap. The Points to check
Minor
The rest of the diff is mechanical type substitution and lockfile churn, and it looks consistent. The Automated review by Claude (Anthropic) · sonnet · custom prompt |
🤖 Codex Code ReviewLooks good overall: this PR is narrowly targeted, the threat model is well understood, and replacing unseeded
If you want, I can do a second pass focused only on the LEVM-side touched structures and revert/rollback behavior. Automated review by OpenAI Codex · gpt-5.4 · custom prompt |
🤖 Kimi Code ReviewI'll review this PR which replaces Security AssessmentCritical: Hash collision attack mitigation — well addressed. The PR correctly identifies that Code Review
|
| Severity | Issue | Location |
|---|---|---|
| Moderate | write() fallthrough pattern — use explicit unwrap |
crates/common/seeded_hash.rs:64-82 |
| Minor | Document endianness assumption | crates/common/seeded_hash.rs module docs |
| Minor | hashes_colliding_byte_strings_apart incomplete assertion |
crates/common/seeded_hash.rs:170 |
The PR is well-structured, thoroughly tested, and addresses a genuine security concern. The main fix needed is making the write() method's infallible paths explicit rather than using if let with silent fallthrough.
Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt
Lines of code reportTotal lines added: Detailed view |
Benchmark Results ComparisonBenchmark Results: MstoreBench
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
|
…mismatch field wider than rustfmt's line limit, and the tooling workspace is formatted separately from the root one.
… the host witness path and in the guest. get_state_transitions lists the updates in CacheDB iteration order, which is now seeded per map, so the host recorded the witness under one order while the guest replayed under another. A removal that leaves a branch with one child reads that child, and an insert into that branch first avoids the read: a host that inserted first produced a witness that a guest removing first could not replay, and proving failed with a missing node. With FxHash both sides built the list in the same order by construction. apply_account_updates_from_trie_with_witness and GuestProgramState::apply_account_updates now sort by address before applying, so the two sides touch the same nodes whatever order the lists come in. The new test records a witness under each input order and replays it on a guest applying each input order, once with the removed account sorting first and once with the inserted one first, so it fails if either side stops sorting. Storage tries were already safe: the guest applies inserts before deletes, which reads a subset of the nodes any interleaving reads.
…ss accounts its block never touched. Both checklists are seeded sets, so iter().next() named a different entry on every node and every run for the same invalid block. min() is only reached once the block has already failed, so valid blocks pay nothing. Two tests inject 256 extraneous reads and 256 extraneous accounts and expect the lowest of each. The levm runner's state diff and state_v2's storage mismatch report also sort what they print, by address and by slot. The per-transaction checks in validate_tx_execution still return the first offender their maps yield. Every BAL rejection maps to the same exception class, so this changes only which entry a message names, and fixing it there would cost a sort on every transaction of a valid block.
…them on FxHash. A keccak output is no safer as a key than its preimage: a deployer can grind trailing bytecode until the code hash lands in a chosen bucket, at about one keccak per bucket for each key, and a contract deployed once can be called again for 100 gas, with every call looking its code up in GeneralizedDatabase::codes. codes, code_metadata, the CachingDatabase code cache and its prefetch dedup set, StoreVmDatabase's batch read map and the witness's code dedup set now hash with SeededState. None of them is iterated, so their order never leaves the process. created_accounts and selfdestruct_set stay on FxHash, since each entry costs a CREATE or a SELFDESTRUCT. The seeded_hash module docs now say so, note that a walk over a seeded map must be sorted before its order leaves the process, and spell out what zkVM guests get: foldhash finds no clock or address randomization there, so its seeds follow from the guest program and its input. The tests no longer assert that keys collide under FxHash, which tied them to rustc-hash's SEED2 and would fail on a rustc-hash release with nothing wrong in production. seeded_hash's own tests check that H256 and U256 keys varying in either half, addresses varying in either part, and byte strings on both paths spread across 4096 buckets, which catches a hasher that drops part of a key where the old 64-bit distinctness check did not. The per-map tests check that each map hashes a key differently from a fresh instance of its hasher, which no unseeded hasher does, and replace the five copies of the key-crafting helpers; the StoreVmDatabase test reads the real account_state_cache field instead of a fresh AccountStateCache, and the test that only exercised FxHash is gone. SeededHasher::write branches on the lengths it handles instead of falling through arms that cannot run.
… witness paths. generate_witness_from_account_updates, which the block import pipeline runs when it also collects a witness, still deduplicated with an FxHashSet after the previous commit seeded the same set in generate_witness_for_blocks_with_fee_configs. The logger records one entry per code read and a deployer can grind its code hash into a chosen bucket, the same reason the other code-hash maps are seeded. Both functions now call unique_code_hashes, which keeps each hash once in first-read order; a unit test pins that order.
…eeded the per-block ones. CodeCache, code_metadata_cache and BlockDataBuffer.codes still hashed with FxHash, and they outlive the block that fills them: contracts whose code hashes a deployer ground into one bucket stay cached across blocks, and every block that loads one of them for 2600 gas probes all the others while holding the code cache's lock. The per-block maps sit in front of these caches and do not shield them, since each block loads every contract it touches from the Store once. All three now hash with SeededState. The random eviction picks a victim by IndexMap position, which follows insertion order rather than the hasher, and the one walk over BlockDataBuffer.codes feeds database writes keyed by hash, so no order leaves the process.
…t. It had been inserted between that comment and its function, so rustdoc gave unique_code_hashes the comment about attaching the stateless validator, and attach_stateless_validator lost the explanation of why it must stay out of Evm's constructor.
…alk seeded state. validate_tx_execution's execution-to-BAL pass, validate_bal_withdrawal_index's DB-to-BAL pass and validate_bal_covers' backstop walked CacheDB and each account's storage and returned the first mismatch they met, so an invalid block with several named a different one on every node and every run. They now go through first_failure_by_key, which walks the map once in its own order and, only if something failed, sorts the keys and walks again, returning the failure of the lowest key. The per-slot loops go through it too, so a message names the lowest account and then its lowest slot. A valid block still walks each map once with no sort, which is what the previous commit expected this fix to cost. The checks only read, so the second walk sees what the first did. Three unit tests cover the helper: a failing map names its lowest failure even behind a passing key, a passing map is walked once, and nested maps name the lowest outer key and then the lowest inner one. Two validate_tx_execution tests inject 255 accounts absent from the BAL and 255 unrecorded storage writes and expect the lowest of each; both fail if the helper stops sorting.
…of an account's storage changes. added_storage is a seeded map and the host applies it in that map's order, so the order it records the witness under now changes on every run; the guest applies its inserts before its deletes, which must read a subset of the nodes any host order reads. host_witness now records the storage nodes as well and builds the witness the way the RPC path does, through RpcExecutionWitness::into_execution_witness. Two scenarios run every host order against every guest order, each forced by rebuilding the map until it iterates in that order: a delete and an insert in one branch, which fails if the guest stops putting inserts first, and two deletes from one branch, which pins that deletes read the same nodes in either order.
… filters in ethrex-storage. Their keys are the trie paths a block's state writes touch, which a contract picks with ordinary stores, and a cold read probes each layer's map once for up to 128 layers on every block while the layers live, so a crafted set of paths would be paid for once and probed repeatedly. The maps the storage crate still keeps on FxHash are keyed by block hashes, transaction hashes, block numbers and state roots, each of which costs a block or a transaction to produce, or belong to the in-memory backend that only serves the memory datadir. A unit test in layering.rs checks that paths sharing one FxHash bucket are hashed apart by the layer and overlay maps, so reverting either hasher fails it.
2 participants
Motivation
#7373 seeded the transient storage map because contracts choose its keys and rustc-hash 2.x is unseeded. The same holds for many more maps on the execution and validation path: storage slots, touched addresses, precompile calldata and
(address, slot)tuples are all chosen by contract code or by the block producer. rustc-hash hashes a key of 17 to 32 bytes asmultiply_mix(SEED2 ^ word(bytes[len - 16..len - 8]), ..) ^ len, withSEED2a public constant andmultiply_mix(0, _) == 0, so keys that carrySEED2in that word all hash tolen, land in one hashbrown bucket, and turn every lookup into a scan: O(n) per access, O(n²) per block, at flat opcode gas.Description
Swaps hashers; no map changes shape, capacity hints or semantics. Where a seeded map's order would leave the process, the code now sorts first (below). Every seeded map now uses
SeededHashMap/SeededHashSetfrom the newethrex_common::seeded_hashmodule.SeededStatewraps foldhash'sRandomState(seeded per process and per map, as #7373 used it) and its hasher feeds a 32-byte key as twou128writes and a 20-byte key as au128plus au32, which foldhash folds with two multiplies instead of the four its byte path spends. That detail is what keeps the change perf-neutral: with plainfoldhash::HashMapthe SSTORE microbenchmark was 21% slower, because a warm SSTORE hashes severalH256andAddresskeys. The resistance is the same either way, since every multiply still mixes in a seed the key does not know.TransientStoragemoves to the same type, so the whole tree hashes these keys one way, and foldhash becomes a dependency of ethrex-common alone.Seeded:
LevmAccount.storage,CacheDB, bothCallFrameBackupmaps, theSubstateaccessed-address and accessed-slot sets (now the public aliasesAccessedAddressesandAccessedStorageSlots),PrepareRegionBackupMarker,VM.storage_original_values,GeneralizedDatabase.accessed_accounts,codesandcode_metadata, theCachingDatabaseaccount, storage and code caches and its code-prefetch dedup set, the per-precompile calldata map of the precompile cache, andTransientStorage.AccountUpdate.added_storage,BalSynthesisItem.added_storageandsynthesize_bal_updates,BalAddressIndex.addr_to_idxandslot_idx_by_account, and the recorder'stx_initial_storage.unread_storage_reads,unaccessed_pure_accounts,pure_reads,canonical) and thevalidate_tx_executionparameters.BalSynthesisItemmaps, the witness accumulator and hashed-address cache, the prewarmer's walked-account and walked-slot sets,StoreVmDatabase's account-state cache and batch code-read map,ReplayedVmDatabase's updates, and the code-hash dedup both witness paths share (unique_code_hashes).CodeCacheandcode_metadata_cache, andBlockDataBuffer.codes, which keep code across blocks; the trie diff-layer node maps, the deep-reorg overlay's four maps, the bloom filters in front of both, and the reconciliation key set incommit_to_disk.A keccak output is no safer as a key than its preimage: whoever picks the preimage (a CREATE2 salt, the trailing bytes of deployed code) can grind it until the hash lands in a chosen bucket, at about one keccak per bucket of the table for each key. That is why code hashes are seeded: a contract deployed once can be called again for 100 gas, and every call looks its code up by hash, in the per-block maps and in the Store caches behind them. What keeps a map on FxHash safe is the gas a block pays for each key it adds or looks up. Left on FxHash on that ground: created and self-destructed accounts (each entry costs a CREATE or a SELFDESTRUCT, and only SELFDESTRUCT looks them up), EIP-7702 authority sets (each entry costs an authorization), recovered senders and transaction hashes (each costs a transaction), and maps keyed by block access index, which a block assigns by position rather than choosing. Also still on FxHash: the merkleizer's per-shard maps keyed by hashed address, which get one entry per account the block modifies and are probed once per entry. The trie diff layers are seeded on the same ground: their keys are the paths a block's state writes touch, a contract picks those with ordinary stores, and a cold read probes the chain once per layer for up to 128 layers on every block while the layers live, so a crafted set of paths is paid for once and probed for as long as it stays cached. What the Store still keeps on FxHash is keyed by block hashes, transaction hashes, block numbers and state roots, each of which costs a block or a transaction, or belongs to the in-memory backend that only serves the memory datadir used by dev and tests. Out of scope here and worth a separate look: the mempool's maps keyed by transaction hash and sender are bounded by the pool's limits but sit on a path peers feed directly. rustc-hash's own
FxSeededStatewould not have helped the seeded ones: its byte path ignores the seed, so crafted byte-string keys still collapse.Consensus-visible bytes are unchanged: every iteration over a seeded map feeds a trie, a
BTreeMap, the sorted and deduplicated witness, a validation check whose verdict does not depend on order, or the message of a BAL rejection, which names the same offender whatever order the maps walk in (below). The L2 store serialisesVec<AccountUpdate>with bincode only to decode it again. Guest programs hash with the sameSeededState. In a zkVM foldhash finds no clock and no address randomization, so its seeds follow from the guest program and its input: crafting keys against a guest takes replaying it up to the map under attack, which is harder than against FxHash's public constants but possible.One iteration order mattered without changing any bytes.
get_state_transitionslists a block's account updates inCacheDBorder, and the host records the witness while applying that list to the state trie: a removal that leaves a branch with one child reads that child, while an insert into the branch first avoids the read. With FxHash the host and the guest built the list in the same order. With seeded maps the host could insert first and the guest remove first, and the guest would need a node the witness never recorded, so proving failed with a missing node.apply_account_updates_from_trie_with_witnessandGuestProgramState::apply_account_updatesnow apply the updates in address order, so both sides touch the same nodes whatever order the lists come in.test/tests/blockchain/witness_update_order_tests.rsrecords a witness under each input order and replays it on a guest applying each input order, once with the removed account sorting first and once with the inserted one first; it fails if either side stops sorting. Storage updates need no sort: the guest applies inserts before deletes, which reads a subset of the nodes any host order reads. Two more tests in the same file record a storage witness under every order of an account's changes, a delete and an insert in one branch and two deletes from one branch, and replay it under every guest order; the first fails if the guest stops putting inserts first. The sweep for the same shape found nothing else: the BAL builder sorts, the tracers emitBTreeMaps, the merkleizer and prefetcher are order-free, and the L2 committer only reapplies stored updates to a trie.Rejections name the same offender on every node. The BAL checks used to stop at whichever offender a walk over a seeded set or over
CacheDBmet first, so the same invalid block got a different message on every node and every run. The checks of leftover declared reads and pure-access accounts now name the lowest leftover.validate_tx_execution's execution-to-BAL pass,validate_bal_withdrawal_index's DB-to-BAL pass andvalidate_bal_covers' backstop go throughfirst_failure_by_key, which walks a map once in its own order and, only if something failed, walks it again in key order and returns the lowest failure; the per-slot loops inside them do the same. A valid block pays no sort. The levm runner's state diff and state_v2's storage mismatch report sort what they print, by address and by slot.Tests:
seeded_hash's unit tests check thatH256,U256,Addressand byte-string keys varying in either part spread across 4096 buckets, and that two maps hash the same key apart; they no longer assert that keys collide under FxHash, so a rustc-hash release cannot break them.test/tests/levm/seeded_hashers_tests.rstakes each public map as production code builds it and checks that it hashes a key differently from a fresh instance of its hasher, which no unseeded hasher does, so reverting any public map's hasher fails a test; the private maps (precompile cache, warming caches, account-state cache, replayed updates, trie layers and overlay) get the same check as unit tests next to them.unique_code_hasheshas a unit test pinning first-read order. The lowest-offender rule is covered by two block-import tests (256 extraneous reads, 256 extraneous accounts), twovalidate_tx_executiontests (255 accounts absent from the BAL, 255 unrecorded storage writes) and unit tests offirst_failure_by_key; thevalidate_tx_executiontests fail if the helper stops sorting.Lockfiles: relative to main, every workspace that builds the changed crates moves the
foldhash 0.2.0edge fromethrex-levmtoethrex-common(make check-cargo-lockpasses). The tooling state_v2 runner now compares and reports storage asBTreeMaps sorted by slot, and drops its now-unusedrustc-hashdependency.Benchmarks on the same machine (Apple M-series laptop), main vs. this branch, hyperfine with 30 interleaved runs each, median and standard deviation in ms. They were taken before the code-hash maps and the Store's code caches were seeded and have not been re-run since:
l2-1k-erc20.rlp(1110 blocks, s)The import row is the blocks-exec perf CI recipe (
ethrex --network fixtures/genesis/perf-ci.json --force import fixtures/blockchain/l2-1k-erc20.rlp --removedbon a scratch datadir, hyperfine, 30 interleaved runs per side). The trie-layer seeding was measured on its own with the same recipe, 20 interleaved runs per side, against the commit before it: medians 24.89 s (sd 2.92, one 37.8 s outlier) before and 24.23 s (sd 0.84) after.build_block_benchmark(Ggas/s, higher is better, 30 samples)benches/build_block_benchmarkdoes not run on main as checked in: its setup funds each account withu64::MAX, below the cost of 1000 transactions atmax_fee_per_gas = u64::MAX, and its 192 x 1000 transactions trip the mempool's 90% occupancy gap gate at the default pool size. The rows above come from the unmodified benchmark body with two setup-only changes applied identically to both sides and not committed here: genesis balanceU256::MAXandmax_mempool_size: 1_000_000. Its round-to-round swing on main alone (3.14 to 3.48) is wider than the difference between the sides.Checklist
STORE_SCHEMA_VERSION(crates/storage/lib.rs) if the PR includes breaking changes to theStorerequiring a re-sync.