feat(x/staking): implement RotateConsPubKey method
#18188
Conversation
atheeshp
changed the title
RotateConsPubKey method
WalkthroughWalkthroughThe changes revolve around the introduction and management of consensus public key rotation for validators within the Cosmos SDK's staking module. New functions and data structures have been added to handle rotation history, enforce rotation limits, and manage fees associated with key rotation. The protobuf definitions have been updated to reflect these changes, and mock implementations have been adjusted to accommodate the new context parameters and methods. Changes
TipsChat with CodeRabbit Bot (
|
|
@atheeshp your pull request is missing a changelog! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Configuration used: CodeRabbit UI
Files ignored due to filter (2)
- store/go.mod
- store/go.sum
Files selected for processing (12)
- baseapp/testutil/mock/mocks.go (1 hunks)
- proto/cosmos/staking/v1beta1/tx.proto (1 hunks)
- store/mock/cosmos_cosmos_db_DB.go (1 hunks)
- x/slashing/testutil/expected_keepers_mocks.go (1 hunks)
- x/staking/keeper/cons_pubkey.go (1 hunks)
- x/staking/keeper/keeper.go (2 hunks)
- x/staking/keeper/msg_server.go (2 hunks)
- x/staking/testutil/expected_keepers_mocks.go (1 hunks)
- x/staking/types/codec.go (1 hunks)
- x/staking/types/errors.go (1 hunks)
- x/staking/types/expected_keepers.go (1 hunks)
- x/staking/types/keys.go (1 hunks)
Files skipped from review due to trivial changes (6)
- proto/cosmos/staking/v1beta1/tx.proto
- store/mock/cosmos_cosmos_db_DB.go
- x/slashing/testutil/expected_keepers_mocks.go
- x/staking/types/codec.go
- x/staking/types/errors.go
- x/staking/types/expected_keepers.go
Additional comments: 9
x/staking/types/keys.go (1)
- 62-66: The new keys introduced are for managing the rotation of consensus public keys. They are used to track the rotation history, set the unbonding period time on each rotation, restrict the validator's next rotation within the waiting (unbonding) period, and map rotated consensus addresses to new consensus addresses. Ensure that these keys are used appropriately in the rest of the codebase and that they are unique to avoid any potential conflicts.
x/staking/testutil/expected_keepers_mocks.go (1)
- 230-242: The new method
SendCoinsFromAccountToModuleand its corresponding recorder method have been added to theMockBankKeeperstruct. This method is used to mock the transfer of coins from an account to a module. Ensure that this method is used correctly in the tests and that the expected behavior is properly defined.baseapp/testutil/mock/mocks.go (4)
210-215: The method signature of
SelectTxForProposalhas been updated to include acontext.Contextparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.218-221: The method signature of
SelectTxForProposalin theMockTxSelectorMockRecorderhas been updated to include acontext.Contextparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.224-229: The method signature of
SelectedTxshas been updated to include acontext.Contextparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.232-234: The method signature of
SelectedTxsin theMockTxSelectorMockRecorderhas been updated to include acontext.Contextparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.x/staking/keeper/keeper.go (2)
96-105: The new fields added to the
Keeperstruct are related to the rotation of consensus public keys. These fields are used to store the rotation history of consensus public keys by validator and by block height, to restrict the next rotation of a validator's consensus key within the unbonding period, to set the unbonding period time on each rotation, and to map rotated consensus addresses to new consensus addresses. These changes are in line with the PR summary and seem to be necessary for the new functionality being introduced.239-278: The
NewKeeperfunction has been updated to initialize the new fields added to theKeeperstruct. The key formats for these fields are specified and the corresponding collections are created using thecollections.NewMapandcollections.NewItemfunctions. The key formats and collection types seem to be appropriate for the data being stored. However, it's important to ensure that these changes are compatible with the rest of the codebase and that the new keys do not conflict with existing keys.x/staking/keeper/msg_server.go (1)
- 19-19: The import
distrtypes "github.com/cosmos/cosmos-sdk/x/distribution/types"has been added. Ensure that this import is used in the new code and is necessary.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (5)
- x/evidence/go.mod
- x/feegrant/go.mod
- x/gov/go.mod
- x/group/go.mod
- x/nft/go.mod
Files selected for processing (1)
- x/staking/keeper/msg_server.go (2 hunks)
Additional comments: 1
x/staking/keeper/msg_server.go (1)
- 609-681: The
RotateConsPubKeyfunction has been implemented with the following operations:
- Extracts the public key from
msg.NewPubkeyand checks its type.- Checks if the new public key is already used by a validator.
- Converts the validator's address from string to bytes.
- Retrieves the validator using the converted address.
- Checks the status of the validator.
- Checks if the validator is exceeding the maximum allowed consensus public key rotations within the unbonding period.
- Checks if the signing account has enough balance to pay the key rotation fee.
- Sends the key rotation fee from the signing account to the module.
- Overwrites the new public key in
validator.ConsPubKey.- Adds a consensus public key rotation history record.
The function returns an error if any of the above operations fail. The error messages are descriptive and should help in debugging any issues.
However, there are a few potential issues and improvements that could be made:
On line 620, the function retrieves a validator using the new consensus address but does not check for errors. It only checks if the validator is nil. It would be better to also handle any errors that might occur during this operation.
On line 625, the function converts the validator's address from string to bytes. However, the validator could be nil at this point if it was not found in the previous operation. This could lead to a nil pointer dereference. It would be better to move this operation after the check for nil validator on line 631.
On line 657, the function sends coins from the signing account to the module. However, it does not check if the signing account has enough balance before doing this. It would be better to add a check for this before attempting to send the coins.
On line 663, the function asserts that the validator is of type
types.Validator. However, this assertion could fail and cause a panic. It would be better to handle this error gracefully.On line 669, the function sets the consensus public key rotation history. However, it does not check if the operation was successful. It would be better to add a check for this.
The function does not seem to update the validator's consensus public key after rotating it. It would be better to add this operation.
609: func (k msgServer) RotateConsPubKey(goCtx context.Context, msg *types.MsgRotateConsPubKey) (res *types.MsgRotateConsPubKeyResponse, err error) { 610: ctx := sdk.UnwrapSDKContext(goCtx) 611: 612: pk, ok := msg.NewPubkey.GetCachedValue().(cryptotypes.PubKey) 613: if !ok { 614: return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "expecting cryptotypes.PubKey, got %T", pk) 615: } 616: 617: newConsAddr := sdk.ConsAddress(pk.Address()) 618: 619: // checks if NewPubKey is not duplicated on ValidatorsByConsAddr 620: validator, err := k.Keeper.ValidatorByConsAddr(ctx, newConsAddr) 621: if err != nil { 622: return nil, err 623: } 624: if validator != nil { 625: return nil, types.ErrConsensusPubKeyAlreadyUsedForAValidator 626: } 627: 628: valAddr, err := k.validatorAddressCodec.StringToBytes(validator.GetOperator()) 629: if err != nil { 630: return nil, err 631: } 632: 633: validator, err = k.Keeper.GetValidator(ctx, valAddr) 634: if err != nil { 635: return nil, types.ErrNoValidatorFound 636: } 637: 638: if validator.GetStatus() != types.Bonded { 639: return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "invalid validator status: %s", validator.GetStatus()) 640: } 641: 642: // Check if the validator is exceeding parameter MaxConsPubKeyRotations within the 643: // unbonding period by iterating ConsPubKeyRotationHistory. 644: isExceedingLimit, err := k.CheckLimitOfMaxRotationsExceed(ctx, valAddr) 645: if err != nil { 646: return nil, err 647: } 648: 649: if isExceedingLimit { 650: return nil, types.ErrExceedingMaxConsPubKeyRotations 651: } 652: 653: // Check if the signing account has enough balance to pay KeyRotationFee 654: // KeyRotationFees are sent to the community fund. 655: params, err := k.Params.Get(ctx) 656: if err != nil { 657: return nil, err 658: } 659: 660: if k.Keeper.bankKeeper.HasCoins(ctx, sdk.AccAddress(valAddr), sdk.NewCoins(params.KeyRotationFee)) { 661: err = k.Keeper.bankKeeper.SendCoinsFromAccountToModule(ctx, sdk.AccAddress(valAddr), distrtypes.ModuleName, sdk.NewCoins(params.KeyRotationFee)) 662: if err != nil { 663: return nil, err </blockquote></details></blockquote></details> </details>
x/staking/keeper/msg_server.go
Outdated
| } | ||
|
|
||
| // Add ConsPubKeyRotationHistory for tracking rotation | ||
| err = k.SetConsPubKeyRotationHistory( |
There was a problem hiding this comment.
Will this error if a keypair already exists in the history? e.g. if I rotate from A->B, then A->B again or if I rotate A->B, then to C and back to B?
There was a problem hiding this comment.
Will this error if a keypair already exists in the history?
No, The current implementation won't error in this case, but we can do that if needed.
There was a problem hiding this comment.
Yeah I think we should prohibit the ability to rotate to a key we are currently rotated to.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- x/staking/types/staking.pb.go
Files selected for processing (2)
- api/cosmos/staking/v1beta1/staking.pulsar.go (22 hunks)
- proto/cosmos/staking/v1beta1/staking.proto (2 hunks)
Additional comments: 22
proto/cosmos/staking/v1beta1/staking.proto (2)
415-421: The
operator_addressfield inConsPubKeyRotationHistoryhas been changed from typestringto typebytes. This change may impact data encoding and handling of validator addresses. Ensure that all parts of the codebase that interact with this field are updated to handle the new data type.429-433: The
addressesfield inValAddrsOfRotatedConsKeyshas been changed from typestringto typebytes. This change may impact data encoding and handling of validator addresses. Ensure that all parts of the codebase that interact with this field are updated to handle the new data type.api/cosmos/staking/v1beta1/staking.pulsar.go (20)
13412-13415: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.13458-13458: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.13484-13484: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.13511-13511: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.13545-13545: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.13607-13607: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.13850-13874: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [13843-13867]
The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.
14038-14038: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.14049-14059: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.14073-14073: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.14266-14266: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.14284-14284: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.14356-14357: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.14459-14481: > Note: This review was outside of the patch, so it was mapped to the patch with the greatest overlap. Original lines [14452-14478]
The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.
15940-15940: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.15971-15975: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.16013-16013: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.16036-16036: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.16421-16421: The
OperatorAddressfield is now a byte slice instead of a string. Ensure that all usages of this field are updated to handle bytes instead of strings.16442-16442: The
Addressesfield is now a slice of byte slices instead of a slice of strings. Ensure that all usages of this field are updated to handle bytes instead of strings.
github-actions
bot
removed
C:x/gov
C:x/mint
C:x/bank
C:x/auth
C:x/evidence
C:x/feegrant
C:x/authz
C:x/nft
labels
There was a problem hiding this comment.
Review Status
Actionable comments generated: 3
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/staking/keeper/msg_server.go (3 hunks)
Additional comments: 4
x/staking/keeper/msg_server.go (4)
2-16: The import section has been updated to include new dependencies. Ensure that these new dependencies are properly managed and that their versions are compatible with the project.
25-26: The
msgServerstruct is correctly embedding theKeeperstruct, allowing it to call methods defined onKeeper. This is a common pattern in Go for composition.28-31: The comment explains the reason for duplicating the distribution module's name. It's important to ensure that this name is kept in sync with the actual module name to avoid future issues.
615-692: This hunk introduces the
RotateConsPubKeyfunction, which contains several logical steps for rotating a validator's consensus public key. The function performs type assertions, checks for existing keys, validates the validator status, deducts fees, and updates rotation history. Each of these steps should be carefully reviewed for correctness and security, especially the error handling and state-changing operations.
x/staking/keeper/msg_server.go
Outdated
| if err != nil && !errors.Is(err, collections.ErrNotFound) { | ||
| return nil, err | ||
| } | ||
|
|
||
| if rotatedTo != nil { | ||
| return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, | ||
| "the address is already present in rotation history, please try with new one") | ||
| } |
There was a problem hiding this comment.
The error message could be more descriptive as previously suggested. It should clearly state that the issue is with the public key being already present in the rotation history.
- return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress,
- "the address is already present in rotation history, please try with new one")
+ return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress,
+ "the new public key is already present in rotation history, please try with a different one")Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| if err != nil && !errors.Is(err, collections.ErrNotFound) { | |
| return nil, err | |
| } | |
| if rotatedTo != nil { | |
| return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, | |
| "the address is already present in rotation history, please try with new one") | |
| } | |
| if err != nil && !errors.Is(err, collections.ErrNotFound) { | |
| return nil, err | |
| } | |
| if rotatedTo != nil { | |
| return nil, errorsmod.Wrap(sdkerrors.ErrInvalidAddress, | |
| "the new public key is already present in rotation history, please try with a different one") | |
| } |
x/staking/keeper/msg_server.go
Outdated
| validator, err = k.Keeper.GetValidator(ctx, valAddr) | ||
| if err != nil { | ||
| return nil, types.ErrNoValidatorFound | ||
| } |
There was a problem hiding this comment.
The validator variable is being redeclared, which could lead to confusion. It's important to ensure that the variable names are clear and distinct to avoid potential bugs.
- validator, err = k.Keeper.GetValidator(ctx, valAddr)
+ validator2, err := k.Keeper.GetValidator(ctx, valAddr)Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| validator, err = k.Keeper.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| return nil, types.ErrNoValidatorFound | |
| } | |
| validator2, err := k.Keeper.GetValidator(ctx, valAddr) | |
| if err != nil { | |
| return nil, types.ErrNoValidatorFound | |
| } |
x/staking/keeper/msg_server.go
Outdated
| val, ok := validator.(types.Validator) | ||
| if !ok { | ||
| return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting types.Validator, got %T", validator) | ||
| } |
There was a problem hiding this comment.
The error message should be improved to provide more context as previously suggested. It should clearly indicate the failure in casting the validator.
- if !ok {
- return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting types.Validator, got %T", validator)
- }
+ if !ok {
+ return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "failed to cast validator to types.Validator, got %T", validator)
+ }Commitable suggestion
[!IMPORTANT]
Carefully review the code before committing. Make sure it correctly replaces the highlighted code, has no missing lines and indentaion issues.
| val, ok := validator.(types.Validator) | |
| if !ok { | |
| return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "Expecting types.Validator, got %T", validator) | |
| } | |
| val, ok := validator.(types.Validator) | |
| if !ok { | |
| return nil, errorsmod.Wrapf(sdkerrors.ErrInvalidType, "failed to cast validator to types.Validator, got %T", validator) | |
| } |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/staking/keeper/msg_server.go (2 hunks)
- x/staking/types/keys.go (2 hunks)
Additional comments: 10
x/staking/types/keys.go (2)
24-32: The addition of
DistributionModuleNameis to avoid a cyclic dependency, as explained in the comment. This is a common practice when modules need to reference each other's names but cannot import them directly due to dependency cycles. Ensure that this constant remains in sync with the actual module name in thex/distributionmodule.63-71: The new global variables introduced here are related to the consensus public key rotation feature. They are used as prefixes for different aspects of the rotation history and record management. It's important to ensure that these prefixes are unique and do not collide with existing store prefixes to avoid data corruption.
x/staking/keeper/msg_server.go (8)
3-16: The import section has been updated to include new packages. Ensure that these new dependencies are properly managed (e.g., using go modules) and that their versions are compatible with the rest of the project.
610-681: The
RotateConsPubKeyfunction has been implemented with several checks and operations. Here are some points to consider:618-626: The check for the new public key in the rotation history is good for preventing reuse of keys. However, the error message on line 624-625 could be more specific about the nature of the error (e.g., "public key reuse is not allowed").
630-633: The check to ensure the new public key is not already associated with a validator is important for the uniqueness of consensus keys.
646-647: The check for the validator's status is crucial to ensure that only bonded validators can rotate keys.
652-655: The function
exceedsMaxRotationsis used to enforce limits on the number of rotations, which is a good practice to prevent abuse of the rotation feature.664-667: Sending coins from the account to the module as a fee for key rotation is a good economic measure to prevent spamming the network with rotation requests. Ensure that the
SendCoinsFromAccountToModulefunction has proper error handling and that the transaction is atomic.670-678: Recording the rotation history is essential for auditability and tracking purposes. Ensure that the
setConsPubKeyRotationHistoryfunction handles errors appropriately and that the history is stored securely.
Overall, the function seems to cover the necessary checks and operations for rotating a consensus public key. Ensure that all new keeper functions called within this method (RotatedConsKeyMapIndex.Get,exceedsMaxRotations,setConsPubKeyRotationHistory) are thoroughly tested and handle edge cases properly.
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...
!to the type prefix if API or client breaking changeCHANGELOG.mdmake lintandmake testReviewers 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...
!in the type prefix if API or client breaking changeSummary by CodeRabbit
New Features
Style
Documentation