Validators updated to zero power should be unbonded (deleted from the power store)

[cwgoes]

Presently, if a validator is set to zero power - through slashing, for example - but not through the unbond command (which does perform this check), the validator will be left in the power store with zero power (and possibly passed to Tendermint, if zero power would still put it in the top 100 validators).

Demonstrative failing testcase in #1514.

[cwgoes]

cc @rigelrozanski Do you think this check should be put in slashing (checking whenever we slash a validator) or in staking (checking in stakeKeeper.UpdateValidator)?

[cwgoes]

We can see two (!) such validators at the last block of gaia-6002 - http://gaia-6002.coinculture.net:46657/validators?height=721617.

[rigelrozanski]

I'd recommend that this check be added to staking and not slashing - although the problem may be triggered through slashing, fundamentally, staking is the interface to Tendermint and thus shouldn't allow these kinds of behaviours

[cwgoes]

I'd recommend that this check be added to staking and not slashing - although the problem may be triggered through slashing, fundamentally, staking is the interface to Tendermint and thus shouldn't allow these kinds of behaviours

Agreed.

edit: I think we can put it in the .Slash() method though.

[cwgoes]

Per discussion with @rigelrozanski, in the .Slash() method, if a validator is reduced to zero power, the validator will be deleted - just as if a validator is unbonded from and hits zero power. This allows us to retain the invariant that all stored validators have nonzero power.

[cwgoes]

Implemented in #1514.

[cwgoes]

Per discussion with @rigelrozanski, in the .Slash() method, if a validator is reduced to zero power, the validator will be deleted - just as if a validator is unbonded from and hits zero power. This allows us to retain the invariant that all stored validators have nonzero power.

The naive implementation of this means that validators we attempt to slash may or may not exist in the store (if a validator is slashed twice at 50%, for example, then again at any amount). We could get rid of that sanity check - and just ignore slashes to nonexistent validators - although we need to be careful that we still check for unbonding delegations / redelegations that might need to be slashed.

[rigelrozanski]

If a validator has been slashed 100% then all redelegations/unbondings should have also been slashed 100% at that moment - so shouldn't be a problem. This said I don't think we should remove the sanity check, but should check explicitly before attempting to retrieve the validator - if the validator doesn't exist we should make it clear with a special log message that the protocol has reached "overslashing" - in big red letters if possible

edit: haven't looked at the code, not totally certain on the sanity check your mentioning - just assumed it was during retrieving a non-existent validator?

[AdityaSripal]

Does this conflict with #1056 ?

[cwgoes]

Does this conflict with #1056 ?

Good question, we need to think about that.

[cwgoes]

Per suggestion from @rigelrozanski, we can keep the invariant that all stored validators have nonzero power by introducing a new Msg type to combine declare-candidacy-for-other and delegate-to-other (for #1056).