From 3458f64f0aeb68796e56dd334c04b5e066ebf089 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 8 Oct 2021 22:02:16 -0400 Subject: [PATCH] fix!: x/staking - remove delegation with amount is zero (#10254) --- CHANGELOG.md | 1 + x/staking/genesis.go | 13 +++++-- x/staking/keeper/delegation.go | 8 +++-- x/staking/keeper/invariants.go | 2 -- x/staking/keeper/migrations.go | 10 +++++- x/staking/migrations/v040/types.go | 4 +++ x/staking/migrations/v045/keys.go | 11 ++++++ x/staking/migrations/v045/store.go | 57 ++++++++++++++++++++++++++++++ x/staking/module.go | 7 +++- 9 files changed, 104 insertions(+), 9 deletions(-) create mode 100644 x/staking/migrations/v045/keys.go create mode 100644 x/staking/migrations/v045/store.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e6668767192..faf09150246d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -150,6 +150,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### State Machine Breaking +* (x/staking) [#10254](https://github.com/cosmos/cosmos-sdk/pull/10254) Instead of using the shares to determine if a delegation should be removed, use the truncated (token) amount. * (store) [#10247](https://github.com/cosmos/cosmos-sdk/pull/10247) Charge gas for the key length in gas meter. * (store) [#10218](https://github.com/cosmos/cosmos-sdk/pull/10218) Charge gas even when there are no entries while seeking. * (x/auth)[\#9596](https://github.com/cosmos/cosmos-sdk/pull/9596) Enable creating periodic vesting accounts with a transactions instead of requiring them to be created in genesis. diff --git a/x/staking/genesis.go b/x/staking/genesis.go index 55daae2ce9f8..c019359c3526 100644 --- a/x/staking/genesis.go +++ b/x/staking/genesis.go @@ -77,6 +77,7 @@ func InitGenesis( } keeper.SetDelegation(ctx, delegation) + // Call the after-modification hook if not exported if !data.Exported { if err := keeper.AfterDelegationModified(ctx, delegatorAddress, delegation.GetValidatorAddr()); err != nil { @@ -110,15 +111,18 @@ func InitGenesis( if bondedPool == nil { panic(fmt.Sprintf("%s module account has not been set", types.BondedPoolName)) } - // TODO remove with genesis 2-phases refactor https://github.com/cosmos/cosmos-sdk/issues/2862 + + // TODO: remove with genesis 2-phases refactor https://github.com/cosmos/cosmos-sdk/issues/2862 bondedBalance := bankKeeper.GetAllBalances(ctx, bondedPool.GetAddress()) if bondedBalance.IsZero() { accountKeeper.SetModuleAccount(ctx, bondedPool) } + // if balance is different from bonded coins panic because genesis is most likely malformed if !bondedBalance.IsEqual(bondedCoins) { panic(fmt.Sprintf("bonded pool balance is different from bonded coins: %s <-> %s", bondedBalance, bondedCoins)) } + notBondedPool := keeper.GetNotBondedPool(ctx) if notBondedPool == nil { panic(fmt.Sprintf("%s module account has not been set", types.NotBondedPoolName)) @@ -128,10 +132,13 @@ func InitGenesis( if notBondedBalance.IsZero() { accountKeeper.SetModuleAccount(ctx, notBondedPool) } - // if balance is different from non bonded coins panic because genesis is most likely malformed + + // If balance is different from non bonded coins panic because genesis is most + // likely malformed. if !notBondedBalance.IsEqual(notBondedCoins) { panic(fmt.Sprintf("not bonded pool balance is different from not bonded coins: %s <-> %s", notBondedBalance, notBondedCoins)) } + // don't need to run Tendermint updates if we exported if data.Exported { for _, lv := range data.LastValidatorPowers { @@ -139,6 +146,7 @@ func InitGenesis( if err != nil { panic(err) } + keeper.SetLastValidatorPower(ctx, valAddr, lv.Power) validator, found := keeper.GetValidator(ctx, valAddr) @@ -152,6 +160,7 @@ func InitGenesis( } } else { var err error + res, err = keeper.ApplyAndReturnValidatorSetUpdates(ctx) if err != nil { panic(err) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index ca4ab27658fe..7c942aa30216 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -680,8 +680,11 @@ func (k Keeper) Unbond( validator = k.mustGetValidator(ctx, validator.GetOperator()) } - // remove the delegation - if delegation.Shares.IsZero() { + // Remove the delegation if the resulting shares yield a truncated zero amount + // of tokens in the delegation. Note, in this this case, the shares themselves + // may not be zero, but rather a small fractional amount. Otherwise, we update + // the delegation object. + if validator.TokensFromShares(delegation.Shares).TruncateInt().IsZero() || delegation.Shares.IsZero() { err = k.RemoveDelegation(ctx, delegation) } else { k.SetDelegation(ctx, delegation) @@ -696,7 +699,6 @@ func (k Keeper) Unbond( // remove the shares and coins from the validator // NOTE that the amount is later (in keeper.Delegation) moved between staking module pools validator, amount = k.RemoveValidatorTokensAndShares(ctx, validator, shares) - if validator.DelegatorShares.IsZero() && validator.IsUnbonded() { // if not unbonded, we must instead remove validator in EndBlocker once it finishes its unbonding period k.RemoveValidator(ctx, validator.GetOperator()) diff --git a/x/staking/keeper/invariants.go b/x/staking/keeper/invariants.go index 3516316f6c8b..ed88ec62518e 100644 --- a/x/staking/keeper/invariants.go +++ b/x/staking/keeper/invariants.go @@ -137,13 +137,11 @@ func PositiveDelegationInvariant(k Keeper) sdk.Invariant { for _, delegation := range delegations { if delegation.Shares.IsNegative() { count++ - msg += fmt.Sprintf("\tdelegation with negative shares: %+v\n", delegation) } if delegation.Shares.IsZero() { count++ - msg += fmt.Sprintf("\tdelegation with zero shares: %+v\n", delegation) } } diff --git a/x/staking/keeper/migrations.go b/x/staking/keeper/migrations.go index b694e8b8ec7a..ed90ea9ce61a 100644 --- a/x/staking/keeper/migrations.go +++ b/x/staking/keeper/migrations.go @@ -3,6 +3,7 @@ package keeper import ( sdk "github.com/cosmos/cosmos-sdk/types" v043 "github.com/cosmos/cosmos-sdk/x/staking/migrations/v043" + v045 "github.com/cosmos/cosmos-sdk/x/staking/migrations/v045" ) // Migrator is a struct for handling in-place store migrations. @@ -12,10 +13,17 @@ type Migrator struct { // NewMigrator returns a new Migrator. func NewMigrator(keeper Keeper) Migrator { - return Migrator{keeper: keeper} + return Migrator{ + keeper: keeper, + } } // Migrate1to2 migrates from version 1 to 2. func (m Migrator) Migrate1to2(ctx sdk.Context) error { return v043.MigrateStore(ctx, m.keeper.storeKey) } + +// Migrate2to3 migrates x/staking state from consensus version 2 to 3. +func (m Migrator) Migrate2to3(ctx sdk.Context) error { + return v045.MigrateStore(ctx, m.keeper.storeKey, m.keeper.cdc) +} diff --git a/x/staking/migrations/v040/types.go b/x/staking/migrations/v040/types.go index 03a6f1c934b6..ba3be48cb6e6 100644 --- a/x/staking/migrations/v040/types.go +++ b/x/staking/migrations/v040/types.go @@ -183,6 +183,10 @@ func (v Validator) String() string { return string(out) } +func (v Validator) TokensFromShares(shares sdk.Dec) sdk.Dec { + return (shares.MulInt(v.Tokens)).Quo(v.DelegatorShares) +} + // Validators is a collection of Validator type Validators []Validator diff --git a/x/staking/migrations/v045/keys.go b/x/staking/migrations/v045/keys.go new file mode 100644 index 000000000000..75a51df3cea7 --- /dev/null +++ b/x/staking/migrations/v045/keys.go @@ -0,0 +1,11 @@ +package v045 + +import ( + sdk "github.com/cosmos/cosmos-sdk/types" + "github.com/cosmos/cosmos-sdk/types/address" + v040staking "github.com/cosmos/cosmos-sdk/x/staking/migrations/v040" +) + +func getValidatorKey(operatorAddr sdk.ValAddress) []byte { + return append(v040staking.ValidatorsKey, address.MustLengthPrefix(operatorAddr)...) +} diff --git a/x/staking/migrations/v045/store.go b/x/staking/migrations/v045/store.go new file mode 100644 index 000000000000..4a13a3488a25 --- /dev/null +++ b/x/staking/migrations/v045/store.go @@ -0,0 +1,57 @@ +package v045 + +import ( + "github.com/cosmos/cosmos-sdk/codec" + "github.com/cosmos/cosmos-sdk/store/prefix" + storetypes "github.com/cosmos/cosmos-sdk/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" + v040staking "github.com/cosmos/cosmos-sdk/x/staking/migrations/v040" +) + +// MigrateStore performs in-place store migrations from v0.43/v0.44 to v0.45. +// The migration includes: +// +// - Removing delegations that have a zero share or token amount. +func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec) error { + store := ctx.KVStore(storeKey) + + return purgeDelegations(store, cdc) +} + +func purgeDelegations(store sdk.KVStore, cdc codec.BinaryCodec) error { + prefixDelStore := prefix.NewStore(store, v040staking.DelegationKey) + + delStoreIter := prefixDelStore.Iterator(nil, nil) + defer delStoreIter.Close() + + valCache := make(map[string]v040staking.Validator) + + for ; delStoreIter.Valid(); delStoreIter.Next() { + var delegation v040staking.Delegation + if err := cdc.Unmarshal(delStoreIter.Value(), &delegation); err != nil { + return err + } + + validator, ok := valCache[delegation.ValidatorAddress] + if !ok { + valAddr, err := sdk.ValAddressFromBech32(delegation.ValidatorAddress) + if err != nil { + return err + } + + if err := cdc.Unmarshal(store.Get(getValidatorKey(valAddr)), &validator); err != nil { + return err + } + + valCache[delegation.ValidatorAddress] = validator + } + + // TODO: On-chain, we call BeforeDelegationRemoved prior to removing the + // object from state. Do we need to do the same here? + if validator.TokensFromShares(delegation.Shares).TruncateInt().IsZero() || delegation.Shares.IsZero() { + prefixDelStore.Delete(delStoreIter.Key()) + } + } + + return nil +} diff --git a/x/staking/module.go b/x/staking/module.go index aa395b80ca17..6dfd0c67f8a9 100644 --- a/x/staking/module.go +++ b/x/staking/module.go @@ -24,6 +24,10 @@ import ( "github.com/cosmos/cosmos-sdk/x/staking/types" ) +const ( + consensusVersion uint64 = 3 +) + var ( _ module.AppModule = AppModule{} _ module.AppModuleBasic = AppModuleBasic{} @@ -140,6 +144,7 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { m := keeper.NewMigrator(am.keeper) cfg.RegisterMigration(types.ModuleName, 1, m.Migrate1to2) + cfg.RegisterMigration(types.ModuleName, 2, m.Migrate2to3) } // InitGenesis performs genesis initialization for the staking module. It returns @@ -160,7 +165,7 @@ func (am AppModule) ExportGenesis(ctx sdk.Context, cdc codec.JSONCodec) json.Raw } // ConsensusVersion implements AppModule/ConsensusVersion. -func (AppModule) ConsensusVersion() uint64 { return 2 } +func (AppModule) ConsensusVersion() uint64 { return consensusVersion } // BeginBlock returns the begin blocker for the staking module. func (am AppModule) BeginBlock(ctx sdk.Context, _ abci.RequestBeginBlock) {