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.

Sign up for GitHub

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

Jump to bottom

Add CommitmentProver trait, and add KZG prover to it #62

Merged
merged 5 commits into from
Jan 25, 2024
Merged

Add CommitmentProver trait, and add KZG prover to it #62

merged 5 commits into from
Jan 25, 2024

Conversation

arnaucube
Copy link
Collaborator

@arnaucube arnaucube commented Jan 21, 2024 •
edited
Loading

  • Add CommitmentProver trait as a interface for the CommittedInstances that will use this trait to commit to the witness (and if needed to the error terms)
  • Addapt pedersen.rs to the new CommitmentProver trait (and move it under the new src/commitment/ dir
  • Add KZGProver implementing the CommitmentProver trait, by adapting arkworks/poly-commit::kzg10 commit & open implementation

At line kzg.rs#4 there is the rationale on why doing the CommitmentProver trait:

The motivation to do so, is that we want to be able to use KZG / Pedersen for committing to
vectors indistinctly, and the arkworks KZG10 implementation contains all the methods under the
same trait, which requires the Pairing trait, where the prover does not need access to the
Pairing but only to G1.
For our case, we want the folding schemes prover to be agnostic to pairings, since in the
non-ethereum cases we may use non-pairing-friendly curves with Pedersen commitments, so the
trait & types that we use should not depend on the Pairing type for the prover. Therefore, we
separate the CommitmentSchemeProver from the setup and verify phases, so the prover can be
defined without depending on pairings.

@arnaucube
Add KZG commitment scheme adapted to vector commitment
4409850 
Add KZG commitment scheme adapted to vector commitment
Also move the `src/pedersen.rs` into `src/commitment/pedersen.rs` where
it will coexist with `kzg.rs` and the trait defined in
`src/commitment/mod.rs`.
@arnaucube
Adapt Pedersen into the new CommitmentProver trait
9805151
@arnaucube
add CommitmentProver (Pedersen&KZG) homomorphic property test
8f87fd1
@arnaucube
polishing
586d3d2
@arnaucube arnaucube requested review from han0110 and CPerezz January 21, 2024 11:21
han0110
han0110 reviewed Jan 24, 2024
View reviewed changes
Copy link
Collaborator

@han0110 han0110 left a comment

Choose a reason for hiding this comment

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

LGTM, just got a small question

src/commitment/kzg.rs Outdated Show resolved Hide resolved
src/commitment/kzg.rs Outdated Show resolved Hide resolved
CPerezz
CPerezz approved these changes Jan 25, 2024
View reviewed changes
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

LGTM overall! Nice job.

Left some suggestions. But feel free to do what you think makes more sense. For me it can still be merged this way. 😄

src/commitment/kzg.rs Show resolved Hide resolved
src/commitment/kzg.rs Show resolved Hide resolved
src/commitment/kzg.rs Show resolved Hide resolved
src/commitment/mod.rs Outdated Show resolved Hide resolved
src/commitment/mod.rs Outdated Show resolved Hide resolved
src/commitment/mod.rs Outdated Show resolved Hide resolved
src/commitment/mod.rs Outdated Show resolved Hide resolved
src/commitment/mod.rs Show resolved Hide resolved
src/commitment/mod.rs Show resolved Hide resolved
@arnaucube arnaucube force-pushed the feature/kzg-commitment-trait branch 2 times, most recently from 37c4285 to 37e69b3 Compare January 25, 2024 11:05
@arnaucube arnaucube force-pushed the feature/kzg-commitment-trait branch from 37e69b3 to 48625e6 Compare January 25, 2024 11:07
@arnaucube arnaucube enabled auto-merge January 25, 2024 13:42
@arnaucube arnaucube disabled auto-merge January 25, 2024 13:43
@arnaucube arnaucube added this pull request to the merge queue Jan 25, 2024
Merged via the queue into main with commit 47e47cc Jan 25, 2024
3 checks passed
@arnaucube arnaucube deleted the feature/kzg-commitment-trait branch January 26, 2024 09:21
dmpierre pushed a commit that referenced this pull request Feb 2, 2024
@arnaucube @han0110 @dmpierre
Add CommitmentProver trait, and add KZG prover to it (#62)
644440b 
* Add KZG commitment scheme adapted to vector commitment

Add KZG commitment scheme adapted to vector commitment
Also move the `src/pedersen.rs` into `src/commitment/pedersen.rs` where
it will coexist with `kzg.rs` and the trait defined in
`src/commitment/mod.rs`.

* Adapt Pedersen into the new CommitmentProver trait

* add CommitmentProver (Pedersen&KZG) homomorphic property test

* polishing

* Use divide_with_q_and_r, rename skip_first_zero_coeffs

Co-authored-by: han0110 <[email protected]>

---------

Co-authored-by: han0110 <[email protected]>
@arnaucube arnaucube mentioned this pull request May 11, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CPerezz CPerezz CPerezz approved these changes

@han0110 han0110 han0110 approved these changes

Assignees
No one assigned
Labels
common
Projects
None yet
Milestone
Minimal first flow (Nova in Ethereum)
Development

Successfully merging this pull request may close these issues.
3 participants
@arnaucube @han0110 @CPerezz