Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ADR 016: Validator consensus key rotation #5233

Merged
merged 36 commits into from
Jan 5, 2020
Merged
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
0db84f2
Create adr-016-validator-consensus-key-rotation.md
Hyung-bharvest Oct 23, 2019
8706fe8
typo line 10
Hyung-bharvest Oct 24, 2019
df3524c
typo line 26
Hyung-bharvest Oct 24, 2019
d0a8030
typo line 25
Hyung-bharvest Oct 24, 2019
352cac4
typo line 27
Hyung-bharvest Oct 24, 2019
65c1f40
typo line 29
Hyung-bharvest Oct 24, 2019
4181be9
remove rotation fee
Hyung-bharvest Oct 24, 2019
a73be81
add ADR 016 in ADR Table of Contents
Hyung-bharvest Oct 24, 2019
922fe9d
remove pruning and further implementation part
Hyung-bharvest Oct 24, 2019
1a62db4
add a step on procedure about HSM and KMS users
Hyung-bharvest Oct 24, 2019
d7e9c53
Merge branch 'master' into master
alexanderbez Oct 24, 2019
afffccd
RotateValConsensusKey -> MsgRotateConsPubkey
Hyung-bharvest Oct 25, 2019
7708e5e
Add workflow
dongsam Oct 25, 2019
953a8f8
Merge branch 'master' into master
alexanderbez Oct 28, 2019
4c932ee
Add checking MaxConsPubKeyRotations to workflow
dongsam Oct 29, 2019
889e382
Merge branch 'master' into master
alexanderbez Oct 29, 2019
a92ce8b
clearer statement regarding to HSM and KMS
Hyung-bharvest Oct 30, 2019
3e9f2d5
Update docs/architecture/adr-016-validator-consensus-key-rotation.md
Hyung-bharvest Nov 6, 2019
6992552
Update docs/architecture/adr-016-validator-consensus-key-rotation.md
Hyung-bharvest Nov 6, 2019
3fbc112
Merge branch 'master' into master
alexanderbez Nov 7, 2019
92e8b7e
add key rotation fee
Hyung-bharvest Nov 28, 2019
69b58a1
add new genesis parameters in considerations
Hyung-bharvest Nov 28, 2019
0da99f7
Merge branch 'master' into master
alexanderbez Dec 2, 2019
71ffdc9
Merge branch 'master' into master
dongsam Dec 9, 2019
ebdc907
update several things
Hyung-bharvest Dec 10, 2019
21f710a
Merge branch 'master' into master
dongsam Dec 11, 2019
0b65286
add suggested default value for genesis parameters
Hyung-bharvest Dec 11, 2019
897a2da
all additional features will be implemented on `staking` module
Hyung-bharvest Dec 12, 2019
744b2f1
Merge branch 'master' into master
Hyung-bharvest Dec 12, 2019
523a493
Merge branch 'master' into master
fedekunze Dec 12, 2019
f00e565
Update docs/architecture/adr-016-validator-consensus-key-rotation.md
dongsam Dec 16, 2019
4b81ab4
add "key rotation costs related to LCD and IBC" in considerations and…
Hyung-bharvest Dec 16, 2019
f190c9d
fix `KeyRotationFee` formula
Hyung-bharvest Dec 16, 2019
a4119f6
Merge branch 'master' into master
Hyung-bharvest Dec 16, 2019
c39d3a2
Merge branch 'master' into master
alexanderbez Dec 16, 2019
aa8b632
Merge branch 'master' into master
alexanderbez Jan 5, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions docs/architecture/README.MD
Original file line number Diff line number Diff line change
Expand Up @@ -31,3 +31,4 @@ Please add a entry below in your Pull Request for an ADR.
- [ADR 010: Modular AnteHandler](./adr-010-modular-antehandler.md)
- [ADR 011: Generalize Genesis Accounts](./adr-011-generalize-genesis-accounts.md)
- [ADR 012: State Accessors](./adr-012-state-accessors.md)
- [ADR 016: Validator Consensus Key Rotation](./adr-016-validator-consensus-key-rotation.md)
120 changes: 120 additions & 0 deletions docs/architecture/adr-016-validator-consensus-key-rotation.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
# 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consensus key rotation implementation mostly onto Cosmos-SDK.

I know this has been asked a few times already, but I'd be nice if you could add the reasoning of why does this feature fit into the SDK instead of Tendermint.

Copy link
Contributor Author

@Hyung-bharvest Hyung-bharvest Dec 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for noting that. I add below sentences to clarify the reason.

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.


## 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.
Copy link

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.

Copy link
Contributor Author

@Hyung-bharvest Hyung-bharvest Nov 6, 2019

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@Hyung-bharvest Hyung-bharvest Nov 11, 2019

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.

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 SignProposal and SignVote messages, although it seems that may not work for an MVP.

Copy link
Contributor Author

@Hyung-bharvest Hyung-bharvest Nov 28, 2019

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!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This ADR includes only the simplest form of consensus key rotation without considering multiple consensus key situation.

This should be stated in the Context section above

Copy link
Contributor Author

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.

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.
- 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` = `InitialKeyRotationFee` * 2^(number of rotations in `ConsPubKeyRotationHistory` in recent unbonding period)
- slash module
- slash module can search corresponding consensus key for any height so that it can decide which consensus key is supposed to be used for given height.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the double sign evidence is now handled by the x/evidence module, which gets the registered pubkey from the x/slashing module. So ultimately the evidence module will have to look up the corresponding consensus address at the given height.

cc: @alexanderbez

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for pointing that out! as you said, evidence module gets pubkey from slashing keeper to use for double signing detection. edited as below

- 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
fedekunze marked this conversation as resolved.
Show resolved Hide resolved
- `MaxConsPubKeyRotations` : maximum number of rotation can be executed by a validator in recent unbonding period.
- `InitialKeyRotationFee` : the initial key rotation fee when no key rotation has happened in recent unbonding period.

fedekunze marked this conversation as resolved.
Show resolved Hide resolved

### Workflow
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about which module will this feature be a part of. I assume staking? Can you add that as a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added two parts as below

  1. in Considerations
  • new genesis parameters in staking module
  1. in Workflow
    - Note : All above features shall be implemented in staking module.


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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is the signer of this message?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validator account key is the signer of MsgRotateConsPubKey

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this change the priority of the validator in the RoundRobin proposer selection in Tendermint?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`

Hyung-bharvest marked this conversation as resolved.
Show resolved Hide resolved

## 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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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