diff --git a/consensus/state.go b/consensus/state.go index 4dc4b99d7..905247343 100644 --- a/consensus/state.go +++ b/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 diff --git a/types/block_test.go b/types/block_test.go index adbb72184..69a54346c 100644 --- a/types/block_test.go +++ b/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") } } diff --git a/types/vote_set.go b/types/vote_set.go index 1400d6b1d..bdd37fc59 100644 --- a/types/vote_set.go +++ b/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 } //-------------------------------------------------------------------------------- diff --git a/types/vote_set_test.go b/types/vote_set_test.go index 0f25dcdc3..277d83964 100644 --- a/types/vote_set_test.go +++ b/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) diff --git a/types/vote_test.go b/types/vote_test.go index 9a19eb79a..065a65285 100644 --- a/types/vote_test.go +++ b/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")