R4R: Fix Cliff Validator Update Bugs #1858
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -191,13 +191,13 @@ func (k Keeper) ClearTendermintUpdates(ctx sdk.Context) { | |
|
|
||
| //___________________________________________________________________________ | ||
|
|
||
| // Perform all the necessary steps for when a validator changes its power. This | ||
| // function updates all validator stores as well as tendermint update store. | ||
| // It may kick out validators if new validator is entering the bonded validator | ||
| // group. | ||
| // | ||
| // nolint: gocyclo | ||
| // TODO: Remove above nolint, function needs to be simplified! | ||
| func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) types.Validator { | ||
| store := ctx.KVStore(k.storeKey) | ||
| pool := k.GetPool(ctx) | ||
|
|
@@ -210,19 +210,26 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type | |
| cliffPower := k.GetCliffValidatorPower(ctx) | ||
|
|
||
| switch { | ||
| // if the validator is already bonded and the power is increasing, we need | ||
| // perform the following: | ||
| // a) update Tendermint | ||
| // b) check if the cliff validator needs to be updated | ||
| case powerIncreasing && !validator.Revoked && | ||
| (oldFound && oldValidator.Status == sdk.Bonded): | ||
|
|
||
| bz := k.cdc.MustMarshalBinary(validator.ABCIValidator()) | ||
| store.Set(GetTendermintUpdatesKey(validator.Owner), bz) | ||
|
|
||
| if cliffPower != nil { | ||
| k.updateCliffValidator(ctx, validator) | ||
| } | ||
|
|
||
| // if is a new validator and the new power is less than the cliff validator | ||
| case cliffPower != nil && !oldFound && | ||
| bytes.Compare(valPower, cliffPower) == -1: //(valPower < cliffPower | ||
| // skip to completion | ||
|
|
||
| // if was unbonded and the new power is less than the cliff validator | ||
| case cliffPower != nil && | ||
| (oldFound && oldValidator.Status == sdk.Unbonded) && | ||
| bytes.Compare(valPower, cliffPower) == -1: //(valPower < cliffPower | ||
|
|
@@ -232,11 +239,10 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type | |
| // a) not-bonded and now has power-rank greater than cliff validator | ||
| // b) bonded and now has decreased in power | ||
| default: | ||
| // update the validator set for this validator | ||
| updatedVal, updated := k.UpdateBondedValidators(ctx, validator) | ||
| if updated { | ||
| // the validator has changed bonding status | ||
| validator = updatedVal | ||
| break | ||
| } | ||
|
|
@@ -246,13 +252,54 @@ func (k Keeper) UpdateValidator(ctx sdk.Context, validator types.Validator) type | |
| bz := k.cdc.MustMarshalBinary(validator.ABCIValidator()) | ||
| store.Set(GetTendermintUpdatesKey(validator.Owner), bz) | ||
| } | ||
| } | ||
|
|
||
| k.SetValidator(ctx, validator) | ||
| return validator | ||
| } | ||
|
|
||
| // updateCliffValidator determines if the current cliff validator needs to be | ||
|
There was a problem hiding this comment. I like to call this, "maybeUpdateCliffValidator", which implies that it only happens conditionally. There was a problem hiding this comment. ehhhh I'm not totally sure about There was a problem hiding this comment. Not of the idea of putting Use of this type of language for func naming would be cool to document in the There was a problem hiding this comment. Agreed more appropriate nomenclature could be defined and standardized, but I'm not convinced There was a problem hiding this comment.
There was a problem hiding this comment. (then we should comment explaining what the postcondition is) There was a problem hiding this comment. At first it seemed to me, Now that I think about it more, it kinda makes sense. There was a problem hiding this comment. @alexanderbez Did we reach consensus on updating the name to |
||
| // updated or swapped. | ||
| func (k Keeper) updateCliffValidator(ctx sdk.Context, affectedVal types.Validator) { | ||
| cliffAddr := sdk.AccAddress(k.GetCliffValidator(ctx)) | ||
| if !bytes.Equal(cliffAddr, affectedVal.Owner) { | ||
| return | ||
| } | ||
|
|
||
| store := ctx.KVStore(k.storeKey) | ||
| pool := k.GetPool(ctx) | ||
|
|
||
| // NOTE: We get the power via affectedVal since the store (by power key) | ||
| // has yet to be updated. | ||
| affectedValPower := affectedVal.GetPower() | ||
|
|
||
| // Create a validator iterator ranging from smallest to largest in power | ||
| // where only the smallest by power is needed. | ||
| iterator := sdk.KVStorePrefixIterator(store, ValidatorsByPowerIndexKey) | ||
|
There was a problem hiding this comment. Hmm I don't think this is the correct logic. The power store includes (a) many validator candidates which are not validators (not in the top hundred by stake) and (b) revoked validators, neither of which should be set as the cliff validator. The cliff validator is simply the "lowest-ranked bonded validator by stake" - but this logic reads the "lowest-ranked validator candidate", which is not necessarily the same. For the case of "same cliff validator, increased power" this rough logic is fine. For the case of "cliff validator increased power, so now no longer cliff validator" I think we need to seek from the old cliff validator power store key and find the next-highest stake validator (which might become the cliff validator). There was a problem hiding this comment. I see. It did not occur to me that the power store held those validators as well. I'll need to refactor this and add test cases where we also have revoked validators and candidates which are not validators. |
||
| if !iterator.Valid() { | ||
| panic("invalid iterator for validator power store") | ||
| } | ||
|
|
||
| ownerAddr := iterator.Value() | ||
| cliffValByPower, found := k.GetValidator(ctx, ownerAddr) | ||
| if !found { | ||
| panic(fmt.Sprintf("validator record not found for address: %v\n", ownerAddr)) | ||
| } | ||
|
|
||
| iterator.Close() | ||
|
|
||
| if bytes.Equal(affectedVal.Owner, cliffValByPower.Owner) { | ||
| // The affected validator remains the cliff validator, however, since | ||
| // the store does not contain the new power, set the new cliff | ||
| // validator to the affected validator. | ||
| k.setCliffValidator(ctx, affectedVal, pool) | ||
|
There was a problem hiding this comment. We just need to update the power, right? I think There was a problem hiding this comment. Correct! I suppose we can save a write, huh? |
||
| } else if affectedValPower.GT(cliffValByPower.GetPower()) { | ||
| // The affected validator no longer remains the cliff validator as it's | ||
| // power is greater than the new current cliff validator. | ||
| k.setCliffValidator(ctx, cliffValByPower, pool) | ||
| } | ||
| } | ||
|
|
||
| func (k Keeper) updateForRevoking(ctx sdk.Context, oldFound bool, oldValidator, newValidator types.Validator) types.Validator { | ||
| if newValidator.Revoked && oldFound && oldValidator.Status == sdk.Bonded { | ||
| newValidator = k.unbondValidator(ctx, newValidator) | ||
|
|
@@ -317,8 +364,9 @@ func (k Keeper) updateValidatorPower(ctx sdk.Context, oldFound bool, oldValidato | |
| // | ||
| // nolint: gocyclo | ||
| // TODO: Remove the above golint | ||
| func (k Keeper) UpdateBondedValidators( | ||
| ctx sdk.Context, affectedValidator types.Validator, | ||
| ) (updatedVal types.Validator, updated bool) { | ||
|
There was a problem hiding this comment. better indentation: func (k Keeper) UpdateBondedValidators(
ctx sdk.Context, affectedValidator types.Validator) (
updatedVal types.Validator, updated bool) {There was a problem hiding this comment. Tbh, I don't necessarily agree here because it's hard for me to immediately see the return values (and their potential names) due to the location of the opening parenthesis. (e.g. immediate glance seems to show it has no returns). But this is your baby and I'd like to keep in-line with what you've written and the team's standards, so I can update it 😄 |
||
|
|
||
| store := ctx.KVStore(k.storeKey) | ||
|
|
||
|
|
@@ -328,7 +376,8 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, | |
| var validator, validatorToBond types.Validator | ||
| newValidatorBonded := false | ||
|
|
||
| // create a validator iterator ranging from largest to smallest by power | ||
| iterator := sdk.KVStoreReversePrefixIterator(store, ValidatorsByPowerIndexKey) | ||
| for { | ||
| if !iterator.Valid() || bondedValidatorsCount > int(maxValidators-1) { | ||
| break | ||
|
|
@@ -354,6 +403,7 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, | |
| validatorToBond = validator | ||
| newValidatorBonded = true | ||
| } | ||
|
|
||
| bondedValidatorsCount++ | ||
|
|
||
| // sanity check | ||
|
|
@@ -363,6 +413,7 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, | |
|
|
||
| iterator.Next() | ||
| } | ||
|
|
||
| iterator.Close() | ||
|
|
||
| // clear or set the cliff validator | ||
|
|
@@ -372,17 +423,17 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, | |
| k.clearCliffValidator(ctx) | ||
| } | ||
|
|
||
| // swap the cliff validator for a new validator if the affected validator | ||
| // was bonded | ||
| if newValidatorBonded { | ||
| // unbond the cliff validator | ||
| if oldCliffValidatorAddr != nil { | ||
| cliffVal, found := k.GetValidator(ctx, oldCliffValidatorAddr) | ||
| if !found { | ||
| panic(fmt.Sprintf("validator record not found for address: %v\n", oldCliffValidatorAddr)) | ||
| } | ||
|
|
||
| k.unbondValidator(ctx, cliffVal) | ||
| } | ||
|
|
||
| // bond the new validator | ||
|
|
@@ -391,6 +442,7 @@ func (k Keeper) UpdateBondedValidators(ctx sdk.Context, | |
| return validator, true | ||
| } | ||
| } | ||
|
|
||
| return types.Validator{}, false | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from the godoc of k.updateCliffVlaidator, it seems that we should have the cliffPower != nil check in there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I totally agree, I think have the explicit check here makes the context more clear opposed to always calling
updateCliffValidator.