Validator set updates can skip current validators
Description
When calculating the validator updates for the current block, the ApplyAndReturnValidatorSetUpdates function iterates through maxValidators to find any validators that have changed voting power.
func (k Keeper) ApplyAndReturnValidatorSetUpdates(ctx context.Context) ([]abci.ValidatorUpdate, error) {
last, err := k.getLastValidatorsByAddr(ctx)
if err != nil {
return nil, err
}
updates := []abci.ValidatorUpdate{}
maxValidators, err := k.MaxValidators(ctx)
if err != nil {
return nil, err
}
validators, err := k.GetValidators(ctx, maxValidators)
if err != nil {
return nil, err
}
for _, validator := range validators {
valAddr, err := k.validatorAddressCodec.StringToBytes(validator.GetOperator())
if err != nil {
return nil, err
}
oldPower, found := last[validator.GetOperator()]
newPower := validator.ConsensusPower()
// zero power validator removed from validator set
if newPower <= 0 {
continue
}
if !found || oldPower != newPower {
updates = append(updates, validator.ABCIValidatorUpdate())
if err := k.SetLastValidatorPower(ctx, valAddr, newPower); err != nil {
return nil, err
}
}
delete(last, validator.GetOperator())
}
noLongerBonded, err := sortNoLongerBonded(last, k.validatorAddressCodec)
if err != nil {
return nil, err
}Any validators that remain in the last map are considered to now be unbonded.
The issue is that GetValidators is not sorted, so there is a chance that a currently bonded validator will not be part of the maxValidators returned. This will cause them to be marked as no longer bonded, and they will try to be removed:
for _, valAddrBytes := range noLongerBonded {
validator := k.mustGetValidator(ctx, sdk.ValAddress(valAddrBytes))
if validator.ConsPower > 0 {
return nil, errors.New("deleting validator cannot have positive power")
}
valAddr, err := k.validatorAddressCodec.StringToBytes(validator.GetOperator())
if err != nil {
return nil, err
}
if err := k.RemoveValidator(ctx, valAddr); err != nil {
return nil, err
}In this case, the check for validator.ConsPower > 0 would fail, and an error would be returned.
Impact
Since ApplyAndReturnValidatorSetUpdates is called in the EndBlocker, if an error is returned, it will cause a consensus failure and halt the chain.
Recommendations
All the validators should be checked, or GetValidators should be sorted by consensus power before iterating through them.