test: add tests for cons pubkey rotation #18965
Conversation
| // exceedsMaxRotations returns true if the key rotations exceed the limit, currently we are limiting one rotation for unbonding period. | ||
| func (k Keeper) exceedsMaxRotations(ctx context.Context, valAddr sdk.ValAddress) error { | ||
| // ExceedsMaxRotations returns true if the key rotations exceed the limit, currently we are limiting one rotation for unbonding period. | ||
| func (k Keeper) ExceedsMaxRotations(ctx context.Context, valAddr sdk.ValAddress) error { |
There was a problem hiding this comment.
exporting this to use in sims, hope this doesn't effect much since it is just reading the state.
WalkthroughThe overarching change involves updating the staking and slashing modules to handle validator consensus public key rotation. Method names and function calls have been updated for consistency, and new error handling has been added to accommodate changes in validators' consensus addresses. The test suite has been expanded with new imports, additional test functions for key rotation and infraction handling, and adjustments to initialization code. A new operation for rotating consensus public keys has been introduced to the simulation framework. 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}
| if err != nil { | ||
| // if the validator not found try to find it in the map of `OldToNewConsKeyMap` because validator may've rotated it's key. | ||
| if !errors.Is(err, collections.ErrNotFound) { | ||
| return types.Validator{}, err | ||
| } | ||
|
|
||
| newConsAddr, err := k.OldToNewConsKeyMap.Get(ctx, consAddr) | ||
| newConsAddr, err := k.OldToNewConsKeyMap.Get(ctx, consAddr.Bytes()) | ||
| if err != nil { | ||
| if errors.Is(err, collections.ErrNotFound) { | ||
| return types.Validator{}, types.ErrNoValidatorFound | ||
| } | ||
| return types.Validator{}, err | ||
| } | ||
|
|
||
| operatorAddr, err := k.ValidatorByConsensusAddress.Get(ctx, newConsAddr) | ||
| if err != nil { | ||
| if errors.Is(err, collections.ErrNotFound) { | ||
| return types.Validator{}, types.ErrNoValidatorFound | ||
| } | ||
| return types.Validator{}, err | ||
| } | ||
|
|
||
| opAddr = newConsAddr | ||
| opAddr = operatorAddr |
There was a problem hiding this comment.
The logic for handling key rotation in GetValidatorByConsAddr seems to be correct. However, there is a potential issue with the variable naming and error handling:
- The variable
opAddris used to store the result fromValidatorByConsensusAddress.Get, which is expected to be an operator address, but the nameopAddrcould be more descriptive. - The error handling could be more concise by returning early if the new consensus address is not found, avoiding deep nesting.
Consider the following improvements for clarity and maintainability:
func (k Keeper) GetValidatorByConsAddr(ctx context.Context, consAddr sdk.ConsAddress) (validator types.Validator, err error) {
- opAddr, err := k.ValidatorByConsensusAddress.Get(ctx, consAddr)
+ operatorAddr, err := k.ValidatorByConsensusAddress.Get(ctx, consAddr)
if err != nil {
if !errors.Is(err, collections.ErrNotFound) {
return types.Validator{}, err
}
- newConsAddr, err := k.OldToNewConsKeyMap.Get(ctx, consAddr.Bytes())
+ newConsAddr, findErr := k.OldToNewConsKeyMap.Get(ctx, consAddr.Bytes())
if findErr != nil {
if errors.Is(findErr, collections.ErrNotFound) {
- return types.Validator{}, types.ErrNoValidatorFound
+ return types.Validator{}, findErr
}
return types.Validator{}, findErr
}
- operatorAddr, err := k.ValidatorByConsensusAddress.Get(ctx, newConsAddr)
+ operatorAddr, getErr := k.ValidatorByConsensusAddress.Get(ctx, newConsAddr)
if getErr != nil {
if errors.Is(getErr, collections.ErrNotFound) {
return types.Validator{}, types.ErrNoValidatorFound
}
return types.Validator{}, getErr
}
- opAddr = operatorAddr
}
- if opAddr == nil {
+ if operatorAddr == nil {
return types.Validator{}, types.ErrNoValidatorFound
}
- return k.GetValidator(ctx, opAddr)
+ return k.GetValidator(ctx, operatorAddr)
}Committable suggestion
❗ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| if err != nil { | |
| // if the validator not found try to find it in the map of `OldToNewConsKeyMap` because validator may've rotated it's key. | |
| if !errors.Is(err, collections.ErrNotFound) { | |
| return types.Validator{}, err | |
| } | |
| newConsAddr, err := k.OldToNewConsKeyMap.Get(ctx, consAddr) | |
| newConsAddr, err := k.OldToNewConsKeyMap.Get(ctx, consAddr.Bytes()) | |
| if err != nil { | |
| if errors.Is(err, collections.ErrNotFound) { | |
| return types.Validator{}, types.ErrNoValidatorFound | |
| } | |
| return types.Validator{}, err | |
| } | |
| operatorAddr, err := k.ValidatorByConsensusAddress.Get(ctx, newConsAddr) | |
| if err != nil { | |
| if errors.Is(err, collections.ErrNotFound) { | |
| return types.Validator{}, types.ErrNoValidatorFound | |
| } | |
| return types.Validator{}, err | |
| } | |
| opAddr = newConsAddr | |
| opAddr = operatorAddr | |
| if err != nil { | |
| // if the validator not found try to find it in the map of `OldToNewConsKeyMap` because validator may've rotated it's key. | |
| if !errors.Is(err, collections.ErrNotFound) { | |
| return types.Validator{}, err | |
| } | |
| newConsAddr, findErr := k.OldToNewConsKeyMap.Get(ctx, consAddr.Bytes()) | |
| if findErr != nil { | |
| if errors.Is(findErr, collections.ErrNotFound) { | |
| return types.Validator{}, findErr | |
| } | |
| return types.Validator{}, findErr | |
| } | |
| operatorAddr, getErr := k.ValidatorByConsensusAddress.Get(ctx, newConsAddr) | |
| if getErr != nil { | |
| if errors.Is(getErr, collections.ErrNotFound) { | |
| return types.Validator{}, types.ErrNoValidatorFound | |
| } | |
| return types.Validator{}, getErr | |
| } | |
| } | |
| if operatorAddr == nil { | |
| return types.Validator{}, types.ErrNoValidatorFound | |
| } | |
| return k.GetValidator(ctx, operatorAddr) |
julienrbrt
changed the title
…-sdk into ap/cons-key-rotation-tests
| func (s *KeeperTestSuite) TestPerformConsensusPubKeyUpdate() { | ||
| ctx, slashingKeeper := s.ctx, s.slashingKeeper | ||
|
|
||
| require := s.Require() | ||
|
|
||
| pks := simtestutil.CreateTestPubKeys(500) | ||
|
|
||
| oldConsAddr := sdk.ConsAddress(pks[0].Address()) | ||
| newConsAddr := sdk.ConsAddress(pks[1].Address()) | ||
|
|
||
| newInfo := slashingtypes.NewValidatorSigningInfo( | ||
| oldConsAddr.String(), | ||
| int64(4), | ||
| int64(3), | ||
| time.Unix(2, 0).UTC(), | ||
| false, | ||
| int64(10), | ||
| ) | ||
|
|
||
| err := slashingKeeper.ValidatorSigningInfo.Set(ctx, oldConsAddr, newInfo) | ||
| require.NoError(err) | ||
|
|
||
| s.stakingKeeper.EXPECT().ValidatorIdentifier(gomock.Any(), oldConsAddr).Return(oldConsAddr, nil) | ||
| err = slashingKeeper.SetMissedBlockBitmapValue(ctx, oldConsAddr, 10, true) | ||
| require.NoError(err) | ||
|
|
||
| err = slashingKeeper.Hooks().AfterConsensusPubKeyUpdate(ctx, pks[0], pks[1], sdk.Coin{}) | ||
| require.NoError(err) | ||
|
|
||
| // check pubkey relation is set properly | ||
| savedPubKey, err := slashingKeeper.GetPubkey(ctx, newConsAddr.Bytes()) | ||
| require.NoError(err) | ||
| require.Equal(savedPubKey, pks[1]) | ||
|
|
||
| // check validator SigningInfo is set properly to new consensus pubkey | ||
| signingInfo, err := slashingKeeper.ValidatorSigningInfo.Get(ctx, newConsAddr) | ||
| require.NoError(err) | ||
| require.Equal(signingInfo, newInfo) | ||
|
|
||
| // missed blocks maps to old cons key only since there is a identifier added to get the missed blocks using the new cons key. | ||
| missedBlocks, err := slashingKeeper.GetValidatorMissedBlocks(ctx, oldConsAddr) | ||
| require.NoError(err) | ||
|
|
||
| require.Len(missedBlocks, 1) | ||
| } |
There was a problem hiding this comment.
The test TestPerformConsensusPubKeyUpdate appears to be comprehensive, testing the update of a consensus public key and its effects. It would be beneficial to include negative test cases to ensure robustness, such as what happens if the update fails or if an invalid public key is provided.
| func (s *SimTestSuite) TestSimulateRotateConsPubKey() { | ||
| require := s.Require() | ||
| blockTime := time.Now().UTC() | ||
| ctx := s.ctx.WithHeaderInfo(header.Info{Time: blockTime}) | ||
|
|
||
| _ = s.getTestingValidator2(ctx) | ||
|
|
||
| // begin a new block | ||
| _, err := s.app.FinalizeBlock(&abci.RequestFinalizeBlock{Height: s.app.LastBlockHeight() + 1, Hash: s.app.LastCommitID().Hash, Time: blockTime}) | ||
| require.NoError(err) | ||
|
|
||
| // execute operation | ||
| op := simulation.SimulateMsgRotateConsPubKey(s.txConfig, s.accountKeeper, s.bankKeeper, s.stakingKeeper) | ||
| operationMsg, futureOperations, err := op(s.r, s.app.BaseApp, ctx, s.accounts, "") | ||
| require.NoError(err) | ||
|
|
||
| var msg types.MsgRotateConsPubKey | ||
| err = proto.Unmarshal(operationMsg.Msg, &msg) | ||
| require.NoError(err) | ||
|
|
||
| require.True(operationMsg.OK) | ||
| require.Equal(sdk.MsgTypeURL(&types.MsgRotateConsPubKey{}), sdk.MsgTypeURL(&msg)) | ||
| require.Equal("cosmosvaloper1p8wcgrjr4pjju90xg6u9cgq55dxwq8j7epjs3u", msg.ValidatorAddress) | ||
| require.Len(futureOperations, 0) | ||
| } |
There was a problem hiding this comment.
The test TestSimulateRotateConsPubKey correctly simulates the rotation of a consensus public key. It's important to ensure that the test covers various scenarios, including failure cases and edge conditions, to fully validate the rotation logic.
| func (s *SimTestSuite) getTestingValidator2(ctx sdk.Context) types.Validator { | ||
| val := s.getTestingValidator0(ctx) | ||
| val.Status = types.Bonded | ||
| s.Require().NoError(s.stakingKeeper.SetValidator(ctx, val)) | ||
| s.Require().NoError(s.stakingKeeper.SetValidatorByConsAddr(ctx, val)) | ||
| return val |
There was a problem hiding this comment.
The helper function getTestingValidator2 is used to set up a bonded validator for testing. It's crucial to ensure that this setup aligns with the expected preconditions of the tests that use it. If there are specific conditions under which a validator is expected to rotate their consensus public key, those should be reflected in the test setup.
Co-authored-by: Aleksandr Bezobchuk <[email protected]>
Description
ref: #18141
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...