Repository: pactus-project/pactus
Pull request #576 (2023-07-05 14:32:36)

refactor: public key aggregate
## Description

Extract Public Key aggregate function from the VerifyAggregated function. Write some tests. There is still a problem

```
func TestPublicKeyAggregate(t *testing.T) {
	pub1, _ := bls.PublicKeyFromString(
		public key string 1)
	pub2, _ := bls.PublicKeyFromString(
		public key string 2)
	agg, _ := bls.PublicKeyFromString(
		aggregated public key)
	assert.True(t, bls.PublicKeyAggregate([]*bls.PublicKey{pub1, pub2}).EqualsTo(agg))
	assert.False(t, pub1.EqualsTo(pub2))
}
```

I dont know how to generate the public key and aggregated public key string value to be used as examples in this test

## Related issue(s)

If this Pull Request is related to an issue, mention it here.


codecov[bot]: ## [Codecov](https://app.codecov.io/gh/pactus-project/pactus/pull/576?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pactus-project) Report
> Merging [#576](https://app.codecov.io/gh/pactus-project/pactus/pull/576?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pactus-project) (ef52258) into [main](https://app.codecov.io/gh/pactus-project/pactus/commit/a967da4e8d762af7d7d720c33d1b5b73921e8aac?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pactus-project) (a967da4) will **increase** coverage by `0.03%`.
> The diff coverage is `83.33%`.


```diff
@@            Coverage Diff             @@
##             main     #576      +/-   ##
==========================================
+ Coverage   83.50%   83.53%   +0.03%     
==========================================
  Files         154      154              
  Lines        7250     7252       +2     
==========================================
+ Hits         6054     6058       +4     
+ Misses        916      915       -1     
+ Partials      280      279       -1     
```



Patch:
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
index 1c82f20f..0fda3bd0 100644
--- a/.github/PULL_REQUEST_TEMPLATE.md
+++ b/.github/PULL_REQUEST_TEMPLATE.md
@@ -6,3 +6,4 @@ Briefly describe the changes introduced by this pull request.
 ## Related issue(s)
 
 If this Pull Request is related to an issue, mention it here.
+- Fixes #(issue number)
\ No newline at end of file
diff --git a/consensus/voteset/voteset.go b/consensus/voteset/voteset.go
index 860463aa..c0006ca4 100644
--- a/consensus/voteset/voteset.go
+++ b/consensus/voteset/voteset.go
@@ -166,7 +166,7 @@ func (vs *VoteSet) ToCertificate() *block.Certificate {
 		committers[i] = val.Number()
 	}
 
-	sig := bls.Aggregate(sigs)
+	sig := bls.SignatureAggregate(sigs)
 
 	return block.NewCertificate(vs.Round(), committers, absentees, sig)
 }
