-
Notifications
You must be signed in to change notification settings - Fork 89
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
Blake round #767
Blake round #767
Conversation
5af79fa
to
db98a63
Compare
f505a0b
to
393fc1f
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #767 +/- ##
=======================================
Coverage 93.34% 93.34%
=======================================
Files 89 89
Lines 11957 11957
Branches 11957 11957
=======================================
Hits 11161 11161
Misses 695 695
Partials 101 101 ☔ View full report in Codecov by Sentry. |
db98a63
to
c28ed04
Compare
393fc1f
to
3a7de20
Compare
c28ed04
to
786d728
Compare
3a7de20
to
7cf2207
Compare
786d728
to
17e294d
Compare
7cf2207
to
1deb28b
Compare
17e294d
to
87a136c
Compare
1deb28b
to
1ba9ec1
Compare
87a136c
to
a27e12e
Compare
1ba9ec1
to
34c742f
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 4 files at r1, 1 of 3 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 12 unresolved discussions (waiting on @spapinistarkware)
crates/prover/src/examples/blake/mod.rs
line 1 at r2 (raw file):
#![allow(unused)]
Add module documentation
You had something that pointed to the paper.
Code quote:
#![allow(unused)]
crates/prover/src/examples/blake/mod.rs
line 48 at r2 (raw file):
fn draw(channel: &mut Blake2sChannel) -> Self { Self { xor12: LookupElements::draw(channel, 3),
make constant
Code quote:
3
crates/prover/src/examples/blake/mod.rs
line 64 at r2 (raw file):
4 => &self.xor4, _ => panic!("Invalid w"), }
Consider to store xor\d in array and have here a general implementation
Same for the accums
Code quote:
match w {
12 => &self.xor12,
9 => &self.xor9,
8 => &self.xor8,
7 => &self.xor7,
4 => &self.xor4,
_ => panic!("Invalid w"),
}
crates/prover/src/examples/blake/mod.rs
line 69 at r2 (raw file):
#[derive(Clone, Copy, Debug)] struct Fu32<F>
Isn't that specific for M31?
Code quote:
struct Fu32<F>
crates/prover/src/examples/blake/round/constraints.rs
line 10 at r2 (raw file):
use crate::examples::blake::Fu32; const I16: BaseField = BaseField::from_u32_unchecked(1 << 15);
Took me some time to understand Inv16.. (rename or document)
Code quote:
const I16: BaseField
crates/prover/src/examples/blake/round/constraints.rs
line 33 at r2 (raw file):
self.g(7, v.get_many_mut([3, 4, 9, 14]).unwrap(), m[14], m[15]); self.logup.push_lookup(
Comment:
// Yield
Code quote:
self.logup.push_lookup(
crates/prover/src/examples/blake/round/constraints.rs
line 53 at r2 (raw file):
Fu32 { l, h } } fn g(&mut self, _round: u32, v: [&mut Fu32<E::F>; 4], m0: Fu32<E::F>, m1: Fu32<E::F>) {
Shouldn't you use it?
Code quote:
_round: u32
crates/prover/src/examples/blake/round/constraints.rs
line 95 at r2 (raw file):
carry_l * (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 0))) * (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 1))),
Consider to extract to is_triplet function
Code quote:
carry_l
* (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 0)))
* (carry_l - E::F::from(BaseField::from_u32_unchecked(1 << 1))),
crates/prover/src/examples/blake/round/constraints.rs
line 116 at r2 (raw file):
} /// Checks that a, b are in range, and computes their xor rotate.
Suggestion:
/// Checks that a, b are in range, and computes their xor rotate right by `r` bits.
crates/prover/src/examples/blake/round/constraints.rs
line 142 at r2 (raw file):
let (bhl, bhh) = self.split_unchecked(b.h, 8); // These also guarantee that all elements are in range.
This comment should be in the xor doc.
Code quote:
// These also guarantee that all elements are in range.
crates/prover/src/examples/blake/round/constraints.rs
line 154 at r2 (raw file):
} /// Checks that a,b in in [0,2^w) and computes their xor.
Suggestion:
/// Checks that a,b are in [0,2^w) and computes their xor.
crates/prover/src/examples/blake/round/gen.rs
line 24 at r2 (raw file):
pub struct BlakeLookupData { xor_lookups: Vec<(u32, [BaseColumn; 3])>,
document everywhere
Code quote:
xor_lookups: Vec<(u32, [BaseColumn; 3])>,
a27e12e
to
d3fe674
Compare
b4400c3
to
676d9fa
Compare
d3fe674
to
fe41cc2
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: 0 of 9 files reviewed, 9 unresolved discussions (waiting on @shaharsamocha7)
crates/prover/src/examples/blake/mod.rs
line 1 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Add module documentation
You had something that pointed to the paper.
Which paper?
crates/prover/src/examples/blake/mod.rs
line 48 at r2 (raw file):
Previously, shaharsamocha7 wrote…
make constant
Did better
crates/prover/src/examples/blake/mod.rs
line 64 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Consider to store xor\d in array and have here a general implementation
Same for the accums
What would I have in the unused slots?
crates/prover/src/examples/blake/mod.rs
line 69 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Isn't that specific for M31?
Yes, but F could be packed.
crates/prover/src/examples/blake/round/constraints.rs
line 53 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Shouldn't you use it?
Nope, but I removed it. It is only used to select the message, but I'm already passing the selecetd messages to the function.
crates/prover/src/examples/blake/round/constraints.rs
line 95 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Consider to extract to is_triplet function
Noted. It is only used here, in these few lines in this relatively short fucntion. Way too early IMO to extract a function. It would also make the function harder to read (anyone would definitely dive into is_triplet). But I did extract a constant.
crates/prover/src/examples/blake/round/constraints.rs
line 116 at r2 (raw file):
} /// Checks that a, b are in range, and computes their xor rotate.
Done.
crates/prover/src/examples/blake/round/constraints.rs
line 154 at r2 (raw file):
} /// Checks that a,b in in [0,2^w) and computes their xor.
Done.
fe41cc2
to
4351076
Compare
676d9fa
to
dc4b524
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: 0 of 9 files reviewed, 9 unresolved discussions (waiting on @shaharsamocha7)
crates/prover/src/examples/blake/round/gen.rs
line 24 at r2 (raw file):
Previously, shaharsamocha7 wrote…
document everywhere
Done.
e0c64fb
to
050ea88
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 9 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 2 of 9 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)
crates/prover/src/constraint_framework/logup.rs
line 147 at r5 (raw file):
- EF::from(self.z) } #[cfg(test)]
can you re-add here?
Code quote:
#[cfg(test)]
crates/prover/src/examples/blake/mod.rs
line 1 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Which paper?
I'm not sure, but we had a page that explained the blake function flow. (wikipedia maybe?)
I think that a reference to this could be good.
crates/prover/src/examples/blake/mod.rs
line 64 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
What would I have in the unused slots?
Panic if key is not in dict?
I meant dict not an array
Previously, shaharsamocha7 wrote…
maybe here |
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 9 files at r3, 1 of 2 files at r4.
Reviewable status: 6 of 9 files reviewed, 11 unresolved discussions (waiting on @spapinistarkware)
crates/prover/src/examples/blake/round/gen.rs
line 24 at r2 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Done.
I don't understand what the xor_lookups means
Specifically it is hard for me to understand what xor() func does.
crates/prover/src/examples/blake/round/gen.rs
line 67 at r5 (raw file):
} impl<'a> TraceGeneratorRow<'a> { fn append_felt(&mut self, val: u32x16) {
Do we want to assert/debug_assert here that those are in renge [0,P)?
Code quote:
fn append_felt(&mut self, val: u32x16) {
crates/prover/src/examples/blake/round/gen.rs
line 132 at r5 (raw file):
} /// Splits a felt at r.
Consider to write why you only append the high part.
Code quote:
/// Splits a felt at r.
crates/prover/src/examples/blake/round/gen.rs
line 150 at r5 (raw file):
let (bhl, bhh) = self.split(b >> 16, r); let _xorll = self.xor(r, all, bll);
Remove the return value or
add an assert that it is correct.
assert_eq(cr, build(xor_ll, xor_lh, ..)
Code quote:
let _xorll = self.xor(r, all, bll);
crates/prover/src/examples/blake/round/gen.rs
line 171 at r5 (raw file):
let _xorlh = self.xor(8, alh, blh); let _xorhl = self.xor(8, ahl, bhl); let _xorhh = self.xor(8, ahh, bhh);
same
Code quote:
let _xorll = self.xor(8, all, bll);
let _xorlh = self.xor(8, alh, blh);
let _xorhl = self.xor(8, ahl, bhl);
let _xorhh = self.xor(8, ahh, bhh);
crates/prover/src/examples/blake/round/gen.rs
line 176 at r5 (raw file):
} /// Checks that a, b are in [0, 2^w) and computes their xor.
Can you state the upper bound on w?
It doesn't work for every xor as you append c
as felt
Code quote:
/// Checks that a, b are in [0, 2^w) and computes their xor.
crates/prover/src/examples/blake/round/gen.rs
line 273 at r5 (raw file):
let p = round_lookup_elements.combine( &lookup_data .round_lookups
why don't you batch those in chunks of two?
Code quote:
&lookup_data
.round_lookups
crates/prover/src/examples/blake/round/mod.rs
line 21 at r5 (raw file):
}; component.evaluate(InfoEvaluator::default()) }
Don't we have a generic impl?
Code quote:
pub fn blake_round_info() -> InfoEvaluator {
let component = BlakeRoundComponent {
log_size: 1,
xor_lookup_elements: BlakeXorElements::dummy(),
round_lookup_elements: RoundElements::dummy(),
claimed_sum: SecureField::zero(),
};
component.evaluate(InfoEvaluator::default())
}
050ea88
to
fd67e73
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: 5 of 9 files reviewed, 10 unresolved discussions (waiting on @shaharsamocha7)
crates/prover/src/constraint_framework/logup.rs
line 147 at r5 (raw file):
Previously, shaharsamocha7 wrote…
can you re-add here?
I used it for the info(), which is trace generation to get the number of columns.
crates/prover/src/examples/blake/mod.rs
line 1 at r2 (raw file):
Previously, shaharsamocha7 wrote…
maybe here
https://en.wikipedia.org/wiki/BLAKE_(hash_function)
Done.
crates/prover/src/examples/blake/mod.rs
line 64 at r2 (raw file):
Previously, shaharsamocha7 wrote…
Panic if key is not in dict?
I meant dict not an array
I think match is more straight forward than a dict, and the compiler will have an easier time optimizing it out.
crates/prover/src/examples/blake/round/gen.rs
line 24 at r2 (raw file):
Previously, shaharsamocha7 wrote…
I don't understand what the xor_lookups means
Specifically it is hard for me to understand what xor() func does.
Done.
crates/prover/src/examples/blake/round/gen.rs
line 67 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Do we want to assert/debug_assert here that those are in renge [0,P)?
Nah...
crates/prover/src/examples/blake/round/gen.rs
line 132 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Consider to write why you only append the high part.
I added a comment above the struct that point tot he constraints, so that you can look at both.
I think writing here everythin about the constraints is duplication.
crates/prover/src/examples/blake/round/gen.rs
line 150 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Remove the return value or
add an assert that it is correct.
assert_eq(cr, build(xor_ll, xor_lh, ..)
Done.
crates/prover/src/examples/blake/round/gen.rs
line 176 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Can you state the upper bound on w?
It doesn't work for every xor as you appendc
as felt
Done.
crates/prover/src/examples/blake/round/gen.rs
line 273 at r5 (raw file):
Previously, shaharsamocha7 wrote…
why don't you batch those in chunks of two?
It's the remainder. A single lookup.
All the previous ones (even number) were already batched.
crates/prover/src/examples/blake/round/mod.rs
line 21 at r5 (raw file):
Previously, shaharsamocha7 wrote…
Don't we have a generic impl?
Nope. We need to instantiate the component somehow, and that is not something provided by FrameworkComponent.
fd67e73
to
952d01c
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 9 files at r3, 1 of 2 files at r6, 1 of 2 files at r7.
Reviewable status: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @spapinistarkware)
crates/prover/src/constraint_framework/logup.rs
line 147 at r5 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I used it for the info(), which is trace generation to get the number of columns.
Let's recall to change poseidon accordingly, and also think of the correct way to retreive N_COLUMNS
Can merge it for now
crates/prover/src/examples/blake/round/gen.rs
line 28 at r6 (raw file):
xor_lookups: Vec<(u32, [BaseColumn; 3])>, /// A column of round lookup values (v_in, v_out, m). round_lookups: [BaseColumn; 16 * 3 * 2],
We are going to have BlakeLookupData for the scheduler, no?
Suggestion:
pub struct BlakeRoundLookupData {
/// A vector of (w, [a_col, b_col, c_col]) for each xor lookup.
/// w is the xor width. c_col is the xor col of a_col and b_col.
xor_lookups: Vec<(u32, [BaseColumn; 3])>,
/// A column of round lookup values (v_in, v_out, m).
round_lookup: [BaseColumn; 16 * 3 * 2],
crates/prover/src/examples/blake/round/mod.rs
line 88 at r6 (raw file):
let xor_lookup_elements = BlakeXorElements::dummy(); let round_lookup_elements = RoundElements::dummy();
Can you use non dummy elements here?
Code quote:
let xor_lookup_elements = BlakeXorElements::dummy();
let round_lookup_elements = RoundElements::dummy();
952d01c
to
4ffac8d
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: 6 of 10 files reviewed, 2 unresolved discussions (waiting on @shaharsamocha7)
crates/prover/src/examples/blake/round/gen.rs
line 28 at r6 (raw file):
Previously, shaharsamocha7 wrote…
We are going to have BlakeLookupData for the scheduler, no?
Done.
crates/prover/src/examples/blake/round/mod.rs
line 88 at r6 (raw file):
Previously, shaharsamocha7 wrote…
Can you use non dummy elements here?
I think it's ok for tests.
4ffac8d
to
c845ef5
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 r5, 1 of 2 files at r6, 1 of 2 files at r7, 1 of 1 files at r8, 1 of 1 files at r9.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @spapinistarkware)
This change is![Reviewable](https://camo.githubusercontent.com/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)