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

Extract permutation argument and introduce typed challenges #70

Merged
merged 12 commits into from
Dec 2, 2020

Conversation

str4d
Copy link
Contributor

@str4d str4d commented Nov 25, 2020

Part of #62.

@codecov-io
Copy link

codecov-io commented Nov 25, 2020

Codecov Report

Merging #70 (4d4c79b) into main (3bcfe78) will increase coverage by 0.08%.
The diff coverage is 85.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #70      +/-   ##
==========================================
+ Coverage   80.63%   80.72%   +0.08%     
==========================================
  Files          24       28       +4     
  Lines        3254     3330      +76     
==========================================
+ Hits         2624     2688      +64     
- Misses        630      642      +12     
Impacted Files Coverage Δ
src/arithmetic.rs 98.26% <ø> (-0.06%) ⬇️
src/plonk.rs 98.18% <ø> (ø)
src/poly/multiopen.rs 97.94% <ø> (ø)
src/plonk/verifier.rs 73.27% <72.97%> (-2.75%) ⬇️
src/plonk/permutation/verifier.rs 80.51% <80.51%> (ø)
src/plonk/permutation/prover.rs 83.70% <83.70%> (ø)
src/plonk/keygen.rs 95.65% <87.50%> (+0.61%) ⬆️
src/plonk/prover.rs 86.59% <88.37%> (+2.72%) ⬆️
src/plonk/permutation/keygen.rs 89.28% <89.28%> (ø)
src/poly/multiopen/prover.rs 91.13% <91.66%> (+2.25%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bcfe78...4d4c79b. Read the comment docs.

Base automatically changed from small-optimisations to main November 25, 2020 14:01
@str4d str4d added this to the Core Sprint 2020-47 milestone Nov 25, 2020
@str4d str4d changed the title Introduce internal APIs Extract permutation argument and introduce typed challenges Nov 30, 2020
@str4d str4d marked this pull request as ready for review November 30, 2020 16:03
@str4d
Copy link
Contributor Author

str4d commented Nov 30, 2020

Rebased to fix merge conflicts with #59.

mod verifier;

#[derive(Debug, Clone)]
pub(crate) struct Proof<C: CurveAffine> {
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 currently uses a single permutation "proof" to represent all the permutations added to the circuit. This was the most straight-forward refactoring of the existing code (making review of the moving pieces easier), but we will probably want to alter this in a subsequent PR to instead have a proof per permutation (similar to the lookup argument PR).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've now extracted the permutation keygen logic as well, and I've done so in the same way as the lookup argument (Vec<permutation::Argument>).

src/plonk.rs Outdated Show resolved Hide resolved
src/plonk/keygen.rs Show resolved Hide resolved
src/plonk/permutation.rs Show resolved Hide resolved
.chain(
permutation_product_cosets_owned
.into_iter()
.map(move |coset| Polynomial::one_minus(coset) * &pk.l0),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting tidbit: strictly speaking the pk.l0 is needed for just the permutation argument, but because it's also used by the lookup argument (not yet introduced) keeping it in pk is fine with me. Just worth pointing out.

src/plonk/permutation/prover.rs Show resolved Hide resolved
// Sample gamma challenge
let gamma = ChallengeGamma::get(&mut transcript);

// Commit to permutations, if any.
Copy link
Contributor

@ebfull ebfull Dec 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we wouldn't need to sample beta and gamma in the first place if there are no permutations/lookups -- though this scenario is unlikely to begin with, so it might not be worth fussing over.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's look at this again once the lookups PR is merged. Opened #82

src/plonk/permutation/prover.rs Outdated Show resolved Hide resolved
src/poly/commitment.rs Outdated Show resolved Hide resolved
src/poly/commitment/prover.rs Outdated Show resolved Hide resolved
src/poly/multiopen/prover.rs Outdated Show resolved Hide resolved
This also centralises the challenge generation logic in Challenge::get,
ensuring it is consistent across the codebase.
Once we refactor the permutation argument implementation to be integrated
as Vec<permutation::Proof>, we can change this again to just map from the
Vec<permutation::Argument> inside ConstraintSystem.
@str4d
Copy link
Contributor Author

str4d commented Dec 1, 2020

Rebased to fix conflicts with #61.

Also added types for these challenges, even though it's not technically
necessary yet because we don't pass these around anywhere.
The argument to the poly::commitment prover and verifier was mistakenly
represented as a challenge, when in fact the commitments may be opened at
any scalar (which just happens to be a challenge within poly::multiopen).

The poly::commitment APIs are now public again.
@ebfull ebfull merged commit d5927d6 into main Dec 2, 2020
@str4d str4d deleted the internal-api branch December 2, 2020 17:31
@str4d str4d restored the internal-api branch December 2, 2020 18:31
@str4d str4d deleted the internal-api branch December 2, 2020 18:32
str4d added a commit that referenced this pull request Jan 19, 2022
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.

4 participants