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

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

Merged
merged 3 commits into from
Oct 26, 2023

Conversation

mmagician
Copy link
Member

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.

  • Targeted PR against correct branch (master)
  • 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

@mmagician mmagician requested a review from a team as a code owner October 4, 2023 19:47
@mmagician mmagician requested review from z-tech, Pratyush and weikengchen and removed request for a team October 4, 2023 19:47
@mmagician mmagician mentioned this pull request Oct 4, 2023
6 tasks
@Pratyush
Copy link
Member

Pratyush commented Oct 4, 2023

cc @weikengchen

IIRC these are useful for the constraint versions.

@mmagician
Copy link
Member Author

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

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

Ok, I think a cleaner solution, in 0435c2a, 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.

@mmagician mmagician changed the title PreparedCommitment & PreparedVerifierKey were never used PreparedCommitment & PreparedVerifierKey don't belong on PolynomialCommitment trait Oct 26, 2023
@Pratyush Pratyush added this pull request to the merge queue Oct 26, 2023
Merged via the queue into arkworks-rs:master with commit 4921327 Oct 26, 2023
4 checks passed
@mmagician mmagician deleted the remove-prepared-commitment branch October 26, 2023 18:12
mmagician added a commit to NP-Eng/poly-commit that referenced this pull request Nov 15, 2023
…ialCommitment` trait (arkworks-rs#127)

* PreparedCommitment was never used

* Remove PreparedVerifierKey

* place `Prepared` types on `PCCheckVar` trait
amend
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.

kzg10::PreparedVerifierKey is never used
2 participants