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 Pluto-Eris cycle of curves #98

Merged
merged 2 commits into from
Dec 4, 2023
Merged

Add Pluto-Eris cycle of curves #98

merged 2 commits into from
Dec 4, 2023

Conversation

davidnevadoc
Copy link
Contributor

Implementation of the half-pairing cycle of prime-order curves described here: https://github.com/daira/pluto-eris

Pluto is a Barreto–Naehrig curve and its pairing implementation closely resembles that of bn256 curve. See the Optimal Ate pairing described in the article: https://eprint.iacr.org/2010/354.pdf.

The constants for the fields and its extensions are derived these notebooks: https://github.com/davidnevadoc/ec-constants/tree/main/pluto_eris

@davidnevadoc davidnevadoc marked this pull request as ready for review November 3, 2023 16:44
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

First of all, nice work. It's really cool to get these curves implemented!

I have a couple of things that have written down while reviewing:

  • README.md needs to be updated mentioning that pluto/eris curves are supported.
  • All tests should be inside test modules please.
  • Add comments for functions left unimplemented. Why were they unimplemented?
  • Add a macro for all Fq/p/p12/6...Repr structs. All of them impl the same things over and over. So it's just boilerplate that we can save up.
  • All of the things that are publicly exported but not docummented should do the exercise of: 1. Think if they need to be exported. 2. if so, add docs. Otherwise change visibility.

src/pluto_eris/curve.rs Outdated Show resolved Hide resolved
src/pluto_eris/curve.rs Show resolved Hide resolved
src/pluto_eris/curve.rs Outdated Show resolved Hide resolved
src/pluto_eris/curve.rs Show resolved Hide resolved
src/pluto_eris/engine.rs Outdated Show resolved Hide resolved
src/pluto_eris/fields/fq.rs Outdated Show resolved Hide resolved
src/pluto_eris/fields/fq.rs Outdated Show resolved Hide resolved
src/pluto_eris/fields/fq.rs Show resolved Hide resolved
src/pluto_eris/fields/fq.rs Outdated Show resolved Hide resolved
src/pluto_eris/fields/mod.rs Outdated Show resolved Hide resolved
@davidnevadoc davidnevadoc mentioned this pull request Nov 23, 2023
@davidnevadoc
Copy link
Contributor Author

davidnevadoc commented Nov 23, 2023

I have addressed the comments relevant to this PR. However there are still some unresolved that I think should be fixed in a different PR. Mainly:

  1. Tests and benchmarks
    Fixing these consistently would involved modifying other curves which is outside of the scope of this PR. ( Just added the test vectors specific for this curves). This should be fixed in Upgrade testing across the lib #76
  2. Documentation and unimplemented functions
    There are some unimplemented and undocumented functions. These are unimplemented or undocumented in bn256.
    I plan to fix both issues in a single PR to avoid duplicating work. I also would like to introduce a common macro for the BN pairing and do some refactoring to improve the code quality in Refactor BN curves. #105

@davidnevadoc davidnevadoc requested a review from CPerezz November 24, 2023 10:15
@davidnevadoc davidnevadoc force-pushed the nev@pluto-eris branch 6 times, most recently from 3446308 to 9b7512d Compare November 29, 2023 18:24
Co-authored-by: Zhiyong-Gong <[email protected]>

address review comments

Remove leftover

add: test vectors for `test_from_u512`

add and clean docs
Complete Pluto Eris docs
Copy link
Member

@CPerezz CPerezz left a comment

Choose a reason for hiding this comment

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

We are inconsistent between our comments.

We have the hex representation of only some values for the constants that are defined in this PR.

I think we should either have them all or none. But it's quite confusing to only have "some of them".

We can address in a follow-up PR if you want. But this has to be addressed at some point.

Other than that, LGTM!

Good work with this!!

@CPerezz
Copy link
Member

CPerezz commented Dec 4, 2023

cc: @daira this might be of your interest!

@davidnevadoc davidnevadoc added this pull request to the merge queue Dec 4, 2023
Merged via the queue into main with commit bdb27cc Dec 4, 2023
7 checks passed
@CPerezz CPerezz deleted the nev@pluto-eris branch December 4, 2023 15:40
jonathanpwang pushed a commit to axiom-crypto/halo2curves that referenced this pull request Dec 15, 2023
* add: Pluto/Eris curves

Co-authored-by: Zhiyong-Gong <[email protected]>

address review comments

Remove leftover

add: test vectors for `test_from_u512`

add and clean docs

* Update Legendre impl

Complete Pluto Eris docs
jonathanpwang pushed a commit to axiom-crypto/halo2curves that referenced this pull request Dec 15, 2023
* add: Pluto/Eris curves

Co-authored-by: Zhiyong-Gong <[email protected]>

address review comments

Remove leftover

add: test vectors for `test_from_u512`

add and clean docs

* Update Legendre impl

Complete Pluto Eris docs
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.

2 participants