refactor(consensus): print err from SignAndCheckVote (backport #2346) (…

…#2441) This is an automatic backport of pull request #2346 done by [Mergify](https://mergify.com). Cherry-pick of e623c44 has failed: ``` On branch mergify/bp/v0.38.x/pr-2346 Your branch is up to date with 'origin/v0.38.x'. You are currently cherry-picking commit e623c44. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: consensus/state.go modified: types/vote.go Unmerged paths: (use "git add <file>..." to mark resolution) both modified: types/vote_test.go ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details> --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
cometbft · Feb 26, 2024 · 8cd4a69 · 8cd4a69
1 parent 27c8055
commit 8cd4a69
Show file tree

Hide file tree

Showing 3 changed files with 138 additions and 17 deletions.
diff --git a/consensus/state.go b/consensus/state.go
@@ -2395,7 +2395,7 @@ func (cs *State) signVote(
 
 	recoverable, err := types.SignAndCheckVote(vote, cs.privValidator, cs.state.ChainID, extEnabled && (msgType == cmtproto.PrecommitType))
 	if err != nil && !recoverable {
-		panic(fmt.Sprintf("non-recoverable error when signing vote (%d/%d)", vote.Height, vote.Round))
+		panic(fmt.Sprintf("non-recoverable error when signing vote %v: %v", vote, err))
 	}
 
 	return vote, err

diff --git a/types/vote.go b/types/vote.go
@@ -47,6 +47,15 @@ func NewConflictingVoteError(vote1, vote2 *Vote) *ErrVoteConflictingVotes {
 	}
 }
 
+// The vote extension is only valid for non-nil precommits.
+type ErrVoteExtensionInvalid struct {
+	ExtSignature []byte
+}
+
+func (err *ErrVoteExtensionInvalid) Error() string {
+	return fmt.Sprintf("extensions must be present IFF vote is a non-nil Precommit; extension signature: %X", err.ExtSignature)
+}
+
 // Address is hex bytes.
 type Address = crypto.Address
 
@@ -396,6 +405,9 @@ func VotesToProto(votes []*Vote) []*cmtproto.Vote {
 	return res
 }
 
+// SignAndCheckVote signs the vote with the given privVal and checks the vote.
+// It returns an error if the vote is invalid and a boolean indicating if the
+// error is recoverable or not.
 func SignAndCheckVote(
 	vote *Vote,
 	privVal PrivValidator,
@@ -404,33 +416,30 @@ func SignAndCheckVote(
 ) (bool, error) {
 	v := vote.ToProto()
 	if err := privVal.SignVote(chainID, v); err != nil {
-		// Failing to sign a vote has always been a recoverable error, this function keeps it that way
-		return true, err // true = recoverable
+		// Failing to sign a vote has always been a recoverable error, this
+		// function keeps it that way.
+		return true, err
 	}
 	vote.Signature = v.Signature
 
 	isPrecommit := vote.Type == cmtproto.PrecommitType
 	if !isPrecommit && extensionsEnabled {
 		// Non-recoverable because the caller passed parameters that don't make sense
-		return false, fmt.Errorf("only Precommit votes may have extensions enabled; vote type: %d", vote.Type)
+		return false, &ErrVoteExtensionInvalid{ExtSignature: v.ExtensionSignature}
 	}
 
 	isNil := vote.BlockID.IsZero()
 	extSignature := (len(v.ExtensionSignature) > 0)
 	if extSignature == (!isPrecommit || isNil) {
 		// Non-recoverable because the vote is malformed
-		return false, fmt.Errorf(
-			"extensions must be present IFF vote is a non-nil Precommit; present %t, vote type %d, is nil %t",
-			extSignature,
-			vote.Type,
-			isNil,
-		)
+		return false, &ErrVoteExtensionInvalid{ExtSignature: v.ExtensionSignature}
 	}
 
 	vote.ExtensionSignature = nil
 	if extensionsEnabled {
 		vote.ExtensionSignature = v.ExtensionSignature
 	}
+
 	vote.Timestamp = v.Timestamp
 
 	return true, nil

diff --git a/types/vote_test.go b/types/vote_test.go
@@ -1,6 +1,7 @@
 package types
 
 import (
+	"fmt"
 	"testing"
 	"time"
 
@@ -22,12 +23,13 @@ func examplePrevote() *Vote {
 
 func examplePrecommit() *Vote {
 	vote := exampleVote(byte(cmtproto.PrecommitType))
+	vote.Extension = []byte("extension")
 	vote.ExtensionSignature = []byte("signature")
 	return vote
 }
 
 func exampleVote(t byte) *Vote {
-	var stamp, err = time.Parse(TimeFormat, "2017-12-25T03:00:01.234Z")
+	stamp, err := time.Parse(TimeFormat, "2017-12-25T03:00:01.234Z")
 	if err != nil {
 		panic(err)
 	}
@@ -61,7 +63,6 @@ func TestVoteSignable(t *testing.T) {
 }
 
 func TestVoteSignBytesTestVectors(t *testing.T) {
-
 	tests := []struct {
 		chainID string
 		vote    *Vote
@@ -85,7 +86,8 @@ func TestVoteSignBytesTestVectors(t *testing.T) {
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
 				0x2a, // (field_number << 3) | wire_type
 				// remaining fields (timestamp):
-				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
+				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1,
+			},
 		},
 		// with proper (fixed size) height and round (PreVote):
 		2: {
@@ -100,7 +102,8 @@ func TestVoteSignBytesTestVectors(t *testing.T) {
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
 				0x2a, // (field_number << 3) | wire_type
 				// remaining fields (timestamp):
-				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
+				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1,
+			},
 		},
 		3: {
 			"", &Vote{Height: 1, Round: 1},
@@ -112,7 +115,8 @@ func TestVoteSignBytesTestVectors(t *testing.T) {
 				0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, // round
 				// remaining fields (timestamp):
 				0x2a,
-				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1},
+				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1,
+			},
 		},
 		// containing non-empty chain_id:
 		4: {
@@ -128,7 +132,8 @@ func TestVoteSignBytesTestVectors(t *testing.T) {
 				0xb, 0x8, 0x80, 0x92, 0xb8, 0xc3, 0x98, 0xfe, 0xff, 0xff, 0xff, 0x1, // timestamp
 				// (field_number << 3) | wire_type
 				0x32,
-				0xd, 0x74, 0x65, 0x73, 0x74, 0x5f, 0x63, 0x68, 0x61, 0x69, 0x6e, 0x5f, 0x69, 0x64}, // chainID
+				0xd, 0x74, 0x65, 0x73, 0x74, 0x5f, 0x63, 0x68, 0x61, 0x69, 0x6e, 0x5f, 0x69, 0x64,
+			}, // chainID
 		},
 		// containing vote extension
 		5: {
@@ -309,7 +314,7 @@ func TestVoteVerify(t *testing.T) {
 
 func TestVoteString(t *testing.T) {
 	str := examplePrecommit().String()
-	expected := `Vote{56789:6AF1F4111082 12345/02/SIGNED_MSG_TYPE_PRECOMMIT(Precommit) 8B01023386C3 000000000000 000000000000 @ 2017-12-25T03:00:01.234Z}` //nolint:lll //ignore line length for tests
+	expected := `Vote{56789:6AF1F4111082 12345/02/SIGNED_MSG_TYPE_PRECOMMIT(Precommit) 8B01023386C3 000000000000 657874656E73 @ 2017-12-25T03:00:01.234Z}` //nolint:lll //ignore line length for tests
 	if str != expected {
 		t.Errorf("got unexpected string for Vote. Expected:\n%v\nGot:\n%v", expected, str)
 	}
@@ -486,3 +491,110 @@ func TestVoteProtobuf(t *testing.T) {
 		}
 	}
 }
+
+func TestSignAndCheckVote(t *testing.T) {
+	privVal := NewMockPV()
+
+	testCases := []struct {
+		name              string
+		extensionsEnabled bool
+		vote              *Vote
+		expectError       bool
+	}{
+		{
+			name:              "precommit with extension signature",
+			extensionsEnabled: true,
+			vote:              examplePrecommit(),
+			expectError:       false,
+		},
+		{
+			name:              "precommit with extension signature",
+			extensionsEnabled: false,
+			vote:              examplePrecommit(),
+			expectError:       false,
+		},
+		{
+			name:              "precommit with extension signature for a nil block",
+			extensionsEnabled: true,
+			vote: func() *Vote {
+				v := examplePrecommit()
+				v.BlockID = BlockID{make([]byte, 0), PartSetHeader{0, make([]byte, 0)}}
+				return v
+			}(),
+			expectError: true,
+		},
+		{
+			name:              "precommit with extension signature for a nil block",
+			extensionsEnabled: false,
+			vote: func() *Vote {
+				v := examplePrecommit()
+				v.BlockID = BlockID{make([]byte, 0), PartSetHeader{0, make([]byte, 0)}}
+				return v
+			}(),
+			expectError: true,
+		},
+		{
+			name:              "precommit without extension",
+			extensionsEnabled: true,
+			vote: func() *Vote {
+				v := examplePrecommit()
+				v.Extension = make([]byte, 0)
+				return v
+			}(),
+			expectError: false,
+		},
+		{
+			name:              "precommit without extension",
+			extensionsEnabled: false,
+			vote: func() *Vote {
+				v := examplePrecommit()
+				v.Extension = make([]byte, 0)
+				return v
+			}(),
+			expectError: false,
+		},
+		{
+			name:              "prevote",
+			extensionsEnabled: true,
+			vote:              examplePrevote(),
+			expectError:       true,
+		},
+		{
+			name:              "prevote",
+			extensionsEnabled: false,
+			vote:              examplePrevote(),
+			expectError:       false,
+		},
+		{
+			name:              "prevote with extension",
+			extensionsEnabled: true,
+			vote: func() *Vote {
+				v := examplePrevote()
+				v.Extension = []byte("extension")
+				return v
+			}(),
+			expectError: true,
+		},
+		{
+			name:              "prevote with extension",
+			extensionsEnabled: false,
+			vote: func() *Vote {
+				v := examplePrevote()
+				v.Extension = []byte("extension")
+				return v
+			}(),
+			expectError: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(fmt.Sprintf("%s (extensionsEnabled: %t) ", tc.name, tc.extensionsEnabled), func(t *testing.T) {
+			_, err := SignAndCheckVote(tc.vote, privVal, "test_chain_id", tc.extensionsEnabled)
+			if tc.expectError {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+			}
+		})
+	}
+}