From c2aecb8b0ef439f5861458d6d29d5f7e10bd44f3 Mon Sep 17 00:00:00 2001 From: Alexander Bezobchuk Date: Fri, 1 Mar 2019 16:19:27 -0500 Subject: [PATCH] Merge PR #3762: Allow Validator w/ No Self-Delegation to Unjail (Transfers Disabled) --- PENDING.md | 3 ++ cmd/gaia/app/app.go | 1 + cmd/gaia/app/genesis.go | 4 +-- cmd/gaia/cmd/gaiadebug/hack.go | 2 +- docs/gaia/validators/validator-setup.md | 16 ++++++--- x/slashing/app_test.go | 5 +-- x/slashing/handler.go | 21 ++++++++---- x/slashing/handler_test.go | 45 ++++++++++++++++++++++--- x/slashing/keeper.go | 8 ++++- x/slashing/keeper_contracts.go | 9 +++++ x/slashing/test_common.go | 32 +++++++++++++++--- 11 files changed, 120 insertions(+), 26 deletions(-) create mode 100644 x/slashing/keeper_contracts.go diff --git a/PENDING.md b/PENDING.md index 36a499659..4e2703229 100644 --- a/PENDING.md +++ b/PENDING.md @@ -19,6 +19,9 @@ ### Gaia +* [\#3760] Allow unjailing when a validator has no self-delegation and during +disabled transfers. + ### SDK * [\#3669] Ensure consistency in message naming, codec registration, and JSON diff --git a/cmd/gaia/app/app.go b/cmd/gaia/app/app.go index d5b031272..3efc36fd8 100644 --- a/cmd/gaia/app/app.go +++ b/cmd/gaia/app/app.go @@ -133,6 +133,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, traceStore io.Writer, loadLatest b app.keySlashing, &stakingKeeper, app.paramsKeeper.Subspace(slashing.DefaultParamspace), slashing.DefaultCodespace, + app.bankKeeper, ) app.govKeeper = gov.NewKeeper( app.cdc, diff --git a/cmd/gaia/app/genesis.go b/cmd/gaia/app/genesis.go index 74e396962..ba48769b4 100644 --- a/cmd/gaia/app/genesis.go +++ b/cmd/gaia/app/genesis.go @@ -188,8 +188,8 @@ func GaiaAppGenState(cdc *codec.Codec, genDoc tmtypes.GenesisDoc, appGenTxs []js for _, acc := range genesisState.Accounts { for _, coin := range acc.Coins { if coin.Denom == genesisState.StakingData.Params.BondDenom { - stakingData.Pool.NotBondedTokens = stakingData.Pool.NotBondedTokens. - Add(coin.Amount) // increase the supply + // increase the supply + stakingData.Pool.NotBondedTokens = stakingData.Pool.NotBondedTokens.Add(coin.Amount) } } } diff --git a/cmd/gaia/cmd/gaiadebug/hack.go b/cmd/gaia/cmd/gaiadebug/hack.go index e7a89cea7..9a0dc1d63 100644 --- a/cmd/gaia/cmd/gaiadebug/hack.go +++ b/cmd/gaia/cmd/gaiadebug/hack.go @@ -180,7 +180,7 @@ func NewGaiaApp(logger log.Logger, db dbm.DB, baseAppOptions ...func(*bam.BaseAp // add handlers app.bankKeeper = bank.NewBaseKeeper(app.accountKeeper, app.paramsKeeper.Subspace(bank.DefaultParamspace), bank.DefaultCodespace) app.stakingKeeper = staking.NewKeeper(app.cdc, app.keyStaking, app.tkeyStaking, app.bankKeeper, app.paramsKeeper.Subspace(staking.DefaultParamspace), staking.DefaultCodespace) - app.slashingKeeper = slashing.NewKeeper(app.cdc, app.keySlashing, app.stakingKeeper, app.paramsKeeper.Subspace(slashing.DefaultParamspace), slashing.DefaultCodespace) + app.slashingKeeper = slashing.NewKeeper(app.cdc, app.keySlashing, app.stakingKeeper, app.paramsKeeper.Subspace(slashing.DefaultParamspace), slashing.DefaultCodespace, app.bankKeeper) // register message routes app.Router(). diff --git a/docs/gaia/validators/validator-setup.md b/docs/gaia/validators/validator-setup.md index cac1508d0..ca129ded9 100644 --- a/docs/gaia/validators/validator-setup.md +++ b/docs/gaia/validators/validator-setup.md @@ -84,11 +84,17 @@ __Note__: This command automatically store your `gentx` in `~/.gaiad/config/gent Consult `gaiad gentx --help` for more information on the flags defaults. ::: -A `gentx` is a JSON file carrying a self-delegation. All genesis transactions are collected by a `genesis coordinator` and validated against an initial `genesis.json`. Such initial `genesis.json` contains only a list of accounts and their coins. Once the transactions are processed, they are merged in the `genesis.json`'s `gentxs` field. +A `gentx` is a JSON file carrying a self-delegation. All genesis transactions are +collected by a `genesis coordinator` and validated against an initial `genesis.json`. +Such an initial `genesis.json` contains only a list of accounts and their coins. +Once the transactions are processed, they are merged in the `genesis.json`'s +`gentxs` field. -### Case 2: The initial stake comes from a delegator's address +### Case 2: The initial stake comes from (on behalf of) a delegator's address -In this case, you need both the signature of the validator and the delegator. Start by creating an unsigned `create-validator` transaction, and save it in a file called `unsignedValTx`: +In this case, you need both the signature of the validator and the delegator. +Start by creating an unsigned `create-validator` transaction, and save it in a +file called `unsignedValTx`: ```bash gaiacli tx staking create-validator \ @@ -96,11 +102,11 @@ gaiacli tx staking create-validator \ --pubkey=$(gaiad tendermint show-validator) \ --moniker="choose a moniker" \ --chain-id= \ - --from= \ + --from= \ --commission-rate="0.10" \ --commission-max-rate="0.20" \ --commission-max-change-rate="0.01" \ - --address-delegator="address of the delegator" \ + --address-delegator= \ --generate-only \ > unsignedValTx.json ``` diff --git a/x/slashing/app_test.go b/x/slashing/app_test.go index 6f15e4aee..b2d0acde8 100644 --- a/x/slashing/app_test.go +++ b/x/slashing/app_test.go @@ -33,10 +33,11 @@ func getMockApp(t *testing.T) (*mock.App, staking.Keeper, Keeper) { bankKeeper := bank.NewBaseKeeper(mapp.AccountKeeper, mapp.ParamsKeeper.Subspace(bank.DefaultParamspace), bank.DefaultCodespace) stakingKeeper := staking.NewKeeper(mapp.Cdc, keyStaking, tkeyStaking, bankKeeper, mapp.ParamsKeeper.Subspace(staking.DefaultParamspace), staking.DefaultCodespace) - keeper := NewKeeper(mapp.Cdc, keySlashing, stakingKeeper, mapp.ParamsKeeper.Subspace(DefaultParamspace), DefaultCodespace) + mbk := mockBankKeeper{true} + keeper := NewKeeper(mapp.Cdc, keySlashing, stakingKeeper, mapp.ParamsKeeper.Subspace(DefaultParamspace), DefaultCodespace, mbk) + mapp.Router().AddRoute(staking.RouterKey, staking.NewHandler(stakingKeeper)) mapp.Router().AddRoute(RouterKey, NewHandler(keeper)) - mapp.SetEndBlocker(getEndBlocker(stakingKeeper)) mapp.SetInitChainer(getInitChainer(mapp, stakingKeeper)) diff --git a/x/slashing/handler.go b/x/slashing/handler.go index d9ae9f2d8..41a54ec1d 100644 --- a/x/slashing/handler.go +++ b/x/slashing/handler.go @@ -25,14 +25,23 @@ func handleMsgUnjail(ctx sdk.Context, msg MsgUnjail, k Keeper) sdk.Result { return ErrNoValidatorForAddress(k.codespace).Result() } - // cannot be unjailed if no self-delegation exists selfDel := k.validatorSet.Delegation(ctx, sdk.AccAddress(msg.ValidatorAddr), msg.ValidatorAddr) - if selfDel == nil { - return ErrMissingSelfDelegation(k.codespace).Result() - } - if validator.GetDelegatorShareExRate().Mul(selfDel.GetShares()).TruncateInt().LT(validator.GetMinSelfDelegation()) { - return ErrSelfDelegationTooLowToUnjail(k.codespace).Result() + // A validator attempting to unjail may only do so if its self-bond amount + // is at least their declared min self-delegation. However, during disabled + // transfers, we allow validators to unjail if they have no self-delegation. + // This is to allow newly created validators during a time when transfers are + // disabled to successfully unjail. + if k.bk.GetSendEnabled(ctx) { + if selfDel == nil { + // cannot be unjailed if no self-delegation exists + return ErrMissingSelfDelegation(k.codespace).Result() + } + + valSelfBond := validator.GetDelegatorShareExRate().Mul(selfDel.GetShares()).TruncateInt() + if valSelfBond.LT(validator.GetMinSelfDelegation()) { + return ErrSelfDelegationTooLowToUnjail(k.codespace).Result() + } } // cannot be unjailed if not jailed diff --git a/x/slashing/handler_test.go b/x/slashing/handler_test.go index 07cf45001..fdf9e8c3f 100644 --- a/x/slashing/handler_test.go +++ b/x/slashing/handler_test.go @@ -53,19 +53,56 @@ func TestCannotUnjailUnlessMeetMinSelfDelegation(t *testing.T) { undelegateMsg := staking.NewMsgUndelegate(sdk.AccAddress(addr), addr, sdk.OneDec()) got = staking.NewHandler(sk)(ctx, undelegateMsg) - + require.True(t, got.IsOK()) require.True(t, sk.Validator(ctx, addr).GetJailed()) - // assert non-jailed validator can't be unjailed + // assert jailed validator can't be unjailed due to min self-delegation got = slh(ctx, NewMsgUnjail(addr)) require.False(t, got.IsOK(), "allowed unjail of validator with less than MinSelfDelegation") require.EqualValues(t, CodeValidatorNotJailed, got.Code) require.EqualValues(t, DefaultCodespace, got.Codespace) } +func TestUnjailNoTransferValidator(t *testing.T) { + // initial setup + ctx, bk, sk, _, keeper := createTestInput(t, DefaultParams()) + keeper.bk = mockBankKeeper{false} + + slh := NewHandler(keeper) + amtInt := int64(100) + delAddr := sdk.AccAddress(addrs[0]) + valAddr, pubKey, amt := addrs[1], pks[1], sdk.TokensFromTendermintPower(amtInt) + + msg := NewTestMsgCreateValidatorOnBehalfOf(delAddr, valAddr, pubKey, amt, amt) + got := staking.NewHandler(sk)(ctx, msg) + require.True(t, got.IsOK()) + + staking.EndBlocker(ctx, sk) + require.Equal( + t, bk.GetCoins(ctx, delAddr), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins.Sub(amt))}, + ) + require.Equal( + t, bk.GetCoins(ctx, sdk.AccAddress(valAddr)), + sdk.Coins{sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins)}, + ) + + sk.Jail(ctx, sdk.ConsAddress(valAddr)) + + val := sk.Validator(ctx, valAddr) + selfDel := keeper.validatorSet.Delegation(ctx, sdk.AccAddress(valAddr), valAddr) + require.Nil(t, selfDel) + require.True(t, val.GetJailed()) + + got = slh(ctx, NewMsgUnjail(valAddr)) + require.True(t, got.IsOK(), "expected validator to be unjailed") + + val = sk.Validator(ctx, valAddr) + require.False(t, val.GetJailed()) +} + func TestJailedValidatorDelegations(t *testing.T) { ctx, _, stakingKeeper, _, slashingKeeper := createTestInput(t, DefaultParams()) - stakingParams := stakingKeeper.GetParams(ctx) stakingParams.UnbondingTime = 0 stakingKeeper.SetParams(ctx, stakingParams) @@ -107,7 +144,7 @@ func TestJailedValidatorDelegations(t *testing.T) { require.True(t, found) require.True(t, validator.GetJailed()) - // verify the validator cannot unjail itself + // verify the validator cannot unjail itself (with transfers enabled) got = NewHandler(slashingKeeper)(ctx, NewMsgUnjail(valAddr)) require.False(t, got.IsOK(), "expected jailed validator to not be able to unjail, got: %v", got) diff --git a/x/slashing/keeper.go b/x/slashing/keeper.go index 8b8360492..c66d5f02d 100644 --- a/x/slashing/keeper.go +++ b/x/slashing/keeper.go @@ -17,16 +17,22 @@ type Keeper struct { cdc *codec.Codec validatorSet sdk.ValidatorSet paramspace params.Subspace + bk BankKeeper // codespace codespace sdk.CodespaceType } // NewKeeper creates a slashing keeper -func NewKeeper(cdc *codec.Codec, key sdk.StoreKey, vs sdk.ValidatorSet, paramspace params.Subspace, codespace sdk.CodespaceType) Keeper { +func NewKeeper( + cdc *codec.Codec, key sdk.StoreKey, vs sdk.ValidatorSet, + paramspace params.Subspace, codespace sdk.CodespaceType, bk BankKeeper, +) Keeper { + keeper := Keeper{ storeKey: key, cdc: cdc, + bk: bk, validatorSet: vs, paramspace: paramspace.WithKeyTable(ParamKeyTable()), codespace: codespace, diff --git a/x/slashing/keeper_contracts.go b/x/slashing/keeper_contracts.go new file mode 100644 index 000000000..15abdaf27 --- /dev/null +++ b/x/slashing/keeper_contracts.go @@ -0,0 +1,9 @@ +package slashing + +import sdk "github.com/cosmos/cosmos-sdk/types" + +// BankKeeper defines the bank keeper interfact contract the slashing module +// requires. It is needed in order to determine if transfers are enabled. +type BankKeeper interface { + GetSendEnabled(ctx sdk.Context) bool +} diff --git a/x/slashing/test_common.go b/x/slashing/test_common.go index 13279d614..9658855ba 100644 --- a/x/slashing/test_common.go +++ b/x/slashing/test_common.go @@ -39,6 +39,16 @@ var ( initCoins = sdk.TokensFromTendermintPower(200) ) +var _ BankKeeper = (*mockBankKeeper)(nil) + +type mockBankKeeper struct { + sendEnabled bool +} + +func (mbk mockBankKeeper) GetSendEnabled(_ sdk.Context) bool { + return mbk.sendEnabled +} + func createTestCodec() *codec.Codec { cdc := codec.New() sdk.RegisterCodec(cdc) @@ -80,14 +90,15 @@ func createTestInput(t *testing.T, defaults Params) (sdk.Context, bank.Keeper, s _, err = staking.InitGenesis(ctx, sk, genesis) require.Nil(t, err) + initCoins := sdk.NewCoin(sk.GetParams(ctx).BondDenom, initCoins) for _, addr := range addrs { - _, _, err = ck.AddCoins(ctx, sdk.AccAddress(addr), sdk.Coins{ - {sk.GetParams(ctx).BondDenom, initCoins}, - }) + _, _, err = ck.AddCoins(ctx, sdk.AccAddress(addr), sdk.Coins{initCoins}) + require.Nil(t, err) } - require.Nil(t, err) + + mbk := mockBankKeeper{true} paramstore := paramsKeeper.Subspace(DefaultParamspace) - keeper := NewKeeper(cdc, keySlashing, &sk, paramstore, DefaultCodespace) + keeper := NewKeeper(cdc, keySlashing, &sk, paramstore, DefaultCodespace, mbk) sk.SetHooks(keeper.Hooks()) require.NotPanics(t, func() { @@ -120,6 +131,17 @@ func NewTestMsgCreateValidator(address sdk.ValAddress, pubKey crypto.PubKey, amt ) } +func NewTestMsgCreateValidatorOnBehalfOf( + delAddr sdk.AccAddress, valAddr sdk.ValAddress, pubKey crypto.PubKey, amt, msb sdk.Int, +) staking.MsgCreateValidator { + + commission := staking.NewCommissionMsg(sdk.ZeroDec(), sdk.ZeroDec(), sdk.ZeroDec()) + return staking.NewMsgCreateValidatorOnBehalfOf( + delAddr, valAddr, pubKey, sdk.NewCoin(sdk.DefaultBondDenom, amt), + staking.Description{}, commission, msb, + ) +} + func newTestMsgDelegate(delAddr sdk.AccAddress, valAddr sdk.ValAddress, delAmount sdk.Int) staking.MsgDelegate { amount := sdk.NewCoin(sdk.DefaultBondDenom, delAmount) return staking.NewMsgDelegate(delAddr, valAddr, amount)