Fixed trie hash collision issue when constructing storage tries #75
Conversation
This file contains 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
- Previously we did not have access to the account key when we were
constructing an account's storage trie from compact. To get around
this, we made the "bad" assumption that a trie's root would uniquely
identify the storage trie and used a mapping of `trie_root` --> `trie`
to connect tries with accounts at the end of the decoding process.
- However, it is possible for a trie with hashed out nodes to have
the same hash as one that does not.
- This was causing accounts with a specific trie to be rarely swapped
out with one that had different nodes hashed/unhashed.
dvdplm
reviewed
Mar 1, 2024
| ) -> CompactParsingResult<()> { | ||
| for (i, slot) in children.iter().enumerate().take(16) { | ||
| if let Some(child) = slot { | ||
| // TODO: Seriously update `mpt_trie` to have a better API... |
There was a problem hiding this comment.
Curious about what you have in mind here!
There was a problem hiding this comment.
I think in this case I was not that happy that this was two lines:
let mut new_k = curr_key;
new_k.push_nibble_back(i as Nibble);What might be kind of nice here is to have another method type coexisting with push_* that clones and does an operation to the Nibbles (maybe append_*?).
trace_decoder/src/types.rs
Outdated
| /// A type alias around `u64` for a block height. | ||
| pub type BlockHeight = u64; | ||
| /// A type alias around `[U256; 8]` for a bloom filter. | ||
| pub type Bloom = [U256; 8]; | ||
| /// A type alias around `H256` for a code hash. | ||
| pub type CodeHash = H256; | ||
| /// A type alias for `H256` for an account address's hash. | ||
| /// A type alias around `H256` for an account address's hash. |
There was a problem hiding this comment.
I think "for" is correct here (and below). :)
There was a problem hiding this comment.
Although I'm indeed not a fan of the double for
There was a problem hiding this comment.
We could replace the second for with of maybe?
Nashtare
added
the
crate: trace_decoder
Nashtare
approved these changes
Mar 4, 2024
| let remaining_node = remaining_entry | ||
| .into_node() | ||
| .expect("Final node in compact entries was not a node! This is a bug!"); | ||
| fn process_code(&mut self, c_bytes: Vec<u8>) -> CompactParsingResult<()>; |
There was a problem hiding this comment.
Some doc on what the methods are doing for this new trait would be nice
There was a problem hiding this comment.
Yeah fair I agree. Let me add this.
trace_decoder/src/types.rs
Outdated
| /// A type alias around `u64` for a block height. | ||
| pub type BlockHeight = u64; | ||
| /// A type alias around `[U256; 8]` for a bloom filter. | ||
| pub type Bloom = [U256; 8]; | ||
| /// A type alias around `H256` for a code hash. | ||
| pub type CodeHash = H256; | ||
| /// A type alias for `H256` for an account address's hash. | ||
| /// A type alias around `H256` for an account address's hash. |
There was a problem hiding this comment.
Although I'm indeed not a fan of the double for
Co-authored-by: Robin Salen <30937548+Nashtare@users.noreply.github.com>
muursh
approved these changes
Mar 5, 2024
- Not directly releated to this PR, but it came up in the PR discussion that the comments for these types could be better.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes the issue with hitting a hash node on block
17735424.Essentially the issue was we were making the assumption that a trie's root would always uniquely identify it, which is not the case when you have two otherwise identical tries except different nodes are hashed/unhashed. The previous impl did not have access to the account key when parsing the embedded storage trie within the account trie and relied on creating a temporary map between
trie_hash-->trie. This map would at the end of the decoding process be used to connect accounts with their storage tries.However, with the above issue, this meant that sometimes (apparently extremely rarely) an account's storage trie would get swapped out with another "identical" trie that had different nodes hashed out. In the case of
17735424, an account was getting a trie that was entirely hashed out due to another storage trie overwriting it in the map.A lot of the work in this PR is removing this old map of
trie_hash-->trieand instead embedding the trie directly inAccountNodeDataitself. Also needed to changecompact_to_partial_trie.rs(which is used to parse both compact state and storage tries) to use a bit of specific logic for each.