ADR 016: Validator consensus key rotation #5233
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,126 @@ | ||
| # ADR 016: Validator Consensus Key Rotation | ||
|
|
||
| ## Changelog | ||
|
|
||
| - 2019 Oct 23: Initial draft | ||
| - 2019 Nov 28: Add key rotation fee | ||
|
|
||
| ## Context | ||
|
|
||
| Validator consensus key rotation feature has been discussed and requested for a long time, for the sake of safer validator key management policy (e.g. https://github.com/tendermint/tendermint/issues/1136). So, we suggest one of the simplest form of validator consensus key rotation implementation mostly onto Cosmos-SDK. | ||
|
|
||
| We don't need to make any update on consensus logic in Tendermint because Tendermint does not have any mapping information of consensus key and validator operator key, meaning that from Tendermint point of view, a consensus key rotation of a validator is simply a replacement of a consensus key to another. | ||
|
|
||
| Also, it should be noted that this ADR includes only the simplest form of consensus key rotation without considering multiple consensus keys concept. Such multiple consensus keys concept shall remain a long term goal of Tendermint and Cosmos-SDK. | ||
|
|
||
| ## Decision | ||
|
|
||
| ### Pseudo procedure for consensus key rotation | ||
|
|
||
| - create new random consensus key. | ||
| - create and broadcast a transaction with a `MsgRotateConsPubKey` that states the new consensus key is now coupled with the validator operator with signature from the validator's operator key. | ||
| - old consensus key becomes unable to participate on consensus immediately after the update of key mapping state on-chain. | ||
| - start validating with new consensus key. | ||
| - validators using HSM and KMS should update the consensus key in HSM to use the new rotated key after the height `h` when `MsgRotateConsPubKey` committed to the blockchain. | ||
alexanderbez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
|
|
||
alexanderbez marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ### Considerations | ||
|
|
||
| - consensus key mapping information management strategy | ||
| - store history of each key mapping changes in the kvstore. | ||
| - the state machine can search corresponding consensus key paired with given validator operator for any arbitrary height in a recent unbonding period. | ||
| - the state machine does not need any historical mapping information which is past more than unbonding period. | ||
| - key rotation costs related to LCD and IBC | ||
| - LCD and IBC will have traffic/computation burden when there exists frequent power changes | ||
| - In current Tendermint design, consensus key rotations are seen as power changes from LCD or IBC perspective | ||
| - Therefore, to minimize unnecessary frequent key rotation behavior, we limited maximum number of rotation in recent unbonding period and also applied exponentially increasing rotation fee | ||
| - limits | ||
| - a validator cannot rotate its consensus key more than `MaxConsPubKeyRotations` time for any unbonding period, to prevent spam. | ||
| - parameters can be decided by governance and stored in genesis file. | ||
| - key rotation fee | ||
| - a validator should pay `KeyRotationFee` to rotate the consensus key which is calculated as below | ||
| - `KeyRotationFee` = (max(`VotingPowerPercentage` * 100, 1) * `InitialKeyRotationFee`) * 2^(number of rotations in `ConsPubKeyRotationHistory` in recent unbonding period) | ||
| - evidence module | ||
| - evidence module can search corresponding consensus key for any height from slashing keeper so that it can decide which consensus key is supposed to be used for given height. | ||
| - abci.ValidatorUpdate | ||
| - tendermint already has ability to change a consensus key by ABCI communication(`ValidatorUpdate`). | ||
| - validator consensus key update can be done via creating new + delete old by change the power to zero. | ||
| - therefore, we expect we even do not need to change tendermint codebase at all to implement this feature. | ||
| - new genesis parameters in `staking` module | ||
| - `MaxConsPubKeyRotations` : maximum number of rotation can be executed by a validator in recent unbonding period. default value 10 is suggested(11th key rotation will be rejected) | ||
| - `InitialKeyRotationFee` : the initial key rotation fee when no key rotation has happened in recent unbonding period. default value 1atom is suggested(1atom fee for the first key rotation in recent unbonding period) | ||
|
|
||
fedekunze marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| ### Workflow | ||
|
There was a problem hiding this comment. I'm confused about which module will this feature be a part of. I assume staking? Can you add that as a comment? There was a problem hiding this comment. I added two parts as below
|
||
|
|
||
| 1. The validator generates a new consensus keypair. | ||
| 2. The validator generates and signs a `MsgRotateConsPubKey` tx with their operator key and new ConsPubKey | ||
|
|
||
| ```go | ||
| type MsgRotateConsPubKey struct { | ||
|
There was a problem hiding this comment. Who is the signer of this message? There was a problem hiding this comment. The validator account key is the signer of |
||
| ValidatorAddress sdk.ValAddress | ||
| NewPubKey crypto.PubKey | ||
| } | ||
| ``` | ||
|
|
||
| 3. `handleMsgRotateConsPubKey` gets `MsgRotateConsPubKey`, calls `RotateConsPubKey` with emits event | ||
| 4. `RotateConsPubKey` | ||
| - checks if `NewPubKey` is not duplicated on `ValidatorsByConsAddr` | ||
| - checks if the validator is does not exceed parameter `MaxConsPubKeyRotations` by iterating `ConsPubKeyRotationHistory` | ||
| - checks if the signing account has enough balance to pay `KeyRotationFee` | ||
| - pays `KeyRotationFee` to community fund | ||
| - overwrites `NewPubKey` in `validator.ConsPubKey` | ||
| - deletes old `ValidatorByConsAddr` | ||
| - `SetValidatorByConsAddr` for `NewPubKey` | ||
| - Add `ConsPubKeyRotationHistory` for tracking rotation | ||
|
|
||
| ```go | ||
| type ConsPubKeyRotationHistory struct { | ||
| OperatorAddress sdk.ValAddress | ||
| OldConsPubKey crypto.PubKey | ||
| NewConsPubKey crypto.PubKey | ||
| RotatedHeight int64 | ||
| } | ||
| ``` | ||
|
|
||
| 5. `ApplyAndReturnValidatorSetUpdates` checks if there is `ConsPubKeyRotationHistory` with `ConsPubKeyRotationHistory.RotatedHeight == ctx.BlockHeight()` and if so, generates 2 `ValidatorUpdate` , one for a remove validator and one for create new validator | ||
|
There was a problem hiding this comment. Will this change the priority of the validator in the RoundRobin proposer selection in Tendermint? There was a problem hiding this comment. The new rotated key will have the lowest accum because it is seen as a new validator. So it is a slight disadvantage to the key rotated validator |
||
|
|
||
| ```go | ||
| abci.ValidatorUpdate{ | ||
| PubKey: tmtypes.TM2PB.PubKey(OldConsPubKey), | ||
| Power: 0, | ||
| } | ||
|
|
||
| abci.ValidatorUpdate{ | ||
| PubKey: tmtypes.TM2PB.PubKey(NewConsPubKey), | ||
| Power: v.ConsensusPower(), | ||
| } | ||
| ``` | ||
|
|
||
| 6. at `previousVotes` Iteration logic of `AllocateTokens`, `previousVote` using `OldConsPubKey` match up with `ConsPubKeyRotationHistory`, and replace validator for token allocation | ||
| 7. Migrate `ValidatorSigningInfo` and `ValidatorMissedBlockBitArray` from `OldConsPubKey` to `NewConsPubKey` | ||
| - Note : All above features shall be implemented in `staking` module. | ||
|
|
||
| ## Status | ||
|
|
||
| Proposed | ||
|
|
||
| ## Consequences | ||
|
|
||
| ### Positive | ||
|
|
||
| - Validators can immediately or periodically rotate their consensus key to have better security policy | ||
| - improved security against Long-Range attacks (https://nearprotocol.com/blog/long-range-attacks-and-a-new-fork-choice-rule) given a validator throws away the old consensus key(s) | ||
|
|
||
Hyung-bharvest marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ### Negative | ||
|
There was a problem hiding this comment. Missing an important negative of it allows a validator to effectively sell their entity, in a way that before would require hardware assumptions. (They can change their key to give it to new management, without any oversight from their delegators) |
||
|
|
||
| - Slash module needs more computation because it needs to lookup corresponding consensus key of validators for each height | ||
| - frequent key rotations will make light client bisection less efficient | ||
|
|
||
Hyung-bharvest marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| ### Neutral | ||
|
|
||
| ## References | ||
|
|
||
| - on tendermint repo : https://github.com/tendermint/tendermint/issues/1136 | ||
| - on cosmos-sdk repo : https://github.com/cosmos/cosmos-sdk/issues/5231 | ||
| - about multiple consensus keys : https://github.com/tendermint/tendermint/issues/1758#issuecomment-545291698 | ||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I opened a KMS issue about supporting more than one registered key per chain, and ways that Tendermint could signal which key to use (e.g. if it detects a misconfiguration or invalid signature when requesting a signature from the new key):
tendermint/tmkms#368
I think it's important to be able to handle failures during key rotations without requiring manual intervention, although perhaps it isn't required for a key rotation MVP. Without a single place to coordinate flipping which key is active, the rollback procedure will be much more complex, IMO.
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.
If i am understanding right, tendermint/tmkms#368 seems similar to something like an OTP, even though it will be not practically one time.
Trigger of automatic key rotation can be decided by latest block hash. We can encode such block hash into an integer, and then key rotation is decided by the remainder of the integer divided by some high integer number. Also we can design an algorithm so that the rotation happens statistically every N blocks.
But these ideas are too complex to put it in this ADR at the moment.
There will be definitely an only struct in SDK which stores the ChangeLog of consensus key rotation. But a rollback on this data seems very risky, because there exists high possibility that the validator has either lost or been stolen already rotated keys. So, intuitively, even though the state of chain being rolled back, the consensus key for rolled back chain on genesis should not be rolled back but should be provided by each validator manually with their signature by the operator key.
I cannot think of any simple automatic solution to this yet.
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.
That seems overly complex and it's not immediately clear to me how that would work. As far as I see it, one of the main issues to address is once the Tx is committed in a block, KMS will somehow have to be notified automatically (or manually?) that it should use the new key in the keyring.
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.
I think notifying KMS is strictly KMS related job and it is quite independent from this initial key rotation feature on "Cosmos-SDK". Our intention is to deploy the simplest form of rotation feature on "Cosmos-SDK" so that we make no change on tendermint at all.
I think, after implementation of this feature, KMS expert (like @tarcieri) can hack Cosmos-SDK to support such notification by monitoring the mapping struc between valoper and valconspub. It should monitor a struc in SDK because the mapping information is not stored in tendermint.
Until then, I think manual update on KMS seems reasonable.
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.
Ideally I think Tendermint would include the public key to use for the signature in the
SignProposalandSignVotemessages, although it seems that may not work for an MVP.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.
@tarcieri
No. This ADR includes only the simplest form of consensus key rotation without considering multiple consensus key situation. We think it should be remain as a long term goal to pursue because it needs a breaking structural change on Tendermint about validator and consensus key abstract definition and their relationship.
If you think it should be in this ADR, then please reply another comment, or we will proceed without it.
Thank you for your opinion!
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.
This should be stated in the
Contextsection aboveThere 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.
adding below statement in the context section
Also, it should be noted that this ADR includes only the simplest form of consensus key rotation without considering multiple consensus keys concept. Such multple consensus keys concept shall be remained as a long term goal of Tendermint and Cosmos-SDK.