From 4255d5d23366dfc875cffb8a35ea1dc907589200 Mon Sep 17 00:00:00 2001 From: Sergio Mena Date: Thu, 22 Dec 2022 18:20:26 +0100 Subject: [PATCH] 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 * Addressed @lasarojc's comment * Addressed @jmalicevic's comment Co-authored-by: Lasaro --- consensus/state.go | 17 +++++----- light/trust_options.go | 2 +- mempool/v1/mempool_test.go | 12 +++---- state/validation_test.go | 7 ++++ types/block.go | 4 +-- types/test_util.go | 40 ++--------------------- types/vote.go | 65 ++++++++++++++++++++++++++++++++++---- 7 files changed, 85 insertions(+), 62 deletions(-) diff --git a/consensus/state.go b/consensus/state.go index 61c0a3a14..1a754c8fe 100644 --- a/consensus/state.go +++ b/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 } diff --git a/light/trust_options.go b/light/trust_options.go index cbf3b1cd8..f9b8d400f 100644 --- a/light/trust_options.go +++ b/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", diff --git a/mempool/v1/mempool_test.go b/mempool/v1/mempool_test.go index d873dcfa0..10e3548f2 100644 --- a/mempool/v1/mempool_test.go +++ b/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() diff --git a/state/validation_test.go b/state/validation_test.go index b0f671e33..00f09afd8 100644 --- a/state/validation_test.go +++ b/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) { diff --git a/types/block.go b/types/block.go index bf5dff51d..9d3aea077 100644 --- a/types/block.go +++ b/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 { diff --git a/types/test_util.go b/types/test_util.go index a0ec69d7c..aa8702b63 100644 --- a/types/test_util.go +++ b/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 } diff --git a/types/vote.go b/types/vote.go index 47a4685ca..3d5b6410b 100644 --- a/types/vote.go +++ b/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 +}