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

pairing: Adds support for BLS12381 using CIRCL library #49

Merged
merged 3 commits into from
Feb 21, 2024

Conversation

armfazh
Copy link

@armfazh armfazh commented Apr 13, 2023

This PR adds the functionality of BLS12381 pairings using the CIRCL implementation as backend.

CIRCL implementation uses formally-verified implementation (using fiat-crypto) for the prime field arithmetic.

cc: @thibmeu @nikkolasg

Copy link

@nikkolasg nikkolasg left a comment

Choose a reason for hiding this comment

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

Really cool ! Thanks a lot for making this !!!
@AnomalRoil @CluEleSsUK there is one thing im wondering is if this should live inside kyber or be its separate repo so kyber stays mostly the interface definition ? I'm not sure.

func (p *G1Elt) IsInCorrectGroup() bool { return p.inner.IsOnG1() }

func (p *G1Elt) Hash(msg []byte) kyber.Point { p.inner.Hash(msg, nil); return p }
func (p *G1Elt) Hash2(msg, dst []byte) kyber.Point { p.inner.Hash(msg, dst); return p }

Choose a reason for hiding this comment

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

If we want to make that generic, could we put the "dst" as a field in the struct, to avoid having multiple hash functions ?

Copy link
Author

Choose a reason for hiding this comment

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

Once #50 get fixed, there will be only one (correct) API for hashes.

// It's important to note that the Point function will generate a point
// compatible with public keys only (group G2) where the signature must be
// used as a point from the group G1.
type SuiteBLS12381 struct {

Choose a reason for hiding this comment

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

I'm confused by the need for this struct - why do you have it ? Why not just "Suite" is enough ?

Copy link
Author

Choose a reason for hiding this comment

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

it is enough, I was just mimicking what is already done in the other pairing package.

@thibmeu
Copy link

thibmeu commented Apr 13, 2023

Great job! I confirm this does not break tests. It does not replace internal use of github.com/drand/kyber-bls12381 either.

In term of benchmark, I end up with the following on my machine

The current implementation of drand/kyber-bls12381 using go test -bench=.

goos: linux
goarch: amd64
pkg: github.com/drand/kyber-bls12381
BenchmarkPairingSeparate-20    	    1026	   1096506 ns/op
BenchmarkPairingInv-20         	    1720	    724972 ns/op

This commit implementation armfazh/add_cirl_drand using go test -bench=. ./pairing/bls12381

goos: linux
goarch: amd64
pkg: github.com/drand/kyber/pairing/bls12381
BenchmarkPairingSeparate-20          310           3983837 ns/op
BenchmarkPairingInv-20               442           2599874 ns/op

There is about a 4x difference between the two. I'm not sure if there is a safer/more portable implementation which might explain the difference.

Given there is no modification of actual usage (ibe implementation is not modified for instance), the commit is not going to impact downstream dependencies.

@armfazh
Copy link
Author

armfazh commented Apr 13, 2023

There is about a 4x difference between the two. I'm not sure if there is a safer/more portable implementation which might explain the difference.

Regarding performance, one of the reasons of this difference is that the kilic library uses assembly for the prime field arithmetic, while CIRCL uses formally-verified code.

@AnomalRoil
Copy link
Member

Given the way Kyber abstract stuff I think it would make sense to have a kyber-circle repo just like we have a kyber-bls12381 repo that does the interface between kyber and the underlying implementation of the curve.

WDYT?

@AnomalRoil AnomalRoil merged commit ab02d54 into drand:master Feb 21, 2024
1 check passed
@thibmeu
Copy link

thibmeu commented Feb 21, 2024

is the motivation to merge this change the deprecation of kilic/bls12-381, used by drand/kyber-bls12381 ?

@AnomalRoil
Copy link
Member

Not entirely. We are going to be working on merging back drand/kyber into dedis/kyber for the v4 release of dedis/kyber.

However, I have started looking into maintaining a fork of kilic/bls12-381 but the more I dig into it the more bugs I find, so yeah, switching to circl_bls12-381 seems like a plausible option. We'll just need to do some more benchmarking and profiling since we'd really want to be able to run a ~30 node network under the 1s period and need to double check how that would behave wrt aggregation times and so.
If you want to help, it'd be great to have aggregation time benchmarks for various size of polynomials 😇

@armfazh
Copy link
Author

armfazh commented Mar 10, 2024

News: BLS signatures are supported in CIRCL (See bls.go).
Let us know if more functionality is required.

@AnomalRoil
Copy link
Member

The initial goal of Kyber was to have all operations implemented in pure go, so having these kind of wrappers built-in the library is already a bit much, so I don't think the goal is to move away from the current Kyber implementation of primitives other than the elliptic curves anytime soon

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