feat: collision-resistant memory-location hashing via seed + u64 tag - #514
Draft
datnguyencse wants to merge 1 commit into
Draft
feat: collision-resistant memory-location hashing via seed + u64 tag#514datnguyencse wants to merge 1 commit into
datnguyencse wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a per-execution random seed and a secondary LocationTag to mitigate hash collision attacks and safely disambiguate colliding memory locations in the multi-version data structure. However, the review identifies two critical correctness bugs: first, in MvMemory::record, writes to different locations that collide on MemoryLocationHash will overwrite each other in the inner BTreeMap because it is keyed only by TxIdx; second, in PevmRise::build_mv_memory, colliding estimates will lose their correct tags because estimated_locations is keyed only by MemoryLocationHash, causing subsequent reads to skip the estimate and read stale values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+120
to
124
| for (location, tag, value) in write_set { | ||
| self.data.entry(location).or_default().insert( | ||
| tx_version.tx_idx, | ||
| MemoryEntry::Data(tx_version.tx_incarnation, value), | ||
| MemoryEntry::Data(tx_version.tx_incarnation, tag, value), | ||
| ); |
There was a problem hiding this comment.
Critical Correctness Bug: Collision Overwriting in Multi-Version Memory
There is a critical correctness issue in how MvMemory::record handles writes when two different memory locations collide on their MemoryLocationHash (even with different LocationTags).
The Issue
self.data is defined as:
DashMap<MemoryLocationHash, BTreeMap<TxIdx, MemoryEntry>, BuildIdentityHasher>The inner BTreeMap is keyed by TxIdx (transaction index). This means it can only store one MemoryEntry per transaction for a given hash bucket.
If a transaction writes to two different locations that happen to collide on MemoryLocationHash (e.g., address1 and address2 both hashing to the same u64 bucket), write_set will contain two entries:
(hash, tag1, value1)(hash, tag2, value2)
During the loop in MvMemory::record:
for (location, tag, value) in write_set {
self.data.entry(location).or_default().insert(
tx_version.tx_idx,
MemoryEntry::Data(tx_version.tx_incarnation, tag, value),
);
}The second write will overwrite the first write in the BTreeMap because they share the same tx_version.tx_idx.
The Impact (Wrong State / Chain Fork)
When a subsequent transaction reads the first location (address1 with tag1), it looks up hash in self.data. It finds the entry for tx_version.tx_idx, but its tag is tag2 (since it was overwritten).
Because tag2 != tag1, the reader skips this entry:
Some((_, entry)) if entry.tag() != location_tag => continueThe reader then completely misses the write to address1 and reads a stale value from storage or an older transaction. This leads to silent state corruption / wrong state execution, not just a liveness/re-execution issue.
Suggested Fixes
To fully support collision resistance, MvMemory::data must be able to store multiple entries for the same TxIdx if they belong to different locations (tags).
-
Option A: Key the
BTreeMapby(TxIdx, LocationTag)
Change the map type to:DashMap<MemoryLocationHash, BTreeMap<(TxIdx, LocationTag), MemoryEntry>, BuildIdentityHasher>
Since
BTreeMapkeys are ordered lexicographically, queryingrange(..(self.tx_idx, 0))will still return all entries withTxIdx < self.tx_idxin order, allowing you to iterate backwards and find the matching tag. -
Option B: Change the
BTreeMapvalue to a collection
Change the map type to:DashMap<MemoryLocationHash, BTreeMap<TxIdx, SmallVec<[MemoryEntry; 1]>>, BuildIdentityHasher>
When recording a write, append to or update the matching tag in the
SmallVec.
Comment on lines
+130
to
+137
| for address in recipients { | ||
| let (hash, tag) = basic_hash_and_tag(address, seed); | ||
| estimated_locations | ||
| .entry(hash) | ||
| .or_insert_with(|| (tag, Vec::with_capacity(txs.len()))) | ||
| .1 | ||
| .push(index); | ||
| } |
There was a problem hiding this comment.
Correctness Bug: Colliding Estimates Lose Tags
In build_mv_memory, estimated_locations is a HashMap<MemoryLocationHash, (LocationTag, Vec<TxIdx>)>.
If any of the recipients (e.g., beneficiary, BASE_FEE_RECIPIENT, L1_FEE_RECIPIENT, OPERATOR_FEE_RECIPIENT) happen to collide on their MemoryLocationHash, the or_insert_with call will reuse the LocationTag of the first recipient that was inserted:
estimated_locations
.entry(hash)
.or_insert_with(|| (tag, Vec::with_capacity(txs.len())))This means the estimate entry in MvMemory for the colliding recipient will be created with the wrong tag.
When a transaction subsequently reads that colliding recipient, it will compute its correct tag, but skip the estimate entry in MvMemory because the tag doesn't match:
Some((_, entry)) if entry.tag() != location_tag => continueAs a result, the reader will not block on the estimate and will read a stale value, leading to missing dependencies and race conditions.
Suggested Fix
Key estimated_locations by (MemoryLocationHash, LocationTag) instead of just MemoryLocationHash to ensure each unique location's tag is preserved.
…y tag The u64 MemoryLocationHash doubles as both the MvMemory map key and the location's identity. Two distinct locations that collide in the u64 share an entry, so a crafted (FxHash is invertible) or random (birthday ~2^32) collision can make a read return another location's value and validate cleanly — wrong state. This fixes it with two cheap, independent levers: - Per-execution random seed mixed into every location hash (and tag). Unknown to submitters until the block runs, so collisions cannot be aimed at this block. - A second, independently seeded fingerprint (tag) stored in each MemoryEntry. Reads skip entries whose tag differs from the location being read, so a collision is only harmful if both the hash AND the tag collide (~2^-128). The seed never escapes into results (state is keyed by real addresses), so output is identical for any seed — verified by the full test suite. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
datnguyencse
force-pushed
the
feat/seeded-tagged-location-hash
branch
from
June 4, 2026 07:48
073d22a to
d007312
Compare
Contributor
Author
|
benchmark |
|
✅ Gigagas benchmark for commit d007312 Detail |
|
✅ Mainnet benchmark for commit d007312 This pr: Detail |
datnguyencse
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
MemoryLocationHash = u64is used as theMvMemorymap key itself, so it doubles as both the bucket key and the location's identity. Two distinct locations that collide in theu64share one entry:Fix — two cheap, independent levers
RandomState(no new dependency) and stored onMvMemory.!seed) stored in eachMemoryEntry. Reads skip entries whose tag differs from the location being read, so a collision is only harmful if both the hash and the tag collide (~2⁻¹²⁸). This restores the key-comparison a normalHashMapdoes, without widening theu64map key.The seed never escapes into results (state is keyed by real
Addresses), so output is identical for any seed.Cost
Read path (common, no-collision case): one extra
u64compare + one extra hash per location. Per block: oneRandomState::new().Residual (documented, adversarially unreachable)
ReadSet/ validation stay keyed by the bareu64hash, so two colliding locations touched by one tx can still conflate there — but that only ever forces a spurious re-execution (liveness), never wrong state, and the secret seed makes it impossible to trigger on purpose.Verification
cargo clippy --workspace --all-targets— cleancargo test --workspace --release -- --test-threads=1— all green, including the fullethereum/testsstate-test suite, RISE/mainnet/uniswap blocks (these compare pevm output to expected/sequential, proving the change is output-preserving)Benchmarks (
gigagas,mainnet) pending — opening as draft for perf review.