From 505c356f20d0d8ecbe3ccaeaba7f63f0a018ca38 Mon Sep 17 00:00:00 2001 From: Sunny Aggarwal Date: Thu, 18 Oct 2018 12:58:18 -0700 Subject: [PATCH] Merge PR #2514: Refactor validator deletion --- PENDING.md | 1 + docs/spec/staking/end_block.md | 12 +++-- docs/spec/staking/transactions.md | 5 +- x/stake/keeper/delegation.go | 5 +- x/stake/keeper/delegation_test.go | 82 ++++++++++++++++++++++++++++++- x/stake/keeper/slash.go | 4 +- x/stake/keeper/validator.go | 6 ++- 7 files changed, 102 insertions(+), 13 deletions(-) diff --git a/PENDING.md b/PENDING.md index c278722b9..ca0c316be 100644 --- a/PENDING.md +++ b/PENDING.md @@ -44,6 +44,7 @@ BREAKING CHANGES * [simulation] \#2162 Added back correct supply invariants * [x/slashing] \#2430 Simulate more slashes, check if validator is jailed before jailing * [x/stake] \#2393 Removed `CompleteUnbonding` and `CompleteRedelegation` Msg types, and instead added unbonding/redelegation queues to endblocker + * [x/stake] \#1673 Validators are no longer deleted until they can no longer possibly be slashed * SDK * [core] \#2219 Update to Tendermint 0.24.0 diff --git a/docs/spec/staking/end_block.md b/docs/spec/staking/end_block.md index 6439ca0c7..e1d769ed4 100644 --- a/docs/spec/staking/end_block.md +++ b/docs/spec/staking/end_block.md @@ -3,15 +3,19 @@ ## Unbonding Validator Queue For all unbonding validators that have finished their unbonding period, this switches their validator.Status -from sdk.Unbonding to sdk.Unbonded +from sdk.Unbonding to sdk.Unbonded if they still have any delegation left. Otherwise, it deletes it from state. ```golang validatorQueue(currTime time.Time): // unbonding validators are in ordered queue from oldest to newest for all unbondingValidators whose CompleteTime < currTime: validator = GetValidator(unbondingValidator.ValidatorAddr) - validator.Status = sdk.Bonded - SetValidator(unbondingValidator) + if validator.DelegatorShares == 0 { + RemoveValidator(unbondingValidator) + } else { + validator.Status = sdk.Unbonded + SetValidator(unbondingValidator) + } return ``` @@ -61,4 +65,4 @@ redelegationQueue(currTime time.Time): for all redelegations whose CompleteTime < currTime: removeRedelegation(redelegation) return -``` \ No newline at end of file +``` diff --git a/docs/spec/staking/transactions.md b/docs/spec/staking/transactions.md index 74ea67c85..ee5b2dc93 100644 --- a/docs/spec/staking/transactions.md +++ b/docs/spec/staking/transactions.md @@ -180,7 +180,7 @@ startUnbonding(tx TxStartUnbonding): validator = updateValidator(validator) - if validator.DelegatorShares == 0 { + if validator.Status == Unbonded && validator.DelegatorShares == 0 { removeValidator(validator.Operator) return @@ -189,8 +189,7 @@ startUnbonding(tx TxStartUnbonding): ### TxRedelegation The redelegation command allows delegators to instantly switch validators. Once -the unbonding period has passed, the redelegation must be completed with -txRedelegationComplete. +the unbonding period has passed, the redelegation is automatically completed in the EndBlocker. ```golang type TxRedelegate struct { diff --git a/x/stake/keeper/delegation.go b/x/stake/keeper/delegation.go index 25ea3d08b..8bb468d99 100644 --- a/x/stake/keeper/delegation.go +++ b/x/stake/keeper/delegation.go @@ -425,8 +425,8 @@ func (k Keeper) unbond(ctx sdk.Context, delAddr sdk.AccAddress, valAddr sdk.ValA // remove the coins from the validator validator, amount = k.RemoveValidatorTokensAndShares(ctx, validator, shares) - if validator.DelegatorShares.IsZero() && validator.Status != sdk.Bonded { - // if bonded, we must remove in EndBlocker instead + if validator.DelegatorShares.IsZero() && validator.Status == sdk.Unbonded { + // if not unbonded, we must instead remove validator in EndBlocker once it finishes its unbonding period k.RemoveValidator(ctx, validator.OperatorAddr) } @@ -501,6 +501,7 @@ func (k Keeper) BeginUnbonding(ctx sdk.Context, } k.SetUnbondingDelegation(ctx, ubd) k.InsertUnbondingQueue(ctx, ubd) + return ubd, nil } diff --git a/x/stake/keeper/delegation_test.go b/x/stake/keeper/delegation_test.go index 46bde785a..24476ca3c 100644 --- a/x/stake/keeper/delegation_test.go +++ b/x/stake/keeper/delegation_test.go @@ -1,6 +1,7 @@ package keeper import ( + "fmt" "testing" "time" @@ -392,7 +393,13 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { require.True(t, ctx.BlockHeader().Time.Add(params.UnbondingTime).Equal(validator.UnbondingMinTime)) // unbond the validator - keeper.unbondingToUnbonded(ctx, validator) + ctx = ctx.WithBlockTime(validator.UnbondingMinTime) + keeper.UnbondAllMatureValidatorQueue(ctx) + + // Make sure validator is still in state because there is still an outstanding delegation + validator, found = keeper.GetValidator(ctx, addrVals[0]) + require.True(t, found) + require.Equal(t, validator.Status, sdk.Unbonded) // unbond some of the other delegation's shares _, err = keeper.BeginUnbonding(ctx, addrDels[0], addrVals[0], sdk.NewDec(6)) @@ -401,6 +408,79 @@ func TestUndelegateFromUnbondedValidator(t *testing.T) { // no ubd should have been found, coins should have been returned direcly to account ubd, found := keeper.GetUnbondingDelegation(ctx, addrDels[0], addrVals[0]) require.False(t, found, "%v", ubd) + + // unbond rest of the other delegation's shares + _, err = keeper.BeginUnbonding(ctx, addrDels[0], addrVals[0], sdk.NewDec(4)) + require.NoError(t, err) + + // now validator should now be deleted from state + validator, found = keeper.GetValidator(ctx, addrVals[0]) + fmt.Println(validator) + require.False(t, found) +} + +func TestUnbondingAllDelegationFromValidator(t *testing.T) { + ctx, _, keeper := CreateTestInput(t, false, 0) + pool := keeper.GetPool(ctx) + pool.LooseTokens = sdk.NewDec(20) + + //create a validator with a self-delegation + validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + + validator, pool, issuedShares := validator.AddTokensFromDel(pool, sdk.NewInt(10)) + require.Equal(t, int64(10), issuedShares.RoundInt64()) + keeper.SetPool(ctx, pool) + validator = TestingUpdateValidator(keeper, ctx, validator) + pool = keeper.GetPool(ctx) + val0AccAddr := sdk.AccAddress(addrVals[0].Bytes()) + selfDelegation := types.Delegation{ + DelegatorAddr: val0AccAddr, + ValidatorAddr: addrVals[0], + Shares: issuedShares, + } + keeper.SetDelegation(ctx, selfDelegation) + + // create a second delegation to this validator + keeper.DeleteValidatorByPowerIndex(ctx, validator, pool) + validator, pool, issuedShares = validator.AddTokensFromDel(pool, sdk.NewInt(10)) + require.Equal(t, int64(10), issuedShares.RoundInt64()) + keeper.SetPool(ctx, pool) + validator = TestingUpdateValidator(keeper, ctx, validator) + pool = keeper.GetPool(ctx) + delegation := types.Delegation{ + DelegatorAddr: addrDels[0], + ValidatorAddr: addrVals[0], + Shares: issuedShares, + } + keeper.SetDelegation(ctx, delegation) + + ctx = ctx.WithBlockHeight(10) + ctx = ctx.WithBlockTime(time.Unix(333, 0)) + + // unbond the all self-delegation to put validator in unbonding state + _, err := keeper.BeginUnbonding(ctx, val0AccAddr, addrVals[0], sdk.NewDec(10)) + require.NoError(t, err) + + // end block + updates := keeper.ApplyAndReturnValidatorSetUpdates(ctx) + require.Equal(t, 1, len(updates)) + + // unbond all the remaining delegation + _, err = keeper.BeginUnbonding(ctx, addrDels[0], addrVals[0], sdk.NewDec(10)) + require.NoError(t, err) + + // validator should still be in state and still be in unbonding state + validator, found := keeper.GetValidator(ctx, addrVals[0]) + require.True(t, found) + require.Equal(t, validator.Status, sdk.Unbonding) + + // unbond the validator + ctx = ctx.WithBlockTime(validator.UnbondingMinTime) + keeper.UnbondAllMatureValidatorQueue(ctx) + + // validator should now be deleted from state + _, found = keeper.GetValidator(ctx, addrVals[0]) + require.False(t, found) } // Make sure that that the retrieving the delegations doesn't affect the state diff --git a/x/stake/keeper/slash.go b/x/stake/keeper/slash.go index a1562da33..af59726ae 100644 --- a/x/stake/keeper/slash.go +++ b/x/stake/keeper/slash.go @@ -105,8 +105,8 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh k.SetPool(ctx, pool) // remove validator if it has no more tokens - if validator.Tokens.IsZero() && validator.Status != sdk.Bonded { - // if bonded, we must remove in ApplyAndReturnValidatorSetUpdates instead + if validator.DelegatorShares.IsZero() && validator.Status == sdk.Unbonded { + // if not unbonded, we must instead remove validator in EndBlocker once it finishes its unbonding period k.RemoveValidator(ctx, validator.OperatorAddr) } diff --git a/x/stake/keeper/validator.go b/x/stake/keeper/validator.go index 41cfe6faa..95e5f0001 100644 --- a/x/stake/keeper/validator.go +++ b/x/stake/keeper/validator.go @@ -343,7 +343,11 @@ func (k Keeper) UnbondAllMatureValidatorQueue(ctx sdk.Context) { if !found || val.GetStatus() != sdk.Unbonding { continue } - k.unbondingToUnbonded(ctx, val) + if val.GetDelegatorShares().IsZero() { + k.RemoveValidator(ctx, val.OperatorAddr) + } else { + k.unbondingToUnbonded(ctx, val) + } } store.Delete(validatorTimesliceIterator.Key()) }