-
Notifications
You must be signed in to change notification settings - Fork 87
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
Constraints #39
base: master
Are you sure you want to change the base?
Constraints #39
Conversation
086920a
to
49f128b
Compare
Merging with the additional conflicts with the fix to the main branch... |
btw, is it possible to have a "reviewing" guide here? Which parts should I focus on, which parts are "trivial", etc. Ditto for This can be as simple as leaving comments on the files that need most scrutiny. |
No problem. Will do for both. |
Thanks, much appreciated =) |
src/ahp/indexer.rs
Outdated
end_timer!(padding_time); | ||
let matrix_processing_time = start_timer!(|| "Processing matrices"); | ||
ics.inline_all_lcs(); | ||
ics.outline_lcs(); | ||
make_matrices_square_for_indexer(ics.clone()); |
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.
Review needed: note, the order has been reversed here.
src/fiat_shamir/constraints.rs
Outdated
use ark_relations::r1cs::{ConstraintSystemRef, LinearCombination, SynthesisError}; | ||
use core::marker::PhantomData; | ||
|
||
/// Vars for a RNG for Fiat-Shamir purposes |
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.
Review needed: note that this would merge with sponge
one day.
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.
This looks good! Fixed some comments
// Requires F to be Alt_Bn128Fr | ||
let full_rounds = 8; | ||
let partial_rounds = 29; | ||
let alpha = 17; |
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.
TODO: A remark is likely needed to say that these parameters may not work for all the curves. In the future, the sponge would be supposed to find good parameters for different curves,
use core::marker::PhantomData; | ||
|
||
/// Vars for a RNG for Fiat-Shamir purposes | ||
pub trait FiatShamirRngVar<F: PrimeField, CF: PrimeField, PFS: FiatShamirRng<F, CF>>: |
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.
Do we plan to have other kinds of FiatShamirRngVar
than FiatShamirAlgebraicSpongeRngVar
?
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.
Eventually this will become a SpongeGadget
, corresponding to the API in the ark-sponge
crate
(We would pause here and leave the changes in the constraints branch, and we will come back after the paper deadline.) |
No description provided.