feat: Modify MakeCommit and AggregateSignature to one operation at on…

…ce (#210)

* feat: Modify `MakeCommit` and `AggregateSignature` to atomic operation

* fix: fix panic error message with `MakeCommit` and add this test

* fix: Correct variable name

* fix: fix flag location

* refactor: move `isEqualVoteWithoutSignature` to vote_test.go and separate `MakeCommit` panic test

* fix: add assert.Fail and log when ed25519 only

consensus/state.go

@@ -1101,7 +1101,6 @@ func (cs *State) createProposalBlock(round int) (block *types.Block, blockParts
 	case cs.LastCommit.HasTwoThirdsMajority():
 		// Make the commit from LastCommit
 		commit = cs.LastCommit.MakeCommit()
-		commit.AggregateSignatures()
 	default: // This shouldn't happen.
 		cs.Logger.Error("enterPropose: Cannot propose anything: No commit for the previous block")
 		return

types/block_test.go

@@ -9,6 +9,7 @@ import (
 	"math"
 	"os"
 	"reflect"
+	"strings"
 	"testing"
 	"time"
 
@@ -288,7 +289,12 @@ func TestCommit(t *testing.T) {
 	require.NotNil(t, commit.BitArray())
 	assert.Equal(t, bits.NewBitArray(10).Size(), commit.BitArray().Size())
 
-	assert.Equal(t, voteSet.GetByIndex(0), commit.GetByIndex(0))
+	if len(voteSet.GetByIndex(0).Signature) != bls.SignatureSize {
+		assert.Equal(t, voteSet.GetByIndex(0), commit.GetByIndex(0))
+	} else {
+		assert.NotNil(t, commit.AggregatedSignature)
+		isEqualVoteWithoutSignature(t, voteSet.GetByIndex(0), commit.GetByIndex(0))
+	}
 }
 
 func TestCommitValidateBasic(t *testing.T) {
@@ -800,23 +806,65 @@ func TestCommitToVoteSet(t *testing.T) {
 	lastID := makeBlockIDRandom()
 	h := int64(3)
 
-	voteSet, _, valSet, vals := randVoteSet(h-1, 1, PrecommitType, 10, 1)
+	voteSet, _, voterSet, vals := randVoteSet(h-1, 1, PrecommitType, 10, 1)
 	commit, err := MakeCommit(lastID, h-1, 1, voteSet, vals, time.Now())
 	assert.NoError(t, err)
 
 	chainID := voteSet.ChainID()
-	voteSet2 := CommitToVoteSet(chainID, commit, valSet)
-
+	voteSet2 := CommitToVoteSet(chainID, commit, voterSet)
 	for i := 0; i < len(vals); i++ {
+		// This is the vote before `MakeCommit`.
 		vote1 := voteSet.GetByIndex(i)
+		// This is the vote created from `CommitToVoteSet`
 		vote2 := voteSet2.GetByIndex(i)
+		// This is the vote created from `MakeCommit`
 		vote3 := commit.GetVote(i)
 
-		vote1bz := cdc.MustMarshalBinaryBare(vote1)
-		vote2bz := cdc.MustMarshalBinaryBare(vote2)
-		vote3bz := cdc.MustMarshalBinaryBare(vote3)
-		assert.Equal(t, vote1bz, vote2bz)
-		assert.Equal(t, vote1bz, vote3bz)
+		if len(vote1.Signature) != bls.SignatureSize {
+			vote1bz := cdc.MustMarshalBinaryBare(vote1)
+			vote2bz := cdc.MustMarshalBinaryBare(vote2)
+			vote3bz := cdc.MustMarshalBinaryBare(vote3)
+			assert.Equal(t, vote1bz, vote2bz)
+			assert.Equal(t, vote1bz, vote3bz)
+		} else {
+			vote2bz := cdc.MustMarshalBinaryBare(vote2)
+			vote3bz := cdc.MustMarshalBinaryBare(vote3)
+			assert.Equal(t, vote2bz, vote3bz)
+			assert.NotNil(t, commit.AggregatedSignature)
+			assert.Nil(t, vote2.Signature)
+			assert.Nil(t, vote3.Signature)
+			isEqualVoteWithoutSignature(t, vote1, vote2)
+			isEqualVoteWithoutSignature(t, vote1, vote3)
+		}
+	}
+}
+
+func TestMakeCommitPanicByAggregatedCommitAndVoteSet(t *testing.T) {
+	lastID := makeBlockIDRandom()
+	h := int64(3)
+
+	voteSet, _, voterSet, vals := randVoteSet(h-1, 1, PrecommitType, 10, 1)
+	commit, err := MakeCommit(lastID, h-1, 1, voteSet, vals, time.Now())
+	assert.NoError(t, err)
+
+	chainID := voteSet.ChainID()
+	voteSet2 := CommitToVoteSet(chainID, commit, voterSet)
+
+	// panic test
+	defer func() {
+		err := recover()
+		if err != nil {
+			wantStr := "This signature of commitSig is already aggregated: commitSig: <"
+			gotStr := fmt.Sprintf("%v", err)
+			isPanic := strings.Contains(gotStr, wantStr)
+			assert.True(t, isPanic)
+		} else {
+			t.Log("Not aggregated")
+		}
+	}()
+	if commit.AggregatedSignature != nil {
+		voteSet2.MakeCommit()
+		assert.Fail(t, "Don't reach here")
 	}
 }
 

types/vote_set.go

@@ -586,14 +586,19 @@ func (voteSet *VoteSet) MakeCommit() *Commit {
 	commitSigs := make([]CommitSig, len(voteSet.votes))
 	for i, v := range voteSet.votes {
 		commitSig := v.CommitSig()
+		if !commitSig.Absent() && commitSig.Signature == nil {
+			panic(fmt.Sprintf("This signature of commitSig is already aggregated: commitSig: <%v>", commitSig))
+		}
 		// if block ID exists but doesn't match, exclude sig
 		if commitSig.ForBlock() && !v.BlockID.Equals(*voteSet.maj23) {
 			commitSig = NewCommitSigAbsent()
 		}
 		commitSigs[i] = commitSig
 	}
+	newCommit := NewCommit(voteSet.GetHeight(), voteSet.GetRound(), *voteSet.maj23, commitSigs)
+	newCommit.AggregateSignatures()
 
-	return NewCommit(voteSet.GetHeight(), voteSet.GetRound(), *voteSet.maj23, commitSigs)
+	return newCommit
 }
 
 //--------------------------------------------------------------------------------

types/vote_set_test.go

@@ -649,7 +649,6 @@ func TestMakeCommit(t *testing.T) {
 		}
 
 		commit := voteSet.MakeCommit()
-		commit.AggregateSignatures()
 
 		assert.Equal(t, height, commit.Height)
 		assert.Equal(t, round, commit.Round)

types/vote_test.go

@@ -44,6 +44,16 @@ func exampleVote(t byte) *Vote {
 	}
 }
 
+func isEqualVoteWithoutSignature(t *testing.T, vote1, vote2 *Vote) {
+	assert.Equal(t, vote1.Type, vote2.Type)
+	assert.Equal(t, vote1.Height, vote2.Height)
+	assert.Equal(t, vote1.Round, vote2.Round)
+	assert.Equal(t, vote1.BlockID, vote2.BlockID)
+	assert.Equal(t, vote1.Timestamp, vote2.Timestamp)
+	assert.Equal(t, vote1.ValidatorAddress, vote2.ValidatorAddress)
+	assert.Equal(t, vote1.ValidatorIndex, vote2.ValidatorIndex)
+}
+
 func TestVoteSignable(t *testing.T) {
 	vote := examplePrecommit()
 	signBytes := vote.SignBytes("test_chain_id")