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 Circl bls12-381 and Kilic bls12-381 + benchmarks #499

Closed
wants to merge 4 commits into from

Conversation

matteosz
Copy link
Contributor

@matteosz matteosz commented Feb 26, 2024

This PR adds to kyber the implementation for the pairing-friendly ecc bls12-381. Two versions of it are implemented, respectively based on the library used.

Here are the results of the benchmarks on a M1 Pro with 8-core using benchstat of kilic vs circl:

goos: darwin
goarch: arm64
pkg: go.dedis.ch/kyber/v3/benchmark/bls
                                              │  kilic.txt  │     circl.txt      │
                                              │   sec/op    │   sec/op     vs base   │
Kilic/AggregatePublicKeys-G1_on_1_signs-8       464.0n ± 2%
Kilic/AggregatePublicKeys-G2_on_1_signs-8       241.6n ± 1%
Kilic/AggregateSign-G1_on_1_signs-8             77.68µ ± 1%
Kilic/AggregateSign-G1_on_1_signs#01-8          172.3µ ± 1%
Kilic/AggregatePublicKeys-G1_on_10_signs-8      19.52µ ± 1%
Kilic/AggregatePublicKeys-G2_on_10_signs-8      7.300µ ± 2%
Kilic/AggregateSign-G1_on_10_signs-8            890.7µ ± 0%
Kilic/AggregateSign-G1_on_10_signs#01-8         1.884m ± 1%
Kilic/AggregatePublicKeys-G1_on_100_signs-8     217.6µ ± 0%
Kilic/AggregatePublicKeys-G2_on_100_signs-8     79.61µ ± 0%
Kilic/AggregateSign-G1_on_100_signs-8           9.094m ± 0%
Kilic/AggregateSign-G1_on_100_signs#01-8        18.96m ± 0%
Kilic/AggregatePublicKeys-G1_on_1000_signs-8    2.362m ± 1%
Kilic/AggregatePublicKeys-G2_on_1000_signs-8    820.8µ ± 2%
Kilic/AggregateSign-G1_on_1000_signs-8          91.10m ± 0%
Kilic/AggregateSign-G1_on_1000_signs#01-8       189.8m ± 1%
Kilic/AggregatePublicKeys-G1_on_10000_signs-8   24.61m ± 5%
Kilic/AggregatePublicKeys-G2_on_10000_signs-8   8.810m ± 2%
Kilic/AggregateSign-G1_on_10000_signs-8         913.3m ± 1%
Kilic/AggregateSign-G1_on_10000_signs#01-8       1.906 ± 0%
Kilic/KeyGen-G1-8                               204.4µ ± 1%
Kilic/KeyGen-G2-8                               90.59µ ± 0%
Kilic/Sign-G1-8                                 219.7µ ± 1%
Kilic/Sign-G2-8                                 479.3µ ± 1%
Kilic/Verify-G1-8                               1.334m ± 2%
Kilic/Verify-G2-8                               1.571m ± 0%
Circl/AggregatePublicKeys-G1_on_1_signs-8                     2.059µ ± 1%
Circl/AggregatePublicKeys-G2_on_1_signs-8                     676.5n ± 1%
Circl/AggregateSign-G1_on_1_signs-8                           124.0µ ± 0%
Circl/AggregateSign-G1_on_1_signs#01-8                        245.6µ ± 1%
Circl/AggregatePublicKeys-G1_on_10_signs-8                    19.49µ ± 1%
Circl/AggregatePublicKeys-G2_on_10_signs-8                    6.038µ ± 0%
Circl/AggregateSign-G1_on_10_signs-8                          1.069m ± 1%
Circl/AggregateSign-G1_on_10_signs#01-8                       2.284m ± 0%
Circl/AggregatePublicKeys-G1_on_100_signs-8                   193.9µ ± 0%
Circl/AggregatePublicKeys-G2_on_100_signs-8                   59.62µ ± 0%
Circl/AggregateSign-G1_on_100_signs-8                         10.53m ± 1%
Circl/AggregateSign-G1_on_100_signs#01-8                      22.62m ± 0%
Circl/AggregatePublicKeys-G1_on_1000_signs-8                  1.941m ± 0%
Circl/AggregatePublicKeys-G2_on_1000_signs-8                  597.1µ ± 0%
Circl/AggregateSign-G1_on_1000_signs-8                        105.3m ± 0%
Circl/AggregateSign-G1_on_1000_signs#01-8                     226.0m ± 2%
Circl/AggregatePublicKeys-G1_on_10000_signs-8                 19.54m ± 0%
Circl/AggregatePublicKeys-G2_on_10000_signs-8                 6.032m ± 1%
Circl/AggregateSign-G1_on_10000_signs-8                        1.054 ± 1%
Circl/AggregateSign-G1_on_10000_signs#01-8                     2.264 ± 0%
Circl/KeyGen-G1-8                                             475.4µ ± 1%
Circl/KeyGen-G2-8                                             159.7µ ± 1%
Circl/Sign-G1-8                                               269.4µ ± 0%
Circl/Sign-G2-8                                               965.6µ ± 1%
Circl/Verify-G1-8                                             1.903m ± 0%
Circl/Verify-G2-8                                             2.402m ± 0%

