Introduce CommitSig alias for Vote in Commit (#3245)

* types: memoize height/round in commit instead of first vote

* types: commit.ValidateBasic in VerifyCommit

* types: new CommitSig alias for Vote

In preparation for reducing the redundancy in Commits, we introduce the
CommitSig as an alias for Vote. This is non-breaking on the protocol,
and minor breaking on the Go API, as Commit now contains a list of
CommitSig instead of Vote.

* remove dependence on ToVote

* update some comments

* fix tests

* fix tests

* fixes from review

CHANGELOG_PENDING.md

@@ -11,6 +11,7 @@ Special thanks to external contributors on this release:
 * Apps
 
 * Go API
+  - [types] \#3245 Commit uses `type CommitSig Vote` instead of `Vote` directly.
 
 * Blockchain Protocol
 

blockchain/reactor_test.go

@@ -100,8 +100,8 @@ func newBlockchainReactor(logger log.Logger, genDoc *types.GenesisDoc, privVals
 			lastBlockMeta := blockStore.LoadBlockMeta(blockHeight - 1)
 			lastBlock := blockStore.LoadBlock(blockHeight - 1)
 
-			vote := makeVote(&lastBlock.Header, lastBlockMeta.BlockID, state.Validators, privVals[0])
+			vote := makeVote(&lastBlock.Header, lastBlockMeta.BlockID, state.Validators, privVals[0]).CommitSig()
-			lastCommit = &types.Commit{Precommits: []*types.Vote{vote}, BlockID: lastBlockMeta.BlockID}
+			lastCommit = &types.Commit{Precommits: []*types.CommitSig{vote}, BlockID: lastBlockMeta.BlockID}
 		}
 
 		thisBlock := makeBlock(blockHeight, state, lastCommit)

blockchain/store_test.go

@@ -6,6 +6,7 @@ import (
 	"runtime/debug"
 	"strings"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -20,6 +21,15 @@ import (
 	tmtime "github.com/tendermint/tendermint/types/time"
 )
 
+// make a Commit with a single vote containing just the height and a timestamp
+func makeTestCommit(height int64, timestamp time.Time) *types.Commit {
+	return &types.Commit{
+		Precommits: []*types.CommitSig{
+			{Height: height, Timestamp: timestamp},
+		},
+	}
+}
+
 func makeStateAndBlockStore(logger log.Logger) (sm.State, *BlockStore) {
 	config := cfg.ResetTestRoot("blockchain_reactor_test")
 	// blockDB := dbm.NewDebugDB("blockDB", dbm.NewMemDB())
@@ -86,8 +96,7 @@ var (
 	partSet     = block.MakePartSet(2)
 	part1       = partSet.GetPart(0)
 	part2       = partSet.GetPart(1)
-	seenCommit1 = &types.Commit{Precommits: []*types.Vote{{Height: 10,
+	seenCommit1 = makeTestCommit(10, tmtime.Now())
-		Timestamp: tmtime.Now()}}}
 )
 
 // TODO: This test should be simplified ...
@@ -107,8 +116,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) {
 	// save a block
 	block := makeBlock(bs.Height()+1, state, new(types.Commit))
 	validPartSet := block.MakePartSet(2)
-	seenCommit := &types.Commit{Precommits: []*types.Vote{{Height: 10,
+	seenCommit := makeTestCommit(10, tmtime.Now())
-		Timestamp: tmtime.Now()}}}
 	bs.SaveBlock(block, partSet, seenCommit)
 	require.Equal(t, bs.Height(), block.Header.Height, "expecting the new height to be changed")
 
@@ -127,8 +135,7 @@ func TestBlockStoreSaveLoadBlock(t *testing.T) {
 
 	// End of setup, test data
 
-	commitAtH10 := &types.Commit{Precommits: []*types.Vote{{Height: 10,
+	commitAtH10 := makeTestCommit(10, tmtime.Now())
-		Timestamp: tmtime.Now()}}}
 	tuples := []struct {
 		block      *types.Block
 		parts      *types.PartSet
@@ -351,9 +358,7 @@ func TestBlockFetchAtHeight(t *testing.T) {
 	block := makeBlock(bs.Height()+1, state, new(types.Commit))
 
 	partSet := block.MakePartSet(2)
-	seenCommit := &types.Commit{Precommits: []*types.Vote{{Height: 10,
+	seenCommit := makeTestCommit(10, tmtime.Now())
-		Timestamp: tmtime.Now()}}}
-
 	bs.SaveBlock(block, partSet, seenCommit)
 	require.Equal(t, bs.Height(), block.Header.Height, "expecting the new height to be changed")
 

consensus/replay_test.go

@@ -539,7 +539,7 @@ func makeBlockchainFromWAL(wal WAL) ([]*types.Block, []*types.Commit, error) {
 			if p.Type == types.PrecommitType {
 				thisBlockCommit = &types.Commit{
 					BlockID:    p.BlockID,
-					Precommits: []*types.Vote{p},
+					Precommits: []*types.CommitSig{p.CommitSig()},
 				}
 			}
 		}

consensus/state.go

@@ -489,7 +489,7 @@ func (cs *ConsensusState) reconstructLastCommit(state sm.State) {
 		if precommit == nil {
 			continue
 		}
-		added, err := lastPrecommits.AddVote(precommit)
+		added, err := lastPrecommits.AddVote(seenCommit.ToVote(precommit))
 		if !added || err != nil {
 			cmn.PanicCrisis(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err))
 		}
@@ -1356,7 +1356,7 @@ func (cs *ConsensusState) recordMetrics(height int64, block *types.Block) {
 	missingValidators := 0
 	missingValidatorsPower := int64(0)
 	for i, val := range cs.Validators.Validators {
-		var vote *types.Vote
+		var vote *types.CommitSig
 		if i < len(block.LastCommit.Precommits) {
 			vote = block.LastCommit.Precommits[i]
 		}

consensus/types/round_state_test.go

@@ -16,7 +16,7 @@ func BenchmarkRoundStateDeepCopy(b *testing.B) {
 	// Random validators
 	nval, ntxs := 100, 100
 	vset, _ := types.RandValidatorSet(nval, 1)
-	precommits := make([]*types.Vote, nval)
+	precommits := make([]*types.CommitSig, nval)
 	blockID := types.BlockID{
 		Hash: cmn.RandBytes(20),
 		PartsHeader: types.PartSetHeader{
@@ -25,12 +25,12 @@ func BenchmarkRoundStateDeepCopy(b *testing.B) {
 	}
 	sig := make([]byte, ed25519.SignatureSize)
 	for i := 0; i < nval; i++ {
-		precommits[i] = &types.Vote{
+		precommits[i] = (&types.Vote{
 			ValidatorAddress: types.Address(cmn.RandBytes(20)),
 			Timestamp:        tmtime.Now(),
 			BlockID:          blockID,
 			Signature:        sig,
-		}
+		}).CommitSig()
 	}
 	txs := make([]types.Tx, ntxs)
 	for i := 0; i < ntxs; i++ {

lite/helpers.go

@@ -70,7 +70,7 @@ func (pkz privKeys) ToValidators(init, inc int64) *types.ValidatorSet {
 
 // signHeader properly signs the header with all keys from first to last exclusive.
 func (pkz privKeys) signHeader(header *types.Header, first, last int) *types.Commit {
-	votes := make([]*types.Vote, len(pkz))
+	commitSigs := make([]*types.CommitSig, len(pkz))
 
 	// We need this list to keep the ordering.
 	vset := pkz.ToValidators(1, 0)
@@ -78,12 +78,12 @@ func (pkz privKeys) signHeader(header *types.Header, first, last int) *types.Com
 	// Fill in the votes we want.
 	for i := first; i < last && i < len(pkz); i++ {
 		vote := makeVote(header, vset, pkz[i])
-		votes[vote.ValidatorIndex] = vote
+		commitSigs[vote.ValidatorIndex] = vote.CommitSig()
 	}
 
 	res := &types.Commit{
 		BlockID:    types.BlockID{Hash: header.Hash()},
-		Precommits: votes,
+		Precommits: commitSigs,
 	}
 	return res
 }

state/execution.go

@@ -315,7 +315,7 @@ func getBeginBlockValidatorInfo(block *types.Block, lastValSet *types.ValidatorS
 	// Collect the vote info (list of validators and whether or not they signed).
 	voteInfos := make([]abci.VoteInfo, len(lastValSet.Validators))
 	for i, val := range lastValSet.Validators {
-		var vote *types.Vote
+		var vote *types.CommitSig
 		if i < len(block.LastCommit.Precommits) {
 			vote = block.LastCommit.Precommits[i]
 		}

state/execution_test.go

@@ -65,17 +65,17 @@ func TestBeginBlockValidators(t *testing.T) {
 	prevBlockID := types.BlockID{prevHash, prevParts}
 
 	now := tmtime.Now()
-	vote0 := &types.Vote{ValidatorIndex: 0, Timestamp: now, Type: types.PrecommitType}
+	commitSig0 := (&types.Vote{ValidatorIndex: 0, Timestamp: now, Type: types.PrecommitType}).CommitSig()
-	vote1 := &types.Vote{ValidatorIndex: 1, Timestamp: now}
+	commitSig1 := (&types.Vote{ValidatorIndex: 1, Timestamp: now}).CommitSig()
 
 	testCases := []struct {
 		desc                     string
-		lastCommitPrecommits     []*types.Vote
+		lastCommitPrecommits     []*types.CommitSig
 		expectedAbsentValidators []int
 	}{
-		{"none absent", []*types.Vote{vote0, vote1}, []int{}},
+		{"none absent", []*types.CommitSig{commitSig0, commitSig1}, []int{}},
-		{"one absent", []*types.Vote{vote0, nil}, []int{1}},
+		{"one absent", []*types.CommitSig{commitSig0, nil}, []int{1}},
-		{"multiple absent", []*types.Vote{nil, nil}, []int{0, 1}},
+		{"multiple absent", []*types.CommitSig{nil, nil}, []int{0, 1}},
 	}
 
 	for _, tc := range testCases {
@@ -136,10 +136,10 @@ func TestBeginBlockByzantineValidators(t *testing.T) {
 			types.TM2PB.Evidence(ev2, valSet, now)}},
 	}
 
-	vote0 := &types.Vote{ValidatorIndex: 0, Timestamp: now, Type: types.PrecommitType}
+	commitSig0 := (&types.Vote{ValidatorIndex: 0, Timestamp: now, Type: types.PrecommitType}).CommitSig()
-	vote1 := &types.Vote{ValidatorIndex: 1, Timestamp: now}
+	commitSig1 := (&types.Vote{ValidatorIndex: 1, Timestamp: now}).CommitSig()
-	votes := []*types.Vote{vote0, vote1}
+	commitSigs := []*types.CommitSig{commitSig0, commitSig1}
-	lastCommit := &types.Commit{BlockID: prevBlockID, Precommits: votes}
+	lastCommit := &types.Commit{BlockID: prevBlockID, Precommits: commitSigs}
 	for _, tc := range testCases {
 
 		block, _ := state.MakeBlock(10, makeTxs(2), lastCommit, nil, state.Validators.GetProposer().Address)

types/block.go

@@ -477,39 +477,77 @@ func (h *Header) StringIndented(indent string) string {
 
 //-------------------------------------
 
+// CommitSig is a vote included in a Commit.
+// For now, it is identical to a vote,
+// but in the future it will contain fewer fields
+// to eliminate the redundancy in commits.
+// See https://github.com/tendermint/tendermint/issues/1648.
+type CommitSig Vote
+
+// String returns the underlying Vote.String()
+func (cs *CommitSig) String() string {
+	return cs.toVote().String()
+}
+
+// toVote converts the CommitSig to a vote.
+// Once CommitSig has fewer fields than vote,
+// converting to a Vote will require more information.
+func (cs *CommitSig) toVote() *Vote {
+	if cs == nil {
+		return nil
+	}
+	v := Vote(*cs)
+	return &v
+}
+
 // Commit contains the evidence that a block was committed by a set of validators.
 // NOTE: Commit is empty for height 1, but never nil.
 type Commit struct {
 	// NOTE: The Precommits are in order of address to preserve the bonded ValidatorSet order.
 	// Any peer with a block can gossip precommits by index with a peer without recalculating the
 	// active ValidatorSet.
-	BlockID    BlockID `json:"block_id"`
+	BlockID    BlockID      `json:"block_id"`
-	Precommits []*Vote `json:"precommits"`
+	Precommits []*CommitSig `json:"precommits"`
 
 	// Volatile
-	firstPrecommit *Vote
+	height   int64
-	hash           cmn.HexBytes
+	round    int
-	bitArray       *cmn.BitArray
+	hash     cmn.HexBytes
+	bitArray *cmn.BitArray
 }
 
-// FirstPrecommit returns the first non-nil precommit in the commit.
+// VoteSignBytes constructs the SignBytes for the given CommitSig.
-// If all precommits are nil, it returns an empty precommit with height 0.
+// The only unique part of the SignBytes is the Timestamp - all other fields
-func (commit *Commit) FirstPrecommit() *Vote {
+// signed over are otherwise the same for all validators.
+func (commit *Commit) VoteSignBytes(chainID string, cs *CommitSig) []byte {
+	return cs.toVote().SignBytes(chainID)
+}
+
+// memoizeHeightRound memoizes the height and round of the commit using
+// the first non-nil vote.
+func (commit *Commit) memoizeHeightRound() {
 	if len(commit.Precommits) == 0 {
-		return nil
+		return
 	}
-	if commit.firstPrecommit != nil {
+	if commit.height > 0 {
-		return commit.firstPrecommit
+		return
 	}
 	for _, precommit := range commit.Precommits {
 		if precommit != nil {
-			commit.firstPrecommit = precommit
+			commit.height = precommit.Height
-			return precommit
+			commit.round = precommit.Round
+			return
 		}
 	}
-	return &Vote{
+}
-		Type: PrecommitType,
+
-	}
+// ToVote converts a CommitSig to a Vote.
+// If the CommitSig is nil, the Vote will be nil.
+// When CommitSig is reduced to contain fewer fields,
+// this will need access to the ValidatorSet to properly
+// reconstruct the vote.
+func (commit *Commit) ToVote(cs *CommitSig) *Vote {
+	return cs.toVote()
 }
 
 // Height returns the height of the commit
@@ -517,7 +555,8 @@ func (commit *Commit) Height() int64 {
 	if len(commit.Precommits) == 0 {
 		return 0
 	}
-	return commit.FirstPrecommit().Height
+	commit.memoizeHeightRound()
+	return commit.height
 }
 
 // Round returns the round of the commit
@@ -525,7 +564,8 @@ func (commit *Commit) Round() int {
 	if len(commit.Precommits) == 0 {
 		return 0
 	}
-	return commit.FirstPrecommit().Round
+	commit.memoizeHeightRound()
+	return commit.round
 }
 
 // Type returns the vote type of the commit, which is always VoteTypePrecommit
@@ -554,12 +594,13 @@ func (commit *Commit) BitArray() *cmn.BitArray {
 	return commit.bitArray
 }
 
-// GetByIndex returns the vote corresponding to a given validator index
+// GetByIndex returns the vote corresponding to a given validator index.
+// Implements VoteSetReader.
 func (commit *Commit) GetByIndex(index int) *Vote {
-	return commit.Precommits[index]
+	return commit.Precommits[index].toVote()
 }
 
-// IsCommit returns true if there is at least one vote
+// IsCommit returns true if there is at least one vote.
 func (commit *Commit) IsCommit() bool {
 	return len(commit.Precommits) != 0
 }

types/block_test.go

@@ -198,7 +198,6 @@ func TestCommit(t *testing.T) {
 	commit, err := MakeCommit(lastID, h-1, 1, voteSet, vals)
 	require.NoError(t, err)
 
-	assert.NotNil(t, commit.FirstPrecommit())
 	assert.Equal(t, h-1, commit.Height())
 	assert.Equal(t, 1, commit.Round())
 	assert.Equal(t, PrecommitType, SignedMsgType(commit.Type()))

types/validator_set.go

@@ -368,6 +368,10 @@ func (vals *ValidatorSet) Iterate(fn func(index int, val *Validator) bool) {
 
 // Verify that +2/3 of the set had signed the given signBytes.
 func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height int64, commit *Commit) error {
+
+	if err := commit.ValidateBasic(); err != nil {
+		return err
+	}
 	if vals.Size() != len(commit.Precommits) {
 		return fmt.Errorf("Invalid commit -- wrong set size: %v vs %v", vals.Size(), len(commit.Precommits))
 	}
@@ -380,24 +384,14 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height i
 	}
 
 	talliedVotingPower := int64(0)
-	round := commit.Round()
 
 	for idx, precommit := range commit.Precommits {
 		if precommit == nil {
 			continue // OK, some precommits can be missing.
 		}
-		if precommit.Height != height {
-			return fmt.Errorf("Invalid commit -- wrong height: want %v got %v", height, precommit.Height)
-		}
-		if precommit.Round != round {
-			return fmt.Errorf("Invalid commit -- wrong round: want %v got %v", round, precommit.Round)
-		}
-		if precommit.Type != PrecommitType {
-			return fmt.Errorf("Invalid commit -- not precommit @ index %v", idx)
-		}
 		_, val := vals.GetByIndex(idx)
 		// Validate signature.
-		precommitSignBytes := precommit.SignBytes(chainID)
+		precommitSignBytes := commit.VoteSignBytes(chainID, precommit)
 		if !val.PubKey.VerifyBytes(precommitSignBytes, precommit.Signature) {
 			return fmt.Errorf("Invalid commit -- invalid signature: %v", precommit)
 		}
@@ -481,7 +475,7 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin
 		seen[idx] = true
 
 		// Validate signature.
-		precommitSignBytes := precommit.SignBytes(chainID)
+		precommitSignBytes := commit.VoteSignBytes(chainID, precommit)
 		if !val.PubKey.VerifyBytes(precommitSignBytes, precommit.Signature) {
 			return cmn.NewError("Invalid commit -- invalid signature: %v", precommit)
 		}

types/validator_set_test.go

@@ -565,7 +565,7 @@ func TestValidatorSetVerifyCommit(t *testing.T) {
 	vote.Signature = sig
 	commit := &Commit{
 		BlockID:    blockID,
-		Precommits: []*Vote{vote},
+		Precommits: []*CommitSig{vote.CommitSig()},
 	}
 
 	badChainID := "notmychainID"
@@ -573,7 +573,7 @@ func TestValidatorSetVerifyCommit(t *testing.T) {
 	badHeight := height + 1
 	badCommit := &Commit{
 		BlockID:    blockID,
-		Precommits: []*Vote{nil},
+		Precommits: []*CommitSig{nil},
 	}
 
 	// test some error cases

types/vote.go

@@ -59,6 +59,16 @@ type Vote struct {
 	Signature        []byte        `json:"signature"`
 }
 
+// CommitSig converts the Vote to a CommitSig.
+// If the Vote is nil, the CommitSig will be nil.
+func (vote *Vote) CommitSig() *CommitSig {
+	if vote == nil {
+		return nil
+	}
+	cs := CommitSig(*vote)
+	return &cs
+}
+
 func (vote *Vote) SignBytes(chainID string) []byte {
 	bz, err := cdc.MarshalBinaryLengthPrefixed(CanonicalizeVote(chainID, vote))
 	if err != nil {

types/vote_set.go

@@ -541,11 +541,13 @@ func (voteSet *VoteSet) MakeCommit() *Commit {
 	}
 
 	// For every validator, get the precommit
-	votesCopy := make([]*Vote, len(voteSet.votes))
+	commitSigs := make([]*CommitSig, len(voteSet.votes))
-	copy(votesCopy, voteSet.votes)
+	for i, v := range voteSet.votes {
+		commitSigs[i] = v.CommitSig()
+	}
 	return &Commit{
 		BlockID:    *voteSet.maj23,
-		Precommits: votesCopy,
+		Precommits: commitSigs,
 	}
 }
 

types/vote_test.go

@@ -7,6 +7,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	amino "github.com/tendermint/go-amino"
 	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/ed25519"
 	"github.com/tendermint/tendermint/crypto/tmhash"
@@ -43,6 +44,19 @@ func exampleVote(t byte) *Vote {
 	}
 }
 
+// Ensure that Vote and CommitSig have the same encoding.
+// This ensures using CommitSig isn't a breaking change.
+// This test will fail and can be removed once CommitSig contains only sigs and
+// timestamps.
+func TestVoteEncoding(t *testing.T) {
+	vote := examplePrecommit()
+	commitSig := vote.CommitSig()
+	cdc := amino.NewCodec()
+	bz1 := cdc.MustMarshalBinaryBare(vote)
+	bz2 := cdc.MustMarshalBinaryBare(commitSig)
+	assert.Equal(t, bz1, bz2)
+}
+
 func TestVoteSignable(t *testing.T) {
 	vote := examplePrecommit()
 	signBytes := vote.SignBytes("test_chain_id")