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

Add Absorb trait bound on PCCommitment #143

Open
mmagician opened this issue Feb 5, 2024 · 3 comments · May be fixed by #144
Open

Add Absorb trait bound on PCCommitment #143

mmagician opened this issue Feb 5, 2024 · 3 comments · May be fixed by #144

Comments

@mmagician
Copy link
Member

mmagician commented Feb 5, 2024

Summary

Using the PCS in a wider context will require absorbing the commitment for Fiat Shamir.

The current challenge is that Absorb is not implemented for AffineRepr, and the current pairing-based schemes in this repo have commitments of the form:

pub struct Commitment<E: Pairing>(
    /// The commitment is a group element.
    pub E::G1Affine,
);

We unfortunately can't add a blanket impl like:

impl<T> Absorb for T
where
    T: AffineRepr,
{
    fn to_sponge_bytes(&self, dest: &mut Vec<u8>) {
        todo!()
    }

    fn to_sponge_field_elements<F: PrimeField>(&self, dest: &mut Vec<F>) {
        todo!()
    }
}

Rust complains "conflicting implementations of trait absorb::Absorb for type u8".

Option 1

We could add Absorb on AffineRepr in the ec crate. However, aside from creating a cyclic dependency from ec to crypto-primitives, I think this is not the best design.

Option 2

One solution is to further restrict G: AffineRepr + Absorb. This propagates into a lot of trait bounds in the poly-commit repo, though. The upside is that it doesn't require any upstream changes, and the two current implementors of AffineRepr which are the structs short_weierstrass::Affine and twisted_edwards::Affine already have Absorb implemented.

Option 3

Larger breaking refactor that makes use of the fact that most pairing computations (and certainly our implementations in ec::models) only ever support SW. The proposal is then to rip out the associated type G1Affine from Pairing, and restrict the class of pairings that can be done to SW-curves, so that we can have:

pub struct Commitment<P: SWCurveConfig>(
    /// The commitment is a group element.
    pub Affine<P>,
);

This almost works, as we still have IPA Commitment struct here in poly-commit which uses the trait AffineRepr (i.e. not an associated type from Pairing):

pub struct Commitment<G: AffineRepr> {
    /// A Pedersen commitment to the polynomial.
    pub comm: G,
    ...
}

And so mixing in Option 2 would need to happen anyway, but limited to one PCS.

Option 4

Any other ideas?

@mmagician mmagician linked a pull request Feb 6, 2024 that will close this issue
6 tasks
@Pratyush
Copy link
Member

Pratyush commented Feb 7, 2024

Hm I don't quite see the problem. Presumably, we want PCCommitment: Absorb only because we want to use PC generically, right?

If so, then the only place the P::G1Affine: Absorb requirement will appear is in

  • impl PCCommitment for Commitment<P>, and
  • impl PolyCommit for KZG/IPA/etc

Does it appear anywhere else?

@mmagician
Copy link
Member Author

@Pratyush see #144

@mahmudsudo
Copy link

can i take on this ?

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 a pull request may close this issue.

3 participants