types: CommitVotes struct as last step towards #1648 (#3298)

* VoteSignBytes builds CanonicalVote

* CommitVotes implements VoteSetReader

- new CommitVotes struct holds both the Commit and the ValidatorSet and
  implements VoteSetReader
- ToVote takes a ValidatorSet

* fix TestCommit

* use CommitSig.BlockID

Commits may include votes for a different BlockID, could be nil,
or different altogether. This means we can't use `commit.BlockID`
for reconstructing the sign bytes, since up to -1/3 of the commits
might be for independent BlockIDs. This means CommitSig will need to
include an indicator for what BlockID it signed - if it's not the
committed one or nil, it will need to include it fully in order to be
verified. This is unfortunate but unavoidable so long as we include
votes for non-committed BlockIDs (which we do to track validator
liveness)

* fixes from review

* remove CommitVotes. CommitSig contains address

* remove commit.canonicalVote method

* toVote -> getVote, takes valIdx

* update adr-025

* commit.ToVoteSet -> CommitToVoteSet

* add test

* fix from review

consensus/state.go

@@ -484,16 +484,7 @@ func (cs *ConsensusState) reconstructLastCommit(state sm.State) {
 		return
 	}
 	seenCommit := cs.blockStore.LoadSeenCommit(state.LastBlockHeight)
-	lastPrecommits := types.NewVoteSet(state.ChainID, state.LastBlockHeight, seenCommit.Round(), types.PrecommitType, state.LastValidators)
-	for _, precommit := range seenCommit.Precommits {
-		if precommit == nil {
-			continue
-		}
-		added, err := lastPrecommits.AddVote(seenCommit.ToVote(precommit))
-		if !added || err != nil {
-			panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err))
-		}
-	}
+	lastPrecommits := types.CommitToVoteSet(state.ChainID, seenCommit, state.LastValidators)
 	if !lastPrecommits.HasTwoThirdsMajority() {
 		panic("Failed to reconstruct LastCommit: Does not have +2/3 maj")
 	}

docs/architecture/adr-025-commit.md

@@ -1,14 +1,18 @@
 # ADR 025 Commit
 
 ## Context
+
 Currently the `Commit` structure contains a lot of potentially redundant or unnecessary data.
-In particular it contains an array of every precommit from the validators, which includes many copies of the same data. Such as `Height`, `Round`, `Type`, and `BlockID`. Also the `ValidatorIndex` could be derived from the vote's position in the array, and the `ValidatorAddress` could potentially be derived from runtime context. The only truely necessary data is the `Signature` and `Timestamp` associated with each `Vote`.
+It contains a list of precommits from every validator, where the precommit
+includes the whole `Vote` structure. Thus each of the commit height, round,
+type, and blockID are repeated for every validator, and could be deduplicated.
 
 ```
 type Commit struct {
     BlockID    BlockID `json:"block_id"`
     Precommits []*Vote `json:"precommits"`
 }
+
 type Vote struct {
     ValidatorAddress Address   `json:"validator_address"`
     ValidatorIndex   int       `json:"validator_index"`
@@ -26,7 +30,9 @@ References:
 [#2226](https://github.com/tendermint/tendermint/issues/2226)
 
 ## Proposed Solution
+
 We can improve efficiency by replacing the usage of the `Vote` struct with a subset of each vote, and by storing the constant values (`Height`, `Round`, `BlockID`) in the Commit itself.
+
 ```
 type Commit struct {
     Height  int64
@@ -34,42 +40,56 @@ type Commit struct {
     BlockID    BlockID      `json:"block_id"`
     Precommits []*CommitSig `json:"precommits"`
 }
+
 type CommitSig struct {
+    BlockID  BlockIDFlag
     ValidatorAddress Address
-    Signature []byte
     Timestamp time.Time
+    Signature []byte
 }
-```
-Continuing to store the `ValidatorAddress` in the `CommitSig` takes up extra space, but simplifies the process and allows for easier debugging.
 
-## Status
-Proposed
 
-## Consequences
+// indicate which BlockID the signature is for
+type BlockIDFlag int
 
-### Positive
-The size of a `Commit` transmitted over the network goes from:
+const (
+	BlockIDFlagAbsent BlockIDFlag = iota // vote is not included in the Commit.Precommits
+	BlockIDFlagCommit                    // voted for the Commit.BlockID
+	BlockIDFlagNil                       // voted for nil
+)
 
-|BlockID| + n * (|Address| + |ValidatorIndex| + |Height| + |Round| + |Timestamp| + |Type| + |BlockID| + |Signature|)
+```
 
-to:
+Note the need for an extra byte to indicate whether the signature is for the BlockID or for nil.
+This byte can also be used to indicate an absent vote, rather than using a nil object like we currently do,
+which has been [problematic for compatibility between Amino and proto3](https://github.com/tendermint/go-amino/issues/260).
 
+Note we also continue to store the `ValidatorAddress` in the `CommitSig`.
+While this still takes 20-bytes per signature, it ensures that the Commit has all
+information necessary to reconstruct Vote, which simplifies mapping between Commit and Vote objects
+and with debugging. It also may be necessary for the light-client to know which address a signature corresponds to if
+it is trying to verify a current commit with an older validtor set.
+
+## Status
 
-|BlockID|+|Height|+|Round| + n*(|Address| + |Signature| + |Timestamp|)
+Proposed
 
-This saves:
+## Consequences
 
-n * (|BlockID| + |ValidatorIndex| + |Type|) + (n-1) * (Height + Round)
+### Positive
 
-In the current context, this would concretely be:
-(assuming all ints are int64, and hashes are 32 bytes)
+Removing the Type/Height/Round/Index and the BlockID saves roughly 80 bytes per precommit.
+It varies because some integers are varint. The BlockID contains two 32-byte hashes an integer,
+and the Height is 8-bytes.
 
-n *(72 + 8 + 1 + 8 + 8) - 16 = n * 97 - 16
+For a chain with 100 validators, that's up to 8kB in savings per block!
 
-With 100 validators this is a savings of almost 10KB on every block.
 
 ### Negative
-This would add some complexity to the processing and verification of blocks and commits, as votes would have to be reconstructed to be verified and gossiped. The reconstruction could be relatively straightforward, only requiring the copying of data from the `Commit` itself into the newly created `Vote`.
+
+- Large breaking change to the block and commit structure
+- Requires differentiating in code between the Vote and CommitSig objects, which may add some complexity (votes need to be reconstructed to be verified and gossiped)
 
 ### Neutral
-This design leaves the `ValidatorAddress` in the `CommitSig` and in the `Vote`. These could be removed at some point for additional savings, but that would introduce more complexity, and make printing of `Commit` and `VoteSet` objects less informative, which could harm debugging efficiency and UI/UX.
+
+- Commit.Precommits no longer contains nil values

types/block.go

@@ -500,6 +500,8 @@ func (cs *CommitSig) toVote() *Vote {
 	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 {
@@ -528,11 +530,56 @@ func NewCommit(blockID BlockID, precommits []*CommitSig) *Commit {
 	}
 }
 
+// Construct a VoteSet from the Commit and validator set. Panics
+// if precommits from the commit can't be added to the voteset.
+// Inverse of VoteSet.MakeCommit().
+func CommitToVoteSet(chainID string, commit *Commit, vals *ValidatorSet) *VoteSet {
+	height, round, typ := commit.Height(), commit.Round(), PrecommitType
+	voteSet := NewVoteSet(chainID, height, round, typ, vals)
+	for idx, precommit := range commit.Precommits {
+		if precommit == nil {
+			continue
+		}
+		added, err := voteSet.AddVote(commit.GetVote(idx))
+		if !added || err != nil {
+			panic(fmt.Sprintf("Failed to reconstruct LastCommit: %v", err))
+		}
+	}
+	return voteSet
+}
+
+// GetVote converts the CommitSig for the given valIdx to a Vote.
+// Returns nil if the precommit at valIdx is nil.
+// Panics if valIdx >= commit.Size().
+func (commit *Commit) GetVote(valIdx int) *Vote {
+	commitSig := commit.Precommits[valIdx]
+	if commitSig == nil {
+		return nil
+	}
+
+	// NOTE: this commitSig might be for a nil blockID,
+	// so we can't just use commit.BlockID here.
+	// For #1648, CommitSig will need to indicate what BlockID it's for !
+	blockID := commitSig.BlockID
+	commit.memoizeHeightRound()
+	return &Vote{
+		Type:             PrecommitType,
+		Height:           commit.height,
+		Round:            commit.round,
+		BlockID:          blockID,
+		Timestamp:        commitSig.Timestamp,
+		ValidatorAddress: commitSig.ValidatorAddress,
+		ValidatorIndex:   valIdx,
+		Signature:        commitSig.Signature,
+	}
+}
+
 // 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.
-func (commit *Commit) VoteSignBytes(chainID string, cs *CommitSig) []byte {
-	return commit.ToVote(cs).SignBytes(chainID)
+// Panics if valIdx >= commit.Size().
+func (commit *Commit) VoteSignBytes(chainID string, valIdx int) []byte {
+	return commit.GetVote(valIdx).SignBytes(chainID)
 }
 
 // memoizeHeightRound memoizes the height and round of the commit using
@@ -553,14 +600,6 @@ func (commit *Commit) memoizeHeightRound() {
 	}
 }
 
-// ToVote converts a CommitSig to a Vote.
-// If the CommitSig is nil, the Vote will be nil.
-func (commit *Commit) ToVote(cs *CommitSig) *Vote {
-	// TODO: use commit.validatorSet to reconstruct vote
-	// and deprecate .toVote
-	return cs.toVote()
-}
-
 // Height returns the height of the commit
 func (commit *Commit) Height() int64 {
 	commit.memoizeHeightRound()
@@ -602,8 +641,8 @@ func (commit *Commit) BitArray() *cmn.BitArray {
 // GetByIndex returns the vote corresponding to a given validator index.
 // Panics if `index >= commit.Size()`.
 // Implements VoteSetReader.
-func (commit *Commit) GetByIndex(index int) *Vote {
-	return commit.ToVote(commit.Precommits[index])
+func (commit *Commit) GetByIndex(valIdx int) *Vote {
+	return commit.GetVote(valIdx)
 }
 
 // IsCommit returns true if there is at least one vote.

types/block_test.go

@@ -342,3 +342,27 @@ func TestBlockMaxDataBytesUnknownEvidence(t *testing.T) {
 		}
 	}
 }
+
+func TestCommitToVoteSet(t *testing.T) {
+	lastID := makeBlockIDRandom()
+	h := int64(3)
+
+	voteSet, valSet, vals := randVoteSet(h-1, 1, PrecommitType, 10, 1)
+	commit, err := MakeCommit(lastID, h-1, 1, voteSet, vals)
+	assert.NoError(t, err)
+
+	chainID := voteSet.ChainID()
+	voteSet2 := CommitToVoteSet(chainID, commit, valSet)
+
+	for i := 0; i < len(vals); i++ {
+		vote1 := voteSet.GetByIndex(i)
+		vote2 := voteSet2.GetByIndex(i)
+		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)
+	}
+}

types/validator_set.go

@@ -612,7 +612,7 @@ func (vals *ValidatorSet) VerifyCommit(chainID string, blockID BlockID, height i
 		}
 		_, val := vals.GetByIndex(idx)
 		// Validate signature.
-		precommitSignBytes := commit.VoteSignBytes(chainID, precommit)
+		precommitSignBytes := commit.VoteSignBytes(chainID, idx)
 		if !val.PubKey.VerifyBytes(precommitSignBytes, precommit.Signature) {
 			return fmt.Errorf("Invalid commit -- invalid signature: %v", precommit)
 		}
@@ -689,14 +689,14 @@ func (vals *ValidatorSet) VerifyFutureCommit(newSet *ValidatorSet, chainID strin
 			return cmn.NewError("Invalid commit -- not precommit @ index %v", idx)
 		}
 		// See if this validator is in oldVals.
-		idx, val := oldVals.GetByAddress(precommit.ValidatorAddress)
-		if val == nil || seen[idx] {
+		oldIdx, val := oldVals.GetByAddress(precommit.ValidatorAddress)
+		if val == nil || seen[oldIdx] {
 			continue // missing or double vote...
 		}
-		seen[idx] = true
+		seen[oldIdx] = true
 
 		// Validate signature.
-		precommitSignBytes := commit.VoteSignBytes(chainID, precommit)
+		precommitSignBytes := commit.VoteSignBytes(chainID, idx)
 		if !val.PubKey.VerifyBytes(precommitSignBytes, precommit.Signature) {
 			return cmn.NewError("Invalid commit -- invalid signature: %v", precommit)
 		}

types/vote_set.go

@@ -528,6 +528,9 @@ func (voteSet *VoteSet) sumTotalFrac() (int64, int64, float64) {
 //--------------------------------------------------------------------------------
 // Commit
 
+// MakeCommit constructs a Commit from the VoteSet.
+// Panics if the vote type is not PrecommitType or if
+// there's no +2/3 votes for a single block.
 func (voteSet *VoteSet) MakeCommit() *Commit {
 	if voteSet.type_ != PrecommitType {
 		panic("Cannot MakeCommit() unless VoteSet.Type is PrecommitType")