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

New PCS trait and implementation #116

Merged
merged 14 commits into from
Sep 1, 2022
Merged

New PCS trait and implementation #116

merged 14 commits into from
Sep 1, 2022

Conversation

mrain
Copy link
Contributor

@mrain mrain commented Aug 26, 2022

Description

closes: #62

  • Introduct new PolynomialCommitmentScheme trait and basic implementations
  • Now PlonkKzgSnark use our own KZG10 implementation

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@mrain mrain linked an issue Aug 26, 2022 that may be closed by this pull request
@mrain mrain marked this pull request as draft August 26, 2022 21:24
@mrain mrain marked this pull request as ready for review August 26, 2022 21:48
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/prelude.rs Outdated Show resolved Hide resolved
primitives/src/pcs/prelude.rs Outdated Show resolved Hide resolved
primitives/src/pcs/multilinear_kzg/mod.rs Show resolved Hide resolved
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
Comment on lines 24 to 42
type ProverParam;
/// Verifier parameters
type VerifierParam;
/// Structured reference string
type SRS;
/// Polynomial and its associated types
type Polynomial;
/// Polynomial input domain
type Point;
/// Polynomial Evaluation
type Evaluation;
/// Commitments
type Commitment: CanonicalSerialize;
/// Batch commitments
type BatchCommitment: CanonicalSerialize;
/// Proofs
type Proof;
/// Batch proofs
type BatchProof;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can add more trait bounds such as:
type Proof: Clone + Serialize + CanonicalSerialize + Debug + PartialEq...

can take a look at PairingEngine's associated type, they inherented a lot of basic traits.

Copy link
Contributor

@alxiong alxiong Aug 31, 2022

Choose a reason for hiding this comment

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

@mrain, @chancharles92 maybe use this?

    /// Prover parameters
    type ProverParam: Clone;
    /// Verifier parameters
    type VerifierParam: Clone + CanonicalSerialize + CanonicalDeserialize;
    /// Structured reference string
    type SRS: Clone + Debug;
    /// Polynomial and its associated types
    type Polynomial: Sized + Clone + Debug + Hash + PartialEq + Eq + Add + Neg + Zero + CanonicalSerialize + CanonicalDeserialize + for<'a> AddAssign<&'a Self> + for<'a> AddAssign<(F, &'a Self)> + for<'a> SubAssign<&'a Self>;
    /// Polynomial input domain
    type Point:: Clone + Ord + Debug + Sync + Hash + PartialEq + Eq;
    /// Polynomial Evaluation
    type Evaluation: Field;
    /// Commitments
    type Commitment: Clone + CanonicalSerialize + CanonicalDeserialize + Debug + PartialEq + Eq;
    /// Batch commitments
    type BatchCommitment: Clone + CanonicalSerialize + CanonicalDeserialize + Debug + PartialEq + Eq;
    /// Proofs
    type Proof: Clone + CanonicalSerialize + CanonicalDeserialize + Debug + PartialEq + Eq;
    /// Batch proofs
    type BatchProof: Clone + CanonicalSerialize + CanonicalDeserialize + Debug + PartialEq + Eq;

Copy link
Contributor

@alxiong alxiong Aug 31, 2022

Choose a reason for hiding this comment

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

(I've updated my comment above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @alxiong . But for Polynomial, Rc used in multilinear setting could not fit in this trait bound.
I think it make sense to put polynomial in heap because it's usually quite large.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but similar to the argument for impl Borrow<>, how do you know what type of smart pointer one wants?

I don't think we should fix smart pointer type at the associate type level, (namely MultilinearKzgPCS::Polynomial = Rc<DenseMultilinearExtension<E::Fr>> is not as flexible as just DenseMultilinearExtension<E::Fr>.

and you can always wrap this MLE into some smart pointer, such as Rc, RefCell, or Arc etc.

@chancharles92 what's your opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have a strong opinion. Actually we were using Polynomial = DenseMultilinearExtension, but later Zhenfei changed it to Polynomial = Rc<Dense...> which has a few advantages:

  1. It significantly simplifies the code, o/w we need to add Rc everywhere.
  2. It make sure that the developer never forget to use Rc, otherwise the code performance can be accidentally effected.
  3. When we want to use other smart pointers, we can e.g. use Polynomial = Arc<...> in associated type rather than change it everywhere.

 - Better naming and exporting.
 - Interfaces opt for better efficiency.
@mrain mrain requested review from alxiong and removed request for zhenfeizhang August 30, 2022 20:41
primitives/src/pcs/multilinear_kzg/mod.rs Outdated Show resolved Hide resolved
primitives/src/pcs/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

I give a provisional approval on my comments #116 (comment) being settled.

@mrain mrain merged commit ff43209 into main Sep 1, 2022
@mrain mrain deleted the chengyu/pcs branch September 1, 2022 13:19
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.

audit/improve KZG10 code
3 participants