BM-17: Add basic functianalities of bls crypto #12

gitferry · 2022-06-14T09:41:12Z

Fixes https://babylon-chain.atlassian.net/browse/BM-17

This PR adds a wrapper of blst's go binding: https://github.com/supranational/blst/tree/1bd5899e4af46375d58fdfcb21c5ccf74181a35c/bindings/go

It implements basic functionalities around bls crypto as follows:

GenerateBlsKeyPair
SignMsg
VerifyBlsSig
AggregateBlsSigs
AggregateBlsPubKeys
VerifyBlsMultiSig

SebastianElvis · 2022-06-14T09:46:43Z

Great work! Two quick comments:

Perhaps we need to ignore .idea/ stuff
Can we have shorter API names such as Gen, Sign, Verify, AggSigs, AggPKs, and VerifyMulti?
Since we have VerifyBlsMultiSig and do not invoke AggregateBlsPubKeys from outsides, can we make AggregateBlsPubKeys a private function (eg something like aggregateBlsPubKeys)?

gitferry · 2022-06-14T10:06:25Z

Great work! Two quick comments:

Perhaps we need to ignore .idea/ stuff

Can we have shorter API names such as Gen, Sign, Verify, AggSigs, AggPKs, and VerifyMulti?

Since we have VerifyBlsMultiSig and do not invoke AggregateBlsPubKeys from outsides, can we make AggregateBlsPubKeys a private function (eg something like aggregateBlsPubKeys)?

Thanks, @SebastianElvis! Re

Absolutely! Nice catch.
I'm not sure shorter API name is better since it may confuse with other crypto APIs that we might use in the future.
Actually, we might use AggregateBlsPubKeys to cache aggregated pubkeys or to shorten the pubkey size. This API can also be used along with VerifyBlsSig. This is because, with an aggregated bls sig and an aggregated pub key, the verification of the sig is no different from the verification of an individual bls sig.

SebastianElvis · 2022-06-14T10:33:01Z

I'm not sure shorter API name is better since it may confuse with other crypto APIs that we might use in the future.

When we use these APIs we need to write stuff like bls.Sign anyway. I don't have strong objection in this though.

gitferry · 2022-06-14T12:34:37Z

I'm not sure shorter API name is better since it may confuse with other crypto APIs that we might use in the future.

When we use these APIs we need to write stuff like bls.Sign anyway. I don't have strong objection in this though.

Ah, you are right. Let's hear opinions from others.

aakoshh · 2022-06-14T14:15:39Z

Regarding naming, here's some advice: https://go.dev/doc/effective_go#names

As I understand you always evoke functions through the package name, so in this case it would be bls_multisig.AggregateBlsPubKeys. I reckon there's no need to have "BLS" twice in there, bls.AggrPubKeys would be enough, so I would remove Bls from all methods (SignMsg didn't have it anyway).

I don't know if it should be bls_multisig or bls; the former is a bit long, but it can always be renamed during import.
Also don't mind Generate or Gen - I think both Gen and Aggr are easy to recognise.

aakoshh · 2022-06-14T14:22:01Z

crypto/bls12381/bls_multisig.go

