Cosmos-sdk: Expire unbonding delegations etc. based on combined height/time unbonding period

[cwgoes]

Right now, if I understand correctly, the x/staking module expires entries based solely on time:

// Returns a concatenated list of all the timeslices before currTime, and deletes the timeslices from the queue
func (k Keeper) GetAllMatureValidatorQueue(ctx sdk.Context, currTime time.Time) (matureValsAddrs []sdk.ValAddress) {
    // gets an iterator for all timeslices from time 0 until the current Blockheader time
    validatorTimesliceIterator := k.ValidatorQueueIterator(ctx, ctx.BlockHeader().Time)
    defer validatorTimesliceIterator.Close()

    for ; validatorTimesliceIterator.Valid(); validatorTimesliceIterator.Next() {
        timeslice := sdk.ValAddresses{}
        k.cdc.MustUnmarshalBinaryBare(validatorTimesliceIterator.Value(), &timeslice)

        matureValsAddrs = append(matureValsAddrs, timeslice.Addresses...)
    }

    return matureValsAddrs
}

This mis-matches the new unbonding period, which is a combination of height & time, see the evidence module:

cp := ctx.ConsensusParams()
if cp != nil && cp.Evidence != nil {
        if ageDuration > cp.Evidence.MaxAgeDuration && ageBlocks > cp.Evidence.MaxAgeNumBlocks {
        logger.Info(
            "ignored equivocation; evidence too old",
            "validator", consAddr,
            "infraction_height", infractionHeight,
            "max_age_num_blocks", cp.Evidence.MaxAgeNumBlocks,
            "infraction_time", infractionTime,
            "max_age_duration", cp.Evidence.MaxAgeDuration,
        )
        return
    }
}

Instead, we need to only remove unbonding validators, unbonding delegations, etc. once both height & time have passed.

[cwgoes]

cc @alexanderbez @clevinson

[cwgoes]

We may not actually use GetAllMatureValidatorQueue, but the other such functions also appear to use time, e.g. this.

[alexanderbez]

WRT to the context of _why_ we use a hybrid of both time and height in the x/evidence module, this makes sense. Even if it's very unlikely to be exploited, there is no harm in taking this approach withing staking -- correct me if I'm wrong.

WDYT @ebuchman?

[ebuchman]

Right, we need the evidence to match the unbonding. Eg. if the timestamp was fast forwarded, and evidence is still valid according to the evidence module because the height limit has not passed, but the validator is already finished unbonding because it was only based on timestamp, then the allegedly valid evidence would be useless in punishing the validator. So the conditions for validators being fully unbonded needs to match the evidence validity conditions, which means they should require both the height and time to pass.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[alexanderbez]

@marbar3778 this issue was marked stale even though it already had security label attached. Are you sure the stale config is correct? Do we need to add a config entry for issues and not just PRs?