-
Notifications
You must be signed in to change notification settings - Fork 65
starknet_committer,starknet_patricia: add serialization utilities to node layout #10987
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
starknet_committer,starknet_patricia: add serialization utilities to node layout #10987
Conversation
af9d87d to
fb3bd7a
Compare
bb2af59 to
32b7f4a
Compare
3473676 to
9f2ba58
Compare
32b7f4a to
597a809
Compare
9f2ba58 to
3d47fa4
Compare
597a809 to
4487c12
Compare
3d47fa4 to
84d3218
Compare
dorimedini-starkware
left a comment
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.
@dorimedini-starkware reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp and @yoavGrs).
crates/starknet_committer/src/db/facts_db/db.rs line 57 at r5 (raw file):
let db_filled_node = Self::convert_node_data_and_leaf(filled_node); let key = db_filled_node.get_db_key(key_context, &db_filled_node.hash.0.to_bytes_be());
consider implementing a db_key_suffix method on FilledNode type(s),
and use it here and in the below implementation.
in this case the node index will be ignored of course but in general the DB key suffix can depend on it
Suggestion:
db_filled_node.get_db_key(key_context, &db_filled_node.db_key_suffix(node_index));c6387bb to
3101833
Compare
28d5db7 to
c213f17
Compare
dorimedini-starkware
left a comment
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.
@dorimedini-starkware reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ArielElp and @yoavGrs).
3101833 to
d6d529c
Compare
ArielElp
left a comment
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.
@ArielElp made 2 comments.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware, @nimrod-starkware, and @yoavGrs).
crates/starknet_committer/src/db/facts_db/db.rs line 57 at r5 (raw file):
Previously, dorimedini-starkware wrote…
consider implementing a
db_key_suffixmethod onFilledNodetype(s),
and use it here and in the below implementation.
in this case the node index will be ignored of course but in general the DB key suffix can depend on it
There is suffix on FilledNode<L, HashOutput> that is actually only used in tests ATM, I think I'll refactor this at the top of the stack (originally I thought of removing suffix altogether, but maybe your suggestion is better)
crates/starknet_patricia/src/db_layout.rs line 79 at r4 (raw file):
Previously, nimrod-starkware wrote…
So you convert
HashOutputto()in index layout and in facts layout keep as is?
yes (the into call on path_to_bottom was indeed superfluous, removed)
dorimedini-starkware
left a comment
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.
@dorimedini-starkware reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @nimrod-starkware and @yoavGrs).
crates/starknet_committer/src/db/facts_db/db.rs line 57 at r5 (raw file):
Previously, ArielElp wrote…
There is suffix on FilledNode<L, HashOutput> that is actually only used in tests ATM, I think I'll refactor this at the top of the stack (originally I thought of removing
suffixaltogether, but maybe your suggestion is better)
ok, so can you add TODOs here and below to use the suffix method?
or, if the PR is already open, let me TAL and I'll "unblock" here
nimrod-starkware
left a comment
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.
@nimrod-starkware reviewed all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
d6d529c to
dd42241
Compare
dorimedini-starkware
left a comment
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.
@dorimedini-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @yoavGrs).
dd42241 to
b2e1b62
Compare
1fc6089 to
9ca9928
Compare
b2e1b62 to
0a4c103
Compare
0a4c103 to
c82e3fc
Compare
9ca9928 to
26571b0
Compare
yoavGrs
left a comment
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.
@yoavGrs resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
yoavGrs
left a comment
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.
@yoavGrs reviewed 2 files and all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ArielElp).
1d254b3

No description provided.