types: more test cases for TestValidatorSet_VerifyCommit (#5018)

Refs #4926

types/block.go

@@ -763,10 +763,15 @@ func (commit *Commit) GetVote(valIdx int32) *Vote {
 	}
 }
 
-// VoteSignBytes constructs the SignBytes for the given CommitSig.
-// The only unique part of the SignBytes is the Timestamp - all other fields
-// signed over are otherwise the same for all validators.
+// VoteSignBytes returns the bytes of the Vote corresponding to valIdx for
+// signing.
+//
+// The only unique part is the Timestamp - all other fields signed over are
+// otherwise the same for all validators.
+//
 // Panics if valIdx >= commit.Size().
+//
+// See VoteSignBytes
 func (commit *Commit) VoteSignBytes(chainID string, valIdx int32) []byte {
 	v := commit.GetVote(valIdx).ToProto()
 	return VoteSignBytes(chainID, v)

types/canonical.go

@@ -33,10 +33,12 @@ func CanonicalizeBlockID(bid tmproto.BlockID) *tmproto.CanonicalBlockID {
 	return cbid
 }
 
+// CanonicalizeVote transforms the given PartSetHeader to a CanonicalPartSetHeader.
 func CanonicalizePartSetHeader(psh tmproto.PartSetHeader) tmproto.CanonicalPartSetHeader {
 	return tmproto.CanonicalPartSetHeader(psh)
 }
 
+// CanonicalizeVote transforms the given Proposal to a CanonicalProposal.
 func CanonicalizeProposal(chainID string, proposal *tmproto.Proposal) tmproto.CanonicalProposal {
 	return tmproto.CanonicalProposal{
 		Type:      tmproto.ProposalType,
@@ -49,6 +51,8 @@ func CanonicalizeProposal(chainID string, proposal *tmproto.Proposal) tmproto.Ca
 	}
 }
 
+// CanonicalizeVote transforms the given Vote to a CanonicalVote, which does
+// not contain ValidatorIndex and ValidatorAddress fields.
 func CanonicalizeVote(chainID string, vote *tmproto.Vote) tmproto.CanonicalVote {
 	return tmproto.CanonicalVote{
 		Type:      vote.Type,

types/proposal.go

@@ -90,7 +90,12 @@ func (p *Proposal) String() string {
 		CanonicalTime(p.Timestamp))
 }
 
-// ProposalSignBytes returns the Proposal bytes for signing
+// ProposalSignBytes returns the proto-encoding of the canonicalized Proposal,
+// for signing.
+//
+// Panics if the marshaling fails.
+//
+// See CanonicalizeProposal
 func ProposalSignBytes(chainID string, p *tmproto.Proposal) []byte {
 	pb := CanonicalizeProposal(chainID, p)
 	bz, err := protoio.MarshalDelimited(&pb)

types/validator_set_test.go

@@ -18,7 +18,6 @@ import (
 	tmmath "github.com/tendermint/tendermint/libs/math"
 	tmrand "github.com/tendermint/tendermint/libs/rand"
 	tmproto "github.com/tendermint/tendermint/proto/types"
-	tmtime "github.com/tendermint/tendermint/types/time"
 )
 
 func TestValidatorSetBasic(t *testing.T) {
@@ -663,64 +662,69 @@ func TestSafeSubClip(t *testing.T) {
 
 //-------------------------------------------------------------------
 
-func TestValidatorSetVerifyCommit(t *testing.T) {
-	privKey := ed25519.GenPrivKey()
-	pubKey := privKey.PubKey()
-	v1 := NewValidator(pubKey, 1000)
-	vset := NewValidatorSet([]*Validator{v1})
-
-	// good
+func TestValidatorSet_VerifyCommit(t *testing.T) {
 	var (
-		chainID = "mychainID"
-		blockID = makeBlockIDRandom()
-		height  = int64(5)
+		privKey = ed25519.GenPrivKey()
+		pubKey  = privKey.PubKey()
+		v1      = NewValidator(pubKey, 1000)
+		vset    = NewValidatorSet([]*Validator{v1})
+
+		chainID = "Lalande21185"
 	)
-	vote := &Vote{
-		ValidatorAddress: v1.Address,
-		ValidatorIndex:   0,
-		Height:           height,
-		Round:            0,
-		Timestamp:        tmtime.Now(),
-		Type:             tmproto.PrecommitType,
-		BlockID:          blockID,
-	}
 
+	vote := examplePrecommit()
 	v := vote.ToProto()
 	sig, err := privKey.Sign(VoteSignBytes(chainID, v))
-	assert.NoError(t, err)
+	require.NoError(t, err)
 	vote.Signature = sig
-	commit := NewCommit(vote.Height, vote.Round, blockID, []CommitSig{vote.CommitSig()})
 
-	// bad
-	var (
-		badChainID = "notmychainID"
-		badBlockID = BlockID{Hash: []byte("goodbye")}
-		badHeight  = height + 1
-		badCommit  = NewCommit(badHeight, 0, blockID, []CommitSig{{BlockIDFlag: BlockIDFlagAbsent}})
-	)
+	commit := NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{vote.CommitSig()})
+
+	vote2 := *vote
+	sig2, err := privKey.Sign(VoteSignBytes("EpsilonEridani", v))
+	require.NoError(t, err)
+	vote2.Signature = sig2
 
-	// test some error cases
-	// TODO: test more cases!
-	cases := []struct {
-		chainID string
-		blockID BlockID
-		height  int64
-		commit  *Commit
+	testCases := []struct {
+		description string
+		chainID     string
+		blockID     BlockID
+		height      int64
+		commit      *Commit
+		expErr      bool
 	}{
-		{badChainID, blockID, height, commit},
-		{chainID, badBlockID, height, commit},
-		{chainID, blockID, badHeight, commit},
-		{chainID, blockID, height, badCommit},
-	}
+		{"good", chainID, vote.BlockID, vote.Height, commit, false},
+
+		{"wrong signature (#0)", "EpsilonEridani", vote.BlockID, vote.Height, commit, true},
+		{"wrong block ID", chainID, makeBlockIDRandom(), vote.Height, commit, true},
+		{"wrong height", chainID, vote.BlockID, vote.Height - 1, commit, true},
+
+		{"wrong set size: 1 vs 0", chainID, vote.BlockID, vote.Height,
+			NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{}), true},
+
+		{"wrong set size: 1 vs 2", chainID, vote.BlockID, vote.Height,
+			NewCommit(vote.Height, vote.Round, vote.BlockID,
+				[]CommitSig{vote.CommitSig(), {BlockIDFlag: BlockIDFlagAbsent}}), true},
+
+		{"insufficient voting power: got 0, needed more than 666", chainID, vote.BlockID, vote.Height,
+			NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{{BlockIDFlag: BlockIDFlagAbsent}}), true},
 
-	for i, c := range cases {
-		err := vset.VerifyCommit(c.chainID, c.blockID, c.height, c.commit)
-		assert.NotNil(t, err, i)
+		{"wrong signature (#0)", chainID, vote.BlockID, vote.Height,
+			NewCommit(vote.Height, vote.Round, vote.BlockID, []CommitSig{vote2.CommitSig()}), true},
 	}
 
-	// test a good one
-	err = vset.VerifyCommit(chainID, blockID, height, commit)
-	assert.Nil(t, err)
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.description, func(t *testing.T) {
+			err := vset.VerifyCommit(tc.chainID, tc.blockID, tc.height, tc.commit)
+			if tc.expErr {
+				assert.Error(t, err)
+				assert.Contains(t, err.Error(), tc.description)
+			} else {
+				assert.NoError(t, err)
+			}
+		})
+	}
 }
 
 func TestEmptySet(t *testing.T) {
@@ -1407,7 +1411,7 @@ func TestValSetUpdateOverflowRelated(t *testing.T) {
 	}
 }
 
-func TestVerifyCommitTrusting(t *testing.T) {
+func TestValidatorSet_VerifyCommitTrusting(t *testing.T) {
 	var (
 		blockID                       = makeBlockIDRandom()
 		voteSet, originalValset, vals = randVoteSet(1, 1, tmproto.PrecommitType, 6, 1)
@@ -1448,7 +1452,7 @@ func TestVerifyCommitTrusting(t *testing.T) {
 	}
 }
 
-func TestVerifyCommitTrustingErrorsOnOverflow(t *testing.T) {
+func TestValidatorSet_VerifyCommitTrustingErrorsOnOverflow(t *testing.T) {
 	var (
 		blockID               = makeBlockIDRandom()
 		voteSet, valSet, vals = randVoteSet(1, 1, tmproto.PrecommitType, 1, MaxTotalVotingPower)

types/vote.go

@@ -82,8 +82,12 @@ func (vote *Vote) CommitSig() CommitSig {
 	}
 }
 
-//VoteSignBytes take the chainID & a vote, represented in protobuf, and creates a signature.
-// If any error arises this will panic
+// VoteSignBytes returns the proto-encoding of the canonicalized Vote, for
+// signing.
+//
+// Panics if the marshaling fails.
+//
+// See CanonicalizeVote
 func VoteSignBytes(chainID string, vote *tmproto.Vote) []byte {
 	pb := CanonicalizeVote(chainID, vote)
 	bz, err := protoio.MarshalDelimited(&pb)

types/vote_set_test.go

@@ -13,82 +13,19 @@ import (
 	tmtime "github.com/tendermint/tendermint/types/time"
 )
 
-// NOTE: privValidators are in order
-func randVoteSet(
-	height int64,
-	round int32,
-	signedMsgType tmproto.SignedMsgType,
-	numValidators int,
-	votingPower int64,
-) (*VoteSet, *ValidatorSet, []PrivValidator) {
-	valSet, privValidators := RandValidatorSet(numValidators, votingPower)
-	return NewVoteSet("test_chain_id", height, round, signedMsgType, valSet), valSet, privValidators
-}
-
-// Convenience: Return new vote with different validator address/index
-func withValidator(vote *Vote, addr []byte, idx int32) *Vote {
-	vote = vote.Copy()
-	vote.ValidatorAddress = addr
-	vote.ValidatorIndex = idx
-	return vote
-}
-
-// Convenience: Return new vote with different height
-func withHeight(vote *Vote, height int64) *Vote {
-	vote = vote.Copy()
-	vote.Height = height
-	return vote
-}
-
-// Convenience: Return new vote with different round
-func withRound(vote *Vote, round int32) *Vote {
-	vote = vote.Copy()
-	vote.Round = round
-	return vote
-}
-
-// Convenience: Return new vote with different type
-func withType(vote *Vote, signedMsgType byte) *Vote {
-	vote = vote.Copy()
-	vote.Type = tmproto.SignedMsgType(signedMsgType)
-	return vote
-}
-
-// Convenience: Return new vote with different blockHash
-func withBlockHash(vote *Vote, blockHash []byte) *Vote {
-	vote = vote.Copy()
-	vote.BlockID.Hash = blockHash
-	return vote
-}
-
-// Convenience: Return new vote with different blockParts
-func withBlockPartsHeader(vote *Vote, blockPartsHeader PartSetHeader) *Vote {
-	vote = vote.Copy()
-	vote.BlockID.PartsHeader = blockPartsHeader
-	return vote
-}
-
-func TestAddVote(t *testing.T) {
+func TestVoteSet_AddVote_Good(t *testing.T) {
 	height, round := int64(1), int32(0)
 	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrevoteType, 10, 1)
 	val0 := privValidators[0]
 
-	// t.Logf(">> %v", voteSet)
-
 	val0p, err := val0.GetPubKey()
 	require.NoError(t, err)
 	val0Addr := val0p.Address()
 
-	if voteSet.GetByAddress(val0Addr) != nil {
-		t.Errorf("expected GetByAddress(val0.Address) to be nil")
-	}
-	if voteSet.BitArray().GetIndex(0) {
-		t.Errorf("expected BitArray.GetIndex(0) to be false")
-	}
+	assert.Nil(t, voteSet.GetByAddress(val0Addr))
+	assert.False(t, voteSet.BitArray().GetIndex(0))
 	blockID, ok := voteSet.TwoThirdsMajority()
-	if ok || !blockID.IsZero() {
-		t.Errorf("there should be no 2/3 majority")
-	}
+	assert.False(t, ok || !blockID.IsZero(), "there should be no 2/3 majority")
 
 	vote := &Vote{
 		ValidatorAddress: val0Addr,
@@ -100,23 +37,90 @@ func TestAddVote(t *testing.T) {
 		BlockID:          BlockID{nil, PartSetHeader{}},
 	}
 	_, err = signAddVote(val0, vote, voteSet)
-	if err != nil {
-		t.Error(err)
+	require.NoError(t, err)
+
+	assert.NotNil(t, voteSet.GetByAddress(val0Addr))
+	assert.True(t, voteSet.BitArray().GetIndex(0))
+	blockID, ok = voteSet.TwoThirdsMajority()
+	assert.False(t, ok || !blockID.IsZero(), "there should be no 2/3 majority")
+}
+
+func TestVoteSet_AddVote_Bad(t *testing.T) {
+	height, round := int64(1), int32(0)
+	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrevoteType, 10, 1)
+
+	voteProto := &Vote{
+		ValidatorAddress: nil,
+		ValidatorIndex:   -1,
+		Height:           height,
+		Round:            round,
+		Timestamp:        tmtime.Now(),
+		Type:             tmproto.PrevoteType,
+		BlockID:          BlockID{nil, PartSetHeader{}},
 	}
 
-	if voteSet.GetByAddress(val0Addr) == nil {
-		t.Errorf("expected GetByAddress(val0.Address) to be present")
+	// val0 votes for nil.
+	{
+		pubKey, err := privValidators[0].GetPubKey()
+		require.NoError(t, err)
+		addr := pubKey.Address()
+		vote := withValidator(voteProto, addr, 0)
+		added, err := signAddVote(privValidators[0], vote, voteSet)
+		if !added || err != nil {
+			t.Errorf("expected VoteSet.Add to succeed")
+		}
 	}
-	if !voteSet.BitArray().GetIndex(0) {
-		t.Errorf("expected BitArray.GetIndex(0) to be true")
+
+	// val0 votes again for some block.
+	{
+		pubKey, err := privValidators[0].GetPubKey()
+		require.NoError(t, err)
+		addr := pubKey.Address()
+		vote := withValidator(voteProto, addr, 0)
+		added, err := signAddVote(privValidators[0], withBlockHash(vote, tmrand.Bytes(32)), voteSet)
+		if added || err == nil {
+			t.Errorf("expected VoteSet.Add to fail, conflicting vote.")
+		}
 	}
-	blockID, ok = voteSet.TwoThirdsMajority()
-	if ok || !blockID.IsZero() {
-		t.Errorf("there should be no 2/3 majority")
+
+	// val1 votes on another height
+	{
+		pubKey, err := privValidators[1].GetPubKey()
+		require.NoError(t, err)
+		addr := pubKey.Address()
+		vote := withValidator(voteProto, addr, 1)
+		added, err := signAddVote(privValidators[1], withHeight(vote, height+1), voteSet)
+		if added || err == nil {
+			t.Errorf("expected VoteSet.Add to fail, wrong height")
+		}
+	}
+
+	// val2 votes on another round
+	{
+		pubKey, err := privValidators[2].GetPubKey()
+		require.NoError(t, err)
+		addr := pubKey.Address()
+		vote := withValidator(voteProto, addr, 2)
+		added, err := signAddVote(privValidators[2], withRound(vote, round+1), voteSet)
+		if added || err == nil {
+			t.Errorf("expected VoteSet.Add to fail, wrong round")
+		}
+	}
+
+	// val3 votes of another type.
+	{
+		pubKey, err := privValidators[3].GetPubKey()
+		require.NoError(t, err)
+		addr := pubKey.Address()
+		vote := withValidator(voteProto, addr, 3)
+		added, err := signAddVote(privValidators[3], withType(vote, byte(tmproto.PrecommitType)), voteSet)
+		if added || err == nil {
+			t.Errorf("expected VoteSet.Add to fail, wrong type")
+		}
 	}
 }
 
-func Test2_3Majority(t *testing.T) {
+func TestVoteSet_2_3Majority(t *testing.T) {
 	height, round := int64(1), int32(0)
 	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrevoteType, 10, 1)
 
@@ -136,14 +140,10 @@ func Test2_3Majority(t *testing.T) {
 		addr := pubKey.Address()
 		vote := withValidator(voteProto, addr, i)
 		_, err = signAddVote(privValidators[i], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 	}
 	blockID, ok := voteSet.TwoThirdsMajority()
-	if ok || !blockID.IsZero() {
-		t.Errorf("there should be no 2/3 majority")
-	}
+	assert.False(t, ok || !blockID.IsZero(), "there should be no 2/3 majority")
 
 	// 7th validator voted for some blockhash
 	{
@@ -152,13 +152,9 @@ func Test2_3Majority(t *testing.T) {
 		addr := pubKey.Address()
 		vote := withValidator(voteProto, addr, 6)
 		_, err = signAddVote(privValidators[6], withBlockHash(vote, tmrand.Bytes(32)), voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 		blockID, ok = voteSet.TwoThirdsMajority()
-		if ok || !blockID.IsZero() {
-			t.Errorf("there should be no 2/3 majority")
-		}
+		assert.False(t, ok || !blockID.IsZero(), "there should be no 2/3 majority")
 	}
 
 	// 8th validator voted for nil.
@@ -168,17 +164,13 @@ func Test2_3Majority(t *testing.T) {
 		addr := pubKey.Address()
 		vote := withValidator(voteProto, addr, 7)
 		_, err = signAddVote(privValidators[7], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 		blockID, ok = voteSet.TwoThirdsMajority()
-		if !ok || !blockID.IsZero() {
-			t.Errorf("there should be 2/3 majority for nil")
-		}
+		assert.True(t, ok || blockID.IsZero(), "there should be 2/3 majority for nil")
 	}
 }
 
-func Test2_3MajorityRedux(t *testing.T) {
+func TestVoteSet_2_3MajorityRedux(t *testing.T) {
 	height, round := int64(1), int32(0)
 	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrevoteType, 100, 1)
 
@@ -203,14 +195,11 @@ func Test2_3MajorityRedux(t *testing.T) {
 		addr := pubKey.Address()
 		vote := withValidator(voteProto, addr, i)
 		_, err = signAddVote(privValidators[i], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 	}
 	blockID, ok := voteSet.TwoThirdsMajority()
-	if ok || !blockID.IsZero() {
-		t.Errorf("there should be no 2/3 majority")
-	}
+	assert.False(t, ok || !blockID.IsZero(),
+		"there should be no 2/3 majority")
 
 	// 67th validator voted for nil
 	{
@@ -219,13 +208,10 @@ func Test2_3MajorityRedux(t *testing.T) {
 		adrr := pubKey.Address()
 		vote := withValidator(voteProto, adrr, 66)
 		_, err = signAddVote(privValidators[66], withBlockHash(vote, nil), voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 		blockID, ok = voteSet.TwoThirdsMajority()
-		if ok || !blockID.IsZero() {
-			t.Errorf("there should be no 2/3 majority: last vote added was nil")
-		}
+		assert.False(t, ok || !blockID.IsZero(),
+			"there should be no 2/3 majority: last vote added was nil")
 	}
 
 	// 68th validator voted for a different BlockParts PartSetHeader
@@ -236,13 +222,10 @@ func Test2_3MajorityRedux(t *testing.T) {
 		vote := withValidator(voteProto, addr, 67)
 		blockPartsHeader := PartSetHeader{blockPartsTotal, crypto.CRandBytes(32)}
 		_, err = signAddVote(privValidators[67], withBlockPartsHeader(vote, blockPartsHeader), voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 		blockID, ok = voteSet.TwoThirdsMajority()
-		if ok || !blockID.IsZero() {
-			t.Errorf("there should be no 2/3 majority: last vote added had different PartSetHeader Hash")
-		}
+		assert.False(t, ok || !blockID.IsZero(),
+			"there should be no 2/3 majority: last vote added had different PartSetHeader Hash")
 	}
 
 	// 69th validator voted for different BlockParts Total
@@ -253,13 +236,10 @@ func Test2_3MajorityRedux(t *testing.T) {
 		vote := withValidator(voteProto, addr, 68)
 		blockPartsHeader := PartSetHeader{blockPartsTotal + 1, blockPartsHeader.Hash}
 		_, err = signAddVote(privValidators[68], withBlockPartsHeader(vote, blockPartsHeader), voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 		blockID, ok = voteSet.TwoThirdsMajority()
-		if ok || !blockID.IsZero() {
-			t.Errorf("there should be no 2/3 majority: last vote added had different PartSetHeader Total")
-		}
+		assert.False(t, ok || !blockID.IsZero(),
+			"there should be no 2/3 majority: last vote added had different PartSetHeader Total")
 	}
 
 	// 70th validator voted for different BlockHash
@@ -269,13 +249,10 @@ func Test2_3MajorityRedux(t *testing.T) {
 		addr := pubKey.Address()
 		vote := withValidator(voteProto, addr, 69)
 		_, err = signAddVote(privValidators[69], withBlockHash(vote, tmrand.Bytes(32)), voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 		blockID, ok = voteSet.TwoThirdsMajority()
-		if ok || !blockID.IsZero() {
-			t.Errorf("there should be no 2/3 majority: last vote added had different BlockHash")
-		}
+		assert.False(t, ok || !blockID.IsZero(),
+			"there should be no 2/3 majority: last vote added had different BlockHash")
 	}
 
 	// 71st validator voted for the right BlockHash & BlockPartsHeader
@@ -285,92 +262,14 @@ func Test2_3MajorityRedux(t *testing.T) {
 		addr := pubKey.Address()
 		vote := withValidator(voteProto, addr, 70)
 		_, err = signAddVote(privValidators[70], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
-		blockID, ok = voteSet.TwoThirdsMajority()
-		if !ok || !blockID.Equals(BlockID{blockHash, blockPartsHeader}) {
-			t.Errorf("there should be 2/3 majority")
-		}
-	}
-}
-
-func TestBadVotes(t *testing.T) {
-	height, round := int64(1), int32(0)
-	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrevoteType, 10, 1)
-
-	voteProto := &Vote{
-		ValidatorAddress: nil,
-		ValidatorIndex:   -1,
-		Height:           height,
-		Round:            round,
-		Timestamp:        tmtime.Now(),
-		Type:             tmproto.PrevoteType,
-		BlockID:          BlockID{nil, PartSetHeader{}},
-	}
-
-	// val0 votes for nil.
-	{
-		pubKey, err := privValidators[0].GetPubKey()
 		require.NoError(t, err)
-		addr := pubKey.Address()
-		vote := withValidator(voteProto, addr, 0)
-		added, err := signAddVote(privValidators[0], vote, voteSet)
-		if !added || err != nil {
-			t.Errorf("expected VoteSet.Add to succeed")
-		}
-	}
-
-	// val0 votes again for some block.
-	{
-		pubKey, err := privValidators[0].GetPubKey()
-		require.NoError(t, err)
-		addr := pubKey.Address()
-		vote := withValidator(voteProto, addr, 0)
-		added, err := signAddVote(privValidators[0], withBlockHash(vote, tmrand.Bytes(32)), voteSet)
-		if added || err == nil {
-			t.Errorf("expected VoteSet.Add to fail, conflicting vote.")
-		}
-	}
-
-	// val1 votes on another height
-	{
-		pubKey, err := privValidators[1].GetPubKey()
-		require.NoError(t, err)
-		addr := pubKey.Address()
-		vote := withValidator(voteProto, addr, 1)
-		added, err := signAddVote(privValidators[1], withHeight(vote, height+1), voteSet)
-		if added || err == nil {
-			t.Errorf("expected VoteSet.Add to fail, wrong height")
-		}
-	}
-
-	// val2 votes on another round
-	{
-		pubKey, err := privValidators[2].GetPubKey()
-		require.NoError(t, err)
-		addr := pubKey.Address()
-		vote := withValidator(voteProto, addr, 2)
-		added, err := signAddVote(privValidators[2], withRound(vote, round+1), voteSet)
-		if added || err == nil {
-			t.Errorf("expected VoteSet.Add to fail, wrong round")
-		}
-	}
-
-	// val3 votes of another type.
-	{
-		pubKey, err := privValidators[3].GetPubKey()
-		require.NoError(t, err)
-		addr := pubKey.Address()
-		vote := withValidator(voteProto, addr, 3)
-		added, err := signAddVote(privValidators[3], withType(vote, byte(tmproto.PrecommitType)), voteSet)
-		if added || err == nil {
-			t.Errorf("expected VoteSet.Add to fail, wrong type")
-		}
+		blockID, ok = voteSet.TwoThirdsMajority()
+		assert.True(t, ok && blockID.Equals(BlockID{blockHash, blockPartsHeader}),
+			"there should be 2/3 majority")
 	}
 }
 
-func TestConflicts(t *testing.T) {
+func TestVoteSet_Conflicts(t *testing.T) {
 	height, round := int64(1), int32(0)
 	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrevoteType, 4, 1)
 	blockHash1 := tmrand.Bytes(32)
@@ -403,12 +302,8 @@ func TestConflicts(t *testing.T) {
 	{
 		vote := withValidator(voteProto, val0Addr, 0)
 		added, err := signAddVote(privValidators[0], withBlockHash(vote, blockHash1), voteSet)
-		if added {
-			t.Errorf("expected VoteSet.Add to fail, conflicting vote.")
-		}
-		if err == nil {
-			t.Errorf("expected VoteSet.Add to return error, conflicting vote.")
-		}
+		assert.False(t, added, "conflicting vote")
+		assert.Error(t, err, "conflicting vote")
 	}
 
 	// start tracking blockHash1
@@ -418,12 +313,8 @@ func TestConflicts(t *testing.T) {
 	{
 		vote := withValidator(voteProto, val0Addr, 0)
 		added, err := signAddVote(privValidators[0], withBlockHash(vote, blockHash1), voteSet)
-		if !added {
-			t.Errorf("expected VoteSet.Add to succeed, called SetPeerMaj23().")
-		}
-		if err == nil {
-			t.Errorf("expected VoteSet.Add to return error, conflicting vote.")
-		}
+		assert.True(t, added, "called SetPeerMaj23()")
+		assert.Error(t, err, "conflicting vote")
 	}
 
 	// attempt tracking blockHash2, should fail because already set for peerA.
@@ -433,12 +324,8 @@ func TestConflicts(t *testing.T) {
 	{
 		vote := withValidator(voteProto, val0Addr, 0)
 		added, err := signAddVote(privValidators[0], withBlockHash(vote, blockHash2), voteSet)
-		if added {
-			t.Errorf("expected VoteSet.Add to fail, duplicate SetPeerMaj23() from peerA")
-		}
-		if err == nil {
-			t.Errorf("expected VoteSet.Add to return error, conflicting vote.")
-		}
+		assert.False(t, added, "duplicate SetPeerMaj23() from peerA")
+		assert.Error(t, err, "conflicting vote")
 	}
 
 	// val1 votes for blockHash1.
@@ -491,12 +378,8 @@ func TestConflicts(t *testing.T) {
 		addr := pv.Address()
 		vote := withValidator(voteProto, addr, 2)
 		added, err := signAddVote(privValidators[2], withBlockHash(vote, blockHash1), voteSet)
-		if !added {
-			t.Errorf("expected VoteSet.Add to succeed")
-		}
-		if err == nil {
-			t.Errorf("expected VoteSet.Add to return error, conflicting vote")
-		}
+		assert.True(t, added)
+		assert.Error(t, err, "conflicting vote")
 	}
 
 	// check
@@ -510,10 +393,9 @@ func TestConflicts(t *testing.T) {
 	if !voteSet.HasTwoThirdsAny() {
 		t.Errorf("we should have 2/3 if any votes")
 	}
-
 }
 
-func TestMakeCommit(t *testing.T) {
+func TestVoteSet_MakeCommit(t *testing.T) {
 	height, round := int64(1), int32(0)
 	voteSet, _, privValidators := randVoteSet(height, round, tmproto.PrecommitType, 10, 1)
 	blockHash, blockPartsHeader := crypto.CRandBytes(32), PartSetHeader{123, crypto.CRandBytes(32)}
@@ -553,9 +435,7 @@ func TestMakeCommit(t *testing.T) {
 		vote = withBlockPartsHeader(vote, PartSetHeader{123, tmrand.Bytes(32)})
 
 		_, err = signAddVote(privValidators[6], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 	}
 
 	// The 8th voted like everyone else.
@@ -565,9 +445,7 @@ func TestMakeCommit(t *testing.T) {
 		addr := pv.Address()
 		vote := withValidator(voteProto, addr, 7)
 		_, err = signAddVote(privValidators[7], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 	}
 
 	// The 9th voted for nil.
@@ -579,17 +457,13 @@ func TestMakeCommit(t *testing.T) {
 		vote.BlockID = BlockID{}
 
 		_, err = signAddVote(privValidators[8], vote, voteSet)
-		if err != nil {
-			t.Error(err)
-		}
+		require.NoError(t, err)
 	}
 
 	commit := voteSet.MakeCommit()
 
 	// Commit should have 10 elements
-	if len(commit.Signatures) != 10 {
-		t.Errorf("expected commit to include %d elems, got %d", 10, len(commit.Signatures))
-	}
+	assert.Equal(t, 10, len(commit.Signatures))
 
 	// Ensure that Commit is good.
 	if err := commit.ValidateBasic(); err != nil {
@@ -636,3 +510,58 @@ func buildVoteSetForBlock(height int64,
 	}
 	return voteSet, valSet, privValidators
 }
+
+// NOTE: privValidators are in order
+func randVoteSet(
+	height int64,
+	round int32,
+	signedMsgType tmproto.SignedMsgType,
+	numValidators int,
+	votingPower int64,
+) (*VoteSet, *ValidatorSet, []PrivValidator) {
+	valSet, privValidators := RandValidatorSet(numValidators, votingPower)
+	return NewVoteSet("test_chain_id", height, round, signedMsgType, valSet), valSet, privValidators
+}
+
+// Convenience: Return new vote with different validator address/index
+func withValidator(vote *Vote, addr []byte, idx int32) *Vote {
+	vote = vote.Copy()
+	vote.ValidatorAddress = addr
+	vote.ValidatorIndex = idx
+	return vote
+}
+
+// Convenience: Return new vote with different height
+func withHeight(vote *Vote, height int64) *Vote {
+	vote = vote.Copy()
+	vote.Height = height
+	return vote
+}
+
+// Convenience: Return new vote with different round
+func withRound(vote *Vote, round int32) *Vote {
+	vote = vote.Copy()
+	vote.Round = round
+	return vote
+}
+
+// Convenience: Return new vote with different type
+func withType(vote *Vote, signedMsgType byte) *Vote {
+	vote = vote.Copy()
+	vote.Type = tmproto.SignedMsgType(signedMsgType)
+	return vote
+}
+
+// Convenience: Return new vote with different blockHash
+func withBlockHash(vote *Vote, blockHash []byte) *Vote {
+	vote = vote.Copy()
+	vote.BlockID.Hash = blockHash
+	return vote
+}
+
+// Convenience: Return new vote with different blockParts
+func withBlockPartsHeader(vote *Vote, blockPartsHeader PartSetHeader) *Vote {
+	vote = vote.Copy()
+	vote.BlockID.PartsHeader = blockPartsHeader
+	return vote
+}