Skip to content

Conversation

@davidnevadoc
Copy link

@davidnevadoc davidnevadoc commented Mar 23, 2024

Description

In order to verify a KZG proof, it is necessary to provide the commitment scheme parameters ParamsVerifier. Currently, these parameters for KZG are the same struct as the parameters for the prover. For IPA, this is not a big issue, but it is for KZG, where the prover parameters are linear in size with number of constraints of the circuit, while the verifier is just a single G2 point.

Changes

  • Add a new struct ParamsVerifierZKG with only the necessary elements for a KZG based proof verification.
    Note: This proof verification not only involves a KZG verification, but also a commitment to the PI values of the circuit. As a result, these parameters contain a vector of commitments to the Lagrange polynomials of the corresponding domain. The size of this vector doesn't have to be the full n (size of the domain), it can be adjusted to the number of expected PI.

  • Remove get_g from Params.

  • Change verifier_params(...) -> into_verifier_params(...)
    Since the Verifier params are now a different struct, this function now consumes the parameters and creates a new instance of the verifier parameters, instead of dealing with references.


This PR solves the bulky parameters issue but the result is not 100% satisfactory. I think a refactor around the commitment scheme parameters traits and structs is needed. Here are some of the reasons:

  • We could have a better separation or organization around what the commitment scheme verifier and proof verifier.
    Ideally the KZG verifier parameters should only contain the s_g2 point, not the commitments to Lagrange polynomials.
  • The interfaces defined by the traits Params, ProverParams and VerifierParams have some problems:
    • trimed_size does not make a lot of sense for IPA parametes, it should be something specific to KZG.
    • downsize may not be suitable for Params since KZG verifier params cannot be downsized.
    • commit_lagrange may not be suitable for Params either. The verifier parameters should not need to support this capability.

After removing the G1 and G2 generators from the parameters ( credit to Miguel for noticing this! ) another design issue was revealed: The verifier (Shplonk and Gwc) are not using the parameters at all! The reason is quite simple, the output of verify_proof is a Guard, an object that accumulates partially verified proofs and that can be finalized to complete the verification of a batch of proof. The pairing is only used in the last step, so the KZG parameters are not needed until then.

@davidnevadoc davidnevadoc marked this pull request as ready for review March 23, 2024 22:20
@davidnevadoc davidnevadoc force-pushed the nev@verifier-kzg-params branch 2 times, most recently from c0dcb52 to 2331baa Compare March 24, 2024 15:32
 - modify `verifier_params` -> `into_verifier_params`
 This function now consumes the Commitment Scheme parameters.
Copy link

@miguel-ambrona miguel-ambrona left a comment

Choose a reason for hiding this comment

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

Very nice!

The case of IPA is similar to that of Shplonk or Gwc in that we can define a verifier that does not use the actual parameters and returns Guard. And we postpone the use of the verifier parameters until the very end.
The fact that this design is possible in all candidate polynomial commitment schemes could be taken into account for the future refactor.


fn downsize(&mut self, _k: u32) {
// Verifier parameters cannot be downsized since they do not contain the original powers of g.
unimplemented!()

Choose a reason for hiding this comment

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

Panic with a message instead?

.map(|_| <E::G1Affine as SerdeCurveAffine>::read(reader, format))
.collect::<Result<Vec<_>, _>>()?,
SerdeFormat::RawBytesUnchecked => (0..trimed_size)
.map(|_| <E::G1Affine as SerdeCurveAffine>::read(reader, format).unwrap())

Choose a reason for hiding this comment

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

Can't this fail in the unwrap?
I'm trying to understand the difference between RawBytes and RawBytesUnchecked.

Copy link
Author

Choose a reason for hiding this comment

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

AFAIK, the main difference is that RawBytes will ensure that the coordinate pair is in fact a valid point, while the unchecked version won't. It seems that, in addition to this, the unchecked version doesn't bother with error handling.
This part of the code is inhereted from ParamsKZG but I wonder if we could improve it.
I notice that the Procesed branch can read the parameters from a buffer in parallel, while for the other formats branches this is not the case. I guess this is because they are not really used since they are twice the size.

Comment on lines +95 to +99
pub trait ParamsVerifier<'params, C: CurveAffine>: Params<'params, C> {
/// Returns the size of the trimed parameters.
/// This is the maximum size of the PI that these params can be used to commit to.
fn trimed_size(&self) -> u64;
}

Choose a reason for hiding this comment

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

I agree that it is weird to mention the PI in the commitment scheme.

Copy link
Author

Choose a reason for hiding this comment

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

In a full refactor of the verifier we should try to make a better distinction between the verifier of the PCS and the verifier of a proof. Currently, the boundaries are not well defined and it can get a bit messy...

Comment on lines +59 to +64
impl<'params, C: CurveAffine> ParamsVerifier<'params, C> for ParamsIPA<C> {
fn trimed_size(&self) -> u64 {
// IPA parameters are never trimed so their sized is always the full size of the domain.
self.n
}
}

Choose a reason for hiding this comment

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

Is it not, thought? Let's say you have parameters of 2^20, but you work with a polynomial of only 2^16. You could trim the parameters to only use what you need.

Copy link
Author

Choose a reason for hiding this comment

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

In that case we would use downsize, which not only trims the G1 vector to the specified size, but also recomputes the lagrange polynomial commitments and adjusts the values for n and k.

Comment on lines +501 to +504
/// Writes params to a buffer.
fn write<W: io::Write>(&self, writer: &mut W) -> io::Result<()> {
self.write_custom(writer, SerdeFormat::RawBytes)
}

Choose a reason for hiding this comment

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

Why default RawBytes? that writes uncompressed points, instead of compressed. intentional?

Copy link
Author

Choose a reason for hiding this comment

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

I can't say 100% since this part of the code is inherited from the previous params.
This options skips the computation of y and allows for loading the points without performing curve check either, so I guess this was preferred due the performance benefits.

@davidnevadoc davidnevadoc changed the base branch from fixed_73408a1 to main April 1, 2024 13:45
@davidnevadoc davidnevadoc merged commit 5f6bed0 into input-output-hk:main Apr 1, 2024
iquerejeta added a commit that referenced this pull request May 8, 2024
…actions/checkout-4

chore(deps): bump actions/checkout from 3 to 4
dkaidalov pushed a commit that referenced this pull request Jul 1, 2025
* Compare polynomials and polynomial commitments by reference.
Issue #1

Not permit duplicate queries
Issue #3

* Add Length of PIs to the transcript
Issue #2

* Check trailing bytes
Issue #6

* Panic if two points are equals in lagrange_interpolate.
Issue #10

* Hash the byte representation of the VK in the transcript Issue #5

We do not add the architecture, as the verifier is already parametrised by that.

* Consistency in polynomial multiplication Issue #7

We mutate the polynomial (with all zeroes) to be consistent with the behaviour.

* Make commitment homomorphic, and don't send evals of pieces - Issue#0
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.

3 participants