Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

trie: iterate values pre-order and fix seek behavior #27838

Merged
merged 7 commits into from
Jun 4, 2024

Conversation

roysc
Copy link
Contributor

@roysc roysc commented Aug 2, 2023

Proposed fix for #27837

@rjl493456442
Copy link
Member

i think it's a valid fix, could you please add more relevant unit tests?

@roysc roysc force-pushed the fix-node-iterator-seek branch from c41b8d4 to 149951b Compare August 3, 2023 09:55
@roysc roysc marked this pull request as ready for review August 3, 2023 10:04
@roysc
Copy link
Contributor Author

roysc commented Aug 3, 2023

In order to properly change the order of traversal, I added a fix that will jump to the full node's leaf first, then back to iterate its child nodes normally, and skip the leaf at the end. It's a little funky, but seems to work fine.

I updated the unit tests to reflect the new ordering, which should sufficiently cover the behavior change. I also added an explicit case to check that seeking does not produce a leaf whose key prefixes the target key.

@roysc roysc changed the title trie: normalize compared node paths in iterator seek trie: iterate values pre-order and fix seek behavior Aug 3, 2023
trie/iterator.go Outdated Show resolved Hide resolved
@roysc roysc requested a review from holiman August 11, 2023 11:19
@rjl493456442 rjl493456442 self-assigned this Aug 15, 2023
@roysc
Copy link
Contributor Author

roysc commented Aug 24, 2023

Is there anything else this needs?

@rjl493456442
Copy link
Member

Wrt the path comparison, I think for ethereum state trie, this issue will never occur because of the same key length.

@roysc
Copy link
Contributor Author

roysc commented Oct 25, 2023

Right, I tried to acknowledge that in the issue (#27837 (comment)) - it won't show up in normal Geth operations, but it creates this inconsistency between a pre-order traversal of nodes vs. post-order traversal of values. I think it's at least worth documenting if not fixing.

@rjl493456442 rjl493456442 force-pushed the fix-node-iterator-seek branch from 59c555e to bbc0c64 Compare December 12, 2023 08:11
@rjl493456442 rjl493456442 added this to the 1.14.4 milestone Jun 4, 2024
@rjl493456442 rjl493456442 merged commit 68c0ec0 into ethereum:master Jun 4, 2024
3 checks passed
@roysc roysc deleted the fix-node-iterator-seek branch June 4, 2024 05:18
jorgemmsilva pushed a commit to iotaledger/go-ethereum that referenced this pull request Jun 17, 2024
This pull request fixes the pre-order trie traversal by defining 
a more accurate iterator order and path comparison rule.

Co-authored-by: Gary Rong <garyrong0905@gmail.com>
stwiname pushed a commit to subquery/data-node-go-ethereum that referenced this pull request Sep 9, 2024
This pull request fixes the pre-order trie traversal by defining 
a more accurate iterator order and path comparison rule.

Co-authored-by: Gary Rong <garyrong0905@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants