chore: reorganize hash_helper for ethhash #1137
Conversation
The error was previously reporting the partial path before the current node. However, we need the full path to the failing node, which includes the partial path from the node that failed. Also renamed "partial_path" in the checker error to "path". The partial path isn't valuable at this point.
The accumulated partial path didn't include the partial path of the branch, so if you ever had a branch with a partial path and visited a child, the partial_path to the child was incorrect.
We can do 50 iterations in about 0.3s, which seems like a decent fuzz test.
The hash generated in gen_test_trie was incorrect, as it didn't account for the branch's partial path.
If you run this test with ethhash enabled, it fails, because the hash isn't computed the same way for account nodes. This logic is in hash_helper.
The error now contains the full path, not the partial path
The error now contains the full path, not the partial path
…s/firewood into rkuris/diff-fuzz-checker
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
rkuris
left a comment
rkuris
left a comment
There was a problem hiding this comment.
I don't like this change as it adds another allocation in the critical path.
rkuris
left a comment
rkuris
left a comment
There was a problem hiding this comment.
This code needs to run the eth compatible hash tests, or bootstrap from genesis and verify hashes. I'm not 100% confident all this restructuring is correct.
Would have preferred this split into PathGuard changes first, which I really like and is much more straightforward, and the rest of the code, which could be breaking compatibility with ethereum's hash.
| } else { | ||
| // not a single child | ||
| None | ||
| // not an account branch - does not matter what we return here |
There was a problem hiding this comment.
🤔
I bet we can structure this code so this statement is not reachable, or let's wrap this in an Option for readability.
| #[expect( | ||
| clippy::arithmetic_side_effects, | ||
| reason = "hashed and unhashed can have at most 16 elements" | ||
| )] | ||
| let num_children = hashed.len() + num_unhashed; |
There was a problem hiding this comment.
nit: just use wrapping_add, which is free. The comment can stay as to why if you want.
| #[expect( | |
| clippy::arithmetic_side_effects, | |
| reason = "hashed and unhashed can have at most 16 elements" | |
| )] | |
| let num_children = hashed.len() + num_unhashed; | |
| // this sum can never exceed 16 | |
| let num_children = hashed.len().wrapping_add(num_unhashed); |
qusuyan
added
the
DO NOT MERGE
|
This PR will be split into smaller PRs. First one here: #1202 |
This is a subset of changes in #1137.
No description provided.