-
Notifications
You must be signed in to change notification settings - Fork 71
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
Use SecureField in interaction. #642
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## dev #642 +/- ##
=======================================
Coverage 89.50% 89.50%
=======================================
Files 75 75
Lines 9640 9640
Branches 9640 9640
=======================================
Hits 8628 8628
Misses 932 932
Partials 80 80 ☔ View full report in Codecov by Sentry. |
b1c52c7
to
aecb1e4
Compare
66b6bf8
to
f873404
Compare
aecb1e4
to
ec7f140
Compare
f873404
to
33fc002
Compare
ec7f140
to
942e4d3
Compare
33fc002
to
7c247f4
Compare
942e4d3
to
fe5d057
Compare
7c247f4
to
cba4dae
Compare
90b274e
to
7141a2d
Compare
cba4dae
to
43176a4
Compare
7141a2d
to
f54597b
Compare
43176a4
to
120236e
Compare
f54597b
to
f478872
Compare
120236e
to
ae9d116
Compare
f478872
to
2745ab2
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 11 files at r1, 1 of 11 files at r2, all commit messages.
Reviewable status: 2 of 12 files reviewed, 3 unresolved discussions (waiting on @alonh5 and @shaharsamocha7)
crates/prover/src/core/utils.rs
line 94 at r3 (raw file):
/// Securely combines the given values using the given random alpha and z. /// Alpha and z should be secure field elements for soundness. pub fn shifted_secure_combination<F: ExtensionOf<BaseField>>(
Is this needed?
crates/prover/src/core/air/mod.rs
line 82 at r3 (raw file):
trace: &ColumnVec<&CircleEvaluation<B, BaseField, BitReversedOrder>>, elements: &InteractionElements, ) -> ColumnVec<SecureEvaluation<B>>;
I don't think it is always a bunch of secure columns in general.
It should be exactly like trace, just a bunch of basefield columns.
Code quote:
ColumnVec<SecureEvaluation<B>>
crates/prover/src/core/prover/mod.rs
line 74 at r3 (raw file):
let interaction_elements = air.interaction_elements(channel); let interaction_trace_polys = air.interact(&trace, &interaction_elements);
- This is not using the precomputed twiddles.
- Do we need to change the interface? Can't interact() reutrn teh evaluation?
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 12 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/core/utils.rs
line 94 at r3 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Is this needed?
Yes the values could be M31 (in the prover constraint evaluation) and QM31 (in the verifier oods constraint evaluation).
crates/prover/src/core/air/mod.rs
line 82 at r3 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I don't think it is always a bunch of secure columns in general.
It should be exactly like trace, just a bunch of basefield columns.
Done.
crates/prover/src/core/prover/mod.rs
line 74 at r3 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
- This is not using the precomputed twiddles.
- Do we need to change the interface? Can't interact() reutrn teh evaluation?
Done.
b20b7cc
to
b4b6328
Compare
cc96916
to
909c178
Compare
ccd2fb8
to
88b9ad2
Compare
909c178
to
7443767
Compare
0dcbd87
to
de6e3a6
Compare
7443767
to
60c196d
Compare
88f8a6d
to
c6aaf57
Compare
c6aaf57
to
6a16e22
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 11 files at r2, 9 of 9 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @alonh5 and @shaharsamocha7)
crates/prover/src/examples/wide_fibonacci/component.rs
line 135 at r4 (raw file):
let trace_values = trace.iter().map(|eval| &eval.values[..]).collect_vec(); let (alpha, z) = (elements[ALPHA_ID], elements[Z_ID]); let values = write_lookup_column(&trace_values, alpha, z);
For efficientcy, we probably want write_lookup_column()
to already return a SecureColumn.
You can add a TODO for that.
crates/prover/src/examples/wide_fibonacci/component.rs
line 139 at r4 (raw file):
for (i, value) in values.into_iter().enumerate() { secure_column.set(i, value); }
I think we have a FromIterator impl for SecureColumn. so, you can use collect()
Code quote:
for (i, value) in values.into_iter().enumerate() {
secure_column.set(i, value);
}
crates/prover/src/examples/wide_fibonacci/constraint_eval.rs
line 95 at r4 (raw file):
// Lookup constraints. let lookup_value = SecureCirclePoly::<CpuBackend>::eval_from_partial_evals(std::array::from_fn(|j| {
Both of us know what this function does, but I fear other people will be confused by it. Maybe we can consult with the team how to name this.
Code quote:
eval_from_partial_evals
6a16e22
to
96ac8ba
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 @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/wide_fibonacci/component.rs
line 135 at r4 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
For efficientcy, we probably want
write_lookup_column()
to already return a SecureColumn.
You can add a TODO for that.
Done.
crates/prover/src/examples/wide_fibonacci/component.rs
line 139 at r4 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
I think we have a FromIterator impl for SecureColumn. so, you can use
collect()
Done, thanks.
crates/prover/src/examples/wide_fibonacci/constraint_eval.rs
line 95 at r4 (raw file):
Previously, spapinistarkware (Shahar Papini) wrote…
Both of us know what this function does, but I fear other people will be confused by it. Maybe we can consult with the team how to name this.
Yes, I also think we should move it to be a method of secure field and then it will be more clear when it's next to from_m31_array()
. I added a TODO also.
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: 11 of 13 files reviewed, 3 unresolved discussions (waiting on @shaharsamocha7 and @spapinistarkware)
crates/prover/src/examples/wide_fibonacci/constraint_eval.rs
line 95 at r4 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Yes, I also think we should move it to be a method of secure field and then it will be more clear when it's next to
from_m31_array()
. I added a TODO also.
PR 679
96ac8ba
to
34a1233
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 2 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @shaharsamocha7)
This change is