Divergences in comparison with #9620. Part 4: Other changes spotted (#9927)

* Make mempool v1 UTs more predictable

* Simple changes

* Reuse new signVote tests in production code

* Fix `IsNil` problem from cherry-pick: should be `IsZero`

* Fix linter issue

* Apply suggestions from code review

Co-authored-by: Lasaro <lasaro@informal.systems>

* Addressed @lasarojc's comment

* Addressed @jmalicevic's comment

Co-authored-by: Lasaro <lasaro@informal.systems>

consensus/state.go

@@ -2119,7 +2119,7 @@ func (cs *State) addVote(vote *types.Vote, peerID p2p.ID) (added bool, err error
 		}
 		// Verify VoteExtension if precommit and not nil
 		// https://github.com/tendermint/tendermint/issues/8487
-		if vote.Type == tmproto.PrecommitType && len(vote.BlockID.Hash) != 0 &&
+		if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsZero() &&
 			!bytes.Equal(vote.ValidatorAddress, myAddr) { // Skip the VerifyVoteExtension call if the vote was issued by this validator.
 
 			// The core fields of the vote message were already validated in the
@@ -2308,10 +2308,11 @@ func (cs *State) signVote(
 		BlockID:          types.BlockID{Hash: hash, PartSetHeader: header},
 	}
 
-	if msgType == tmproto.PrecommitType && len(vote.BlockID.Hash) != 0 {
+	extEnabled := cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height)
+	if msgType == tmproto.PrecommitType && !vote.BlockID.IsZero() {
 		// if the signedMessage type is for a non-nil precommit, add
 		// VoteExtension
-		if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height) {
+		if extEnabled {
 			ext, err := cs.blockExec.ExtendVote(vote)
 			if err != nil {
 				return nil, err
@@ -2319,11 +2320,11 @@ func (cs *State) signVote(
 			vote.Extension = ext
 		}
 	}
-	v := vote.ToProto()
-	err := cs.privValidator.SignVote(cs.state.ChainID, v)
-	vote.Signature = v.Signature
-	vote.ExtensionSignature = v.ExtensionSignature
-	vote.Timestamp = v.Timestamp
+
+	recoverable, err := types.SignAndCheckVote(vote, cs.privValidator, cs.state.ChainID, extEnabled && (msgType == tmproto.PrecommitType))
+	if err != nil && !recoverable {
+		panic(fmt.Sprintf("non-recoverable error when signing vote (%d/%d)", vote.Height, vote.Round))
+	}
 
 	return vote, err
 }

light/trust_options.go

@@ -41,7 +41,7 @@ func (opts TrustOptions) ValidateBasic() error {
 		return errors.New("negative or zero period")
 	}
 	if opts.Height <= 0 {
-		return errors.New("negative or zero height")
+		return errors.New("zero or negative height")
 	}
 	if len(opts.Hash) != tmhash.Size {
 		return fmt.Errorf("expected hash size to be %d bytes, got %d bytes",

mempool/v1/mempool_test.go

@@ -464,9 +464,9 @@ func TestTxMempool_ConcurrentTxs(t *testing.T) {
 
 	wg.Add(1)
 	go func() {
-		for i := 0; i < 20; i++ {
+		for i := 0; i < 10; i++ {
 			_ = checkTxs(t, txmp, 100, 0)
-			dur := rng.Intn(1000-500) + 500
+			dur := rng.Intn(1000-500) + 200
 			time.Sleep(time.Duration(dur) * time.Millisecond)
 		}
 
@@ -476,7 +476,7 @@ func TestTxMempool_ConcurrentTxs(t *testing.T) {
 
 	wg.Add(1)
 	go func() {
-		ticker := time.NewTicker(time.Second)
+		ticker := time.NewTicker(500 * time.Millisecond)
 		defer ticker.Stop()
 		defer wg.Done()
 
@@ -521,7 +521,7 @@ func TestTxMempool_ConcurrentTxs(t *testing.T) {
 
 func TestTxMempool_ExpiredTxs_Timestamp(t *testing.T) {
 	txmp := setup(t, 5000)
-	txmp.config.TTLDuration = 5 * time.Millisecond
+	txmp.config.TTLDuration = 50 * time.Millisecond
 
 	added1 := checkTxs(t, txmp, 10, 0)
 	require.Equal(t, len(added1), txmp.Size())
@@ -538,11 +538,11 @@ func TestTxMempool_ExpiredTxs_Timestamp(t *testing.T) {
 	//
 	// The exact intervals are not important except that the delta should be
 	// large relative to the cost of CheckTx (ms vs. ns is fine here).
-	time.Sleep(3 * time.Millisecond)
+	time.Sleep(30 * time.Millisecond)
 	added2 := checkTxs(t, txmp, 10, 1)
 
 	// Wait a while longer, so that the first batch will expire.
-	time.Sleep(3 * time.Millisecond)
+	time.Sleep(30 * time.Millisecond)
 
 	// Trigger an update so that pruning will occur.
 	txmp.Lock()

state/validation_test.go

@@ -114,6 +114,13 @@ func TestValidateBlockHeader(t *testing.T) {
 		require.NoError(t, err, "height %d", height)
 		lastCommit = lastExtCommit.ToCommit()
 	}
+
+	nextHeight := validationTestsStopHeight
+	block := makeBlock(state, nextHeight, lastCommit)
+	state.InitialHeight = nextHeight + 1
+	err := blockExec.ValidateBlock(state, block)
+	require.Error(t, err, "expected an error when state is ahead of block")
+	assert.Contains(t, err.Error(), "lower than initial height")
 }
 
 func TestValidateBlockCommit(t *testing.T) {

types/block.go

@@ -1204,8 +1204,8 @@ func (ec *ExtendedCommit) ValidateBasic() error {
 	}
 
 	if ec.Height >= 1 {
-		if len(ec.BlockID.Hash) == 0 {
-			return errors.New("commit cannot be for nil block")
+		if ec.BlockID.IsZero() {
+			return errors.New("extended commit cannot be for nil block")
 		}
 
 		if len(ec.ExtendedSignatures) == 0 {

types/test_util.go

@@ -39,47 +39,11 @@ func MakeExtCommit(blockID BlockID, height int64, round int32,
 	return voteSet.MakeExtendedCommit(), nil
 }
 
-func signAndCheckVote(
-	vote *Vote,
-	privVal PrivValidator,
-	chainID string,
-	extensionsEnabled bool,
-) error {
-	v := vote.ToProto()
-	if err := privVal.SignVote(chainID, v); err != nil {
-		return err
-	}
-	vote.Signature = v.Signature
-
-	isPrecommit := vote.Type == tmproto.PrecommitType
-	if !isPrecommit && extensionsEnabled {
-		return fmt.Errorf("only Precommit votes may have extensions enabled; vote type: %d", vote.Type)
-	}
-
-	isNil := vote.BlockID.IsZero()
-	extSignature := (len(v.ExtensionSignature) > 0)
-	if extSignature == (!isPrecommit || isNil) {
-		return 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,
-		)
-	}
-
-	vote.ExtensionSignature = nil
-	if extensionsEnabled {
-		vote.ExtensionSignature = v.ExtensionSignature
-	}
-
-	return nil
-}
-
 func signAddVote(privVal PrivValidator, vote *Vote, voteSet *VoteSet) (bool, error) {
 	if vote.Type != voteSet.signedMsgType {
 		return false, fmt.Errorf("vote and voteset are of different types; %d != %d", vote.Type, voteSet.signedMsgType)
 	}
-	if err := signAndCheckVote(vote, privVal, voteSet.ChainID(), voteSet.extensionsEnabled); err != nil {
+	if _, err := SignAndCheckVote(vote, privVal, voteSet.ChainID(), voteSet.extensionsEnabled); err != nil {
 		return false, err
 	}
 	return voteSet.AddVote(vote)
@@ -111,7 +75,7 @@ func MakeVote(
 	}
 
 	extensionsEnabled := step == tmproto.PrecommitType
-	if err := signAndCheckVote(vote, val, chainID, extensionsEnabled); err != nil {
+	if _, err := SignAndCheckVote(vote, val, chainID, extensionsEnabled); err != nil {
 		return nil, err
 	}
 

types/vote.go

@@ -261,7 +261,7 @@ func (vote *Vote) VerifyVoteAndExtension(chainID string, pubKey crypto.PubKey) e
 // VerifyExtension checks whether the vote extension signature corresponds to the
 // given chain ID and public key.
 func (vote *Vote) VerifyExtension(chainID string, pubKey crypto.PubKey) error {
-	if vote.Type != tmproto.PrecommitType || len(vote.BlockID.Hash) == 0 {
+	if vote.Type != tmproto.PrecommitType || vote.BlockID.IsZero() {
 		return nil
 	}
 	v := vote.ToProto()
@@ -280,8 +280,8 @@ func (vote *Vote) ValidateBasic() error {
 		return errors.New("invalid Type")
 	}
 
-	if vote.Height < 0 {
-		return errors.New("negative Height")
+	if vote.Height <= 0 {
+		return errors.New("negative or zero Height")
 	}
 
 	if vote.Round < 0 {
@@ -320,19 +320,30 @@ func (vote *Vote) ValidateBasic() error {
 	// We should only ever see vote extensions in non-nil precommits, otherwise
 	// this is a violation of the specification.
 	// https://github.com/tendermint/tendermint/issues/8487
-	if vote.Type != tmproto.PrecommitType || (vote.Type == tmproto.PrecommitType && len(vote.BlockID.Hash) == 0) {
+	if vote.Type != tmproto.PrecommitType || vote.BlockID.IsZero() {
 		if len(vote.Extension) > 0 {
-			return errors.New("unexpected vote extension")
+			return fmt.Errorf(
+				"unexpected vote extension; vote type %d, isNil %t",
+				vote.Type, vote.BlockID.IsZero(),
+			)
 		}
 		if len(vote.ExtensionSignature) > 0 {
 			return errors.New("unexpected vote extension signature")
 		}
 	}
 
-	if vote.Type == tmproto.PrecommitType && len(vote.BlockID.Hash) != 0 {
+	if vote.Type == tmproto.PrecommitType && !vote.BlockID.IsZero() {
+		// It's possible that this vote has vote extensions but
+		// they could also be disabled and thus not present thus
+		// we can't do all checks
 		if len(vote.ExtensionSignature) > MaxSignatureSize {
 			return fmt.Errorf("vote extension signature is too big (max: %d)", MaxSignatureSize)
 		}
+
+		// NOTE: extended votes should have a signature regardless of
+		// of whether there is any data in the extension or not however
+		// we don't know if extensions are enabled so we can only
+		// enforce the signature when extension size is not nil
 		if len(vote.ExtensionSignature) == 0 && len(vote.Extension) != 0 {
 			return fmt.Errorf("vote extension signature absent on vote with extension")
 		}
@@ -348,7 +359,7 @@ func (vote *Vote) EnsureExtension() error {
 	if vote.Type != tmproto.PrecommitType {
 		return nil
 	}
-	if len(vote.BlockID.Hash) == 0 {
+	if vote.BlockID.IsZero() {
 		return nil
 	}
 	if len(vote.ExtensionSignature) > 0 {
@@ -393,3 +404,43 @@ func VotesToProto(votes []*Vote) []*tmproto.Vote {
 	}
 	return res
 }
+
+func SignAndCheckVote(
+	vote *Vote,
+	privVal PrivValidator,
+	chainID string,
+	extensionsEnabled bool,
+) (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
+	}
+	vote.Signature = v.Signature
+
+	isPrecommit := vote.Type == tmproto.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)
+	}
+
+	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,
+		)
+	}
+
+	vote.ExtensionSignature = nil
+	if extensionsEnabled {
+		vote.ExtensionSignature = v.ExtensionSignature
+	}
+	vote.Timestamp = v.Timestamp
+
+	return true, nil
+}