diff --git a/crypto/bls/bls.go b/crypto/bls/bls.go
index 940db5b4..ab5bf7af 100644
--- a/crypto/bls/bls.go
+++ b/crypto/bls/bls.go
@@ -8,7 +8,7 @@ import (
 // https://datatracker.ietf.org/doc/html/draft-irtf-cfrg-bls-signature-04#section-4.2.1
 var dst = []byte("BLS_SIG_BLS12381G1_XMD:SHA-256_SSWU_RO_NUL_")
 
-func Aggregate(sigs []*Signature) *Signature {
+func SignatureAggregate(sigs []*Signature) *Signature {
 	if len(sigs) == 0 {
 		return nil
 	}
@@ -26,22 +26,27 @@ func Aggregate(sigs []*Signature) *Signature {
 	}
 }
 
-func VerifyAggregated(sig *Signature, pubs []*PublicKey, msg []byte) bool {
+func PublicKeyAggregate(pubs []*PublicKey) *PublicKey {
 	if len(pubs) == 0 {
-		return false
+		return nil
 	}
 	g2 := bls12381.NewG2()
 	aggPointG2 := pubs[0].pointG2
 	for i := 1; i < len(pubs); i++ {
 		if g2.IsZero(&pubs[i].pointG2) {
-			return false
+			return nil
 		}
 		g2.Add(
 			&aggPointG2,
 			&aggPointG2,
 			&pubs[i].pointG2)
 	}
+	return &PublicKey{
+		pointG2: aggPointG2,
+	}
+}
 
-	aggPub := PublicKey{pointG2: aggPointG2}
+func VerifyAggregated(sig *Signature, pubs []*PublicKey, msg []byte) bool {
+	aggPub := PublicKeyAggregate(pubs)
 	return aggPub.Verify(msg, sig) == nil
 }


Test patch:
diff --git a/consensus/consensus_test.go b/consensus/consensus_test.go
index 4c579e33..e76ed65b 100644
--- a/consensus/consensus_test.go
+++ b/consensus/consensus_test.go
@@ -290,7 +290,7 @@ func (td *testData) commitBlockForAllStates(t *testing.T) (*block.Block, *block.
 	sig2 := td.signers[1].SignData(sb).(*bls.Signature)
 	sig4 := td.signers[3].SignData(sb).(*bls.Signature)
 
-	sig := bls.Aggregate([]*bls.Signature{sig1, sig2, sig4})
+	sig := bls.SignatureAggregate([]*bls.Signature{sig1, sig2, sig4})
 	cert := block.NewCertificate(0, []int32{0, 1, 2, 3}, []int32{2}, sig)
 	block := p.Block()
 
diff --git a/crypto/bls/bls_test.go b/crypto/bls/bls_test.go
index 26d73e9b..da802584 100644
--- a/crypto/bls/bls_test.go
+++ b/crypto/bls/bls_test.go
@@ -39,7 +39,7 @@ func TestSignatureAggregate(t *testing.T) {
 	sig1 := prv1.Sign(msg).(*bls.Signature)
 	sig2 := prv2.Sign(msg).(*bls.Signature)
 
-	assert.True(t, bls.Aggregate([]*bls.Signature{sig1, sig2}).EqualsTo(agg))
+	assert.True(t, bls.SignatureAggregate([]*bls.Signature{sig1, sig2}).EqualsTo(agg))
 	assert.False(t, prv1.EqualsTo(prv2))
 }
 
@@ -59,17 +59,22 @@ func TestAggregateFailed(t *testing.T) {
 	sig3 := prv3.Sign(msg1).(*bls.Signature)
 	sig4 := prv4.Sign(msg1).(*bls.Signature)
 
-	agg1 := bls.Aggregate([]*bls.Signature{sig1, sig2, sig3})
-	agg2 := bls.Aggregate([]*bls.Signature{sig1, sig2, sig4})
-	agg3 := bls.Aggregate([]*bls.Signature{sig11, sig2, sig3})
-	agg4 := bls.Aggregate([]*bls.Signature{sig1, sig2})
-	agg5 := bls.Aggregate([]*bls.Signature{sig3, sig2, sig1})
+	agg1 := bls.SignatureAggregate([]*bls.Signature{sig1, sig2, sig3})
+	agg2 := bls.SignatureAggregate([]*bls.Signature{sig1, sig2, sig4})
+	agg3 := bls.SignatureAggregate([]*bls.Signature{sig11, sig2, sig3})
+	agg4 := bls.SignatureAggregate([]*bls.Signature{sig1, sig2})
+	agg5 := bls.SignatureAggregate([]*bls.Signature{sig3, sig2, sig1})
 
 	pubs1 := []*bls.PublicKey{pub1, pub2, pub3}
 	pubs2 := []*bls.PublicKey{pub1, pub2, pub4}
 	pubs3 := []*bls.PublicKey{pub1, pub2}
 	pubs4 := []*bls.PublicKey{pub3, pub2, pub1}
 
+	pubAgg1 := bls.PublicKeyAggregate(pubs1)
+	pubAgg2 := bls.PublicKeyAggregate(pubs2)
+	pubAgg3 := bls.PublicKeyAggregate(pubs3)
+	pubAgg4 := bls.PublicKeyAggregate(pubs4)
+
 	assert.NoError(t, pub1.Verify(msg1, sig1))
 	assert.NoError(t, pub2.Verify(msg1, sig2))
 	assert.NoError(t, pub3.Verify(msg1, sig3))
@@ -91,10 +96,24 @@ func TestAggregateFailed(t *testing.T) {
 	assert.False(t, bls.VerifyAggregated(agg1, pubs3, msg1))
 	assert.True(t, bls.VerifyAggregated(agg5, pubs1, msg1))
 	assert.True(t, bls.VerifyAggregated(agg1, pubs4, msg1))
+
+	assert.Nil(t, pubAgg1.Verify(msg1, agg1))
+	assert.NotNil(t, pubAgg1.Verify(msg2, agg1))
+	assert.NotNil(t, pubAgg1.Verify(msg1, agg2))
+	assert.NotNil(t, pubAgg2.Verify(msg1, agg1))
+	assert.Nil(t, pubAgg2.Verify(msg1, agg2))
+	assert.NotNil(t, pubAgg2.Verify(msg2, agg2))
+	assert.NotNil(t, pubAgg1.Verify(msg1, agg3))
+	assert.NotNil(t, pubAgg1.Verify(msg2, agg3))
+	assert.NotNil(t, pubAgg1.Verify(msg1, agg4))
+	assert.NotNil(t, pubAgg3.Verify(msg1, agg1))
+	assert.Nil(t, pubAgg1.Verify(msg1, agg5))
+	assert.Nil(t, pubAgg4.Verify(msg1, agg1))
 }
 
 func TestAggregateNil(t *testing.T) {
-	assert.Nil(t, bls.Aggregate(nil))
+	assert.Nil(t, bls.SignatureAggregate(nil))
+	assert.Nil(t, bls.PublicKeyAggregate(nil))
 }
 
 func TestAggregateOnlyOneSignature(t *testing.T) {
@@ -103,11 +122,20 @@ func TestAggregateOnlyOneSignature(t *testing.T) {
 	_, prv1 := ts.RandomBLSKeyPair()
 	msg1 := []byte("zarb")
 	sig1 := prv1.Sign(msg1).(*bls.Signature)
-	agg1 := bls.Aggregate([]*bls.Signature{sig1})
+	agg1 := bls.SignatureAggregate([]*bls.Signature{sig1})
 
 	assert.True(t, agg1.EqualsTo(sig1))
 }
 
+func TestAggregateOnlyOnePublicKey(t *testing.T) {
+	ts := testsuite.NewTestSuite(t)
+
+	pub1, _ := ts.RandomBLSKeyPair()
+	agg1 := bls.PublicKeyAggregate([]*bls.PublicKey{pub1})
+
+	assert.True(t, agg1.EqualsTo(pub1))
+}
+
 // TODO: should we check for duplication here?
 func TestDuplicatedAggregate(t *testing.T) {
 	ts := testsuite.NewTestSuite(t)
@@ -120,15 +148,21 @@ func TestDuplicatedAggregate(t *testing.T) {
 	sig1 := prv1.Sign(msg1).(*bls.Signature)
 	sig2 := prv2.Sign(msg1).(*bls.Signature)
 
-	agg1 := bls.Aggregate([]*bls.Signature{sig1, sig2, sig1})
-	agg2 := bls.Aggregate([]*bls.Signature{sig1, sig2})
+	agg1 := bls.SignatureAggregate([]*bls.Signature{sig1, sig2, sig1})
+	agg2 := bls.SignatureAggregate([]*bls.Signature{sig1, sig2})
 	assert.False(t, agg1.EqualsTo(agg2))
 
 	pubs1 := []*bls.PublicKey{pub1, pub2}
+	pubs2 := []*bls.PublicKey{pub1, pub2, pub1}
+	pubAgg1 := bls.PublicKeyAggregate(pubs1)
+	pubAgg2 := bls.PublicKeyAggregate(pubs2)
+	assert.False(t, pubAgg1.EqualsTo(pubAgg2))
+
 	assert.False(t, bls.VerifyAggregated(agg1, pubs1, msg1))
+	assert.NotNil(t, pubAgg1.Verify(msg1, agg1))
 
-	pubs2 := []*bls.PublicKey{pub1, pub2, pub1}
 	assert.True(t, bls.VerifyAggregated(agg1, pubs2, msg1))
+	assert.Nil(t, pubAgg2.Verify(msg1, agg1))
 }
 
 // TestHashToCurve ensures that the hash-to-curve function in kilic/bls12-381
diff --git a/state/state_test.go b/state/state_test.go
index 07b4bd2b..e3d5db6c 100644
--- a/state/state_test.go
+++ b/state/state_test.go
@@ -151,7 +151,7 @@ func (td *testData) makeCertificateAndSign(t *testing.T, blockHash hash.Hash, ro
 	}
 
 	absentees := util.Subtracts(committers, signedBy)
-	return block.NewCertificate(round, committers, absentees, bls.Aggregate(sigs))
+	return block.NewCertificate(round, committers, absentees, bls.SignatureAggregate(sigs))
 }
 
 func (td *testData) commitBlockForAllStates(t *testing.T, b *block.Block, c *block.Certificate) {
@@ -449,7 +449,7 @@ func TestSortition(t *testing.T) {
 	sigs[1] = td.valSigner3.SignData(sb).(*bls.Signature)
 	sigs[2] = td.valSigner4.SignData(sb).(*bls.Signature)
 	sigs[3] = signer.SignData(sb).(*bls.Signature)
-	c14 := block.NewCertificate(3, []int32{4, 0, 1, 2, 3}, []int32{0}, bls.Aggregate(sigs))
+	c14 := block.NewCertificate(3, []int32{4, 0, 1, 2, 3}, []int32{0}, bls.SignatureAggregate(sigs))
 
 	height++
 	assert.NoError(t, st1.CommitBlock(height, b14, c14))
diff --git a/state/validation_test.go b/state/validation_test.go
index 68f317eb..f2e28cf4 100644
--- a/state/validation_test.go
+++ b/state/validation_test.go
@@ -16,7 +16,7 @@ func aggregate(sigs []crypto.Signature) *bls.Signature {
 	for i, s := range sigs {
 		blsSigs[i] = s.(*bls.Signature)
 	}
-	return bls.Aggregate(blsSigs)
+	return bls.SignatureAggregate(blsSigs)
 }
 
 func TestCertificateValidation(t *testing.T) {
diff --git a/util/testsuite/testsuite.go b/util/testsuite/testsuite.go
index 4516091c..d65ff52e 100644
--- a/util/testsuite/testsuite.go
+++ b/util/testsuite/testsuite.go
@@ -287,7 +287,7 @@ func (ts *TestSuite) GenerateTestCertificate(blockHash hash.Hash) *block.Certifi
 		priv3.Sign(blockHash.Bytes()).(*bls.Signature),
 		priv4.Sign(blockHash.Bytes()).(*bls.Signature),
 	}
-	sig := bls.Aggregate(sigs)
+	sig := bls.SignatureAggregate(sigs)
 
 	c1 := ts.RandInt32(10)
 	c2 := ts.RandInt32(10) + 10