starknet_committer,starknet_committer_and_os_cli,starknet_patricia: layout dependent fetch_nodes#10680
Conversation
4cc3165 to
b6aa42e
Compare
021132a to
466fffe
Compare
466fffe to
b4ed9c6
Compare
b6aa42e to
0662437
Compare
b4ed9c6 to
c700f80
Compare
c7ceea5 to
f9be917
Compare
c700f80 to
745b6d9
Compare
745b6d9 to
d1214e3
Compare
00cd7a5 to
560776a
Compare
d1214e3 to
a4c64e9
Compare
560776a to
e1719c0
Compare
a4c64e9 to
ad38e50
Compare
ad38e50 to
b24fb64
Compare
e1719c0 to
68f6987
Compare
b24fb64 to
71c316b
Compare
ArielElp
left a comment
There was a problem hiding this comment.
Reviewable status: 10 of 14 files reviewed, 8 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/starknet_committer/src/db/db_layout.rs line 23 at r8 (raw file):
Previously, nimrod-starkware wrote…
new lines + move the doc next to the def
Done.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 4 at r8 (raw file):
Previously, nimrod-starkware wrote…
please add a todo to move this file somewhere else, now that fetch_nodes is generic in storage layout
It's already moving in the next PRs (2 ahead of this one)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 47 at r8 (raw file):
Previously, nimrod-starkware wrote…
Can you please update the doc about with some explanation that this function is generic in the storage layout etc
Done.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 70 at r8 (raw file):
Previously, nimrod-starkware wrote…
please replace all the calls to
get_root_indexwith this
Done.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 79 at r8 (raw file):
Previously, yoavGrs wrote…
Consider handling unmodified subtrees before the for loop.
Done.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 271 at r8 (raw file):
Previously, nimrod-starkware wrote…
Please update the docs here too
Hhhmm I think this doc actually stick
crates/starknet_committer/src/db/facts_db/traversal.rs line 45 at r8 (raw file):
for ((subtree, optional_val), db_key) in subtrees.iter().zip(db_vals.iter()).zip(db_keys) { let Some(val) = optional_val else { Err(StorageError::MissingKey(db_key))? }; let filled_node = DBObject::deserialize(val, &subtree.get_root_context())?;
Done.
yoavGrs
left a comment
There was a problem hiding this comment.
Reviewable status: 10 of 14 files reviewed, 12 unresolved discussions (waiting on @ArielElp and @nimrod-starkware)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 261 at r9 (raw file):
.insert(root_index, OriginalSkeletonNode::UnmodifiedSubTree(filled_root.hash)); if let NodeData::Edge(EdgeData { bottom_data, path_to_bottom }) = filled_root.data {
Would it be a mistake to push the bottom to next_subtrees in both cases?
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 273 at r9 (raw file):
); } }
Share code with unmodified leaf case in handle_subtree.
Code quote:
match SubTree::should_traverse_unmodified_child(bottom_data) {
UnmodifiedChildTraversal::Traverse => {
let (bottom_subtree, _) = subtree.get_bottom_subtree(&path_to_bottom, bottom_data);
next_subtrees.push(bottom_subtree);
}
UnmodifiedChildTraversal::Skip(unmodified_child_hash) => {
skeleton_tree.nodes.insert(
path_to_bottom.bottom_index(root_index),
OriginalSkeletonNode::UnmodifiedSubTree(unmodified_child_hash),
);
}
}crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 279 at r9 (raw file):
/// Handles a subtree referred by an edge or a binary node. Decides whether we deserialize the /// referred subtree or not. fn handle_subtree<'a, SubTree: SubTreeTrait<'a>>(
Suggestion:
handle_child_subtreecrates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 286 at r9 (raw file):
should_fetch_modified_leaves: bool, ) { let is_leaf = subtree.is_leaf();
I prefer the previous structure, but it's personal. Suggestion only, non-blocking:
if !subtree.is_leaf() {
next_subtrees.push(subtree);
} else {
if subtree.is_unmodified() {
maybe_traverse_unmodified_chilld()
} else if should_fetch_modified_leaves {
next_subtrees.push(subtree);
}
}
ArielElp
left a comment
There was a problem hiding this comment.
Reviewable status: 10 of 14 files reviewed, 11 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 261 at r9 (raw file):
Previously, yoavGrs wrote…
Would it be a mistake to push the bottom to
next_subtreesin both cases?
Nope, but you're missing out an optimization for facts layout (and reading an unnecessary node from the db)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 273 at r9 (raw file):
Previously, yoavGrs wrote…
Share code with unmodified leaf case in
handle_subtree.
It's not exactly the same, note that this logic needs a subtree and an index. In the edge node case, I only get the bottom subtree in one of the branches. The only way I can think of to share code implies that I'll need to call get_bottom_subtree in both branches (and this function includes non-trivial logic). Hopefully I'm not missing something obvious.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 286 at r9 (raw file):
Previously, yoavGrs wrote…
I prefer the previous structure, but it's personal. Suggestion only, non-blocking:
if !subtree.is_leaf() { next_subtrees.push(subtree); } else { if subtree.is_unmodified() { maybe_traverse_unmodified_chilld() } else if should_fetch_modified_leaves { next_subtrees.push(subtree); } }
IMO the conditions now are clearer, less nested ifs.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 279 at r9 (raw file):
/// Handles a subtree referred by an edge or a binary node. Decides whether we deserialize the /// referred subtree or not. fn handle_subtree<'a, SubTree: SubTreeTrait<'a>>(
Done.
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware reviewed 3 of 4 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArielElp and @yoavGrs)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 70 at r10 (raw file):
while !current_subtrees.is_empty() { let should_fetch_modified_leaves = config.compare_modified_leaves() || previous_leaves.is_some();
unrelated - but since you are here already
Suggestion:
let mut next_subtrees = Vec::new();
let should_fetch_modified_leaves =
config.compare_modified_leaves() || previous_leaves.is_some();
while !current_subtrees.is_empty() {crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 259 at r10 (raw file):
skeleton_tree .nodes .insert(root_index, OriginalSkeletonNode::UnmodifiedSubTree(filled_root.hash));
Suggestion:
{
// Sanity check.
assert!(subtree.is_unmodified(), "Called `handle_unmodified_subtree` for a modified subtree.");
let root_index = subtree.get_root_index();
skeleton_tree
.nodes
.insert(root_index, OriginalSkeletonNode::UnmodifiedSubTree(filled_root.hash));crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 278 at r10 (raw file):
/// Handles a subtree referred by an edge or a binary node. Decides whether we deserialize the /// referred subtree or not.
Suggestion:
/// Handles a subtree referred by an edge or a binary node. Decides whether we deserialize the
/// referred subtree or not and if we should continue traversing the child's direction.
ArielElp
left a comment
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 8 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 128 at r6 (raw file):
Previously, ArielElp wrote…
It's the last part of handle_subtree, I can extract it to a function
This changed with the introduction of handle_umodified_subtree
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 273 at r9 (raw file):
Previously, ArielElp wrote…
It's not exactly the same, note that this logic needs a subtree and an index. In the edge node case, I only get the bottom subtree in one of the branches. The only way I can think of to share code implies that I'll need to call get_bottom_subtree in both branches (and this function includes non-trivial logic). Hopefully I'm not missing something obvious.
Diverged even further, ptal at handle_unmodified_subtree
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 70 at r10 (raw file):
Previously, nimrod-starkware wrote…
unrelated - but since you are here already
Done.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 259 at r10 (raw file):
skeleton_tree .nodes .insert(root_index, OriginalSkeletonNode::UnmodifiedSubTree(filled_root.hash));
Done.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 278 at r10 (raw file):
/// Handles a subtree referred by an edge or a binary node. Decides whether we deserialize the /// referred subtree or not.
Done.
nimrod-starkware
left a comment
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 6 unresolved discussions (waiting on @ArielElp and @yoavGrs)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 262 at r11 (raw file):
if let NodeData::Edge(EdgeData { bottom_data, path_to_bottom }) = filled_root.data { skeleton_tree.nodes.insert(root_index, OriginalSkeletonNode::Edge(path_to_bottom));
please add an explanation why this is special case.
Suggestion:
// For edge nodes, ......
skeleton_tree.nodes.insert(root_index, OriginalSkeletonNode::Edge(path_to_bottom));
ArielElp
left a comment
There was a problem hiding this comment.
Reviewable status: 13 of 14 files reviewed, 6 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs)
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 262 at r11 (raw file):
Previously, nimrod-starkware wrote…
please add an explanation why this is special case.
Done.
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs reviewed 2 files and all commit messages, made 2 comments, and resolved 4 discussions.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @nimrod-starkware).
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 273 at r9 (raw file):
Previously, ArielElp wrote…
Diverged even further, ptal at handle_unmodified_subtree
I missed the meaning of the call to get_bottom_subtree.
I guess this function is non-trivial and involves heavy computation as well.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 262 at r12 (raw file):
if let NodeData::Edge(EdgeData { bottom_data, path_to_bottom }) = filled_root.data { // Even if a subtree rooted at an edge node is umodified, we still need an
Suggestion:
unmodified
yoavGrs
left a comment
There was a problem hiding this comment.
@yoavGrs made 1 comment and resolved 1 discussion.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArielElp and @nimrod-starkware).
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 262 at r12 (raw file):
if let NodeData::Edge(EdgeData { bottom_data, path_to_bottom }) = filled_root.data { // Even if a subtree rooted at an edge node is umodified, we still need an
Edge case :)
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware reviewed all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp).
nimrod-starkware
left a comment
There was a problem hiding this comment.
please open py side
@nimrod-starkware made 1 comment.
Reviewable status: 13 of 14 files reviewed, 2 unresolved discussions (waiting on @ArielElp, @dorimedini-starkware, and @yoavGrs).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
yes, py side please :)
@dorimedini-starkware reviewed 5 files and all commit messages, and made 8 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ArielElp).
crates/starknet_committer/src/db/facts_db/db.rs line 46 at r13 (raw file):
use crate::patricia_merkle_tree::types::CompiledClassHash; pub struct FactsNodeLayout {}
docstring describing the storage scheme in high-level?
or is there a more natural location for this doc?
Code quote:
pub struct FactsNodeLayout {}crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 64 at r13 (raw file):
L: Leaf, Layout: NodeLayout<'a, L>, FilledNode<L, Layout::NodeData>: DBObject<DeserializeContext = Layout::DeserializationContext>,
hmmmm don't you have
impl<L: Leaf, Layout: NodeLayout> DBObject<DeserializeContext = Layout::DeserializationContext> for FilledNode<L, Layout::NodeData>
that prevents the need for this bound?
Code quote:
FilledNode<L, Layout::NodeData>: DBObject<DeserializeContext = Layout::DeserializationContext>crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 70 at r13 (raw file):
let should_fetch_modified_leaves = config.compare_modified_leaves() || previous_leaves.is_some(); while !current_subtrees.is_empty() {
this swap is technically a logic change that should be in a separate PR, because I had to scan to make sure that previous_leaves is not mutated inside the loop. I see it isn't, so ok, but try to keep this in mind when creating PRs
Code quote:
let should_fetch_modified_leaves =
config.compare_modified_leaves() || previous_leaves.is_some();
while !current_subtrees.is_empty() {crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 254 at r13 (raw file):
subtree: SubTree, ) where SubTree::NodeData: Copy,
please be explicit when a copy is needed
Suggestion:
SubTree::NodeData: Clone,crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 282 at r13 (raw file):
.nodes .insert(root_index, OriginalSkeletonNode::UnmodifiedSubTree(filled_root.hash)); }
- please use an explicit
matchon the three options offilled_root.data, instead ofif let .. else - previously, an unmodified leaf just triggered a warning; now, you seem to be pushing it to the skeleton tree. why?
Code quote:
if let NodeData::Edge(EdgeData { bottom_data, path_to_bottom }) = filled_root.data {
// Even if a subtree rooted at an edge node is unmodified, we still need an
// `OriginalSkeletonNode::Edge` node in the skeleton in case we need to manipulate it later
// (e.g. unify the edge node with an ancestor edge node).
skeleton_tree.nodes.insert(root_index, OriginalSkeletonNode::Edge(path_to_bottom));
match SubTree::should_traverse_unmodified_child(bottom_data) {
UnmodifiedChildTraversal::Traverse => {
let (bottom_subtree, _) = subtree.get_bottom_subtree(&path_to_bottom, bottom_data);
next_subtrees.push(bottom_subtree);
}
UnmodifiedChildTraversal::Skip(unmodified_child_hash) => {
skeleton_tree.nodes.insert(
path_to_bottom.bottom_index(root_index),
OriginalSkeletonNode::UnmodifiedSubTree(unmodified_child_hash),
);
}
}
} else {
skeleton_tree
.nodes
.insert(root_index, OriginalSkeletonNode::UnmodifiedSubTree(filled_root.hash));
}crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 295 at r13 (raw file):
) { let is_leaf = subtree.is_leaf(); let is_modified = !subtree.is_unmodified();
are either of these used more than once? seems like a redundant definition
Code quote:
let is_leaf = subtree.is_leaf();
let is_modified = !subtree.is_unmodified();crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 309 at r13 (raw file):
} return; }
this entire block of logic is basically equivalent to the if we had before - please revert the "unrolling" so it's easier to see the diff
Suggestion:
let is_modified = !subtree.is_unmodified();
if !subtree.is_leaf() || (should_fetch_modified_leaves && is_modified) {
next_subtrees.push(subtree);
} else if !is_modified {
match SubTree..
}
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 6 comments and resolved 2 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware).
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 64 at r13 (raw file):
Previously, dorimedini-starkware wrote…
hmmmm don't you have
impl<L: Leaf, Layout: NodeLayout> DBObject<DeserializeContext = Layout::DeserializationContext> for FilledNode<L, Layout::NodeData>that prevents the need for this bound?
There is a constraint on the layout trait, hence using it requires satisfying the constraint here.
FWIW, the FilledTree part of the stack adds an associated NodeDbObject: DbObject to the layout trait, so these requirements may be unnecessary then.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 254 at r13 (raw file):
Previously, dorimedini-starkware wrote…
please be explicit when a copy is needed
WDYM?
Clone indeed allows more freedom, but in practice our types here are () and HashOutput, both copyable. If I require Clone, then bottom_data will need to be cloned in:
match SubTree::should_traverse_unmodified_child(bottom_data.clone()) {
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 282 at r13 (raw file):
Previously, dorimedini-starkware wrote…
- please use an explicit
matchon the three options offilled_root.data, instead ofif let .. else- previously, an unmodified leaf just triggered a warning; now, you seem to be pushing it to the skeleton tree. why?
- Done
- This is related to the last case of handle_child_subtree.
Before: an unmodified leaf was always added to the skeleton upon encountering its parent, it was never traversed itself (hence the "unexpected" warning)
Now: when we traverse unmodified children, we will go through this leaf in the main loop, so it's no longer unexpected.
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 295 at r13 (raw file):
Previously, dorimedini-starkware wrote…
are either of these used more than once? seems like a redundant definition
No, they're there for clarity
is_modified was Yoav's suggestion IIRC to make the control flow more clear. IMO it does make it more clear (no "not unmodified" in the if condition).
is_leaf is less important so removed
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 309 at r13 (raw file):
Previously, dorimedini-starkware wrote…
this entire block of logic is basically equivalent to the
ifwe had before - please revert the "unrolling" so it's easier to see the diff
Slight pushback: I think the control flow is much clearer now
Internal --> traverse
Modified --> if should_fetch_modified_leaves then traverse
Unmodified --> new logic
crates/starknet_committer/src/db/facts_db/db.rs line 46 at r13 (raw file):
Previously, dorimedini-starkware wrote…
docstring describing the storage scheme in high-level?
or is there a more natural location for this doc?
Done, this is probably the most reasonable place
ArielElp
left a comment
There was a problem hiding this comment.
python side successful
https://github.com/starkware-industries/starkware/pull/39782
@ArielElp made 1 comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @dorimedini-starkware).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
WOOP!
@dorimedini-starkware reviewed 2 files and all commit messages, made 3 comments, and resolved 5 discussions.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ArielElp).
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 254 at r13 (raw file):
Previously, ArielElp wrote…
WDYM?
Clone indeed allows more freedom, but in practice our types here are () and HashOutput, both copyable. If I require Clone, then bottom_data will need to be cloned in:
match SubTree::should_traverse_unmodified_child(bottom_data.clone()) {
that's good IMO, it's the same just explicit; when data is copied I'd like to know
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 309 at r13 (raw file):
Previously, ArielElp wrote…
Slight pushback: I think the control flow is much clearer now
Internal --> traverse
Modified --> if should_fetch_modified_leaves then traverse
Unmodified --> new logic
kibalti
ArielElp
left a comment
There was a problem hiding this comment.
@ArielElp made 1 comment.
Reviewable status: 12 of 14 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @yoavGrs).
crates/starknet_committer/src/db/facts_db/create_facts_tree.rs line 254 at r13 (raw file):
Previously, dorimedini-starkware wrote…
that's good IMO, it's the same just explicit; when data is copied I'd like to know
Done, note that I also changed the constraint in the layout so there are a couple more clones in this file.
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 2 files and all commit messages, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
nimrod-starkware
left a comment
There was a problem hiding this comment.
@nimrod-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
dorimedini-starkware
left a comment
There was a problem hiding this comment.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).

No description provided.