rpc/core: do not lock ConsensusState mutex

in /validators, /consensus_params and /status

Closes #3161

CHANGELOG_PENDING.md

@@ -54,6 +54,7 @@ Friendly reminder, we have a [bug bounty program](https://hackerone.com/tendermi
   - cleveldb: use cleveldb as db backend instead of goleveldb.
   - race: pass -race to go build and enable data race detection.
 - [mempool] [\#4759](https://github.com/tendermint/tendermint/pull/4759) Allow ReapX and CheckTx functions to run in parallel (@melekes)
+- [rpc/core] [\#4844](https://github.com/tendermint/tendermint/pull/4844) Do not lock consensus state in `/validators`, `/consensus_params` and `/status` (@melekes)
 
 ### BUG FIXES:
 

lite2/provider/http/http.go

@@ -13,7 +13,7 @@ import (
 )
 
 // This is very brittle, see: https://github.com/tendermint/tendermint/issues/4740
-var regexpMissingHeight = regexp.MustCompile(`height (must be less than or equal to|\d+ is not available)`)
+var regexpMissingHeight = regexp.MustCompile(`height \d+ (must be less than or equal to|is not available)`)
 
 // SignStatusClient combines a SignClient and StatusClient.
 type SignStatusClient interface {

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,21 +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")
-		}
-		if height > currentHeight {
-			return 0, fmt.Errorf("height must be less than or equal to the current blockchain height")
-		}
-		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
-}

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
 }

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.FastSync()
+	if nodeIsSyncing {
+		return env.BlockStore.Height()
+	}
+	return env.BlockStore.Height() + 1
+}

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.FastSync() {
-		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
 }

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.