-
Notifications
You must be signed in to change notification settings - Fork 66
feat: blake2s #131
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
feat: blake2s #131
Conversation
561b702 to
5569830
Compare
17c6ec5 to
f0b073c
Compare
AvivYossef-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 4 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware)
ensure_no_std/Cargo.toml line 13 at r1 (raw file):
"parity-scale-codec", "num-traits", "hash",
blake2 uses std ...
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 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 9 unresolved discussions (waiting on @dan-starkware and @dorimedini-starkware)
crates/starknet-types-core/src/hash/blake2s.rs line 14 at r1 (raw file):
impl StarkHash for Blake2s { fn hash(felt_0: &Felt, felt_1: &Felt) -> Felt { Self::encode_felt252_data_and_calc_224_bit_blake_hash(&[*felt_0, *felt_1])
Consider using hash_array.
Same below.
Code quote:
Self::encode_felt252_data_and_calc_224_bit_blake_hash(&[*felt_0, *felt_1])crates/starknet-types-core/src/hash/blake2s.rs line 34 at r1 (raw file):
/// /// # Returns /// A flat `Vec<u32>` containing all the unpacked words, in the same order.
Suggestion:
// Encode each `Felt` into 32-bit words:
/// - **Small** values `< 2^63` gets **2** words: `[ high_32_bits, low_32_bits ]` from the last 8
/// bytes of the 256-bit BE representation.
/// - **Large** values `>= 2^63` get **8** words: the full 32-byte big-endian split, **with** the
/// MSB of the first word set as a marker (`+2^255`).
///
/// # Returns
/// A flat `Vec<u32>` containing all the unpacked words, in the same order.crates/starknet-types-core/src/hash/blake2s.rs line 35 at r1 (raw file):
/// # Returns /// A flat `Vec<u32>` containing all the unpacked words, in the same order. pub fn encode_felts_to_u32s(felts: Vec<Felt>) -> Vec<u32> {
Suggestion:
pub fn encode_felts_to_u32s(felts: &[Felt]) -> Vec<u32> {crates/starknet-types-core/src/hash/blake2s.rs line 41 at r1 (raw file):
const BIG_MARKER: u32 = 1 << 31; let mut unpacked_u32s = Vec::new();
What does unpacked stand for?
Code quote:
let mut unpacked_u32s = Vec::new();crates/starknet-types-core/src/hash/blake2s.rs line 45 at r1 (raw file):
let felt_as_be_bytes = felt.to_bytes_be(); if felt < SMALL_THRESHOLD { // small: 2 limbs only, high‐32 then low‐32 of the last 8 bytes
Suggestion:
// small: 2 limbs only, extracts the LSB.crates/starknet-types-core/src/hash/blake2s.rs line 47 at r1 (raw file):
// small: 2 limbs only, high‐32 then low‐32 of the last 8 bytes let hi = u32::from_be_bytes(felt_as_be_bytes[24..28].try_into().unwrap()); let lo = u32::from_be_bytes(felt_as_be_bytes[28..32].try_into().unwrap());
Suggestion:
let high = u32::from_be_bytes(felt_as_be_bytes[24..28].try_into().unwrap());
let low = u32::from_be_bytes(felt_as_be_bytes[28..32].try_into().unwrap());crates/starknet-types-core/src/hash/blake2s.rs line 53 at r1 (raw file):
// big: 8 limbs, big‐endian order let start = unpacked_u32s.len(); for chunk in felt_as_be_bytes.chunks_exact(4) {
Consider defining it as a const.
Code quote:
4crates/starknet-types-core/src/hash/blake2s.rs line 87 at r1 (raw file):
/// with Blake2s-256 and returns the 224-bit truncated digest as a `Felt`. pub fn encode_felt252_data_and_calc_224_bit_blake_hash(data: &[Felt]) -> Felt { // 1) Unpack each Felt into 2 or 8 u32 limbs
?
Code quote:
limbsf0b073c to
8221b3b
Compare
AvivYossef-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: 3 of 4 files reviewed, 5 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, and @noaov1)
crates/starknet-types-core/src/hash/blake2s.rs line 14 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
Consider using
hash_array.
Same below.
Done
crates/starknet-types-core/src/hash/blake2s.rs line 41 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
What does
unpackedstand for?
they use this word in the cairo function
I can change it to encoded_u32s
crates/starknet-types-core/src/hash/blake2s.rs line 34 at r1 (raw file):
/// /// # Returns /// A flat `Vec<u32>` containing all the unpacked words, in the same order.
Why? "values" is plural.
crates/starknet-types-core/src/hash/blake2s.rs line 35 at r1 (raw file):
/// # Returns /// A flat `Vec<u32>` containing all the unpacked words, in the same order. pub fn encode_felts_to_u32s(felts: Vec<Felt>) -> Vec<u32> {
Why not take ownership?
it's like into
crates/starknet-types-core/src/hash/blake2s.rs line 45 at r1 (raw file):
let felt_as_be_bytes = felt.to_bytes_be(); if felt < SMALL_THRESHOLD { // small: 2 limbs only, high‐32 then low‐32 of the last 8 bytes
WTYT?
Yoni-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: 3 of 4 files reviewed, 7 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, and @noaov1)
crates/starknet-types-core/src/hash/blake2s.rs line 10 at r2 (raw file):
use super::traits::StarkHash; pub struct Blake2s;
Rename to reflect the fact that it is not a standard impl of blake.
Code quote:
pub struct Blake2s;crates/starknet-types-core/src/hash/blake2s.rs line 27 at r2 (raw file):
impl Blake2s { // Encode each `Felt` into 32-bit words:
Suggestion:
/ Encodes eac8221b3b to
d267ae4
Compare
AvivYossef-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: 3 of 4 files reviewed, 7 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @noaov1, and @Yoni-Starkware)
crates/starknet-types-core/src/hash/blake2s.rs line 10 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Rename to reflect the fact that it is not a standard impl of blake.
WTYT?
crates/starknet-types-core/src/hash/blake2s.rs line 27 at r2 (raw file):
impl Blake2s { // Encode each `Felt` into 32-bit words:
Done.
Yoni-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: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @AvivYossef-starkware, @dan-starkware, @dorimedini-starkware, and @noaov1)
crates/starknet-types-core/src/hash/blake2s.rs line 10 at r2 (raw file):
Previously, AvivYossef-starkware wrote…
WTYT?
Blake2Felt252 for consistency with Blake2s256 :)
d267ae4 to
14639bc
Compare
AvivYossef-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: 3 of 4 files reviewed, 6 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @noaov1, and @Yoni-Starkware)
crates/starknet-types-core/src/hash/blake2s.rs line 10 at r2 (raw file):
Previously, Yoni-Starkware (Yoni) wrote…
Blake2Felt252 for consistency with Blake2s256 :)
Done.
|
@AvivYossef-starkware @Yoni-Starkware is it ready to be reviewed and merged? |
14639bc to
8a9210b
Compare
AvivYossef-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.
Hi, I still have some tests to complete before the merge, but it’s ready for review.
Reviewable status: 3 of 4 files reviewed, 8 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @noaov1, @tdelabro, and @Yoni-Starkware)
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 r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @AvivYossef-starkware, @dan-starkware, @dorimedini-starkware, @tdelabro, and @Yoni-Starkware)
crates/starknet-types-core/src/hash/blake2s.rs line 35 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
Why not take ownership?
it's likeinto
The input vector is read-only and not reused, mutated, or moved elsewhere — so there's no reason to take ownership.
(non-blocking)
crates/starknet-types-core/src/hash/blake2s.rs line 41 at r1 (raw file):
Previously, AvivYossef-starkware wrote…
they use this word in the cairo function
I can change it toencoded_u32s
I think that encoded_u32s is better (non-blocking)
crates/starknet-types-core/src/hash/blake2s.rs line 103 at r5 (raw file):
let mut hasher = Blake2s256::new(); hasher.update(data); let hash32 = hasher.finalize();
How do you know that hash32 contains at least 7 words?
Code quote:
let hash32 = hasher.finalize();
AvivYossef-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, 7 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @noaov1, @tdelabro, and @Yoni-Starkware)
crates/starknet-types-core/src/hash/blake2s.rs line 35 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
The input vector is read-only and not reused, mutated, or moved elsewhere — so there's no reason to take ownership.
(non-blocking)
sababa
crates/starknet-types-core/src/hash/blake2s.rs line 41 at r1 (raw file):
Previously, noaov1 (Noa Oved) wrote…
I think that
encoded_u32sis better (non-blocking)
I prefer to stick with the Cairo impl
crates/starknet-types-core/src/hash/blake2s.rs line 103 at r5 (raw file):
Previously, noaov1 (Noa Oved) wrote…
How do you know that
hash32contains at least 7 words?
Blake2s256 returns 256 bits
8a9210b to
5c6a42c
Compare
Yoni-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: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @dan-starkware, @dorimedini-starkware, @noaov1, and @tdelabro)
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.
Reviewed 3 of 4 files at r1, 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware and @tdelabro)
Pull Request type:
Feature
Current behavior:
No Blake2s‐based StarkHash implementation.
New behavior:
Adds Blake2s struct implementing StarkHash (hash, hash_array, hash_single) plus encoding helpers and tests.
This change is