Make SDK staking hook changes robust to order of `staking.EndBlock` and `CanComplete`

[danwt]

In the current latest commit

https://github.com/cosmos/cosmos-sdk/blob/c783aea68fbd856c2b188b2d467a7fa5cb4df1e6/x/staking/keeper/unbonding.go#L332-L343

func (k Keeper) validatorUnbondingCanComplete(ctx sdk.Context, id uint64) (found bool, err error) {
    val, found := k.GetValidatorByUnbondingId(ctx, id)
    if !found {
        return false, nil
    }

    if !val.IsMature(ctx.BlockTime(), ctx.BlockHeight()) {
        val.UnbondingOnHold = false
        k.SetValidator(ctx, val)
    } else {
        // If unbonding is mature complete it
        val = k.UnbondingToUnbonded(ctx, val)
        if val.GetDelegatorShares().IsZero() {
            k.RemoveValidator(ctx, val.GetOperator())
        }

        k.DeleteUnbondingIndex(ctx, id)
    }

    return true, nil
}

liveness property for unbonding in all cases is dependent on the order of staking.EndBlock and (at least) validatorUnbondingCanComplete.

Since the CanComplete functions are SDK side and can be called by anyone we must either enforce a contract on ordering, and be OK with that, or change the design slightly to allow liveness in all cases.

The last commit is faulty anyway due to

• https://github.com/cosmos/interchain-security/issues/232

so we have to make some changes anyway.

I have a fix for #232 here https://github.com/danwt/cosmos-sdk/tree/7c48c6c11f6d57ae96ad94466078fdf68df0bf73 but it is explicity requiring a ordering contract (see lines)

// WARNING: precondition:
// Safety only guaranteed if this method is called AFTER staking.EndBlock
func (k Keeper) validatorUnbondingCanComplete(ctx sdk.Context, id uint64) (found bool, err error) {

so the job is not complete.

[mpoke]

Safety only guaranteed if this method is called AFTER staking.EndBlock

If we put such a comment, we should be more precise with the terminology. Safety is guaranteed regardless. If the function is called before and the validator unbonding is matured, then the validator moves to UNBONDED and the code panics when UnbondingToUnbonded is called in staking.EndBlock (see here).

[mpoke]

A solution could be to not do anything on the else branch here https://github.com/cosmos/cosmos-sdk/blob/c783aea68fbd856c2b188b2d467a7fa5cb4df1e6/x/staking/keeper/unbonding.go#L332-L343 and to remove from the ValidatorQueue only the validator addresses that can be unbonded instead of all the addresses that are supposed to unbond at a given time, i.e., https://github.com/cosmos/cosmos-sdk/blob/c783aea68fbd856c2b188b2d467a7fa5cb4df1e6/x/staking/keeper/validator.go#L445

[danwt]

Do we get this for free due to the new implemention that uses reference counting? I will check.

[mpoke]

Probably

[danwt]

I think this is fine.