From 36d3853ed25699c23e5bac0a5a4f0fec58026d66 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Thu, 23 Apr 2020 10:13:17 -0400 Subject: [PATCH 1/8] Use correct API and add TODO --- x/staking/keeper/delegation.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 4a20f74c9026..04d72642203e 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -577,12 +577,13 @@ func (k Keeper) Unbond( isValidatorOperator := delegation.DelegatorAddress.Equals(validator.OperatorAddress) - // if the delegation is the operator of the validator and undelegating will decrease the validator's self delegation below their minimum - // trigger a jail validator + // If the delegation is the operator of the validator and undelegating will decrease the validator's + // self-delegation below their minimum, we jail the validator. if isValidatorOperator && !validator.Jailed && validator.TokensFromShares(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { - k.jailValidator(ctx, validator) + // TODO: Create signing info with jailing info set. + k.Jail(ctx, validator.GetConsAddr()) validator = k.mustGetValidator(ctx, validator.OperatorAddress) } From 8061212378192561f4982a83a0e55b792eb25f80 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 Apr 2020 10:13:00 -0400 Subject: [PATCH 2/8] Allow unjail without signing info --- x/slashing/keeper/unjail.go | 30 ++++++++++++++++++------------ x/staking/keeper/delegation.go | 1 - 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/x/slashing/keeper/unjail.go b/x/slashing/keeper/unjail.go index cf57c1e71c4a..95918d7dd84c 100644 --- a/x/slashing/keeper/unjail.go +++ b/x/slashing/keeper/unjail.go @@ -30,19 +30,25 @@ func (k Keeper) Unjail(ctx sdk.Context, validatorAddr sdk.ValAddress) error { consAddr := sdk.ConsAddress(validator.GetConsPubKey().Address()) + // If the validator has a ValidatorSigningInfo object that signals that the + // validator was bonded and so we must check that the validator is not tombstoned + // and can be unjailed at the current block. + // + // A validator that is jailed but has no ValidatorSigningInfo object signals + // that the validator was never bonded and must've been jailed due to falling + // below their minimum self-delegation. The validator can unjail at point + // assuming they've now bonded above their minimum self-delegation. info, found := k.GetValidatorSigningInfo(ctx, consAddr) - if !found { - return types.ErrNoValidatorForAddress - } - - // cannot be unjailed if tombstoned - if info.Tombstoned { - return types.ErrValidatorJailed - } - - // cannot be unjailed until out of jail - if ctx.BlockHeader().Time.Before(info.JailedUntil) { - return types.ErrValidatorJailed + if found { + // cannot be unjailed if tombstoned + if info.Tombstoned { + return types.ErrValidatorJailed + } + + // cannot be unjailed until out of jail + if ctx.BlockHeader().Time.Before(info.JailedUntil) { + return types.ErrValidatorJailed + } } k.sk.Unjail(ctx, consAddr) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index 04d72642203e..d3b768350c93 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -582,7 +582,6 @@ func (k Keeper) Unbond( if isValidatorOperator && !validator.Jailed && validator.TokensFromShares(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { - // TODO: Create signing info with jailing info set. k.Jail(ctx, validator.GetConsAddr()) validator = k.mustGetValidator(ctx, validator.OperatorAddress) } From e4de915e7bb03b579d731824720a22c6c1923ece Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 Apr 2020 10:47:03 -0400 Subject: [PATCH 3/8] Add unit test --- x/slashing/keeper/keeper_test.go | 82 +++++++++++++++++++++++++++++++- x/slashing/keeper/unjail.go | 9 +++- 2 files changed, 87 insertions(+), 4 deletions(-) diff --git a/x/slashing/keeper/keeper_test.go b/x/slashing/keeper/keeper_test.go index ce28a5607b23..cd09211fa8f0 100644 --- a/x/slashing/keeper/keeper_test.go +++ b/x/slashing/keeper/keeper_test.go @@ -4,9 +4,8 @@ import ( "testing" "time" - abci "github.com/tendermint/tendermint/abci/types" - "github.com/stretchr/testify/require" + abci "github.com/tendermint/tendermint/abci/types" "github.com/cosmos/cosmos-sdk/simapp" sdk "github.com/cosmos/cosmos-sdk/types" @@ -14,6 +13,85 @@ import ( "github.com/cosmos/cosmos-sdk/x/staking" ) +func TestUnJailNotBonded(t *testing.T) { + app := simapp.Setup(false) + ctx := app.BaseApp.NewContext(false, abci.Header{}) + + p := app.StakingKeeper.GetParams(ctx) + p.MaxValidators = 5 + app.StakingKeeper.SetParams(ctx, p) + + amt := sdk.TokensFromConsensusPower(100) + sh := staking.NewHandler(app.StakingKeeper) + + addrDels := simapp.AddTestAddrsIncremental(app, ctx, 6, sdk.TokensFromConsensusPower(200)) + valAddrs := simapp.ConvertAddrsToValAddrs(addrDels) + pks := simapp.CreateTestPubKeys(6) + + // create max (5) validators all with the same power + for i := uint32(0); i < p.MaxValidators; i++ { + addr, val := valAddrs[i], pks[i] + res, err := sh(ctx, keeper.NewTestMsgCreateValidator(addr, val, amt)) + require.NoError(t, err) + require.NotNil(t, res) + } + + staking.EndBlocker(ctx, app.StakingKeeper) + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + + // create a 6th validator with less power than the cliff validator (won't be bonded) + addr, val := valAddrs[5], pks[5] + createValMsg := keeper.NewTestMsgCreateValidator(addr, val, sdk.TokensFromConsensusPower(50)) + createValMsg.MinSelfDelegation = sdk.TokensFromConsensusPower(50) + res, err := sh(ctx, createValMsg) + require.NoError(t, err) + require.NotNil(t, res) + + staking.EndBlocker(ctx, app.StakingKeeper) + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + + validator, ok := app.StakingKeeper.GetValidator(ctx, addr) + require.True(t, ok) + require.False(t, validator.Jailed) + require.Equal(t, sdk.BondStatusUnbonded, validator.GetStatus().String()) + + // unbond below minimum self-delegation + msgUnbond := staking.NewMsgUndelegate(sdk.AccAddress(addr), addr, sdk.NewCoin(p.BondDenom, sdk.TokensFromConsensusPower(1))) + res, err = sh(ctx, msgUnbond) + require.NoError(t, err) + require.NotNil(t, res) + + staking.EndBlocker(ctx, app.StakingKeeper) + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + + // verify that validator is jailed + validator, ok = app.StakingKeeper.GetValidator(ctx, addr) + require.True(t, ok) + require.True(t, validator.Jailed) + + // verify we cannot unjail (yet) + require.Error(t, app.SlashingKeeper.Unjail(ctx, addr)) + + staking.EndBlocker(ctx, app.StakingKeeper) + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + + // bond to meet minimum self-delegation + msgBond := staking.NewMsgDelegate(sdk.AccAddress(addr), addr, sdk.NewCoin(p.BondDenom, sdk.TokensFromConsensusPower(1))) + res, err = sh(ctx, msgBond) + require.NoError(t, err) + require.NotNil(t, res) + + staking.EndBlocker(ctx, app.StakingKeeper) + ctx = ctx.WithBlockHeight(ctx.BlockHeight() + 1) + + // verify we can immediately unjail + require.NoError(t, app.SlashingKeeper.Unjail(ctx, addr)) + + validator, ok = app.StakingKeeper.GetValidator(ctx, addr) + require.True(t, ok) + require.False(t, validator.Jailed) +} + // Test a new validator entering the validator set // Ensure that SigningInfo.StartHeight is set correctly // and that they are not immediately jailed diff --git a/x/slashing/keeper/unjail.go b/x/slashing/keeper/unjail.go index 95918d7dd84c..d10ace134fea 100644 --- a/x/slashing/keeper/unjail.go +++ b/x/slashing/keeper/unjail.go @@ -2,6 +2,7 @@ package keeper import ( sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" "github.com/cosmos/cosmos-sdk/x/slashing/types" ) @@ -19,8 +20,12 @@ func (k Keeper) Unjail(ctx sdk.Context, validatorAddr sdk.ValAddress) error { return types.ErrMissingSelfDelegation } - if validator.TokensFromShares(selfDel.GetShares()).TruncateInt().LT(validator.GetMinSelfDelegation()) { - return types.ErrSelfDelegationTooLowToUnjail + tokens := validator.TokensFromShares(selfDel.GetShares()).TruncateInt() + minSelfBond := validator.GetMinSelfDelegation() + if tokens.LT(minSelfBond) { + return sdkerrors.Wrapf( + types.ErrSelfDelegationTooLowToUnjail, "%s less than %s", tokens, minSelfBond, + ) } // cannot be unjailed if not jailed From 3d8242d8713286ba5a77a37fcd4c19c6c28caa48 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 Apr 2020 10:49:55 -0400 Subject: [PATCH 4/8] Update godoc --- x/slashing/keeper/unjail.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/slashing/keeper/unjail.go b/x/slashing/keeper/unjail.go index d10ace134fea..a8dab94d25d4 100644 --- a/x/slashing/keeper/unjail.go +++ b/x/slashing/keeper/unjail.go @@ -41,7 +41,7 @@ func (k Keeper) Unjail(ctx sdk.Context, validatorAddr sdk.ValAddress) error { // // A validator that is jailed but has no ValidatorSigningInfo object signals // that the validator was never bonded and must've been jailed due to falling - // below their minimum self-delegation. The validator can unjail at point + // below their minimum self-delegation. The validator can unjail at any point // assuming they've now bonded above their minimum self-delegation. info, found := k.GetValidatorSigningInfo(ctx, consAddr) if found { From 11c59278b7023cf14634b49c68da436a6380630c Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 Apr 2020 10:52:07 -0400 Subject: [PATCH 5/8] Add changelog entries --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b65d8f2175f..5b22c96f1ea2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -113,6 +113,8 @@ information on how to implement the new `Keyring` interface. ### Bug Fixes +* (x/staking) [\#6061](https://github.com/cosmos/cosmos-sdk/pull/6061) Allow a validator to immediately unjail when no signing info is present due to +falling below their minimum self-delegation and never having been bonded. The validator may immediately unjail once they've met their minimum self-delegation. * (types) [\#5741](https://github.com/cosmos/cosmos-sdk/issues/5741) Prevent ChainAnteDecorators() from panicking when empty AnteDecorator slice is supplied. * (modules) [\#5569](https://github.com/cosmos/cosmos-sdk/issues/5569) `InitGenesis`, for the relevant modules, now ensures module accounts exist. * (crypto/keyring) [\#5844](https://github.com/cosmos/cosmos-sdk/pull/5844) `Keyring.Sign()` methods no longer decode amino signatures when method receivers @@ -128,6 +130,8 @@ invalid or incomplete requests. ### State Machine Breaking +* (x/staking) [\#6061](https://github.com/cosmos/cosmos-sdk/pull/6061) Allow a validator to immediately unjail when no signing info is present due to +falling below their minimum self-delegation and never having been bonded. The validator may immediately unjail once they've met their minimum self-delegation. * (x/supply) [\#6010](https://github.com/cosmos/cosmos-sdk/pull/6010) Removed the `x/supply` module by merging the existing types and APIs into the `x/bank` module. * (modules) [\#5572](https://github.com/cosmos/cosmos-sdk/pull/5572) Separate balance from accounts per ADR 004. * Account balances are now persisted and retrieved via the `x/bank` module. From deda50a64b48ee2c444e93bdd47657e64b281732 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 Apr 2020 12:02:37 -0400 Subject: [PATCH 6/8] Fix TestUndelegateSelfDelegationBelowMinSelfDelegation --- x/staking/keeper/delegation_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index 8eaee5709bc8..15909fdfa26e 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -327,6 +327,7 @@ func TestUndelegateSelfDelegationBelowMinSelfDelegation(t *testing.T) { app.AccountKeeper.SetModuleAccount(ctx, notBondedPool) validator = keeper.TestingUpdateValidator(app.StakingKeeper, ctx, validator, true) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) require.True(t, validator.IsBonded()) selfDelegation := types.NewDelegation(sdk.AccAddress(addrVals[0].Bytes()), addrVals[0], issuedShares) From c0852591c132bc171568a69da027de61cd27759d Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Fri, 24 Apr 2020 16:45:15 -0400 Subject: [PATCH 7/8] Fix delegation tests --- x/staking/keeper/delegation_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/x/staking/keeper/delegation_test.go b/x/staking/keeper/delegation_test.go index 15909fdfa26e..7c64df120371 100644 --- a/x/staking/keeper/delegation_test.go +++ b/x/staking/keeper/delegation_test.go @@ -381,6 +381,7 @@ func TestUndelegateFromUnbondingValidator(t *testing.T) { //create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) validator, issuedShares := validator.AddTokensFromDel(delTokens) require.Equal(t, delTokens, issuedShares.RoundInt()) @@ -481,6 +482,7 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { // create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) valTokens := sdk.TokensFromConsensusPower(10) validator, issuedShares := validator.AddTokensFromDel(valTokens) @@ -565,6 +567,7 @@ func TestUnbondingAllDelegationFromValidator(t *testing.T) { //create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) valTokens := sdk.TokensFromConsensusPower(10) validator, issuedShares := validator.AddTokensFromDel(valTokens) @@ -821,6 +824,8 @@ func TestRedelegateSelfDelegation(t *testing.T) { //create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) + valTokens := sdk.TokensFromConsensusPower(10) validator, issuedShares := validator.AddTokensFromDel(valTokens) require.Equal(t, valTokens, issuedShares.RoundInt()) @@ -878,6 +883,7 @@ func TestRedelegateFromUnbondingValidator(t *testing.T) { //create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) valTokens := sdk.TokensFromConsensusPower(10) validator, issuedShares := validator.AddTokensFromDel(valTokens) @@ -962,6 +968,7 @@ func TestRedelegateFromUnbondedValidator(t *testing.T) { //create a validator with a self-delegation validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + app.StakingKeeper.SetValidatorByConsAddr(ctx, validator) valTokens := sdk.TokensFromConsensusPower(10) validator, issuedShares := validator.AddTokensFromDel(valTokens) From 337ca37bff27dc980feed6e77a07c5a8f2396ef0 Mon Sep 17 00:00:00 2001 From: Aleksandr Bezobchuk Date: Sun, 26 Apr 2020 13:19:44 -0400 Subject: [PATCH 8/8] Revert API call --- x/staking/keeper/delegation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x/staking/keeper/delegation.go b/x/staking/keeper/delegation.go index d3b768350c93..f70879ca485d 100644 --- a/x/staking/keeper/delegation.go +++ b/x/staking/keeper/delegation.go @@ -582,7 +582,7 @@ func (k Keeper) Unbond( if isValidatorOperator && !validator.Jailed && validator.TokensFromShares(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) { - k.Jail(ctx, validator.GetConsAddr()) + k.jailValidator(ctx, validator) validator = k.mustGetValidator(ctx, validator.OperatorAddress) }