fix(x/staking): Refactor GetLastValidators #19226
Conversation
WalkthroughThe updates to the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| @@ -542,14 +543,14 @@ Every module contains its own CHANGELOG.md. Please refer to the module you are i | |||
|
|
|||
| ### Bug Fixes | |||
|
|
|||
| * [#19106](https://github.com/cosmos/cosmos-sdk/pull/19106) Allow empty public keys when setting signatures. Public keys aren't needed for every transaction. | |||
| * [#19106](https://github.com/cosmos/cosmos-sdk/pull/19106) Allow empty public keys when setting signatures. Public keys aren't needed for every transaction. | |||
There was a problem hiding this comment.
The entry for the server module fix mentions a consensus failure but does not specify the nature of the fix. It would be beneficial to include a brief description of how the issue was resolved.
Consider adding a short description of the fix applied to address the consensus failure issue.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| validators = make([]types.Validator, maxValidators) | ||
|
|
||
| i := 0 | ||
| validators = make([]types.Validator, maxValidators) | ||
|
|
||
| err = k.LastValidatorPower.Walk(ctx, nil, func(key []byte, _ gogotypes.Int64Value) (bool, error) { | ||
| // sanity check | ||
| // Note, we do NOT error here as the MaxValidators param may change via on-chain | ||
| // governance. In cases where the param is increased, this case should never | ||
| // be hit. In cases where the param is decreased, we will simply not return | ||
| // the remainder of the validator set, as the ApplyAndReturnValidatorSetUpdates | ||
| // call should ensure the validators past the cliff will be moved to the | ||
| // unbonding set. | ||
| if i >= int(maxValidators) { | ||
| return true, fmt.Errorf("more validators than maxValidators found") | ||
| return true, nil | ||
| } | ||
|
|
||
| validator, err := k.GetValidator(ctx, key) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [388-408]
The refactored GetLastValidators function now initializes the validators slice after retrieving the maxValidators value from storage, which is a logical change to handle dynamic validator set sizes. However, the function does not handle the case where maxValidators could be zero, which would lead to an unnecessary allocation of a slice. It would be more efficient to check if maxValidators is zero and return an empty slice immediately.
- validators = make([]types.Validator, maxValidators)
+ if maxValidators == 0 {
+ return []types.Validator{}, nil
+ }
+ validators = make([]types.Validator, maxValidators)
julienrbrt
added
backport/v0.47.x
(cherry picked from commit a69836b) # Conflicts: # CHANGELOG.md # x/staking/keeper/validator.go
(cherry picked from commit a69836b) # Conflicts: # CHANGELOG.md # x/staking/keeper/validator.go
| if i >= int(maxValidators) { | ||
| return true, fmt.Errorf("more validators than maxValidators found") | ||
| return true, nil |
There was a problem hiding this comment.
i dont think this is correct, we should add a test case to make sure the validators have entered the unbond queue. We could leave the state machine in a weird limbo if they are still in the bonded queue
Description
Changelog
Refactor
GetLastValidatorsto not error in the sanity check when the cursor moves beyondMaxValidators. We remove the error and return nil so we can handle cases where the validator set is decreased.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...