cometbft · melekes · Mar 5, 2024 · Mar 2, 2024
@@ -0,0 +1,4 @@
+- `[privval]` DO NOT require extension signature from privval if vote
+  extensions are disabled. Remote signers must ONLY sign the extension if
+  `sign_extension` flag in `SignVoteRequest` is true.
+  [\#2496](https://github.com/cometbft/cometbft/pull/2496)
@@ -109,7 +109,7 @@ func (vs *validatorStub) signVote(
 		Extension:        voteExtension,
 	}
 	v := vote.ToProto()
-	if err = vs.PrivValidator.SignVote(test.DefaultTestChainID, v); err != nil {
+	if err = vs.PrivValidator.SignVote(test.DefaultTestChainID, v, true); err != nil {
 		return nil, fmt.Errorf("sign vote failed: %w", err)
 	}
 

@@ -92,7 +92,7 @@ func invalidDoPrevoteFunc(t *testing.T, cs *State, sw *p2p.Switch, pv types.Priv
 			},
 		}
 		p := precommit.ToProto()
-		err = cs.privValidator.SignVote(cs.state.ChainID, p)
+		err = cs.privValidator.SignVote(cs.state.ChainID, p, true)
 		if err != nil {
 			t.Error(err)
 		}

@@ -370,11 +370,11 @@ func TestVerifyDuplicateVoteEvidence(t *testing.T) {
 	vote1 := types.MakeVoteNoError(t, val, chainID, 0, 10, 2, 1, blockID, defaultEvidenceTime)
 
 	v1 := vote1.ToProto()
-	err := val.SignVote(chainID, v1)
+	err := val.SignVote(chainID, v1, false)
 	require.NoError(t, err)
 	badVote := types.MakeVoteNoError(t, val, chainID, 0, 10, 2, 1, blockID, defaultEvidenceTime)
 	bv := badVote.ToProto()
-	err = val2.SignVote(chainID, bv)
+	err = val2.SignVote(chainID, bv, false)
 	require.NoError(t, err)
 
 	vote1.Signature = v1.Signature

@@ -250,9 +250,9 @@ func TestValidateBlockCommit(t *testing.T) {
 		g := goodVote.ToProto()
 		b := badVote.ToProto()
 
-		err = badPrivVal.SignVote(chainID, g)
+		err = badPrivVal.SignVote(chainID, g, false)
 		require.NoError(t, err, "height %d", height)
-		err = badPrivVal.SignVote(chainID, b)
+		err = badPrivVal.SignVote(chainID, b, false)
 		require.NoError(t, err, "height %d", height)
 
 		goodVote.Signature, badVote.Signature = g.Signature, b.Signature

@@ -26,7 +26,7 @@ func MakeCommitFromVoteSet(blockID types.BlockID, voteSet *types.VoteSet, valida
 
 		v := vote.ToProto()
 
-		if err := validators[i].SignVote(voteSet.ChainID(), v); err != nil {
+		if err := validators[i].SignVote(voteSet.ChainID(), v, false); err != nil {
 			return nil, err
 		}
 		vote.Signature = v.Signature
@@ -68,7 +68,7 @@ func MakeCommit(blockID types.BlockID, height int64, round int32, valSet *types.
 
 		v := vote.ToProto()
 
-		if err := privVal.SignVote(chainID, v); err != nil {
+		if err := privVal.SignVote(chainID, v, false); err != nil {
 			return nil, err
 		}
 

@@ -266,8 +266,8 @@ func (pv *FilePV) GetPubKey() (crypto.PubKey, error) {
 
 // SignVote signs a canonical representation of the vote, along with the
 // chainID. Implements PrivValidator.
-func (pv *FilePV) SignVote(chainID string, vote *cmtproto.Vote) error {
-	if err := pv.signVote(chainID, vote); err != nil {
+func (pv *FilePV) SignVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {
+	if err := pv.signVote(chainID, vote, signExtension); err != nil {
 		return fmt.Errorf("error signing vote: %v", err)
 	}
 	return nil
@@ -312,7 +312,7 @@ func (pv *FilePV) String() string {
 // 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 *cmtproto.Vote) error {
+func (pv *FilePV) signVote(chainID string, vote *cmtproto.Vote, signExtension bool) error {
 	height, round, step := vote.Height, vote.Round, voteToStep(vote)
 
 	lss := pv.LastSignState
@@ -324,20 +324,24 @@ func (pv *FilePV) signVote(chainID string, vote *cmtproto.Vote) error {
 
 	signBytes := types.VoteSignBytes(chainID, vote)
 
-	// Vote extensions are non-deterministic, so it is possible that an
-	// 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 == types.PrecommitType && !types.ProtoBlockIDIsNil(&vote.BlockID) {
-		extSignBytes := types.VoteExtensionSignBytes(chainID, vote)
-		extSig, err = pv.Key.PrivKey.Sign(extSignBytes)
-		if err != nil {
-			return err
+	if signExtension {
+		// Vote extensions are non-deterministic, so it is possible that an
+		// 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 == types.PrecommitType && !types.ProtoBlockIDIsNil(&vote.BlockID) {
+			extSignBytes := types.VoteExtensionSignBytes(chainID, vote)
+			extSig, err = pv.Key.PrivKey.Sign(extSignBytes)
+			if err != nil {
+				return err
+			}
+		} else if len(vote.Extension) > 0 {
+			return errors.New("unexpected vote extension - extensions are only allowed in non-nil precommits")
 		}
-	} else if len(vote.Extension) > 0 {
-		return errors.New("unexpected vote extension - extensions are only allowed in non-nil precommits")
+
+		vote.ExtensionSignature = extSig
 	}
 
 	// We might crash before writing to the wal,
@@ -357,8 +361,6 @@ func (pv *FilePV) signVote(chainID string, vote *cmtproto.Vote) error {
 			err = errors.New("conflicting data")
 		}
 
-		vote.ExtensionSignature = extSig
-
 		return err
 	}
 
@@ -369,7 +371,6 @@ func (pv *FilePV) signVote(chainID string, vote *cmtproto.Vote) error {
 	}
 	pv.saveSigned(height, round, step, signBytes, sig)
 	vote.Signature = sig
-	vote.ExtensionSignature = extSig
 
 	return nil
 }