diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 0e7243a59..ea0606dcc 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -19,6 +19,8 @@ program](https://hackerone.com/tendermint). ### IMPROVEMENTS: +- [consensus] \#5143 Only call `privValidator.GetPubKey` once per block (@melekes) + ### BUG FIXES: - [consensus] [\#4895](https://github.com/tendermint/tendermint/pull/4895) Cache the address of the validator to reduce querying a remote KMS (@joe-bowman) diff --git a/consensus/state.go b/consensus/state.go index bf8e884ea..a3d826b94 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -17,6 +17,7 @@ import ( cfg "github.com/tendermint/tendermint/config" cstypes "github.com/tendermint/tendermint/consensus/types" + "github.com/tendermint/tendermint/crypto" tmevents "github.com/tendermint/tendermint/libs/events" "github.com/tendermint/tendermint/p2p" sm "github.com/tendermint/tendermint/state" @@ -31,6 +32,8 @@ var ( ErrInvalidProposalPOLRound = errors.New("Error invalid proposal POL round") ErrAddingVote = errors.New("Error adding vote") ErrVoteHeightMismatch = errors.New("Error vote height mismatch") + + errPubKeyIsNotSet = errors.New("pubkey is not set. Look for \"Can't get private validator pubkey\" errors") ) //----------------------------------------------------------------------------- @@ -95,6 +98,9 @@ type ConsensusState struct { mtx sync.RWMutex cstypes.RoundState state sm.State // State until height-1. + // privValidator pubkey, memoized for the duration of one block + // to avoid extra requests to HSM + privValidatorPubKey crypto.PubKey // state changes may be triggered by: msgs from peers, // msgs from ourself, or by timeouts @@ -251,11 +257,17 @@ func (cs *ConsensusState) GetValidators() (int64, []*types.Validator) { return cs.state.LastBlockHeight, cs.state.Validators.Copy().Validators } -// SetPrivValidator sets the private validator account for signing votes. +// SetPrivValidator sets the private validator account for signing votes. It +// immediately requests pubkey and caches it. func (cs *ConsensusState) SetPrivValidator(priv types.PrivValidator) { cs.mtx.Lock() + defer cs.mtx.Unlock() + cs.privValidator = priv - cs.mtx.Unlock() + + if err := cs.updatePrivValidatorPubKey(); err != nil { + cs.Logger.Error("Can't get private validator pubkey", "err", err) + } } // SetTimeoutTicker sets the local timer. It may be useful to overwrite for testing. @@ -920,8 +932,17 @@ func (cs *ConsensusState) enterPropose(height int64, round int) { return } + logger.Debug("This node is a validator") + + if cs.privValidatorPubKey == nil { + // If this node is a validator & proposer in the current round, it will + // miss the opportunity to create a block. + logger.Error(fmt.Sprintf("enterPropose: %v", errPubKeyIsNotSet)) + return + } + address := cs.privValidatorPubKey.Address() + // if not a validator, we're done - address := cs.privValidator.GetPubKey().Address() if !cs.Validators.HasAddress(address) { logger.Debug("This node is not a validator", "addr", address, "vals", cs.Validators) return @@ -1007,7 +1028,12 @@ func (cs *ConsensusState) isProposalComplete() bool { // is returned for convenience so we can log the proposal block. // Returns nil block upon error. // NOTE: keep it side-effect free for clarity. +// CONTRACT: cs.privValidator is not nil. func (cs *ConsensusState) createProposalBlock() (block *types.Block, blockParts *types.PartSet) { + if cs.privValidator == nil { + panic("entered createProposalBlock with privValidator being nil") + } + var commit *types.Commit switch { case cs.Height == 1: @@ -1017,13 +1043,19 @@ func (cs *ConsensusState) createProposalBlock() (block *types.Block, blockParts case cs.LastCommit.HasTwoThirdsMajority(): // Make the commit from LastCommit commit = cs.LastCommit.MakeCommit() - default: - // This shouldn't happen. - cs.Logger.Error("enterPropose: Cannot propose anything: No commit for the previous block.") + default: // This shouldn't happen. + cs.Logger.Error("enterPropose: Cannot propose anything: No commit for the previous block") return } - proposerAddr := cs.privValidator.GetPubKey().Address() + if cs.privValidatorPubKey == nil { + // If this node is a validator & proposer in the current round, it will + // miss the opportunity to create a block. + cs.Logger.Error(fmt.Sprintf("enterPropose: %v", errPubKeyIsNotSet)) + return + } + proposerAddr := cs.privValidatorPubKey.Address() + return cs.blockExec.CreateProposalBlock(cs.Height, cs.state, commit, proposerAddr) } @@ -1446,6 +1478,11 @@ func (cs *ConsensusState) finalizeCommit(height int64) { fail.Fail() // XXX + // Private validator might have changed it's key pair => refetch pubkey. + if err := cs.updatePrivValidatorPubKey(); err != nil { + cs.Logger.Error("Can't get private validator pubkey", "err", err) + } + // cs.StartTime is already set. // Schedule Round0 to start soon. cs.scheduleRound0(&cs.RoundState) @@ -1467,7 +1504,12 @@ func (cs *ConsensusState) recordMetrics(height int64, block *types.Block) { ) if cs.privValidator != nil { - address = cs.privValidator.GetPubKey().Address() + if cs.privValidatorPubKey == nil { + // Metrics won't be updated, but it's not critical. + cs.Logger.Error(fmt.Sprintf("recordMetrics: %v", errPubKeyIsNotSet)) + } else { + address = cs.privValidatorPubKey.Address() + } } for i, val := range cs.Validators.Validators { @@ -1635,8 +1677,10 @@ func (cs *ConsensusState) tryAddVote(vote *types.Vote, peerID p2p.ID) (bool, err if err == ErrVoteHeightMismatch { return added, err } else if voteErr, ok := err.(*types.ErrVoteConflictingVotes); ok { - addr := cs.privValidator.GetPubKey().Address() - if bytes.Equal(vote.ValidatorAddress, addr) { + if cs.privValidatorPubKey == nil { + return false, errPubKeyIsNotSet + } + if bytes.Equal(vote.ValidatorAddress, cs.privValidatorPubKey.Address()) { cs.Logger.Error( "Found conflicting vote from ourselves. Did you unsafe_reset a validator?", "height", vote.Height, @@ -1823,7 +1867,10 @@ func (cs *ConsensusState) signVote( // and the privValidator will refuse to sign anything. cs.wal.FlushAndSync() - addr := cs.privValidator.GetPubKey().Address() + if cs.privValidatorPubKey == nil { + return nil, errPubKeyIsNotSet + } + addr := cs.privValidatorPubKey.Address() valIndex, _ := cs.Validators.GetByAddress(addr) vote := &types.Vote{ @@ -1860,8 +1907,18 @@ func (cs *ConsensusState) voteTime() time.Time { // sign the vote and publish on internalMsgQueue func (cs *ConsensusState) signAddVote(type_ types.SignedMsgType, hash []byte, header types.PartSetHeader) *types.Vote { - // if we don't have a key or we're not in the validator set, do nothing - if cs.privValidator == nil || !cs.Validators.HasAddress(cs.privValidator.GetPubKey().Address()) { + if cs.privValidator == nil { // the node does not have a key + return nil + } + + if cs.privValidatorPubKey == nil { + // Vote won't be signed, but it's not critical. + cs.Logger.Error(fmt.Sprintf("signAddVote: %v", errPubKeyIsNotSet)) + return nil + } + + // If the node not in the validator set, do nothing. + if !cs.Validators.HasAddress(cs.privValidatorPubKey.Address()) { return nil } vote, err := cs.signVote(type_, hash, header) @@ -1876,6 +1933,18 @@ func (cs *ConsensusState) signAddVote(type_ types.SignedMsgType, hash []byte, he return nil } +// updatePrivValidatorPubKey get's the private validator public key and +// memoizes it. This func returns an error if the private validator is not +// responding or responds with an error. +func (cs *ConsensusState) updatePrivValidatorPubKey() error { + if cs.privValidator == nil { + return nil + } + + cs.privValidatorPubKey = cs.privValidator.GetPubKey() + return nil +} + //--------------------------------------------------------- func CompareHRS(h1 int64, r1 int, s1 cstypes.RoundStepType, h2 int64, r2 int, s2 cstypes.RoundStepType) int {