From e9897c82e54d8da84910b5b09e6cd79d308ec6aa Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Fri, 15 May 2020 15:30:08 +0400 Subject: [PATCH] rpc/core: do not lock ConsensusState mutex in /validators, /consensus_params and /status Closes #3161 --- CHANGELOG_PENDING.md | 1 + rpc/core/blocks.go | 25 ++-------------- rpc/core/consensus.go | 25 ++++++++-------- rpc/core/env.go | 29 +++++++++++++++++++ rpc/core/status.go | 62 ++++++++++++++-------------------------- rpc/swagger/swagger.yaml | 4 +++ 6 files changed, 71 insertions(+), 75 deletions(-) diff --git a/CHANGELOG_PENDING.md b/CHANGELOG_PENDING.md index 7d603b0c6..e93d77d19 100644 --- a/CHANGELOG_PENDING.md +++ b/CHANGELOG_PENDING.md @@ -73,6 +73,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi - [state] [\#4781](https://github.com/tendermint/tendermint/pull/4781) Export `InitStateVersion` for the initial state version (@erikgrinaker) - [p2p/conn] \#4795 Return err on `signChallenge()` instead of panic - [evidence] [\#4839](https://github.com/tendermint/tendermint/pull/4839) Reject duplicate evidence from being proposed (@cmwaters) +- [rpc/core] [\#4844](https://github.com/tendermint/tendermint/pull/4844) Do not lock consensus state in `/validators`, `/consensus_params` and `/status` (@melekes) ### BUG FIXES: diff --git a/rpc/core/blocks.go b/rpc/core/blocks.go index c89a80788..a8e33176a 100644 --- a/rpc/core/blocks.go +++ b/rpc/core/blocks.go @@ -76,7 +76,7 @@ func filterMinMax(base, height, min, max, limit int64) (int64, int64, error) { // If no height is provided, it will fetch the latest block. // More: https://docs.tendermint.com/master/rpc/#/Info/block func Block(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlock, error) { - height, err := getHeight(env.BlockStore.Base(), env.BlockStore.Height(), heightPtr) + height, err := getHeight(env.BlockStore.Height(), heightPtr) if err != nil { return nil, err } @@ -105,7 +105,7 @@ func BlockByHash(ctx *rpctypes.Context, hash []byte) (*ctypes.ResultBlock, error // If no height is provided, it will fetch the commit for the latest block. // More: https://docs.tendermint.com/master/rpc/#/Info/commit func Commit(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultCommit, error) { - height, err := getHeight(env.BlockStore.Base(), env.BlockStore.Height(), heightPtr) + height, err := getHeight(env.BlockStore.Height(), heightPtr) if err != nil { return nil, err } @@ -136,7 +136,7 @@ func Commit(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultCommit, erro // getBlock(h).Txs[5] // More: https://docs.tendermint.com/master/rpc/#/Info/block_results func BlockResults(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlockResults, error) { - height, err := getHeight(env.BlockStore.Base(), env.BlockStore.Height(), heightPtr) + height, err := getHeight(env.BlockStore.Height(), heightPtr) if err != nil { return nil, err } @@ -155,22 +155,3 @@ func BlockResults(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultBlockR ConsensusParamUpdates: results.EndBlock.ConsensusParamUpdates, }, nil } - -func getHeight(currentBase int64, currentHeight int64, heightPtr *int64) (int64, error) { - if heightPtr != nil { - height := *heightPtr - if height <= 0 { - return 0, fmt.Errorf("height must be greater than 0, but got %d", height) - } - if height > currentHeight { - return 0, fmt.Errorf("height %d must be less than or equal to the current blockchain height %d", - height, currentHeight) - } - if height < currentBase { - return 0, fmt.Errorf("height %v is not available, blocks pruned at height %v", - height, currentBase) - } - return height, nil - } - return currentHeight, nil -} diff --git a/rpc/core/consensus.go b/rpc/core/consensus.go index ef9667927..9f5a403bb 100644 --- a/rpc/core/consensus.go +++ b/rpc/core/consensus.go @@ -11,16 +11,14 @@ import ( // Validators gets the validator set at the given block height. // -// If no height is provided, it will fetch the current validator set. Note the -// validators are sorted by their address - this is the canonical order for the -// validators in the set as used in computing their Merkle root. +// If no height is provided, it will fetch the latest validator set. Note the +// validators are sorted by their voting power - this is the canonical order +// for the validators in the set as used in computing their Merkle root. // // More: https://docs.tendermint.com/master/rpc/#/Info/validators func Validators(ctx *rpctypes.Context, heightPtr *int64, page, perPage int) (*ctypes.ResultValidators, error) { - // The latest validator that we know is the - // NextValidator of the last block. - height := env.ConsensusState.GetState().LastBlockHeight + 1 - height, err := getHeight(env.BlockStore.Base(), height, heightPtr) + // The latest validator that we know is the NextValidator of the last block. + height, err := getHeight(latestUncommittedHeight(), heightPtr) if err != nil { return nil, err } @@ -90,21 +88,22 @@ func ConsensusState(ctx *rpctypes.Context) (*ctypes.ResultConsensusState, error) return &ctypes.ResultConsensusState{RoundState: bz}, err } -// ConsensusParams gets the consensus parameters at the given block height. -// If no height is provided, it will fetch the current consensus params. +// ConsensusParams gets the consensus parameters at the given block height. +// If no height is provided, it will fetch the latest consensus params. // More: https://docs.tendermint.com/master/rpc/#/Info/consensus_params func ConsensusParams(ctx *rpctypes.Context, heightPtr *int64) (*ctypes.ResultConsensusParams, error) { - height := env.ConsensusState.GetState().LastBlockHeight + 1 - height, err := getHeight(env.BlockStore.Base(), height, heightPtr) + // The latest consensus params that we know is the consensus params after the + // last block. + height, err := getHeight(latestUncommittedHeight(), heightPtr) if err != nil { return nil, err } - consensusparams, err := sm.LoadConsensusParams(env.StateDB, height) + consensusParams, err := sm.LoadConsensusParams(env.StateDB, height) if err != nil { return nil, err } return &ctypes.ResultConsensusParams{ BlockHeight: height, - ConsensusParams: consensusparams}, nil + ConsensusParams: consensusParams}, nil } diff --git a/rpc/core/env.go b/rpc/core/env.go index ab6114b67..bc7289561 100644 --- a/rpc/core/env.go +++ b/rpc/core/env.go @@ -129,3 +129,32 @@ func validateSkipCount(page, perPage int) int { return skipCount } + +// latestHeight can be either latest committed or uncommitted (+1) height. +func getHeight(latestHeight int64, heightPtr *int64) (int64, error) { + if heightPtr != nil { + height := *heightPtr + if height <= 0 { + return 0, fmt.Errorf("height must be greater than 0, but got %d", height) + } + if height > latestHeight { + return 0, fmt.Errorf("height %d must be less than or equal to the current blockchain height %d", + height, latestHeight) + } + base := env.BlockStore.Base() + if height < base { + return 0, fmt.Errorf("height %v is not available, blocks pruned at height %v", + height, base) + } + return height, nil + } + return latestHeight, nil +} + +func latestUncommittedHeight() int64 { + nodeIsSyncing := env.ConsensusReactor.WaitSync() + if nodeIsSyncing { + return env.BlockStore.Height() + } + return env.BlockStore.Height() + 1 +} diff --git a/rpc/core/status.go b/rpc/core/status.go index c0db6bf47..e3c3b345f 100644 --- a/rpc/core/status.go +++ b/rpc/core/status.go @@ -1,7 +1,6 @@ package core import ( - "bytes" "time" tmbytes "github.com/tendermint/tendermint/libs/bytes" @@ -17,41 +16,40 @@ import ( // More: https://docs.tendermint.com/master/rpc/#/Info/status func Status(ctx *rpctypes.Context) (*ctypes.ResultStatus, error) { var ( - earliestBlockMeta *types.BlockMeta earliestBlockHash tmbytes.HexBytes earliestAppHash tmbytes.HexBytes earliestBlockTimeNano int64 + + earliestBlockHeight = env.BlockStore.Base() ) - earliestBlockHeight := env.BlockStore.Base() - earliestBlockMeta = env.BlockStore.LoadBlockMeta(earliestBlockHeight) - if earliestBlockMeta != nil { + + if earliestBlockMeta := env.BlockStore.LoadBlockMeta(earliestBlockHeight); earliestBlockMeta != nil { earliestAppHash = earliestBlockMeta.Header.AppHash earliestBlockHash = earliestBlockMeta.BlockID.Hash earliestBlockTimeNano = earliestBlockMeta.Header.Time.UnixNano() } - var latestHeight int64 - if env.ConsensusReactor.WaitSync() { - latestHeight = env.BlockStore.Height() - } else { - latestHeight = env.ConsensusState.GetLastHeight() - } - var ( - latestBlockMeta *types.BlockMeta latestBlockHash tmbytes.HexBytes latestAppHash tmbytes.HexBytes latestBlockTimeNano int64 + + latestHeight = env.BlockStore.Height() ) + if latestHeight != 0 { - latestBlockMeta = env.BlockStore.LoadBlockMeta(latestHeight) - latestBlockHash = latestBlockMeta.BlockID.Hash - latestAppHash = latestBlockMeta.Header.AppHash - latestBlockTimeNano = latestBlockMeta.Header.Time.UnixNano() + latestBlockMeta := env.BlockStore.LoadBlockMeta(latestHeight) + if latestBlockMeta != nil { + latestBlockHash = latestBlockMeta.BlockID.Hash + latestAppHash = latestBlockMeta.Header.AppHash + latestBlockTimeNano = latestBlockMeta.Header.Time.UnixNano() + } } + // Return the very last voting power, not the voting power of this validator + // during the last block. var votingPower int64 - if val := validatorAtHeight(latestHeight); val != nil { + if val := validatorAtHeight(latestUncommittedHeight()); val != nil { votingPower = val.VotingPower } @@ -79,27 +77,11 @@ func Status(ctx *rpctypes.Context) (*ctypes.ResultStatus, error) { } func validatorAtHeight(h int64) *types.Validator { - privValAddress := env.PubKey.Address() - - // If we're still at height h, search in the current validator set. - lastBlockHeight, vals := env.ConsensusState.GetValidators() - if lastBlockHeight == h { - for _, val := range vals { - if bytes.Equal(val.Address, privValAddress) { - return val - } - } - } - - // If we've moved to the next height, retrieve the validator set from DB. - if lastBlockHeight > h { - vals, err := sm.LoadValidators(env.StateDB, h) - if err != nil { - return nil // should not happen - } - _, val := vals.GetByAddress(privValAddress) - return val + vals, err := sm.LoadValidators(env.StateDB, h) + if err != nil { + return nil } - - return nil + privValAddress := env.PubKey.Address() + _, val := vals.GetByAddress(privValAddress) + return val } diff --git a/rpc/swagger/swagger.yaml b/rpc/swagger/swagger.yaml index 28f25240d..909fbed5c 100644 --- a/rpc/swagger/swagger.yaml +++ b/rpc/swagger/swagger.yaml @@ -712,6 +712,8 @@ paths: - Info description: | Get consensus state. + + Not safe to call from inside the ABCI application during a block execution. responses: 200: description: consensus state results. @@ -733,6 +735,8 @@ paths: - Info description: | Get consensus state. + + Not safe to call from inside the ABCI application during a block execution. responses: 200: description: consensus state results.