-
Notifications
You must be signed in to change notification settings - Fork 646
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
feat: stub for instant resharding state root creation #12083
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #12083 +/- ##
==========================================
- Coverage 71.59% 71.52% -0.07%
==========================================
Files 818 819 +1
Lines 164532 164694 +162
Branches 164532 164694 +162
==========================================
+ Hits 117792 117796 +4
- Misses 41597 41753 +156
- Partials 5143 5145 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
e47af53
to
71b6358
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, left a few comments but the high level structure makes sense. I didn't have a full proper look so I'll let others approve.
/// Returns id of the node after deletion applied. | ||
fn delete_multi_range_recursive<'a, M: ArenaMemory>( | ||
root: MemTrieNodePtr<'a, M>, | ||
key_nibbles: Vec<u8>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this feels like it could be some more efficient representation like &[u8] but it's hard to say if it matters and how exactly to do it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another option is to cut nibbles from the front of intervals_nibbles
. But as the keys are short and this happens only once, I want to be simple wherever it's possible.
|
||
match node_view { | ||
MemTrieNodeView::Leaf { extension, .. } => { | ||
let extension = NibbleSlice::from_encoded(extension).0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Would be nice to implement a getter method instead of using .0
(I know it's outside of this PR but it's a small change so might as well do it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a lot of places using NibbleSlice::from_encoded(...).0
and I wouldn't touch them for now, seems better to fix them all at once.
Also not sure if a getter method is applicable because from_encoded returns (Self, bool).
@wacban thanks for the great feedback! applied a lot of changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Thanks a lot Alex! 😄
nice start! 🚀 |
@shreyan-gupta answering globally - yeah, reusing even more from MemTrieUpdate works more nicely. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
} | ||
|
||
/// Based on the key and the intervals, makes decision on the subtree exploration. | ||
fn retain_decision(key: &[u8], intervals: &[Range<Vec<u8>>]) -> RetainDecision { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks! :)
Create the code flow intended for resharding state root creation.
The ultimate goal will be the temporary memtries created, which will be used until new memtries are created from flat storage.
This will be a multi-step work. To complete it, all mentioned TODOs must be resolved.
Overview
The core is a
MemTrieUpdate::retain_multi_range
function and a simpletest_retain_multi_range
test. It follows the logic in #12074. I plan to enhance logic sequentially until it will be fit for the actual resharding.Drawback is that code is not used on chain, but setting up full resharding test requires separate work. I suggest to start from the the incomplete flow; hopefully I will need ~5 PRs to set up the full test.
It follows the "top-down" logic as in insert/delete functions: first we pull root node to in-memory update list, then we descend into children. In the end, we call
to_trie_changes
which callspost_order_traverse_updated_nodes
to create full set of changes to be applied to trie.Alternative
For the multi-range deletion, better logic is "bottom-up": we first understand the result of children subtrees, and then mark root node as updated if needed.
It's not clear whether everything must be "bottom-up". It probably has its own challenges. But here it is strictly better because it automatically gives the post traversal order of updated nodes.
We could compute hashes on the fly but it's less LoC to postpone it to reuse existing logic, so I just call
compute_hashes_and_serialized_nodes
in the end.