Removed GetValidator caching to fix concurrency error (#8546)

* Removed GetValidator caching to fix concurrency error

* Fixed linting and added CHANGELOG entry

* Moved benchmark test into its own file

* Moved CHANGELOG entry to bug fix

* Update CHANGELOG.md

Co-authored-by: Cory <cjlevinson@gmail.com>

Co-authored-by: Amaury <amaury.martiny@protonmail.com>
Co-authored-by: Cory <cjlevinson@gmail.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

CHANGELOG.md

@@ -67,6 +67,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (client/keys) [\#8436](https://github.com/cosmos/cosmos-sdk/pull/8436) Fix key migration issue
 * (server) [\#8481](https://github.com/cosmos/cosmos-sdk/pull/8481) Don't create
   files when running `{appd} tendermint show-*` subcommands
+* (x/staking) [\#8546](https://github.com/cosmos/cosmos-sdk/pull/8546) Fix caching bug where concurrent calls to GetValidator could cause a node to crash
 
 ## [v0.40.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.1) - 2021-01-19
 

x/staking/keeper/keeper.go

@@ -12,8 +12,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
-const aminoCacheSize = 500
-
 // Implements ValidatorSet interface
 var _ types.ValidatorSet = Keeper{}
 
@@ -28,7 +26,6 @@ type Keeper struct {
 	bankKeeper         types.BankKeeper
 	hooks              types.StakingHooks
 	paramstore         paramtypes.Subspace
-	validatorCache     map[string]cachedValidator
 	validatorCacheList *list.List
 }
 
@@ -58,7 +55,6 @@ func NewKeeper(
 		bankKeeper:         bk,
 		paramstore:         ps,
 		hooks:              nil,
-		validatorCache:     make(map[string]cachedValidator, aminoCacheSize),
 		validatorCacheList: list.New(),
 	}
 }

x/staking/keeper/validator.go

@@ -10,22 +10,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
-// Cache the amino decoding of validators, as it can be the case that repeated slashing calls
-// cause many calls to GetValidator, which were shown to throttle the state machine in our
-// simulation. Note this is quite biased though, as the simulator does more slashes than a
-// live chain should, however we require the slashing to be fast as noone pays gas for it.
-type cachedValidator struct {
-	val        types.Validator
-	marshalled string // marshalled amino bytes for the validator object (not operator address)
-}
-
-func newCachedValidator(val types.Validator, marshalled string) cachedValidator {
-	return cachedValidator{
-		val:        val,
-		marshalled: marshalled,
-	}
-}
-
 // get a single validator
 func (k Keeper) GetValidator(ctx sdk.Context, addr sdk.ValAddress) (validator types.Validator, found bool) {
 	store := ctx.KVStore(k.storeKey)
@@ -35,30 +19,7 @@ func (k Keeper) GetValidator(ctx sdk.Context, addr sdk.ValAddress) (validator ty
 		return validator, false
 	}
 
-	// If these amino encoded bytes are in the cache, return the cached validator
-	strValue := string(value)
-	if val, ok := k.validatorCache[strValue]; ok {
-		valToReturn := val.val
-		// Doesn't mutate the cache's value
-		valToReturn.OperatorAddress = addr.String()
-
-		return valToReturn, true
-	}
-
-	// amino bytes weren't found in cache, so amino unmarshal and add it to the cache
-	validator = types.MustUnmarshalValidator(k.cdc, value)
-	cachedVal := newCachedValidator(validator, strValue)
-	k.validatorCache[strValue] = newCachedValidator(validator, strValue)
-	k.validatorCacheList.PushBack(cachedVal)
-
-	// if the cache is too big, pop off the last element from it
-	if k.validatorCacheList.Len() > aminoCacheSize {
-		valToRemove := k.validatorCacheList.Remove(k.validatorCacheList.Front()).(cachedValidator)
-		delete(k.validatorCache, valToRemove.marshalled)
-	}
-
 	validator = types.MustUnmarshalValidator(k.cdc, value)
-
 	return validator, true
 }
 

x/staking/keeper/validator_bench_test.go

@@ -0,0 +1,29 @@
+package keeper_test
+
+import "testing"
+
+func BenchmarkGetValidator(b *testing.B) {
+	// 900 is the max number we are allowed to use in order to avoid simapp.CreateTestPubKeys
+	// panic: encoding/hex: odd length hex string
+	var powersNumber = 900
+
+	var totalPower int64 = 0
+	var powers = make([]int64, powersNumber)
+	for i := range powers {
+		powers[i] = int64(i)
+		totalPower += int64(i)
+	}
+
+	app, ctx, _, valAddrs, vals := initValidators(b, totalPower, len(powers), powers)
+
+	for _, validator := range vals {
+		app.StakingKeeper.SetValidator(ctx, validator)
+	}
+
+	b.ResetTimer()
+	for n := 0; n < b.N; n++ {
+		for _, addr := range valAddrs {
+			_, _ = app.StakingKeeper.GetValidator(ctx, addr)
+		}
+	}
+}

x/staking/keeper/validator_test.go

@@ -19,13 +19,13 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/staking/types"
 )
 
-func newMonikerValidator(t *testing.T, operator sdk.ValAddress, pubKey cryptotypes.PubKey, moniker string) types.Validator {
+func newMonikerValidator(t testing.TB, operator sdk.ValAddress, pubKey cryptotypes.PubKey, moniker string) types.Validator {
 	v, err := types.NewValidator(operator, pubKey, types.Description{Moniker: moniker})
 	require.NoError(t, err)
 	return v
 }
 
-func bootstrapValidatorTest(t *testing.T, power int64, numAddrs int) (*simapp.SimApp, sdk.Context, []sdk.AccAddress, []sdk.ValAddress) {
+func bootstrapValidatorTest(t testing.TB, power int64, numAddrs int) (*simapp.SimApp, sdk.Context, []sdk.AccAddress, []sdk.ValAddress) {
 	_, app, ctx := createTestInput()
 
 	addrDels, addrVals := generateAddresses(app, ctx, numAddrs)
@@ -43,12 +43,13 @@ func bootstrapValidatorTest(t *testing.T, power int64, numAddrs int) (*simapp.Si
 	return app, ctx, addrDels, addrVals
 }
 
-func initValidators(t *testing.T, power int64, numAddrs int, powers []int64) (*simapp.SimApp, sdk.Context, []sdk.AccAddress, []sdk.ValAddress, []types.Validator) {
-	app, ctx, addrs, valAddrs := bootstrapValidatorTest(t, 1000, 20)
+func initValidators(t testing.TB, power int64, numAddrs int, powers []int64) (*simapp.SimApp, sdk.Context, []sdk.AccAddress, []sdk.ValAddress, []types.Validator) {
+	app, ctx, addrs, valAddrs := bootstrapValidatorTest(t, power, numAddrs)
+	pks := simapp.CreateTestPubKeys(numAddrs)
 
 	vs := make([]types.Validator, len(powers))
 	for i, power := range powers {
-		vs[i] = teststaking.NewValidator(t, sdk.ValAddress(addrs[i]), PKs[i])
+		vs[i] = teststaking.NewValidator(t, sdk.ValAddress(addrs[i]), pks[i])
 		tokens := sdk.TokensFromConsensusPower(power)
 		vs[i], _ = vs[i].AddTokensFromDel(tokens)
 	}

x/staking/teststaking/validator.go

@@ -11,7 +11,7 @@ import (
 )
 
 // NewValidator is a testing helper method to create validators in tests
-func NewValidator(t *testing.T, operator sdk.ValAddress, pubKey cryptotypes.PubKey) types.Validator {
+func NewValidator(t testing.TB, operator sdk.ValAddress, pubKey cryptotypes.PubKey) types.Validator {
 	v, err := types.NewValidator(operator, pubKey, types.Description{})
 	require.NoError(t, err)
 	return v