-
Notifications
You must be signed in to change notification settings - Fork 65
blockifier: add fn encode_felt252_data_and_calc_blake_hash_cost #7147
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 fn encode_felt252_data_and_calc_blake_hash_cost #7147
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
99986ea to
6448f52
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.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @noaov1)
crates/blockifier/src/execution/execution_utils.rs line 383 at r2 (raw file):
} fn compute_blake_hash_steps(n_big: usize, n_small: usize) -> usize {
For consistency, and I think this is clearer.
Suggestion:
n_big_felts: usize, n_small_felts: usize)
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 r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @avivg-starkware and @noaov1)
noaov1
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 r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @avivg-starkware)
crates/blockifier/src/execution/execution_utils.rs line 367 at r2 (raw file):
} mod blake_cost {
It is an interesting approach using the mod. Can you please explain why you chose it?
Code quote:
mod blake_cost {crates/blockifier/src/execution/execution_utils.rs line 371 at r2 (raw file):
pub const U32S_MESSAGE: usize = 16; pub const U32S_BIG_FELT: usize = 8; pub const U32S_SMALL_FELT: usize = 2;
Suggestion:
pub const N_U32S_MESSAGE: usize = 16;
pub const N_U32S_BIG_FELT: usize = 8;
pub const N_U32S_SMALL_FELT: usize = 2;crates/blockifier/src/execution/execution_utils.rs line 387 at r2 (raw file):
let rem_u32s = total_u32s % blake_cost::U32S_MESSAGE; let base = if rem_u32s == 0 { blake_cost::BASE_STEPS_FULL_MSG
No? Isn't it affected by the number of messages?
Same question applies for the else.
Suggestion:
blake_cost::BASE_STEPS_FULL_MSG * total_u32s / 16crates/blockifier/src/execution/execution_utils.rs line 403 at r2 (raw file):
let n_steps = compute_blake_hash_steps(n_big_felts, n_small_felts); let n_felts = n_big_felts + n_small_felts; let builtins = HashMap::from([(BuiltinName::range_check, n_felts)]);
Please add a doc string.
Suggestion:
// Something like: We need to know the size of each felt...
let builtins = HashMap::from([(BuiltinName::range_check, n_felts)]);6448f52 to
c1913e1
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: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/execution_utils.rs line 367 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
It is an interesting approach using the
mod. Can you please explain why you chose it?
I didn't want to make the constants' names much longer, but I still wanted to include information about their relationship to the Blake hashing cost. Do you think it's better to use the other approach mentioned here or something else?
crates/blockifier/src/execution/execution_utils.rs line 387 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
No? Isn't it affected by the number of messages?
Same question applies for theelse.
This is a one-time addition per run. This base value is different depending on whether the last massage is full or partial
crates/blockifier/src/execution/execution_utils.rs line 403 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Please add a doc string.
WDYT?
noaov1
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 r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @avivg-starkware)
crates/blockifier/src/execution/execution_utils.rs line 387 at r2 (raw file):
Previously, avivg-starkware wrote…
This is a one-time addition per run. This base value is different depending on whether the last massage is full or partial
I see. Is it the base cost of the blake calculation?
crates/blockifier/src/execution/execution_utils.rs line 375 at r3 (raw file):
// Steps counts pub const STEPS_BIG_FELT: usize = 45; pub const STEPS_SMALL_FELT: usize = 15;
Are those the steps of the encoding or the blake or both?
Please document
Code quote:
// Steps counts
pub const STEPS_BIG_FELT: usize = 45;
pub const STEPS_SMALL_FELT: usize = 15;
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: all files reviewed, 2 unresolved discussions (waiting on @noaov1)
crates/blockifier/src/execution/execution_utils.rs line 387 at r2 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I see. Is it the base cost of the blake calculation?
Yes. Though strangely, this base cost is different (and ends up being lower) if the last message is partial
crates/blockifier/src/execution/execution_utils.rs line 375 at r3 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Are those the steps of the encoding or the blake or both?
Please document
Oh! The Blake opcode count is missing here! thank you.
This is one per each full/partial message. I'll add it.
0212d0f to
f6f14cc
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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/utils.rs line 91 at r4 (raw file):
} pub fn safe_add_gas_panic_on_overflow(gas: GasAmount, added_gas: GasAmount) -> GasAmount {
This will be deleted when #7242 is merged
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 r3, 2 of 3 files at r4, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware and @noaov1)
crates/blockifier/src/execution/execution_utils.rs line 375 at r3 (raw file):
Previously, avivg-starkware wrote…
Oh! The Blake opcode count is missing here! thank you.
This is one per each full/partial message. I'll add it.
I'm still not sure I understand the partition. If we only computed Blake without the compression, would the opcode calculation be sufficient?
noaov1
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 3 of 3 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @avivg-starkware and @meship-starkware)
crates/blockifier/src/execution/execution_utils.rs line 404 at r4 (raw file):
fn count_blake_opcode(n_big_felts: usize, n_small_felts: usize) -> usize { // The BLAKE opcode is used once per 16 u32s.
Suggestion:
// The BLAKE opcode is used once per message.crates/blockifier/src/execution/execution_utils.rs line 409 at r4 (raw file):
let mut n_msgs = total_u32s / blake_cost::N_U32S_MESSAGE; n_msgs += if total_u32s % blake_cost::N_U32S_MESSAGE > 0 { 1 } else { 0 };
Is it possible to use div_ceil?
Code quote:
let mut n_msgs = total_u32s / blake_cost::N_U32S_MESSAGE;
n_msgs += if total_u32s % blake_cost::N_U32S_MESSAGE > 0 { 1 } else { 0 };f6f14cc to
99054ea
Compare
a91a3e8 to
736e0f3
Compare
99054ea to
6b768d9
Compare
6b768d9 to
19f6fc3
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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/execution_utils.rs line 375 at r3 (raw file):
Previously, meship-starkware (Meshi Peled) wrote…
I'm still not sure I understand the partition. If we only computed Blake without the compression, would the opcode calculation be sufficient?
The steps are 'the overhead' of running the funcs (in addition to the opcode) without compression, we'll address all feels as big - meaning 45 steps.
I calculated this by running the blake2s function with different inputs
crates/blockifier/src/execution/execution_utils.rs line 409 at r4 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Is it possible to use
div_ceil?
see change
crates/blockifier/src/execution/execution_utils.rs line 404 at r4 (raw file):
fn count_blake_opcode(n_big_felts: usize, n_small_felts: usize) -> usize { // The BLAKE opcode is used once per 16 u32s.
see change (also in func's doc)
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.
move to main
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (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.
Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @noaov1)
a discussion (no related file):
move to main
19f6fc3 to
4f016ce
Compare
4f016ce to
b9a5e39
Compare
c9a5efc to
5bc3098
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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1)
a discussion (no related file):
Previously, meship-starkware (Meshi Peled) wrote…
move to main
Done.
noaov1
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 2 of 3 files at r6, all commit messages.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @meship-starkware)
crates/blockifier/src/utils.rs line 110 at r6 (raw file):
} pub fn add_gas(gas: &mut GasAmount, added_gas: GasAmount) {
With checked_add_panic_on_overflow (courtesy of @meship-starkware), this method is no longer needed and can be removed
Code quote:
pub fn add_gas(gas: &mut GasAmount, added_gas: GasAmount) {
noaov1
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 3 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @avivg-starkware and @meship-starkware)
crates/blockifier/src/execution/execution_utils.rs line 397 at r6 (raw file):
blake_cost::BASE_STEPS_FULL_MSG } else { blake_cost::BASE_STEPS_PARTIAL_MSG + blake_cost::STEPS_PER_2_U32_REMINDER * (rem_u32s / 2)
Can you please explain this?
Code quote:
blake_cost::STEPS_PER_2_U32_REMINDER * (rem_u32s / 2)crates/blockifier/src/execution/execution_utils.rs line 419 at r6 (raw file):
/// Estimates the VM resources for `encode_felt252_data_and_calc_blake_hash` in the Starknet OS. /// Assumes small felts unpack into 2 u32s and big felts into 8 u32s.
Why?
Code quote:
/// Assumes small felts unpack into 2 u32s and big felts into 8 u32s.crates/blockifier/src/execution/execution_utils.rs line 420 at r6 (raw file):
/// Estimates the VM resources for `encode_felt252_data_and_calc_blake_hash` in the Starknet OS. /// Assumes small felts unpack into 2 u32s and big felts into 8 u32s. pub fn cost_of_encode_felt252_data_and_calc_blake_hash<F>(
Is it? Isn't it the cost of blake only? (without the encoding)
Code quote:
pub fn cost_of_encode_felt252_data_and_calc_blake_hash<F>(5cd1cc9 to
abf1d51
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: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/utils.rs line 110 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
With
checked_add_panic_on_overflow(courtesy of @meship-starkware), this method is no longer needed and can be removed
Done.
crates/blockifier/src/execution/execution_utils.rs line 397 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Can you please explain this?
It is based on manual calculations of running blake2s with different inputs.
Added a comment. looks Ok?
crates/blockifier/src/execution/execution_utils.rs line 419 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why?
This is the logic of the encode_felt252_data_and_calc_blake_hash function. Is the comment clearer now?
crates/blockifier/src/execution/execution_utils.rs line 420 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Is it? Isn't it the cost of blake only? (without the encoding)
Yes indeed :) It is evaluating encode_felt252_data_and_calc_blake_hash which will be called by func compiled_class_hash
abf1d51 to
fa7e7c6
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 r8, all commit messages.
Reviewable status: 1 of 3 files reviewed, 4 unresolved discussions (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 r8.
Reviewable status: 2 of 3 files reviewed, 4 unresolved discussions (waiting on @noaov1)
noaov1
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 2 of 2 files at r8, all commit messages.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @avivg-starkware)
crates/blockifier/src/execution/execution_utils.rs line 397 at r6 (raw file):
Previously, avivg-starkware wrote…
It is based on manual calculations of running blake2s with different inputs.
Added a comment. looks Ok?
Out of curiosity: why rem/2?
Is it because 3 % 2 != 0?
crates/blockifier/src/execution/execution_utils.rs line 419 at r6 (raw file):
Previously, avivg-starkware wrote…
This is the logic of the
encode_felt252_data_and_calc_blake_hashfunction. Is the comment clearer now?
Why is this assumption relevant to this method?
crates/blockifier/src/execution/execution_utils.rs line 420 at r6 (raw file):
Previously, avivg-starkware wrote…
Yes indeed :) It is evaluating
encode_felt252_data_and_calc_blake_hashwhich will be called byfunc compiled_class_hash
Where do you evaluate the encode steps?
fa7e7c6 to
6ae455c
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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @meship-starkware and @noaov1)
crates/blockifier/src/execution/execution_utils.rs line 397 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Out of curiosity: why
rem/2?
Is it because3 % 2 != 0?
added a clarifying comment input is always even.
crates/blockifier/src/execution/execution_utils.rs line 419 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Why is this assumption relevant to this method?
It's relevant to the evaluated method and thus to this one as well:)
crates/blockifier/src/execution/execution_utils.rs line 420 at r6 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Where do you evaluate the encode steps?
this evaluate encode_felt252_data_and_calc_blake_hash which calls both encode_felt252_to_u32s and blake_with_opcode.
Added a TODO to consider splitting the evaluation
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 3 files at r7, 2 of 2 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @noaov1)
noaov1
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 r9, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @avivg-starkware)

No description provided.