Side note: researching potential libraries for bls12-381 I found this benchmarks: https://hackmd.io/@gnark/eccbench#BLS12-381, which suggests as best library gnark-crypto, which could be worth to take in consideration.

@matteosz matteosz self-assigned this Feb 26, 2024
@matteosz matteosz marked this pull request as ready for review February 29, 2024 19:39
@matteosz matteosz force-pushed the matteo-bls12381 branch 2 times, most recently from 1ae2641 to 72086f9 Compare March 10, 2024 09:17
@AnomalRoil
Copy link
Contributor

Good point, feel free to do another pr to add a gnark_bls12381 package in the pairing folder 😬

@pierluca
Copy link
Contributor

I seem to recall (I may be wrong here) that we'd discussed it would make sense to put this kind of extra libraries in separate repos. Otherwise it makes it impossible to include Kyber in a project without also depending on all these other extra (competing) dependencies.
Do I remember this incorrectly @matteosz @AnomalRoil ?
We could of course keep everything as-is for now, out of convenience, and just split them out once we're satisfied with the state of the APIs for a v4.

@matteosz
Copy link
Contributor Author

I seem to recall (I may be wrong here) that we'd discussed it would make sense to put this kind of extra libraries in separate repos. Otherwise it makes it impossible to include Kyber in a project without also depending on all these other extra (competing) dependencies. Do I remember this incorrectly @matteosz @AnomalRoil ? We could of course keep everything as-is for now, out of convenience, and just split them out once we're satisfied with the state of the APIs for a v4.

There was already an external repo for kilic made by drand, while the circl was integrated into the kyber drand fork, so the actual original idea was to bring both back into kyber.

My thoughts on that are that I don't see any strong reason for which increasing the dependencies is worse than creating separate repos, one for each extra dependency. This makes the project more fragmented, while still requiring maintenance on each of such separate repos. Besides, it's not a service dependency that we have, where each dependency represents a failure point for the service; those are just stable libraries frozen at some specific versions.

So my preference would go towards having everything centralized but it's just my opinion.

@AnomalRoil
Copy link
Contributor

I guess my only worry with this PR is that it should probably be rebased on #452 and #487 rather than be giant merge commits.

In general I'd advice against every using merge commits except for very small changes or changes on a branch that will be merged back into the original branch (since then the original branch already has these commits).

Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

Also, could you please pull the tests from and add them in the bls12381 folder?
kilic/bls12-381#39

and rename the circl_bls12381 into circl since it's in the bls12381 folder anyway?

Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

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

Ups. Approved by mistake.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ matteosz
❌ moacdevtools
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
25.7% Duplication on New Code (required ≤ 10%)

See analysis details on SonarCloud

@AnomalRoil
Copy link
Contributor

I think you rebased this the wrong way around:

  • take original branch
  • rebase yours ontop
  • Force push to overwrite

@matteosz matteosz mentioned this pull request Mar 25, 2024
@matteosz matteosz closed this Mar 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants