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

fix(trie): update prefix set on the call to RevealedSparseTrie::update_rlp_node_level #14108

Merged
merged 13 commits into from
Jan 30, 2025

Conversation

shekhirin
Copy link
Collaborator

@shekhirin shekhirin commented Jan 30, 2025

Previously, we were doing the std::mem::take for the whole prefix set on each call to RevealedSparseTrie::root, but not on RevealedSparseTrie::update_rlp_node_level.

Effectively, this made update_rlp_node_level have no effect on the subsequent root call, because it was still re-calculating the whole trie.


This PR makes the RevealedSparseTrie::update_rlp_node_level update the prefix set, so that it includes only those paths that are above the specified level. Just doing the take is not correct there, because the nodes above the specified level still need re-calculation.

On the screenshot, before 14:40 is main, after is this branch.

image

@shekhirin shekhirin added C-bug An unexpected or incorrect behavior A-trie Related to Merkle Patricia Trie implementation labels Jan 30, 2025
@shekhirin shekhirin marked this pull request as ready for review January 30, 2025 14:39
@shekhirin shekhirin force-pushed the alexey/sparse-trie-rlp-node-log-paths branch from 17bc2bc to 2278214 Compare January 30, 2025 15:01
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't have enough context to review this in detail

but changes lgtm, would prefer to swap the tuple for a newtype though

crates/trie/sparse/src/trie.rs Show resolved Hide resolved
crates/trie/sparse/src/trie.rs Outdated Show resolved Hide resolved
Copy link
Member

@rkrasiuk rkrasiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some nit qs, seems good otherwise

crates/trie/sparse/src/trie.rs Outdated Show resolved Hide resolved
crates/trie/sparse/src/trie.rs Outdated Show resolved Hide resolved
@shekhirin shekhirin force-pushed the alexey/sparse-trie-rlp-node-log-paths branch from f93c257 to 95dc36b Compare January 30, 2025 19:09
@shekhirin shekhirin enabled auto-merge January 30, 2025 19:12
@shekhirin shekhirin added this pull request to the merge queue Jan 30, 2025
Merged via the queue into main with commit 590b58f Jan 30, 2025
44 checks passed
@shekhirin shekhirin deleted the alexey/sparse-trie-rlp-node-log-paths branch January 30, 2025 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trie Related to Merkle Patricia Trie implementation C-bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants