diff --git a/CHANGELOG.md b/CHANGELOG.md index 577be5e06b..8e07e963f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -100,6 +100,7 @@ IMPROVEMENTS BUG FIXES * [x/slashing] \#1510 Unrevoked validators cannot un-revoke themselves +* [x/stake] \#1513 Validators slashed to zero power are unbonded and removed from the store * [x/stake] \#1567 Validators decreased in power but not unbonded are now updated in Tendermint * [gaia] Added self delegation for validators in the genesis creation * [lcd] tests now don't depend on raw json text diff --git a/x/stake/handler_test.go b/x/stake/handler_test.go index b18b3c98b2..f08dc481a8 100644 --- a/x/stake/handler_test.go +++ b/x/stake/handler_test.go @@ -638,7 +638,7 @@ func TestBondUnbondRedelegateSlashTwice(t *testing.T) { require.Equal(t, sdk.NewRat(3), delegation.Shares) // validator power should have been reduced to zero - validator, found = keeper.GetValidator(ctx, valA) - require.True(t, found) - require.Equal(t, sdk.NewRat(0), validator.GetPower()) + // ergo validator should have been removed from the store + _, found = keeper.GetValidator(ctx, valA) + require.False(t, found) } diff --git a/x/stake/keeper/slash.go b/x/stake/keeper/slash.go index b12360fc3f..f194d656cb 100644 --- a/x/stake/keeper/slash.go +++ b/x/stake/keeper/slash.go @@ -15,8 +15,6 @@ import ( // CONTRACT: // slashFactor is non-negative // CONTRACT: -// Validator exists and can be looked up by public key -// CONTRACT: // Infraction committed equal to or less than an unbonding period in the past, // so all unbonding delegations and redelegations from that height are stored // CONTRACT: @@ -36,7 +34,12 @@ func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, infractionHeight in validator, found := k.GetValidatorByPubKey(ctx, pubkey) if !found { - panic(fmt.Errorf("attempted to slash a nonexistent validator with address %s", pubkey.Address())) + // If not found, the validator must have been overslashed and removed - so we don't need to do anything + // NOTE: Correctness dependent on invariant that unbonding delegations / redelegations must also have been completely + // slashed in this case - which we don't explicitly check, but should be true. + // Log the slash attempt for future reference (maybe we should tag it too) + logger.Error(fmt.Sprintf("WARNING: Ignored attempt to slash a nonexistent validator with address %s, we recommend you investigate immediately", pubkey.Address())) + return } ownerAddress := validator.GetOwner() @@ -92,7 +95,11 @@ func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, infractionHeight in // update the pool k.SetPool(ctx, pool) // update the validator, possibly kicking it out - k.UpdateValidator(ctx, validator) + validator = k.UpdateValidator(ctx, validator) + // remove validator if it has been reduced to zero shares + if validator.PoolShares.Amount.IsZero() { + k.RemoveValidator(ctx, validator.Owner) + } // Log that a slash occurred! logger.Info(fmt.Sprintf("Validator %s slashed by slashFactor %v, removed %v shares and burned %d tokens", pubkey.Address(), slashFactor, sharesToRemove, burned)) diff --git a/x/stake/keeper/slash_test.go b/x/stake/keeper/slash_test.go index dcd38e15e2..f9cd8229a0 100644 --- a/x/stake/keeper/slash_test.go +++ b/x/stake/keeper/slash_test.go @@ -291,10 +291,10 @@ func TestSlashWithUnbondingDelegation(t *testing.T) { // just 1 bonded token burned again since that's all the validator now has require.Equal(t, int64(10), oldPool.BondedTokens-newPool.BondedTokens) // read updated validator - validator, found = keeper.GetValidatorByPubKey(ctx, pk) - require.True(t, found) // power decreased by 1 again, validator is out of stake - require.Equal(t, sdk.NewRat(0), validator.GetPower()) + // ergo validator should have been removed from the store + _, found = keeper.GetValidatorByPubKey(ctx, pk) + require.False(t, found) } // tests Slash at a previous height with a redelegation @@ -387,16 +387,16 @@ func TestSlashWithRedelegation(t *testing.T) { // four more bonded tokens burned require.Equal(t, int64(16), oldPool.BondedTokens-newPool.BondedTokens) // read updated validator - validator, found = keeper.GetValidatorByPubKey(ctx, pk) - require.True(t, found) - // power decreased by 4, down to 0 - require.Equal(t, sdk.NewRat(0), validator.GetPower()) + // validator decreased to zero power, should have been removed from the store + _, found = keeper.GetValidatorByPubKey(ctx, pk) + require.False(t, found) // slash the validator again, by 100% // no stake remains to be slashed ctx = ctx.WithBlockHeight(12) - validator, found = keeper.GetValidatorByPubKey(ctx, pk) - require.True(t, found) + // validator no longer in the store + _, found = keeper.GetValidatorByPubKey(ctx, pk) + require.False(t, found) keeper.Slash(ctx, pk, 10, 10, sdk.OneRat()) // read updating redelegation @@ -409,10 +409,9 @@ func TestSlashWithRedelegation(t *testing.T) { // no more bonded tokens burned require.Equal(t, int64(16), oldPool.BondedTokens-newPool.BondedTokens) // read updated validator - validator, found = keeper.GetValidatorByPubKey(ctx, pk) - require.True(t, found) - // power still zero - require.Equal(t, sdk.NewRat(0), validator.GetPower()) + // power still zero, still not in the store + _, found = keeper.GetValidatorByPubKey(ctx, pk) + require.False(t, found) } // tests Slash at a previous height with both an unbonding delegation and a redelegation diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 9ad6e12bc0..8320f259ce 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -93,6 +93,28 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { require.True(t, keeper.validatorByPowerIndexExists(ctx, power)) } +func TestSlashToZeroPowerRemoved(t *testing.T) { + // initialize setup + ctx, _, keeper := CreateTestInput(t, false, 100) + pool := keeper.GetPool(ctx) + + // add a validator + validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + validator, pool, _ = validator.AddTokensFromDel(pool, 100) + require.Equal(t, sdk.Unbonded, validator.Status()) + require.Equal(t, int64(100), validator.PoolShares.Tokens(pool).RoundInt64()) + keeper.SetPool(ctx, pool) + keeper.SetValidatorByPubKeyIndex(ctx, validator) + validator = keeper.UpdateValidator(ctx, validator) + require.Equal(t, int64(100), validator.PoolShares.Tokens(pool).RoundInt64(), "\nvalidator %v\npool %v", validator, pool) + + // slash the validator by 100% + keeper.Slash(ctx, PKs[0], 0, 100, sdk.OneRat()) + // validator should have been deleted + _, found := keeper.GetValidator(ctx, addrVals[0]) + require.False(t, found) +} + // This function tests UpdateValidator, GetValidator, GetValidatorsBonded, RemoveValidator func TestValidatorBasics(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000)