Divergences in comparison with #9620. Part 1: easy/obvious (#9888)

* Obvious changes (including bugs)

* Update privval/file.go

Co-authored-by: Thane Thomson <connect@thanethomson.com>

Co-authored-by: Thane Thomson <connect@thanethomson.com>

blocksync/pool.go

@@ -566,9 +566,7 @@ func (bpr *bpRequester) setBlock(block *types.Block, extCommit *types.ExtendedCo
 		return false
 	}
 	bpr.block = block
-	if extCommit != nil {
-		bpr.extCommit = extCommit
-	}
+	bpr.extCommit = extCommit
 	bpr.mtx.Unlock()
 
 	select {

blocksync/reactor_test.go

@@ -109,14 +109,12 @@ func newReactor(
 		panic(err)
 	}
 
-	var lastExtCommit *types.ExtendedCommit
-
 	// The commit we are building for the current height.
 	seenExtCommit := &types.ExtendedCommit{}
 
 	// let's add some blocks in
 	for blockHeight := int64(1); blockHeight <= maxBlockHeight; blockHeight++ {
-		lastExtCommit = seenExtCommit.Clone()
+		lastExtCommit := seenExtCommit.Clone()
 
 		thisBlock := state.MakeBlock(blockHeight, nil, lastExtCommit.ToCommit(), nil, state.Validators.Proposer.Address)
 

internal/test/params.go

@@ -8,6 +8,7 @@ import (
 // for use in testing
 func ConsensusParams() *types.ConsensusParams {
 	c := types.DefaultConsensusParams()
+	// enable vote extensions
 	c.ABCI.VoteExtensionsEnableHeight = 1
 	return c
 }

privval/file.go

@@ -304,6 +304,7 @@ func (pv *FilePV) String() string {
 // signVote checks if the vote is good to sign and sets the vote signature.
 // It may need to set the timestamp as well if the vote is otherwise the same as
 // a previously signed vote (ie. we crashed after signing but before the vote hit the WAL).
+// Extension signatures are always signed for non-nil precommits (even if the data is empty).
 func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error {
 	height, round, step := vote.Height, vote.Round, voteToStep(vote)
 
@@ -320,6 +321,7 @@ func (pv *FilePV) signVote(chainID string, vote *tmproto.Vote) error {
 	// application may have created a different extension. We therefore always
 	// re-sign the vote extensions of precommits. For prevotes and nil
 	// precommits, the extension signature will always be empty.
+	// Even if the signed over data is empty, we still add the signature
 	var extSig []byte
 	if vote.Type == tmproto.PrecommitType && !types.ProtoBlockIDIsNil(&vote.BlockID) {
 		extSignBytes := types.VoteExtensionSignBytes(chainID, vote)

proto/tendermint/abci/types.proto

@@ -153,15 +153,19 @@ message RequestProcessProposal {
   bytes proposer_address = 8;
 }
 
-// Extends a vote with application-side injection
+// Extends a vote with application-injected data
 message RequestExtendVote {
+  // the hash of the block  that this vote may be referring to
   bytes hash   = 1;
+  // the height of the extended vote
   int64 height = 2;
 }
 
 // Verify the vote extension
 message RequestVerifyVoteExtension {
+  // the hash of the block that this received vote corresponds to
   bytes hash              = 1;
+  // the validator that signed the vote extension
   bytes validator_address = 2;
   int64 height            = 3;
   bytes vote_extension    = 4;
@@ -329,6 +333,10 @@ message ResponseVerifyVoteExtension {
   enum VerifyStatus {
     UNKNOWN = 0;
     ACCEPT  = 1;
+    // Rejecting the vote extension will reject the entire precommit by the sender.
+    // Incorrectly implementing this thus has liveness implications as it may affect
+    // tendermint's ability to receive 2/3+ valid votes to finalize the block.
+    // Honest nodes should never be rejected.
     REJECT  = 2;
   }
 }

proto/tendermint/types/canonical.proto

@@ -34,7 +34,6 @@ message CanonicalVote {
   CanonicalBlockID          block_id  = 4 [(gogoproto.customname) = "BlockID"];
   google.protobuf.Timestamp timestamp = 5 [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
   string                    chain_id  = 6 [(gogoproto.customname) = "ChainID"];
-  VoteExtensionToSign       vote_extension = 7;
 }
 
 // CanonicalVoteExtension provides us a way to serialize a vote extension from

proto/tendermint/types/types.proto

@@ -89,7 +89,7 @@ message Data {
   repeated bytes txs = 1;
 }
 
-// Vote represents a prevote, precommit, or commit vote from validators for
+// Vote represents a prevote or precommit vote from validators for
 // consensus.
 message Vote {
   SignedMsgType type     = 1;
@@ -109,21 +109,10 @@ message Vote {
   bytes extension = 9;
   // Vote extension signature by the validator if they participated in
   // consensus for the associated block.
+  // Only valid for precommit messages.
   bytes extension_signature = 10;
 }
 
-// VoteExtension is app-defined additional information to the validator votes.
-message VoteExtension {
-  bytes app_data_to_sign             = 1;
-  bytes app_data_self_authenticating = 2;
-}
-
-// VoteExtensionToSign is a subset of VoteExtension that is signed by the validators private key.
-// VoteExtensionToSign is extracted from an existing VoteExtension.
-message VoteExtensionToSign {
-  bytes app_data_to_sign = 1;
-}
-
 // Commit contains the evidence that a block was committed by a set of validators.
 message Commit {
   int64              height     = 1;
@@ -139,7 +128,6 @@ message CommitSig {
   google.protobuf.Timestamp timestamp         = 3
       [(gogoproto.nullable) = false, (gogoproto.stdtime) = true];
   bytes signature = 4;
-  VoteExtensionToSign vote_extension = 5;
 }
 
 message ExtendedCommit {
@@ -151,7 +139,8 @@ message ExtendedCommit {
 }
 
 // ExtendedCommitSig retains all the same fields as CommitSig but adds vote
-// extension-related fields.
+// extension-related fields. We use two signatures to ensure backwards compatibility.
+// That is the digest of the original signature is still the same in prior versions
 message ExtendedCommitSig {
   BlockIDFlag               block_id_flag     = 1;
   bytes                     validator_address = 2;

proxy/app_conn.go

@@ -90,17 +90,17 @@ func (app *appConnConsensus) ProcessProposal(ctx context.Context, req *types.Req
 }
 
 func (app *appConnConsensus) ExtendVote(ctx context.Context, req *types.RequestExtendVote) (*types.ResponseExtendVote, error) {
-	defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "deliver_tx", "type", "async"))()
+	defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "extend_vote"))()
 	return app.appConn.ExtendVote(ctx, req)
 }
 
 func (app *appConnConsensus) VerifyVoteExtension(ctx context.Context, req *types.RequestVerifyVoteExtension) (*types.ResponseVerifyVoteExtension, error) {
-	defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "deliver_tx", "type", "async"))()
+	defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "verify_vote_extension"))()
 	return app.appConn.VerifyVoteExtension(ctx, req)
 }
 
 func (app *appConnConsensus) FinalizeBlock(ctx context.Context, req *types.RequestFinalizeBlock) (*types.ResponseFinalizeBlock, error) {
-	defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "deliver_tx", "type", "async"))()
+	defer addTimeSample(app.metrics.MethodTimingSeconds.With("method", "finalize_block"))()
 	return app.appConn.FinalizeBlock(ctx, req)
 }
 

state/execution.go

@@ -549,7 +549,7 @@ func updateState(
 		nextParams = state.ConsensusParams.Update(abciResponse.ConsensusParamUpdates)
 		err := nextParams.ValidateBasic()
 		if err != nil {
-			return state, fmt.Errorf("updating consensus params: %w", err)
+			return state, fmt.Errorf("validating new consensus params: %w", err)
 		}
 
 		err = state.ConsensusParams.ValidateUpdate(abciResponse.ConsensusParamUpdates, header.Height)

store/store.go

@@ -247,7 +247,7 @@ func (bs *BlockStore) LoadBlockCommit(height int64) *types.Commit {
 // The extended commit is not guaranteed to contain the same +2/3 precommits data
 // as the commit in the block.
 func (bs *BlockStore) LoadBlockExtendedCommit(height int64) *types.ExtendedCommit {
-	var pbec = new(tmproto.ExtendedCommit)
+	pbec := new(tmproto.ExtendedCommit)
 	bz, err := bs.db.Get(calcExtCommitKey(height))
 	if err != nil {
 		panic(fmt.Errorf("fetching extended commit: %w", err))

types/block_test.go

@@ -232,21 +232,21 @@ func TestCommit(t *testing.T) {
 	lastID := makeBlockIDRandom()
 	h := int64(3)
 	voteSet, _, vals := randVoteSet(h-1, 1, tmproto.PrecommitType, 10, 1)
-	commit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, time.Now())
+	extCommit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, time.Now())
 	require.NoError(t, err)
 
-	assert.Equal(t, h-1, commit.Height)
-	assert.EqualValues(t, 1, commit.Round)
-	assert.Equal(t, tmproto.PrecommitType, tmproto.SignedMsgType(commit.Type()))
-	if commit.Size() <= 0 {
-		t.Fatalf("commit %v has a zero or negative size: %d", commit, commit.Size())
+	assert.Equal(t, h-1, extCommit.Height)
+	assert.EqualValues(t, 1, extCommit.Round)
+	assert.Equal(t, tmproto.PrecommitType, tmproto.SignedMsgType(extCommit.Type()))
+	if extCommit.Size() <= 0 {
+		t.Fatalf("commit %v has a zero or negative size: %d", extCommit, extCommit.Size())
 	}
 
-	require.NotNil(t, commit.BitArray())
-	assert.Equal(t, bits.NewBitArray(10).Size(), commit.BitArray().Size())
+	require.NotNil(t, extCommit.BitArray())
+	assert.Equal(t, bits.NewBitArray(10).Size(), extCommit.BitArray().Size())
 
-	assert.Equal(t, voteSet.GetByIndex(0), commit.GetByIndex(0))
-	assert.True(t, commit.IsCommit())
+	assert.Equal(t, voteSet.GetByIndex(0), extCommit.GetByIndex(0))
+	assert.True(t, extCommit.IsCommit())
 }
 
 func TestCommitValidateBasic(t *testing.T) {
@@ -440,11 +440,11 @@ func randCommit(now time.Time) *Commit {
 	lastID := makeBlockIDRandom()
 	h := int64(3)
 	voteSet, _, vals := randVoteSet(h-1, 1, tmproto.PrecommitType, 10, 1)
-	commit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, now)
+	extCommit, err := MakeExtCommit(lastID, h-1, 1, voteSet, vals, now)
 	if err != nil {
 		panic(err)
 	}
-	return commit.ToCommit()
+	return extCommit.ToCommit()
 }
 
 func hexBytesFromString(s string) bytes.HexBytes {

types/vote_set.go

@@ -217,7 +217,7 @@ func (voteSet *VoteSet) addVote(vote *Vote) (added bool, err error) {
 	// Check signature.
 	if voteSet.extensionsEnabled {
 		if err := vote.VerifyVoteAndExtension(voteSet.chainID, val.PubKey); err != nil {
-			return false, fmt.Errorf("failed to verify vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err)
+			return false, fmt.Errorf("failed to verify extended vote with ChainID %s and PubKey %s: %w", voteSet.chainID, val.PubKey, err)
 		}
 	} else {
 		if err := vote.Verify(voteSet.chainID, val.PubKey); err != nil {