-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier: add estimate_casm_blake_hash_computation_resources #7212
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
blockifier: add estimate_casm_blake_hash_computation_resources #7212
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
meship-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.
Reviewed 1 of 2 files at r1, all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @noaov1)
crates/blockifier/src/execution/contract_class.rs line 363 at r1 (raw file):
// Hash all (segment_hash, segment_length) pairs. resources += &cost_of_encode_felt252_data_and_calc_blake_hash(segs.len(), segs.len());
How do we know that n_small_felts = n_big_felts = segs.len()?
Code quote:
resources += &cost_of_encode_felt252_data_and_calc_blake_hash(segs.len(), segs.len());crates/blockifier/src/execution/contract_class.rs line 379 at r1 (raw file):
resources += &leaf_cost(*len); } _ => panic!("Estimating hash cost only supports at most one level of segmentation."),
Why? Does this mean we can only have leaves or a list of leaves?
Suggestion:
_ => panic!("Estimating hash cost only supports at most one level of segmentation."), crates/blockifier/src/execution/contract_class.rs line 389 at r1 (raw file):
/// - TODO(AvivG): Currently ignores entry-point costs. /// - Uses only bytecode size (treats all felts as “big”, ignores the small-felt optimization). /// - This estimation was done by...
Is this comment supposed to be completed?
Code quote:
/// - This estimation was done by...c1913e1 to
1634654
Compare
1d6deef to
f436353
Compare
avivg-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.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs line 363 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
How do we know that n_small_felts = n_big_felts = segs.len()?
for each node, another "segment" that is being hashed is the sequence of felts containing all it's inner segments hashes and their lengths. hash=>"big" felt , length=>"small" felt
crates/blockifier/src/execution/contract_class.rs line 379 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Why? Does this mean we can only have leaves or a list of leaves?
Yes, exactly, this is the current logic in the OS as well. You can see that this structure was copied from the original function estimate_casm_poseidon_hash_computation_resources
And I'm happy to elaborate verbally if you'd like
crates/blockifier/src/execution/contract_class.rs line 389 at r1 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Is this comment supposed to be completed?
As the process is not complete, I wasn't sure what to write (but thought it might be helpful for the reader), removed for now
f436353 to
38cc0d5
Compare
1634654 to
0212d0f
Compare
38cc0d5 to
b7fe029
Compare
0212d0f to
f6f14cc
Compare
b7fe029 to
45c70c9
Compare
meship-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.
Reviewed 1 of 2 files at r2, all commit messages.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @noaov1)
45c70c9 to
b049eba
Compare
f6f14cc to
99054ea
Compare
b049eba to
9a5d2e4
Compare
99054ea to
6b768d9
Compare
9a5d2e4 to
b298bb8
Compare
19f6fc3 to
4f016ce
Compare
4eec2e8 to
19b9d38
Compare
4f016ce to
b9a5e39
Compare
19b9d38 to
b0bee6d
Compare
b9a5e39 to
3a0ba78
Compare
b0bee6d to
8c150fc
Compare
3a0ba78 to
5cd1cc9
Compare
8c150fc to
8297fc3
Compare
5cd1cc9 to
abf1d51
Compare
9a75d2e to
a3e8adc
Compare
abf1d51 to
fa7e7c6
Compare
a3e8adc to
a6c4d79
Compare
fa7e7c6 to
6ae455c
Compare
a6c4d79 to
4817f11
Compare
meship-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.
Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, all discussions resolved (waiting on @noaov1)
meship-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.
Reviewed 1 of 2 files at r4.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @noaov1)
crates/blockifier/src/execution/contract_class.rs line 388 at r5 (raw file):
let segment_overhead = GasAmount::ZERO; // 2) For each segment, hash its felts.
Why 2) should there be 1? I'm also not sure I understand the meaning of this comment
Suggestion:
// For each segment, hash its feel.crates/blockifier/src/execution/contract_class.rs line 403 at r5 (raw file):
let node_hash_cost = cost_of_encode_felt252_data_and_calc_blake_hash( segs.len(), segs.len(),
This is an estimation of the worst case, right? If I understand correctly, n_small_felts + n_big_felts = segs.len()
Is there a way to give a stricter bound? Anyway, this is outside of this PR's scope
Code quote:
segs.len(),
segs.len(),
meship-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.
Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs line 426 at r5 (raw file):
n_memory_holes: 0, builtin_instance_counter: HashMap::from([(BuiltinName::range_check, 3)]), };
Out of curiosity, where did these numbers come from?
Code quote:
let resources = ExecutionResources {
n_steps: 0,
n_memory_holes: 0,
builtin_instance_counter: HashMap::from([(BuiltinName::range_check, 3)]),
};
meship-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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs line 388 at r5 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Why 2) should there be 1? I'm also not sure I understand the meaning of this comment
// For each segment, hash its felts. Sorry auto corrector
4817f11 to
2e5d788
Compare
6ae455c to
a34d4c3
Compare
2e5d788 to
ed487bd
Compare
avivg-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.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs line 403 at r5 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
This is an estimation of the worst case, right? If I understand correctly, n_small_felts + n_big_felts = segs.len()
Is there a way to give a stricter bound? Anyway, this is outside of this PR's scope
Actually, this is an exact estimation :)
We hash both the hash and the length of each inner segment — that is, one big felt (the hash) and one small felt (the length) per segment. So for segs.len() segments, we’re hashing exactly segs.len() big felts and segs.len() small felts.
That’s why passing segs.len(), segs.len() here reflects the exact number of big and small felts involved.
ed487bd to
392a9b7
Compare
avivg-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.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/contract_class.rs line 426 at r5 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
Out of curiosity, where did these numbers come from?
The value 3 is based on observed behavior from manually running the computation.
I changed the TODOs , to revisit this when compiled_class_hash estimation is complete
meship-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.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @noaov1)
crates/blockifier/src/execution/contract_class.rs line 403 at r5 (raw file):
Previously, avivg-starkware wrote…
Actually, this is an exact estimation :)
We hash both the hash and the length of each inner segment — that is, one big felt (the hash) and one small felt (the length) per segment. So forsegs.len()segments, we’re hashing exactlysegs.len()big felts andsegs.len()small felts.
That’s why passingsegs.len(), segs.len()here reflects the exact number of big and small felts involved.
I see that makes sense thanks for the explanetion :)
meship-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.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noaov1)

No description provided.