Ensure definition of IsMature (cosmos-sdk/x/staking/types/validator.go) bug free

[danwt]

In validatorUnbondingCanComplete the following code

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

https://github.com/cosmos/cosmos-sdk/blob/4cc285142ba73844e2c5929dedef59fa6db0f748/x/staking/keeper/unbonding.go#L326-L336

calls

// IsMature - is the current validator ready to unbond their coins
func (v Validator) IsMature(currentTime time.Time, currentHeight int64) bool {
	return v.UnbondingHeight < currentHeight && v.UnbondingTime.Before(currentTime)
}

https://github.com/cosmos/cosmos-sdk/blob/4cc285142ba73844e2c5929dedef59fa6db0f748/x/staking/types/validator.go#L180-L183

which has a different definition of maturity in UnbondAllMatureValidators

		if keyHeight <= blockHeight && (keyTime.Before(blockTime) || keyTime.Equal(blockTime)) {

https://github.com/cosmos/cosmos-sdk/blob/4cc285142ba73844e2c5929dedef59fa6db0f748/x/staking/keeper/validator.go#L420

and seems suspicious.

[mpoke]

Hi @danwt. This issue was already discussed on Slack. Copying here the conversation:

There are two events of interest here:

• Event A: A validator reaches maturity on the provider chain, which results in the validator’s status changing to unbonded if UnbondingOnHold == false. The check of maturity happens here, i.e., it checks if the unbonding time or height are <= than the current time or height.

• Event B: A validator reaches maturity on all consumer chains, which results in either setting UnbondingOnHold to false (i.e., unlocking unbonding) or, if the validator already reached maturity (on the provider), changing the status to unbonded.

The edge case entails equality in the check for maturity in case of event B.

Event A always happens in the EndBlock of the Staking module.

Event B is triggered by an external module calling UnbondingOpCanComplete(). Currently, this is called only by the provider CCV module when receiving the last ACK from the consumer chains; this means that currently event B is triggered during DeliverTx (i.e., before EndBlock). Thus, in case of equality, setting UnbondingOnHold to false should do the trick since the validator will be unbonded in EndBlock(). However, if at some point in the future some external module will call UnbondingOpCanComplete() after the EndBlock of the staking module, then the validator will never unbond.

So, there are two options to go about it:

• Leaving the check for maturity in UnbondingOpCanComplete() as it is now and add a comment clarifying this issue. May lead to potential bugs in the future. (This is how we decided to go about it)

• Change the check for maturity to include equality. This changes a bit the current behavior of Cosmos SDK. We need to make sure that between Event B (which unbonds a validator during DeliverTx) and Event A (which is the time when the SDK would have unbonded the validator) there are no other events that require the validator to be unbonding (e.g., Slash).

For the second option,

• both Slash and Jail are called in BeginBlock, so moving the validator to unbonded during DeliverTx has no impact on them.

• Delegate, Undelegate, and Redelegate seem to also not be affected, but this requires proper verification.

• ApplyAndReturnValidatorSetUpdates() seems to not be impacted.

[mpoke]

@danwt Do you agree?

[danwt]

Yep, it makes sense!

[mpoke]

The logic in #111 (comment) may be broken since UnbondingOpCanComplete is called also by StopConsumerChain

[danwt]

As of

• Add VSCMatured packets instead of acks #188

UnbondingCanComplete calls are deferred to EndBlock and provider.EndBlock occurs always after staking.Endblock

[mpoke]

Issue fixed by cosmos/cosmos-sdk#12537 cosmos/cosmos-sdk#12796