From da88c89b419d8d81651c74e5053569b187fd3566 Mon Sep 17 00:00:00 2001 From: "riccardo.montagnin" Date: Tue, 9 Feb 2021 09:51:47 +0100 Subject: [PATCH 1/5] Removed GetValidator caching to fix concurrency error --- x/staking/keeper/validator.go | 14 ----------- x/staking/keeper/validator_test.go | 37 ++++++++++++++++++++++++++---- x/staking/teststaking/validator.go | 2 +- 3 files changed, 33 insertions(+), 20 deletions(-) diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go index 9fa6a0ca0a0..e7f17a59ed1 100644 --- a/x/staking/keeper/validator.go +++ b/x/staking/keeper/validator.go @@ -35,21 +35,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 { diff --git a/x/staking/keeper/validator_test.go b/x/staking/keeper/validator_test.go index ae98c177ef3..3cad05e0332 100644 --- a/x/staking/keeper/validator_test.go +++ b/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,18 +43,45 @@ 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) } return app, ctx, addrs, valAddrs, vs } +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) + } + } +} + func TestSetValidator(t *testing.T) { app, ctx, _, _ := bootstrapValidatorTest(t, 10, 100) diff --git a/x/staking/teststaking/validator.go b/x/staking/teststaking/validator.go index 901395d76e9..71459581f0e 100644 --- a/x/staking/teststaking/validator.go +++ b/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 From cd1035a30b3d441982ab6b8b0d6f1124bd95ec8c Mon Sep 17 00:00:00 2001 From: "riccardo.montagnin" Date: Tue, 9 Feb 2021 10:05:17 +0100 Subject: [PATCH 2/5] Fixed linting and added CHANGELOG entry --- CHANGELOG.md | 1 + x/staking/keeper/keeper.go | 4 ---- x/staking/keeper/validator.go | 25 ------------------------- 3 files changed, 1 insertion(+), 29 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e745b57961e..b0b8d14e915 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,6 +56,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/bank) [\#8479](https://github.com/cosmos/cosmos-sdk/pull/8479) Adittional client denom metadata validation for `base` and `display` denoms. * (x/ibc) [\#8404](https://github.com/cosmos/cosmos-sdk/pull/8404) Reorder IBC `ChanOpenAck` and `ChanOpenConfirm` handler execution to perform core handler first, followed by application callbacks. * [\#8396](https://github.com/cosmos/cosmos-sdk/pull/8396) Add support for ARM platform +* (x/staking) [\#8546](https://github.com/cosmos/cosmos-sdk/pull/8546) Allow GetValidator to be called concurrently ### Bug Fixes diff --git a/x/staking/keeper/keeper.go b/x/staking/keeper/keeper.go index 74d85a645bf..81613d92bca 100644 --- a/x/staking/keeper/keeper.go +++ b/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(), } } diff --git a/x/staking/keeper/validator.go b/x/staking/keeper/validator.go index e7f17a59ed1..0253c197aed 100644 --- a/x/staking/keeper/validator.go +++ b/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) @@ -36,15 +20,6 @@ func (k Keeper) GetValidator(ctx sdk.Context, addr sdk.ValAddress) (validator ty } validator = types.MustUnmarshalValidator(k.cdc, value) - - // 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 } From 71230614e91820af5791e6f7726c9d50871e6842 Mon Sep 17 00:00:00 2001 From: "riccardo.montagnin" Date: Tue, 9 Feb 2021 11:03:06 +0100 Subject: [PATCH 3/5] Moved benchmark test into its own file --- x/staking/keeper/validator_bench_test.go | 29 ++++++++++++++++++++++++ x/staking/keeper/validator_test.go | 26 --------------------- 2 files changed, 29 insertions(+), 26 deletions(-) create mode 100644 x/staking/keeper/validator_bench_test.go diff --git a/x/staking/keeper/validator_bench_test.go b/x/staking/keeper/validator_bench_test.go new file mode 100644 index 00000000000..54a616c90e5 --- /dev/null +++ b/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) + } + } +} diff --git a/x/staking/keeper/validator_test.go b/x/staking/keeper/validator_test.go index 3cad05e0332..86964407050 100644 --- a/x/staking/keeper/validator_test.go +++ b/x/staking/keeper/validator_test.go @@ -56,32 +56,6 @@ func initValidators(t testing.TB, power int64, numAddrs int, powers []int64) (*s return app, ctx, addrs, valAddrs, vs } -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) - } - } -} - func TestSetValidator(t *testing.T) { app, ctx, _, _ := bootstrapValidatorTest(t, 10, 100) From fe12d43ba361e81d4e6d19e92b73a768d84f7857 Mon Sep 17 00:00:00 2001 From: "riccardo.montagnin" Date: Wed, 10 Feb 2021 08:37:42 +0100 Subject: [PATCH 4/5] Moved CHANGELOG entry to bug fix --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b0b8d14e915..d6a22f02c8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -56,7 +56,6 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (x/bank) [\#8479](https://github.com/cosmos/cosmos-sdk/pull/8479) Adittional client denom metadata validation for `base` and `display` denoms. * (x/ibc) [\#8404](https://github.com/cosmos/cosmos-sdk/pull/8404) Reorder IBC `ChanOpenAck` and `ChanOpenConfirm` handler execution to perform core handler first, followed by application callbacks. * [\#8396](https://github.com/cosmos/cosmos-sdk/pull/8396) Add support for ARM platform -* (x/staking) [\#8546](https://github.com/cosmos/cosmos-sdk/pull/8546) Allow GetValidator to be called concurrently ### Bug Fixes @@ -68,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) Allow GetValidator to be called concurrently ## [v0.40.1](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.40.1) - 2021-01-19 From 8d63e781c8959be4c04b1bd36cf4ac8c71281a41 Mon Sep 17 00:00:00 2001 From: Amaury Date: Fri, 12 Feb 2021 12:40:34 +0100 Subject: [PATCH 5/5] Update CHANGELOG.md Co-authored-by: Cory --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d6a22f02c8b..190f39cac6b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -67,7 +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) Allow GetValidator to be called concurrently +* (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