Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed GetValidator caching to fix concurrency error #8546

Merged
merged 6 commits into from
Feb 12, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
4 changes: 0 additions & 4 deletions x/staking/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import (
"github.com/cosmos/cosmos-sdk/x/staking/types"
)

const aminoCacheSize = 500

// Implements ValidatorSet interface
var _ types.ValidatorSet = Keeper{}

Expand All @@ -28,7 +26,6 @@ type Keeper struct {
bankKeeper types.BankKeeper
hooks types.StakingHooks
paramstore paramtypes.Subspace
validatorCache map[string]cachedValidator
validatorCacheList *list.List
}

Expand Down Expand Up @@ -58,7 +55,6 @@ func NewKeeper(
bankKeeper: bk,
paramstore: ps,
hooks: nil,
validatorCache: make(map[string]cachedValidator, aminoCacheSize),
validatorCacheList: list.New(),
}
}
Expand Down
39 changes: 0 additions & 39 deletions x/staking/keeper/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
}

Expand Down
29 changes: 29 additions & 0 deletions x/staking/keeper/validator_bench_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
11 changes: 6 additions & 5 deletions x/staking/keeper/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one here, maybe we should do that in all the helpers by default.

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)
Expand All @@ -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)
}
Expand Down
2 changes: 1 addition & 1 deletion x/staking/teststaking/validator.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down