-
Notifications
You must be signed in to change notification settings - Fork 148
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 BLS signatures over BLS12-381 #446
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are passing. Mostly documentation related comments.
I think the code should be better documented. This could be with one big comment at the start with reference to the source algorithm/paper/document, or via multiple inline comments.
sign/bls/bls.go
Outdated
k.pub = new(PublicKey[K]) | ||
switch (interface{})(k).(type) { | ||
case *PrivateKey[G1]: | ||
kk := (interface{})(&k.pub.key).(*G1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question on the naming scheme: what is the reason behind naming inner variables with one additional duplicated letter?
For instance, inner variable of k
becomes kk
, for x
it becomes xx
and then xxx
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it obey to conversions between interfaces to specific structs. The pattern of repeating a letter hints about the origin of the variable.
a796343
to
c8d23eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good codewise for me.
ss.SetBytes(OKM) | ||
|
||
if ss.IsZero() == 1 { | ||
digest := sha256.Sum256(salt) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have a test that covers this branch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, this branch happens when HKDF outputs all-zeros, or a multiple of a prime.
var Q GG.G1 | ||
Q.Hash(msg, []byte(dstG1)) | ||
Q.ScalarMult(&k.key, &Q) | ||
return Q.BytesCompressed() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The compression matches the spec also in the corner cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all the points have fixed-size length. or do you mean something different?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Case of point at infinity.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests for serialization here depend only on the unique ciphersuite: BLS12-381 curve
The encoding format is tested in the ecc/bls12381 package, see: https://github.com/cloudflare/circl/blob/main/ecc/bls12381/encoding_test.go#L73
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please address the test for point for infinity. Otherwise it's ok.
Tests for the point at infinity are checked in the ecc/bls12831 package the other case happens when the signing key is zero. |
Implements the basic version of BLS as in IETF draft.
https://github.com/cfrg/draft-irtf-cfrg-bls-signature