+// SignMsg signs on a msg using a bls secret key
+// the returned sig is compressed version with 48 byte size
+func SignMsg(sk *blst.SecretKey, msg []byte) []byte {
+	return new(BlsSig).Sign(sk, msg, dst).Compress()


Shouldn't dst be the name of the return value? I can't see how this works.

It is the Domain Separation Tag for signatures on G1 (minimal-signature-size). See https://github.com/apache/incubator-milagro-crypto-rust/blob/develop/src/bls381/basic.rs.

aakoshh · 2022-06-14T14:23:26Z

crypto/bls12381/bls_multisig.go

+
+// GenerateBlsKeyPair generates a random bls key pair based on a given seed
+// the public key is compressed with 96 byte size
+func GenerateBlsKeyPair(seed []byte) (*blst.SecretKey, []byte) {


It might help to have a struct to wrap the compressed public key, to track the type later.

aakoshh · 2022-06-14T14:24:26Z

crypto/bls12381/bls_multisig.go

+
+// VerifyBlsSig verifies a bls sig over msg with a bls public key
+// the sig and public key are all compressed
+func VerifyBlsSig(sig []byte, pk []byte, msg []byte) bool {


Wrapping signatures and public keys could help make sure we're not mixing them up.

So it would be something like

type Signature []byte type PublicKey []byte func VerifySig(sig Signature, pk PublicKey, msg []byte) bool

aakoshh · 2022-06-14T14:30:29Z

crypto/bls12381/bls_multisig.go

+}
+
+// AggregateBlsSigs aggregates bls sigs into a single bls signature
+func AggregateBlsSigs(sigs [][]byte) ([]byte, bool) {


Would it make sense to differentiate a type Sig []byte from a type MultiSig []byte ? Same for the public key types?

Can it be used to add individual signatures to an already aggregated signature? It should, right?

Can it be used to add individual signatures to an already aggregated signature? It should, right?

Yes. Do I need to add a function like AggrSig(existingSig Signature, newSig Signature)? I think it relates to our previous discussion about accumulating options, right?

Would it make sense to differentiate a type Sig []byte from a type MultiSig []byte ? Same for the public key types?

Would it be better if MultiSig and Sig are both represented as type Signature []byte? They are essentially points on a curve. Also, we can use Verify() to verify both a single bls sig or an aggregated bls sig.

aakoshh · 2022-06-14T14:37:38Z

crypto/bls12381/bls_multisig.go

+}
+
+// AggregateBlsPubKeys aggregates bls public keys into a single bls public key
+func AggregateBlsPubKeys(pks [][]byte) ([]byte, bool) {


Can this aggregate an already aggregated key plus a non-aggregated key?

@aakoshh Yes, a public key and a sig are essentially points on the curve. Do I need to add a function like AggrPK(existingPK PublicKey, newPK PublicKey)?

gitferry · 2022-06-15T10:30:51Z

@SebastianElvis @aakoshh, thanks for your review. I changed the API names to a shorter version and added a wrapper for the signature and public key. Please review it again. Thanks!

SebastianElvis · 2022-06-16T04:42:14Z

crypto/bls12381/types.go

+type Signature []byte
+type PublicKey []byte
+
+func (sig Signature) ToByte() []byte {


Suggested change

func (sig Signature) ToByte() []byte {

func (sig Signature) Bytes() []byte {

This seems to be more consistent with existing Go functions like String(). Not strong objection though.

Agreed. Thanks!

SebastianElvis · 2022-06-16T04:42:25Z

crypto/bls12381/types.go

+	return sig
+}
+
+func (pk PublicKey) ToByte() []byte {


Suggested change

func (pk PublicKey) ToByte() []byte {

func (pk PublicKey) Bytes() []byte {

SebastianElvis · 2022-06-16T04:45:01Z

LGTM except for the above two minor comments without strong objection.

vitsalis

Nice work! Some minor comments:

vitsalis · 2022-06-16T06:52:45Z

crypto/bls12381/bls.go

+
+// GeneKeyPair generates a random bls key pair based on a given seed
+// the public key is compressed with 96 byte size
+func GeneKeyPair(seed []byte) (*blst.SecretKey, PublicKey) {


Suggested change

func GeneKeyPair(seed []byte) (*blst.SecretKey, PublicKey) {

func GenKeyPair(seed []byte) (*blst.SecretKey, PublicKey) {

Nice catch! Thanks.

vitsalis · 2022-06-16T06:55:16Z

crypto/bls12381/bls.go

+		sigBytes[i] = sigs[i].ToByte()
+	}
+	if !aggSig.AggregateCompressed(sigBytes, false) {
+		return nil, false


Maybe it would be better if an error was returned.

or an explanation why we are returning a boolean value.

Agreed. Thanks!

vitsalis · 2022-06-16T06:56:37Z

crypto/bls12381/bls.go

+		pkBytes[i] = pks[i].ToByte()
+	}
+	if !aggPk.AggregateCompressed(pkBytes, false) {
+		return nil, false


Maybe an error is appropriate here as well.

vitsalis · 2022-06-16T06:59:17Z

crypto/bls12381/types.go

+type BlsMultiPubKey = blst.P2Aggregate
+
+// Domain Separation Tag for signatures on G1 (minimal-signature-size)
+var dst = []byte("BLS_SIG_BLS12381G1_XMD:SHA-256_SSWU_RO_NUL_")


Can this be a constant? Also, upper case letters might separate the confusion from the common variable name dst referring to destination.

Nice point. Thanks!

I just found that we may not be able to have this as a constant because Go does not support const array. See https://stackoverflow.com/questions/13137463/declare-a-constant-array

gitferry · 2022-06-16T07:41:29Z

@SebastianElvis @vitsalis Thanks for your comments. I added error return and addressed naming issues.

vitsalis

Thanks for resolving! Some extra very minor comments mostly related to capitalization of BLS across the codebase.

.gitignore

crypto/bls12381/bls.go

crypto/bls12381/bls_test.go

crypto/bls12381/types.go

gitferry · 2022-06-16T10:42:02Z

Thanks @vitsalis for the follow-up comments. I capitalized "bls" in comments but when I changed DST to const, an error occurred. Please see it in the reply. Thanks!

vitsalis

Thanks for the changes! LGTM 🚀

aakoshh · 2022-06-22T11:23:11Z

@gitferry when you merge a PR, please use the "Squash" functionality of Github so main ends up with one commit for your whole PR.

Look, your individual commits are all visible in the main history, versus the other PRs that have a nice commit message pointing at the PR

It's this button you want to click, you can switch to it with the drop down:

gitferry requested a review from SebastianElvis June 14, 2022 09:41

added bls wrapper

9b05bdf

gitferry force-pushed the gai-add-bls-wrapper branch from b41fc7b to 9b05bdf Compare June 14, 2022 10:11

aakoshh reviewed Jun 14, 2022

View reviewed changes

addressed naming according to comments

3812c04

SebastianElvis reviewed Jun 16, 2022

View reviewed changes

SebastianElvis approved these changes Jun 16, 2022

View reviewed changes

vitsalis reviewed Jun 16, 2022

View reviewed changes

added error return

468aac5

vitsalis reviewed Jun 16, 2022

View reviewed changes

capitalized "bls" in comments

73cdfd1

vitsalis approved these changes Jun 16, 2022

View reviewed changes

gitferry merged commit a8c0aa1 into main Jun 16, 2022

gitferry deleted the gai-add-bls-wrapper branch June 16, 2022 12:26

KonradStaniec pushed a commit that referenced this pull request Oct 25, 2023

BTC staking: voting power table for each Babylon height (#12)

982da21

	func (sig Signature) ToByte() []byte {
	func (sig Signature) Bytes() []byte {

	func (pk PublicKey) ToByte() []byte {
	func (pk PublicKey) Bytes() []byte {

	func GeneKeyPair(seed []byte) (*blst.SecretKey, PublicKey) {
	func GenKeyPair(seed []byte) (*blst.SecretKey, PublicKey) {

BM-17: Add basic functianalities of bls crypto #12

BM-17: Add basic functianalities of bls crypto #12

Conversation

gitferry commented Jun 14, 2022

SebastianElvis commented Jun 14, 2022 • edited Loading

gitferry commented Jun 14, 2022

SebastianElvis commented Jun 14, 2022

gitferry commented Jun 14, 2022

aakoshh commented Jun 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry Jun 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry commented Jun 15, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Jun 16, 2022

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitferry Jun 16, 2022 • edited Loading

Choose a reason for hiding this comment

gitferry commented Jun 16, 2022

vitsalis left a comment

Choose a reason for hiding this comment

gitferry commented Jun 16, 2022

vitsalis left a comment

Choose a reason for hiding this comment

aakoshh commented Jun 22, 2022

SebastianElvis commented Jun 14, 2022 •

edited

Loading

gitferry Jun 15, 2022 •

edited

Loading

gitferry Jun 16, 2022 •

edited

Loading