Skip to content
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

R4R: Fix premature RemoveValidator bug #2676

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
fixed removeValidators prematurely
  • Loading branch information
sunnya97 committed Nov 4, 2018
commit 4841e9dd6df528c6642873a59a3b7fb44254e48b
14 changes: 9 additions & 5 deletions x/stake/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,16 @@ func NewHandler(k keeper.Keeper) sdk.Handler {
func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.ValidatorUpdate) {
endBlockerTags := sdk.EmptyTags()

// reset the intra-transaction counter
k.SetIntraTxCounter(ctx, 0)

// calculate validator set changes
ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)

// Unbond all mature validators from the unbonding queue
k.UnbondAllMatureValidatorQueue(ctx)

// Remove all mature unbonding delegations from the ubd queue
matureUnbonds := k.DequeueAllMatureUnbondingQueue(ctx, ctx.BlockHeader().Time)
for _, dvPair := range matureUnbonds {
err := k.CompleteUnbonding(ctx, dvPair.DelegatorAddr, dvPair.ValidatorAddr)
Expand All @@ -49,6 +57,7 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Valid
))
}

// Remove all mature redelegations from the red queue
matureRedelegations := k.DequeueAllMatureRedelegationQueue(ctx, ctx.BlockHeader().Time)
for _, dvvTriplet := range matureRedelegations {
err := k.CompleteRedelegation(ctx, dvvTriplet.DelegatorAddr, dvvTriplet.ValidatorSrcAddr, dvvTriplet.ValidatorDstAddr)
Expand All @@ -63,11 +72,6 @@ func EndBlocker(ctx sdk.Context, k keeper.Keeper) (ValidatorUpdates []abci.Valid
))
}

// reset the intra-transaction counter
k.SetIntraTxCounter(ctx, 0)

// calculate validator set changes
ValidatorUpdates = k.ApplyAndReturnValidatorSetUpdates(ctx)
return
}

Expand Down
11 changes: 6 additions & 5 deletions x/stake/handler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -495,14 +495,15 @@ func TestMultipleMsgCreateValidator(t *testing.T) {
require.True(t, got.IsOK(), "expected msg %d to be ok, got %v", i, got)
var finishTime time.Time
types.MsgCdc.MustUnmarshalBinary(got.Data, &finishTime)

// Jump to finishTime for unbonding period and remove from Unbonding Queue
ctx = ctx.WithBlockTime(finishTime)
EndBlocker(ctx, keeper)

//Check that the account is unbonded
// Check that the validator is deleted from state
validators := keeper.GetValidators(ctx, 100)
require.Equal(t, len(validatorAddrs)-(i+1), len(validators),
"expected %d validators got %d", len(validatorAddrs)-(i+1), len(validators))

_, found = keeper.GetValidator(ctx, validatorAddr)
require.False(t, found)

Expand Down Expand Up @@ -1013,7 +1014,7 @@ func TestBondUnbondRedelegateSlashTwice(t *testing.T) {
EndBlocker(ctx, keeper)

// validator power should have been reduced to zero
// ergo validator should have been removed from the store
_, found = keeper.GetValidator(ctx, valA)
require.False(t, found)
// validator should be in unbonding state
validator, _ = keeper.GetValidator(ctx, valA)
require.Equal(t, validator.GetStatus(), sdk.Unbonding)
}
6 changes: 0 additions & 6 deletions x/stake/keeper/slash.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,12 +108,6 @@ func (k Keeper) Slash(ctx sdk.Context, consAddr sdk.ConsAddress, infractionHeigh
pool.LooseTokens = pool.LooseTokens.Sub(tokensToBurn)
k.SetPool(ctx, pool)

// remove validator if it has no more tokens
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)
}

// Log that a slash occurred!
logger.Info(fmt.Sprintf(
"validator %s slashed by slash factor of %s; burned %v tokens",
Expand Down
24 changes: 12 additions & 12 deletions x/stake/keeper/slash_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -348,9 +348,9 @@ func TestSlashWithUnbondingDelegation(t *testing.T) {
keeper.ApplyAndReturnValidatorSetUpdates(ctx)
// read updated validator
// power decreased by 1 again, validator is out of stake
// ergo validator should have been removed from the store
_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.False(t, found)
// validator should be in unbonding period
validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.Equal(t, validator.GetStatus(), sdk.Unbonding)
}

// tests Slash at a previous height with a redelegation
Expand Down Expand Up @@ -450,16 +450,16 @@ func TestSlashWithRedelegation(t *testing.T) {
// apply TM updates
keeper.ApplyAndReturnValidatorSetUpdates(ctx)
// read updated validator
// validator decreased to zero power, should have been removed from the store
_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.False(t, found)
// validator decreased to zero power, should be in unbonding period
validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.Equal(t, validator.GetStatus(), sdk.Unbonding)

// slash the validator again, by 100%
// no stake remains to be slashed
ctx = ctx.WithBlockHeight(12)
// validator no longer in the store
_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.False(t, found)
// validator still in unbonding period
validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.Equal(t, validator.GetStatus(), sdk.Unbonding)
keeper.Slash(ctx, consAddr, 10, 10, sdk.OneDec())

// read updating redelegation
Expand All @@ -472,9 +472,9 @@ func TestSlashWithRedelegation(t *testing.T) {
// no more bonded tokens burned
require.Equal(t, int64(16), oldPool.BondedTokens.Sub(newPool.BondedTokens).RoundInt64())
// read updated validator
// power still zero, still not in the store
_, found = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.False(t, found)
// power still zero, still in unbonding period
validator, _ = keeper.GetValidatorByConsAddr(ctx, consAddr)
require.Equal(t, validator.GetStatus(), sdk.Unbonding)
}

// tests Slash at a previous height with both an unbonding delegation and a redelegation
Expand Down
5 changes: 0 additions & 5 deletions x/stake/keeper/val_state_change.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,6 @@ func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx sdk.Context) (updates []ab
// bonded to unbonding
k.bondedToUnbonding(ctx, validator)

// remove validator if it has no more tokens
if validator.Tokens.IsZero() {
k.RemoveValidator(ctx, validator.OperatorAddr)
}

// delete from the bonded validator index
k.DeleteLastValidatorPower(ctx, sdk.ValAddress(valAddrBytes))

Expand Down
6 changes: 3 additions & 3 deletions x/stake/keeper/validator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -186,9 +186,9 @@ func TestSlashToZeroPowerRemoved(t *testing.T) {
keeper.Slash(ctx, consAddr0, 0, 100, sdk.OneDec())
// apply TM updates
keeper.ApplyAndReturnValidatorSetUpdates(ctx)
// validator should have been deleted
_, found := keeper.GetValidator(ctx, addrVals[0])
require.False(t, found)
// validator should have be unbonding
validator, _ = keeper.GetValidator(ctx, addrVals[0])
require.Equal(t, validator.GetStatus(), sdk.Unbonding)
}

// This function tests UpdateValidator, GetValidator, GetLastValidators, RemoveValidator
Expand Down