From b92f80283597a6fec1c37ff708cb988a1c07f1e0 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Tue, 3 Jul 2018 02:19:18 +0200 Subject: [PATCH 1/7] Demonstrate failing testcase --- x/stake/keeper/validator_test.go | 38 ++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index c4d197a36b..c7607dd7e9 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -93,6 +93,44 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { require.True(t, keeper.validatorByPowerIndexExists(ctx, power)) } +func TestZeroPowerUnbonded(t *testing.T) { + ctx, _, keeper := CreateTestInput(t, false, 0) + pool := keeper.GetPool(ctx) + + // create a random pool + pool.LooseTokens = 10000 + pool.BondedTokens = 1234 + pool.BondedShares = sdk.NewRat(124) + pool.UnbondingTokens = 13934 + pool.UnbondingShares = sdk.NewRat(145) + pool.UnbondedTokens = 154 + pool.UnbondedShares = sdk.NewRat(1333) + keeper.SetPool(ctx, pool) + + // add a validator + validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) + validator, pool, delSharesCreated := 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.UpdateValidator(ctx, validator) + validator, found := keeper.GetValidator(ctx, addrVals[0]) + require.True(t, found) + require.Equal(t, int64(100), validator.PoolShares.Tokens(pool).RoundInt64(), "\nvalidator %v\npool %v", validator, pool) + + pool = keeper.GetPool(ctx) + + validator, pool, burned := validator.RemoveDelShares(pool, delSharesCreated) + require.Equal(t, int64(100), burned) + keeper.SetPool(ctx, pool) // update the pool + keeper.UpdateValidator(ctx, validator) // update the validator, kicking it out + pool = keeper.GetPool(ctx) + validator, found = keeper.GetValidator(ctx, addrVals[0]) + require.True(t, found) + require.Equal(t, sdk.ZeroRat(), validator.GetPower()) + require.Equal(t, sdk.Unbonded, validator.GetStatus()) +} + // This function tests UpdateValidator, GetValidator, GetValidatorsBonded, RemoveValidator func TestValidatorBasics(t *testing.T) { ctx, _, keeper := CreateTestInput(t, false, 1000) From e8b841080dd89cd4bb6596363502208bf8116d54 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 5 Jul 2018 20:21:56 +0200 Subject: [PATCH 2/7] Update .Slash() and testcase --- x/stake/keeper/slash.go | 6 +++++- x/stake/keeper/validator_test.go | 25 +++++++------------------ 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/x/stake/keeper/slash.go b/x/stake/keeper/slash.go index b12360fc3f..2f620d9d47 100644 --- a/x/stake/keeper/slash.go +++ b/x/stake/keeper/slash.go @@ -92,7 +92,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/validator_test.go b/x/stake/keeper/validator_test.go index c7607dd7e9..c037948da0 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -99,36 +99,25 @@ func TestZeroPowerUnbonded(t *testing.T) { // create a random pool pool.LooseTokens = 10000 - pool.BondedTokens = 1234 - pool.BondedShares = sdk.NewRat(124) - pool.UnbondingTokens = 13934 - pool.UnbondingShares = sdk.NewRat(145) - pool.UnbondedTokens = 154 - pool.UnbondedShares = sdk.NewRat(1333) keeper.SetPool(ctx, pool) // add a validator validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) - validator, pool, delSharesCreated := validator.AddTokensFromDel(pool, 100) + 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) keeper.UpdateValidator(ctx, validator) validator, found := keeper.GetValidator(ctx, addrVals[0]) require.True(t, found) require.Equal(t, int64(100), validator.PoolShares.Tokens(pool).RoundInt64(), "\nvalidator %v\npool %v", validator, pool) - pool = keeper.GetPool(ctx) - - validator, pool, burned := validator.RemoveDelShares(pool, delSharesCreated) - require.Equal(t, int64(100), burned) - keeper.SetPool(ctx, pool) // update the pool - keeper.UpdateValidator(ctx, validator) // update the validator, kicking it out - pool = keeper.GetPool(ctx) - validator, found = keeper.GetValidator(ctx, addrVals[0]) - require.True(t, found) - require.Equal(t, sdk.ZeroRat(), validator.GetPower()) - require.Equal(t, sdk.Unbonded, validator.GetStatus()) + // 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 From 6fa122804d5add2a6f32a56b695c6efb2522b1e7 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Thu, 5 Jul 2018 20:26:50 +0200 Subject: [PATCH 3/7] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b356641404..fedeb5a846 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -85,6 +85,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 * [gaia] Added self delegation for validators in the genesis creation * [lcd] tests now don't depend on raw json text * [stake] error strings lower case From ad3e123c824e19f9a0cfed1be9c53586256ca3c8 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Sat, 7 Jul 2018 01:49:17 +0200 Subject: [PATCH 4/7] Ignore slashes on nonexistent validators; update testcases --- x/stake/handler_test.go | 6 +++--- x/stake/keeper/slash.go | 4 +++- x/stake/keeper/slash_test.go | 25 ++++++++++++------------- 3 files changed, 18 insertions(+), 17 deletions(-) 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 2f620d9d47..263f55c1d8 100644 --- a/x/stake/keeper/slash.go +++ b/x/stake/keeper/slash.go @@ -35,8 +35,10 @@ func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, infractionHeight in // ref https://github.com/cosmos/cosmos-sdk/issues/1471 validator, found := k.GetValidatorByPubKey(ctx, pubkey) + // If not found, the validator must have been overslashed and removed - so we don't need to do anything if !found { - panic(fmt.Errorf("attempted to slash a nonexistent validator with address %s", pubkey.Address())) + logger.Info(fmt.Sprintf("Ignored attempt to slash a nonexistent validator with address %s", pubkey.Address())) + return } ownerAddress := validator.GetOwner() 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 From eafd1e492fbf5893d340577b40d90e23ebe4b4fa Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Sat, 7 Jul 2018 01:53:40 +0200 Subject: [PATCH 5/7] Update comments --- x/stake/keeper/slash.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/x/stake/keeper/slash.go b/x/stake/keeper/slash.go index 263f55c1d8..75cc753a21 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: @@ -35,8 +33,11 @@ func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, infractionHeight in // ref https://github.com/cosmos/cosmos-sdk/issues/1471 validator, found := k.GetValidatorByPubKey(ctx, pubkey) - // If not found, the validator must have been overslashed and removed - so we don't need to do anything if !found { + // 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.Info(fmt.Sprintf("Ignored attempt to slash a nonexistent validator with address %s", pubkey.Address())) return } From dbe7744b14cbe78a215b0cdb938ea453d10c7f97 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Sat, 7 Jul 2018 02:01:02 +0200 Subject: [PATCH 6/7] Simplify test slightly --- x/stake/keeper/validator_test.go | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index f42d8a7d1e..99cf6d4506 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -94,13 +94,10 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { } func TestZeroPowerUnbonded(t *testing.T) { - ctx, _, keeper := CreateTestInput(t, false, 0) + // initialize setup + ctx, _, keeper := CreateTestInput(t, false, 100) pool := keeper.GetPool(ctx) - // create a random pool - pool.LooseTokens = 10000 - keeper.SetPool(ctx, pool) - // add a validator validator := types.NewValidator(addrVals[0], PKs[0], types.Description{}) validator, pool, _ = validator.AddTokensFromDel(pool, 100) From 3630cde63e103a8f56e66102917a49c091d01164 Mon Sep 17 00:00:00 2001 From: Christopher Goes Date: Sat, 7 Jul 2018 02:20:37 +0200 Subject: [PATCH 7/7] Address PR comments --- x/stake/keeper/slash.go | 2 +- x/stake/keeper/validator_test.go | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/x/stake/keeper/slash.go b/x/stake/keeper/slash.go index 75cc753a21..f194d656cb 100644 --- a/x/stake/keeper/slash.go +++ b/x/stake/keeper/slash.go @@ -38,7 +38,7 @@ func (k Keeper) Slash(ctx sdk.Context, pubkey crypto.PubKey, infractionHeight in // 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.Info(fmt.Sprintf("Ignored attempt to slash a nonexistent validator with address %s", pubkey.Address())) + 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() diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 99cf6d4506..8320f259ce 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -93,7 +93,7 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { require.True(t, keeper.validatorByPowerIndexExists(ctx, power)) } -func TestZeroPowerUnbonded(t *testing.T) { +func TestSlashToZeroPowerRemoved(t *testing.T) { // initialize setup ctx, _, keeper := CreateTestInput(t, false, 100) pool := keeper.GetPool(ctx) @@ -105,15 +105,13 @@ func TestZeroPowerUnbonded(t *testing.T) { require.Equal(t, int64(100), validator.PoolShares.Tokens(pool).RoundInt64()) keeper.SetPool(ctx, pool) keeper.SetValidatorByPubKeyIndex(ctx, validator) - keeper.UpdateValidator(ctx, validator) - validator, found := keeper.GetValidator(ctx, addrVals[0]) - require.True(t, found) + 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]) + _, found := keeper.GetValidator(ctx, addrVals[0]) require.False(t, found) }