arkworks-rs / poly-commit

A Rust library for polynomial commitments
Apache License 2.0
309 stars 123 forks source link

`PreparedCommitment` & `PreparedVerifierKey` don't belong on `PolynomialCommitment` trait #127

Closed mmagician closed 8 months ago

mmagician commented 9 months ago

Description

No trait method was even defined over these. I think if at some point someone implements some preprocessing, this can always be swapped out for the actual Commitment/VerifierKey type and instead prepared there at the point of committing or setup - otherwise let's keep the traits simpler if we can.

closes: #91


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.

Pratyush commented 9 months ago

cc @weikengchen

IIRC these are useful for the constraint versions.

mmagician commented 9 months ago

Uhmm yes, sorry this was a bit quick of me to PR this. Indeed it is at least part of the trait definition for PCCheckVar. Since it's only used for the constraint PC and not native, I wonder if we can somehow simplify the interface still.

mmagician commented 8 months ago

I'd be in favour of keeping the base PolynomialCommitment trait as simple as possible.

How about introducing a supertrait PolynomialCommitmentConstraint:

pub trait PolynomialCommitmentConstraint<F: PrimeField, P: Polynomial<F>, S: CryptographicSponge>: PolynomialCommitment<F, P, S>
{
/// The prepared verifier key for the scheme; used to check an evaluation proof.
type PreparedVerifierKey: PCPreparedVerifierKey<Self::VerifierKey> + Clone;
/// The prepared commitment to a polynomial.
type PreparedCommitment: PCPreparedCommitment<Self::Commitment>;

And modifying constraints.rs accordingly?

mmagician commented 8 months ago

Ok, I think a cleaner solution, in https://github.com/arkworks-rs/poly-commit/pull/127/commits/0435c2acd0bfa4a8726e7ee51320dcb113a3c0af, is to place the PreparedVerifierKey and PreparedCommitment types directly on PCCheckVar. This is a breaking change in terms of API, but it's a trivial fix for anyone currently using PCCheckVar: they only need to define 2 extra types on their trait implementation, and since we keep the existing implementors of the Prepared... traits for all schemes as they were. It's only moving the type definition out of the PolynomialCommitment trait, keeping it leaner, and into PCCheckVar where it belongs.