-
Notifications
You must be signed in to change notification settings - Fork 193
Add_hint_for_splitting_data_for_blake_with_no_encoding #2137
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
base: starkware-development
Are you sure you want to change the base?
Add_hint_for_splitting_data_for_blake_with_no_encoding #2137
Conversation
|
Benchmark Results for unmodified programs 🚀
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## starkware-development #2137 +/- ##
=========================================================
- Coverage 96.63% 96.63% -0.01%
=========================================================
Files 104 104
Lines 43903 43959 +56
=========================================================
+ Hits 42427 42481 +54
- Misses 1476 1478 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
f292846
to
31679ec
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.
Reviewed 2 of 3 files at r1, all commit messages.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @yuvalsw)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 283 at r2 (raw file):
offset += val_len */ pub fn blake2s_unpack_felts(
add a comment stating the used endianess for this function and the next one.
Code quote:
pub fn blake2s_unpack_felts(
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 305 at r2 (raw file):
if val < pow2_63 { let (high, low) = val.div_rem(&pow2_32); vec![low, high]
why do we need to change this hint to little endian as well?
who are its users?
Code quote:
vec![low, high]
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 335 at r2 (raw file):
offset += val_len */ pub fn blake2s_split_felts_to_u32s(
any reason not to share code between blake2s_split_felts_to_u32s() and blake2s_unpack_felts(), there is some code duplication
Code quote:
pub fn blake2s_split_felts_to_u32s(
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 348 at r2 (raw file):
let pow2_32 = BigUint::from(1_u32) << 32; // Split value into either 2 or 8 32-bit limbs.
Suggestion:
// Split value into 8 32-bit limbs.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 832 at r2 (raw file):
#[test] #[cfg_attr(target_arch = "wasm32", wasm_bindgen_test)] fn blake2s_split_felts_to_u32s() {
code sharing with the previous test would make the code more compact
Code quote:
n blake2s_split_felts_to_u32s() {
31679ec
to
ea1ca39
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.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @Yael-Starkware and @yuvalsw)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 283 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
add a comment stating the used endianess for this function and the next one.
Done.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 305 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
why do we need to change this hint to little endian as well?
who are its users?
It's used by the bl to hash the program when we use blake. No reason to leave it in BE order. It's Better to have this consistent where possible.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 335 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
any reason not to share code between blake2s_split_felts_to_u32s() and blake2s_unpack_felts(), there is some code duplication
Done.
It's a bit cleaner separated imo, but if you prefer it, I don't really mind.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 832 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
code sharing with the previous test would make the code more compact
Done.
Most of the tests in this repo follow a similar setup. This approach is good for unit tests, imo (better to be explicit), but again, I don't really mind if you prefer it.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 348 at r2 (raw file):
let pow2_32 = BigUint::from(1_u32) << 32; // Split value into either 2 or 8 32-bit limbs.
Done.
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 r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @YairVaknin-starkware and @yuvalsw)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 283 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
Done.
I see it inside one of the match arms, but it true for the other arms as well so please move to a more central place.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 305 at r2 (raw file):
Previously, YairVaknin-starkware wrote…
It's used by the bl to hash the program when we use blake. No reason to leave it in BE order. It's Better to have this consistent where possible.
ok, makes sense,
no external users for this ?
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 813 at r3 (raw file):
0x123456781234u128 as i128, 0x1234abcd5678efab1234abcd, );
can the function get u128 to avoid the as i128 conversion?
Code quote:
let (mut vm, ids_data) = prepare_vm_for_splitting_felts_for_blake(
0x123456781234u128 as i128,
0x1234abcd5678efab1234abcd,
);
ea1ca39
to
f2412d9
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Yael-Starkware and @yuvalsw)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 283 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
I see it inside one of the match arms, but it true for the other arms as well so please move to a more central place.
Done.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 305 at r2 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
ok, makes sense,
no external users for this ?
I think starkent. I helped them add support for hashing empty arrays yesterday, so I'll continue the discussion and communicate this to them.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 813 at r3 (raw file):
Previously, Yael-Starkware (YaelD) wrote…
can the function get u128 to avoid the as i128 conversion?
The underling code from the macro converts them to i128, so I prefer the compiler yells at me here, then getting a runtime error. But the cast is anyway unneeded. This was from a previous change that I didn't revert (note that it has a suffix).
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: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @yuvalsw)
f2412d9
to
131ab72
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.
Reviewed 2 of 3 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @Yael-Starkware)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 272 at r5 (raw file):
pub enum BlakeEncodingMode { /// 2 limbs if val < 2⁶³, otherwise 8 limbs after adding 2^255.
is this some weird unicode? better avoid it.
Code quote:
al < 2⁶³,
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 275 at r5 (raw file):
UseEncoding, /// Always 8 limbs. NoEncoding,
suggest to reverse the order
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 314 at r5 (raw file):
let pow2_32 = BigUint::from(1_u32) << 32; let pow2_63 = BigUint::from(1_u32) << 63; let pow2_255 = BigUint::from(1_u32) << 255;
these are only required for one of the cases.
With helper functions like I suggested below, there might not remain justification for having both the cases in the same function. This will also be more consistent with the other hints, each implemented in a single function.
Need to check how it's gonna look, so not sure yet, but please consider.
Code quote:
let pow2_63 = BigUint::from(1_u32) << 63;
let pow2_255 = BigUint::from(1_u32) << 255;
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 329 at r5 (raw file):
val = q; } limbs
this logic repeats twice
Code quote:
for limb in &mut limbs {
let (q, r) = val.div_rem(&pow2_32);
*limb = r;
val = q;
}
limbs
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 340 at r5 (raw file):
val += &pow2_255; let mut limbs = vec![BigUint::from(0_u32); 8]; for limb in &mut limbs {
why not limbs.iter_mut()?
Code quote:
in &mut limbs {
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 348 at r5 (raw file):
} } })
please split to 2 helper functions. This is too complicated a statement.
Code quote:
.flat_map(|mut val| match mode {
BlakeEncodingMode::NoEncoding => {
// Always 8 limbs.
let mut limbs = vec![BigUint::from(0_u32); 8];
for limb in &mut limbs {
let (q, r) = val.div_rem(&pow2_32);
*limb = r;
val = q;
}
limbs
}
BlakeEncodingMode::UseEncoding => {
if val < pow2_63 {
// 2 limbs: low, high.
let (high, low) = val.div_rem(&pow2_32);
vec![low, high]
} else {
// 8 limbs after adding 2**255.
val += &pow2_255;
let mut limbs = vec![BigUint::from(0_u32); 8];
for limb in &mut limbs {
let (q, r) = val.div_rem(&pow2_32);
*limb = r;
val = q;
}
limbs
}
}
})
131ab72
to
0c26a3f
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.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Yael-Starkware and @yuvalsw)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 340 at r5 (raw file):
Previously, yuvalsw wrote…
why not limbs.iter_mut()?
does the same and reads better. no need for any method iter_mut exposes now.
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 348 at r5 (raw file):
Previously, yuvalsw wrote…
please split to 2 helper functions. This is too complicated a statement.
Done.
Previously, yuvalsw wrote…
You resolved the comment but I don't see the resolution/response. |
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 r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)
vm/src/hint_processor/builtin_hint_processor/blake2s_utils.rs
line 271 at r6 (raw file):
} pub enum BlakeEncodingMode {
please also doc the enum
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 @Yael-Starkware and @YairVaknin-starkware)
0c26a3f
to
f8aa071
Compare
f8aa071
to
2a407bc
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.
Reviewed 1 of 2 files at r6, 1 of 1 files at r7, 2 of 2 files at r8, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @YairVaknin-starkware)
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:complete! all files reviewed, all discussions resolved (waiting on @YairVaknin-starkware)
TITLE
Description
Description of the pull request changes and motivation.
Checklist
This change is