Skip to content

Allow Unjail of Non-Bonded Jailed Validator #6061

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

Merged
merged 11 commits into from
Apr 26, 2020
82 changes: 80 additions & 2 deletions x/slashing/keeper/keeper_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,94 @@ 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"
"github.com/cosmos/cosmos-sdk/x/slashing/keeper"
"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
Expand Down
39 changes: 25 additions & 14 deletions x/slashing/keeper/unjail.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -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
Expand All @@ -30,19 +35,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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer to have some form of context by which we can verify the reason for being jailed for situations like this. In the future it may be that there are other reasons that an unbonded validator may be jailed (and other privileges to being a validator "in waiting"). I could see the Validator Jailed bool field being replaced with a JailedStatus uint8 kind whereby the reason for being jailed could be specified (here, 0 value would probably indicate "not-jailed"). Anyways that's a breaking change, but I think it could be helpful.... probably for another PR or issue!

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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The core change is here. We now allow a validator to unjail if no ValidatorSigningInfo is present. This should only ever be the case under the condition of the reported issue. Essentially, we allow a validator to unjail immediately (once they've met their minimum self-delegation).

// 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)
Expand Down
6 changes: 3 additions & 3 deletions x/staking/keeper/delegation.go
Original file line number Diff line number Diff line change
Expand Up @@ -577,12 +577,12 @@ 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 &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note there is no change here apart from keeping the code DRY and calling k.Jail instead of manually calling the private internal APIs.

validator.TokensFromShares(delegation.Shares).TruncateInt().LT(validator.MinSelfDelegation) {

k.jailValidator(ctx, validator)
k.Jail(ctx, validator.GetConsAddr())
validator = k.mustGetValidator(ctx, validator.OperatorAddress)
}

Expand Down