Skip to content
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

Open
wants to merge 62 commits into
base: master
Choose a base branch
from
Open

Constraints #39

wants to merge 62 commits into from

Conversation

npwardberkeley
Copy link
Contributor

No description provided.

@weikengchen
Copy link
Member

Merging with the additional conflicts with the fix to the main branch...

@Pratyush
Copy link
Member

Pratyush commented Nov 13, 2020

btw, is it possible to have a "reviewing" guide here? Which parts should I focus on, which parts are "trivial", etc.

Ditto for poly-commit.

This can be as simple as leaving comments on the files that need most scrutiny.

@weikengchen
Copy link
Member

No problem. Will do for both.

@Pratyush
Copy link
Member

Thanks, much appreciated =)

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());
Copy link
Member

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/data_structures.rs Show resolved Hide resolved
use ark_relations::r1cs::{ConstraintSystemRef, LinearCombination, SynthesisError};
use core::marker::PhantomData;

/// Vars for a RNG for Fiat-Shamir purposes
Copy link
Member

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.

Copy link
Contributor Author

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;
Copy link
Member

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,

src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
use core::marker::PhantomData;

/// Vars for a RNG for Fiat-Shamir purposes
pub trait FiatShamirRngVar<F: PrimeField, CF: PrimeField, PFS: FiatShamirRng<F, CF>>:
Copy link
Contributor Author

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?

Copy link
Member

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

@weikengchen
Copy link
Member

(We would pause here and leave the changes in the constraints branch, and we will come back after the paper deadline.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants