fix order of BlockID and Timestamp in Vote and Proposal (#3078)

* Consistent order fields of Timestamp/BlockID fields in CanonicalVote and
CanonicalProposal

* update spec too

* Introduce and use IsZero & IsComplete:
 - update IsZero method according to spec and introduce IsComplete
 - use methods in validate basic to validate: proposals come with a
 "complete" blockId and votes are either complete or empty
 - update spec: BlockID.IsNil() -> BlockID.IsZero() and fix typo

* BlockID comes first

* fix tests

CHANGELOG_PENDING.md

@@ -7,6 +7,7 @@ Special thanks to external contributors on this release:
 ### BREAKING CHANGES:
 
 * CLI/RPC/Config
+- [types] consistent field order of `CanonicalVote` and `CanonicalProposal` 
 
 * Apps
 

docs/spec/blockchain/blockchain.md

@@ -109,10 +109,6 @@ Tendermint uses the
 [Google.Protobuf.WellKnownTypes.Timestamp](https://developers.google.com/protocol-buffers/docs/reference/csharp/class/google/protobuf/well-known-types/timestamp)
 format, which uses two integers, one for Seconds and for Nanoseconds.
 
-NOTE: there is currently a small divergence between Tendermint and the
-Google.Protobuf.WellKnownTypes.Timestamp that should be resolved. See [this
-issue](https://github.com/tendermint/go-amino/issues/223) for details.
-
 ## Data
 
 Data is just a wrapper for a list of transactions, where transactions are

docs/spec/blockchain/encoding.md

@@ -301,8 +301,8 @@ type CanonicalVote struct {
 	Type      byte
 	Height    int64            `binary:"fixed64"`
 	Round     int64            `binary:"fixed64"`
-	Timestamp time.Time
 	BlockID   CanonicalBlockID
+	Timestamp time.Time
 	ChainID   string
 }
 ```

docs/spec/consensus/signing.md

@@ -59,9 +59,9 @@ type PartSetHeader struct {
 ```
 
 To be included in a valid vote or proposal, BlockID must either represent a `nil` block, or a complete one.
-We introduce two methods, `BlockID.IsNil()` and `BlockID.IsComplete()` for these cases, respectively.
+We introduce two methods, `BlockID.IsZero()` and `BlockID.IsComplete()` for these cases, respectively.
 
-`BlockID.IsNil()` returns true for BlockID `b` if each of the following
+`BlockID.IsZero()` returns true for BlockID `b` if each of the following
 are true:
 
 ```
@@ -81,7 +81,7 @@ len(b.PartsHeader.Hash) == 32
 
 ## Proposals
 
-The structure of a propsal for signing looks like:
+The structure of a proposal for signing looks like:
 
 ```
 type CanonicalProposal struct {
@@ -118,8 +118,8 @@ type CanonicalVote struct {
 	Type      SignedMsgType // type alias for byte
 	Height    int64         `binary:"fixed64"`
 	Round     int64         `binary:"fixed64"`
-	Timestamp time.Time
 	BlockID   BlockID
+	Timestamp time.Time
 	ChainID   string
 }
 ```
@@ -130,7 +130,7 @@ A vote is valid if each of the following lines evaluates to true for vote `v`:
 v.Type == 0x1 || v.Type == 0x2
 v.Height > 0
 v.Round >= 0
-v.BlockID.IsNil() || v.BlockID.IsValid()
+v.BlockID.IsZero() || v.BlockID.IsComplete()
 ```
 
 In other words, a vote is valid for signing if it contains the type of a Prevote

types/block.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/tendermint/tendermint/crypto"
 	"github.com/tendermint/tendermint/crypto/merkle"
+	"github.com/tendermint/tendermint/crypto/tmhash"
 	cmn "github.com/tendermint/tendermint/libs/common"
 	"github.com/tendermint/tendermint/version"
 )
@@ -788,11 +789,6 @@ type BlockID struct {
 	PartsHeader PartSetHeader `json:"parts"`
 }
 
-// IsZero returns true if this is the BlockID for a nil-block
-func (blockID BlockID) IsZero() bool {
-	return len(blockID.Hash) == 0 && blockID.PartsHeader.IsZero()
-}
-
 // Equals returns true if the BlockID matches the given BlockID
 func (blockID BlockID) Equals(other BlockID) bool {
 	return bytes.Equal(blockID.Hash, other.Hash) &&
@@ -820,6 +816,19 @@ func (blockID BlockID) ValidateBasic() error {
 	return nil
 }
 
+// IsZero returns true if this is the BlockID of a nil block.
+func (blockID BlockID) IsZero() bool {
+	return len(blockID.Hash) == 0 &&
+		blockID.PartsHeader.IsZero()
+}
+
+// IsComplete returns true if this is a valid BlockID of a non-nil block.
+func (blockID BlockID) IsComplete() bool {
+	return len(blockID.Hash) == tmhash.Size &&
+		blockID.PartsHeader.Total > 0 &&
+		len(blockID.PartsHeader.Hash) == tmhash.Size
+}
+
 // String returns a human readable string representation of the BlockID
 func (blockID BlockID) String() string {
 	return fmt.Sprintf(`%v:%v`, blockID.Hash, blockID.PartsHeader)

types/canonical.go

@@ -36,8 +36,8 @@ type CanonicalVote struct {
 	Type      SignedMsgType // type alias for byte
 	Height    int64         `binary:"fixed64"`
 	Round     int64         `binary:"fixed64"`
-	Timestamp time.Time
 	BlockID   CanonicalBlockID
+	Timestamp time.Time
 	ChainID   string
 }
 
@@ -75,8 +75,8 @@ func CanonicalizeVote(chainID string, vote *Vote) CanonicalVote {
 		Type:      vote.Type,
 		Height:    vote.Height,
 		Round:     int64(vote.Round), // cast int->int64 to make amino encode it fixed64 (does not work for int)
-		Timestamp: vote.Timestamp,
 		BlockID:   CanonicalizeBlockID(vote.BlockID),
+		Timestamp: vote.Timestamp,
 		ChainID:   chainID,
 	}
 }

types/part_set.go

@@ -75,7 +75,7 @@ func (psh PartSetHeader) String() string {
 }
 
 func (psh PartSetHeader) IsZero() bool {
-	return psh.Total == 0
+	return psh.Total == 0 && len(psh.Hash) == 0
 }
 
 func (psh PartSetHeader) Equals(other PartSetHeader) bool {

types/proposal.go

@@ -60,6 +60,10 @@ func (p *Proposal) ValidateBasic() error {
 	if err := p.BlockID.ValidateBasic(); err != nil {
 		return fmt.Errorf("Wrong BlockID: %v", err)
 	}
+	// ValidateBasic above would pass even if the BlockID was empty:
+	if !p.BlockID.IsComplete() {
+		return fmt.Errorf("Expected a complete, non-empty BlockID, got: %v", p.BlockID)
+	}
 
 	// NOTE: Timestamp validation is subtle and handled elsewhere.
 

types/vote.go

@@ -52,8 +52,8 @@ type Vote struct {
 	Type             SignedMsgType `json:"type"`
 	Height           int64         `json:"height"`
 	Round            int           `json:"round"`
-	Timestamp        time.Time     `json:"timestamp"`
 	BlockID          BlockID       `json:"block_id"` // zero if vote is nil.
+	Timestamp        time.Time     `json:"timestamp"`
 	ValidatorAddress Address       `json:"validator_address"`
 	ValidatorIndex   int           `json:"validator_index"`
 	Signature        []byte        `json:"signature"`
@@ -127,6 +127,11 @@ func (vote *Vote) ValidateBasic() error {
 	if err := vote.BlockID.ValidateBasic(); err != nil {
 		return fmt.Errorf("Wrong BlockID: %v", err)
 	}
+	// BlockID.ValidateBasic would not err if we for instance have an empty hash but a
+	// non-empty PartsSetHeader:
+	if !vote.BlockID.IsZero() && !vote.BlockID.IsComplete() {
+		return fmt.Errorf("BlockID must be either empty or complete, got: %v", vote.BlockID)
+	}
 	if len(vote.ValidatorAddress) != crypto.AddressSize {
 		return fmt.Errorf("Expected ValidatorAddress size to be %d bytes, got %d bytes",
 			crypto.AddressSize,

types/vote_test.go

@@ -63,8 +63,8 @@ func TestVoteSignableTestVectors(t *testing.T) {
 		{
 			CanonicalizeVote("", &Vote{}),
 			// NOTE: Height and Round are skipped here. This case needs to be considered while parsing.
-			// []byte{0x22, 0x9, 0x9, 0x0, 0x9, 0x6e, 0x88, 0xf1, 0xff, 0xff, 0xff},
-			[]byte{0x22, 0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
+			// []byte{0x2a, 0x9, 0x9, 0x0, 0x9, 0x6e, 0x88, 0xf1, 0xff, 0xff, 0xff},
+			[]byte{0x2a, 0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
 		},
 		// with proper (fixed size) height and round (PreCommit):
 		{
@@ -76,7 +76,7 @@ func TestVoteSignableTestVectors(t *testing.T) {
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // height
 				0x19,                                   // (field_number << 3) | wire_type
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
-				0x22, // (field_number << 3) | wire_type
+				0x2a, // (field_number << 3) | wire_type
 				// remaining fields (timestamp):
 				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
 		},
@@ -90,7 +90,7 @@ func TestVoteSignableTestVectors(t *testing.T) {
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // height
 				0x19,                                   // (field_number << 3) | wire_type
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
-				0x22, // (field_number << 3) | wire_type
+				0x2a, // (field_number << 3) | wire_type
 				// remaining fields (timestamp):
 				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
 		},
@@ -102,7 +102,7 @@ func TestVoteSignableTestVectors(t *testing.T) {
 				0x19,                                   // (field_number << 3) | wire_type
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
 				// remaining fields (timestamp):
-				0x22,
+				0x2a,
 				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
 		},
 		// containing non-empty chain_id:
@@ -114,7 +114,7 @@ func TestVoteSignableTestVectors(t *testing.T) {
 				0x19,                                   // (field_number << 3) | wire_type
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
 				// remaining fields:
-				0x22,                                                                // (field_number << 3) | wire_type
+				0x2a,                                                                // (field_number << 3) | wire_type
 				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1, // timestamp
 				0x32,                                                                               // (field_number << 3) | wire_type
 				0xd, 0x74, 0x65, 0x73, 0x74, 0x5f, 0x63, 0x68, 0x61, 0x69, 0x6e, 0x5f, 0x69, 0x64}, // chainID