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 Ligero PCS #125

Closed
wants to merge 132 commits into from
Closed

Add Ligero PCS #125

wants to merge 132 commits into from

Conversation

autquis
Copy link
Contributor

@autquis autquis commented Sep 29, 2023

So, this is Ligero PCS. We had to copy-paste some code from jellyfish since jellyfish depends on the release 0.4.0 of crypto-primitive, which does not have CanonicalSerialize. Once jellyfish updates its dependency, we can use their crate.

Also, in Cargo.toml, we use a specific revision of crypto-primitive since later commits break KZG and other PCSs in the repo. I'll address this issue in another PR.


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 and others added 30 commits June 27, 2023 12:07
autquis and others added 5 commits September 28, 2023 10:07
* Add Breakdown and Ligero to `README.md`

* Fix newline

* Apply suggestions from code review

remove brakedown reference for now

---------

Co-authored-by: mmagician <marcin.gorny.94@protonmail.com>
* make test util functions cfg-gated

* Add constant_poly_test

* Add quadratic tests

* Add linear poly tests

* Add all other tests from test suite

* fix check_combinations

fix combinations

remove prints

remove old comments

* remove deprecated deny

* change the signature of rand_point to allow other fields

* remove tests superseded by automated test suite

* Create a `new` method for `LigeroPCUniversalParams`

* document `LigeroPCUniversalParams`
…clippy lints (#31)

* replace direct construction by `new` call to LigeroPCUniversalParams

* some clippy fixes

* replace `todo()`s by `default()` or `()`

* move implementation of `Ligero` to data_structures

* Add more info to `Ligero` struct incl. link and no-hiding note
* Copy from `jellyfish` and patch dependency

* Use `borrow` from `ark-std`

* Remove default from `PhantomData`
@autquis autquis requested a review from a team as a code owner September 29, 2023 10:56
@autquis autquis requested review from z-tech, mmagician and weikengchen and removed request for a team September 29, 2023 10:56
@mmagician
Copy link
Member

We would like to thank Modulus Labs @Modulus-Labs for the support, motivation and excellent discussions around the implementation and security of Ligero; and Aleo @AleoHQ for the hackathon sponsorship where some of the development and progress were discussed.

* use Vec/ToString from ark_std

* rayon should only be enabled under the parallel feature

* for no-std targets, enable Float arithmetic from num_traits
Comment on lines +194 to +202
pub(crate) struct IOPTranscript<F: PrimeField> {
transcript: Transcript,
is_empty: bool,
#[doc(hidden)]
phantom: PhantomData<F>,
}

// TODO: merge this with jf_plonk::transcript
impl<F: PrimeField> IOPTranscript<F> {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to me be yet another implementation of a sponge. We should try to consolidate all the usages so that we don't have duplication. cc @mmaker

Copy link
Member

Choose a reason for hiding this comment

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

It is indeed a YASI (yet-another-sponge-implentation).
Let me give some background. The idea was to use concrete instantiation directly, instead of adding more generics for the user to choose from. We already have some hashing-related generics on Ligero due to this PCS inherently relying on hashing in multiple places. Since we also rely on interaction, we additionally need to use a Fiat-Shamir transcript as part of the construction. In principle, one could opt in for a different choice for the hash for transcript than what is used for hashing the columns.

There is actually one further spot where potentially a different hash function can (and maybe later, should) be used: hashing the leaves of the Merkle Tree. This distinction becomes evident especially when we consider sth like Poseidon, where the rate of 2 makes much more sense for MT, whereas a higher rate can speed up hashing large columns. So that brings the total choice of hashing parameters (in the non-interactive scheme) to 3. The current design attempts to simplify that interface by forcing a transcript sponge on the user, and also forcing the same hash parameters for column and MT.

I think we could swap out IOPTranscript (which uses merlin under the hood) used here for something from crypto-primitives - the problem is that we don't have any good defaults provided there for the sponge construction AFAIK, and it wasn't clear to me how to safely choose the parameters for non-testing purposes.

Comment on lines 195 to 196
<<C as Config>::TwoToOneHash as TwoToOneCRHScheme>::Parameters: Debug,
<<C as Config>::LeafHash as CRHScheme>::Parameters: Debug,
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should just make Parameters: Debug in crypto-primitives directly? E.g. a companion to this PR: arkworks-rs/crypto-primitives#114

Copy link
Member

Choose a reason for hiding this comment

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

That should be resolved in 61f3c64

Comment on lines +247 to +248
if F::TWO_ADICITY < self.rho_inv as u32 {
0
Copy link
Member

Choose a reason for hiding this comment

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

We should probably just return an error if this happens

Copy link
Member

Choose a reason for hiding this comment

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

We actually do: https://github.com/HungryCatsStudio/poly-commit/blob/46a6a859c3fb15427a1659124d119c550b415701/src/ligero/mod.rs#L80.
Without changing the interface we just propagate the zero to the caller and check there.

Comment on lines 264 to 265
<<C as Config>::TwoToOneHash as TwoToOneCRHScheme>::Parameters: Debug,
<<C as Config>::LeafHash as CRHScheme>::Parameters: Debug,
Copy link
Member

Choose a reason for hiding this comment

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

If you're ignoring the parameters for debugging, then I guess you don't need this bound, no?

Copy link
Member

Choose a reason for hiding this comment

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

Great point, makes the trait bounds much cleaner!

Copy link
Member

Choose a reason for hiding this comment

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

}
}

pub(crate) type LigeroPCPreparedCommitment = ();
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps this should just be the standard commitment, as opposed to ()?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, but maybe instead: #127

Comment on lines 385 to 395
impl PCRandomness for LigeroPCRandomness {
fn empty() -> Self {}

fn rand<R: RngCore>(
_num_queries: usize,
_has_degree_bound: bool,
_num_vars: Option<usize>,
_rng: &mut R,
) -> Self {
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe these methods should return unimplemented!() for now?

Copy link
Member

Choose a reason for hiding this comment

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

Why not

) -> Result<(Self::CommitterKey, Self::VerifierKey), Self::Error> {
if pp.max_degree() == 0 {
return Err(Error::InvalidParameters(
"This field is not suitable for the proposed parameters".to_string(),
Copy link
Member

Choose a reason for hiding this comment

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

Might make sense to make this a const and reuse this.

Copy link
Member

Choose a reason for hiding this comment

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

Done: 780f525

Comment on lines +189 to +190
Self::Randomness: 'a,
Self::Commitment: 'a,
Copy link
Member

Choose a reason for hiding this comment

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

Should we add 'static bounds to Self::Randomness and Self::Commitment in the trait definition directly?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean like:

        ...
        Self::Randomness: 'static,
        Self::Commitment: 'static,
    {
        let mut proof_array = LPCPArray::default();
        let labeled_commitments: Vec<&LabeledCommitment<Self::Commitment>> =
            commitments.into_iter().collect();

?

@Pratyush
Copy link
Member

btw it would be useful to compare notes with https://github.com/arkworks-rs/ldt and https://github.com/arkworks-rs/bcs and see if any infrastructure can be combined/reused

@mmagician
Copy link
Member

Definitely! I was mostly oblivious to those repos.

@mmagician
Copy link
Member

Closing in favour of #132

@mmagician mmagician closed this Nov 3, 2023
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.

4 participants