ADR 016: Validator consensus key rotation #5233

Hyung-bharvest · 2019-10-23T07:35:41Z

ref: #5231

Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (docs/)
Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
Re-reviewed Files changed in the github PR explorer

For Admin Use:

Added appropriate labels to PR (ex. wip, ready-for-review, docs)
Reviewers Assigned
Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

alexanderbez

Thanks @dlguddus. Can you reference the ADR in ## ADR Table of Contents in docs/architecture/README.MD?

docs/architecture/adr-016-validator-consensus-key-rotation.md

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Hyung-bharvest · 2019-10-24T02:45:02Z

Thanks @dlguddus. Can you reference the ADR in ## ADR Table of Contents in docs/architecture/README.MD?

done!

docs/architecture/adr-016-validator-consensus-key-rotation.md

alexanderbez · 2019-10-24T17:27:21Z

Looks good @dlguddus and @dongsam. Look forward to seeing the changes.

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

alexanderbez

Concept ACK ++

The ADR should also make mention of a new on-chain parameter -- MaxConsKeyRotations. In addition, I would love to see feedback from @zmanian and @tarcieri :)

fedekunze

ACK, pending conflicts resolved and suggested default params added to the ADR.

codecov · 2019-12-11T01:13:06Z

Codecov Report

Merging #5233 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5233      +/-   ##
==========================================
+ Coverage   54.46%   54.51%   +0.05%     
==========================================
  Files         313      311       -2     
  Lines       18892    18740     -152     
==========================================
- Hits        10289    10217      -72     
+ Misses       7812     7743      -69     
+ Partials      791      780      -11

Impacted Files	Coverage Δ
store/types/pruning.go	`50% <0%> (-50%)`	⬇️
store/types/utils.go	`0% <0%> (-19.61%)`	⬇️
store/types/validity.go	`0% <0%> (-16.67%)`	⬇️
types/dec_coin.go	`63.21% <0%> (-14.23%)`	⬇️
x/upgrade/handler.go	`80% <0%> (-5.72%)`	⬇️
x/params/proposal_handler.go	`73.68% <0%> (-4.1%)`	⬇️
crypto/keys/keybase_base.go	`64.7% <0%> (-3.72%)`	⬇️
x/mock/test_utils.go	`60% <0%> (-3.05%)`	⬇️
x/auth/types/stdtx.go	`58.67% <0%> (-2.87%)`	⬇️
client/keys/add.go	`39.35% <0%> (-1.79%)`	⬇️
... and 101 more

add suggested default value for genesis parameters

all additional features will be implemented on `staking` module

zmanian

IMO, this needs some kind of language about costs validator key rotation imposes on light clients and IBC and potentially some solutions to rate limit or charge fees based on that cost.

Hyung-bharvest · 2019-12-13T04:33:38Z

IMO, this needs some kind of language about costs validator key rotation imposes on light clients and IBC and potentially some solutions to rate limit or charge fees based on that cost.

how about this in Considerations section

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

docs/architecture/adr-016-validator-consensus-key-rotation.md

AdityaSripal · 2019-12-13T22:27:44Z

docs/architecture/adr-016-validator-consensus-key-rotation.md

+2. The validator generates and signs a `MsgRotateConsPubKey` tx with their operator key and new ConsPubKey
+
+    ```go
+    type MsgRotateConsPubKey struct {


Who is the signer of this message?

The validator account key is the signer of MsgRotateConsPubKey

AdityaSripal · 2019-12-13T22:31:18Z

docs/architecture/adr-016-validator-consensus-key-rotation.md

+    }
+    ```
+
+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 


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

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

Co-Authored-By: Aditya <adityasripal@gmail.com>

zmanian · 2019-12-16T04:47:25Z

IMO, this needs some kind of language about costs validator key rotation imposes on light clients and IBC and potentially some solutions to rate limit or charge fees based on that cost.

how about this in Considerations section

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

Sounds good to me.

It just means that we always need to track how many times a validator rotated keys in the period so we can charge them appropriately.

Another point might be to charge more for validators to rotate keys if they have more voting power.

Hyung-bharvest · 2019-12-16T05:17:30Z

IMO, this needs some kind of language about costs validator key rotation imposes on light clients and IBC and potentially some solutions to rate limit or charge fees based on that cost.

how about this in Considerations section

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

Sounds good to me.

It just means that we always need to track how many times a validator rotated keys in the period so we can charge them appropriately.

Another point might be to charge more for validators to rotate keys if they have more voting power.

your last sentence seems accurately describe the reasoning factor of KeyRotationFee. what I can suggest is as below.

KeyRotationFee = (max(VotingPowerPercentage * 100, 1) * InitialKeyRotationFee) * 2^(number of rotations in ConsPubKeyRotationHistory in recent unbonding period)

Now KeyRotationFee is linearly dependent to the voting power of the validator. The reason why I used max function in the formula is that, we want to charge minimum key rotation fee even though the validator has very small or no voting power.

How do you like the suggestion?

zmanian · 2019-12-16T06:04:47Z

How do you like the suggestion?

Perfect.

….,.. - add "key rotation costs related to LCD and IBC" in considerations - fix `KeyRotationFee` formula.

Hyung-bharvest · 2019-12-16T08:03:44Z

How do you like the suggestion?

Perfect.

done!

alexanderbez · 2019-12-16T22:21:49Z

@zmanian mind giving an ACK?

Hyung-bharvest · 2019-12-30T09:33:11Z

How do you like the suggestion?

Perfect.

@zmanian Could you confirm the update? Thank you!

Hyung-bharvest · 2020-01-05T02:47:31Z

@zmanian please don't block our process if you dont have any further request. It has been too long time.

ValarDragon · 2021-04-12T17:10:19Z

docs/architecture/adr-016-validator-consensus-key-rotation.md

+- 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)
+
+### Negative


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)

Create adr-016-validator-consensus-key-rotation.md

0db84f2

Hyung-bharvest requested review from alessio, alexanderbez, fedekunze, jackzampolin and rigelrozanski as code owners October 23, 2019 07:35

fedekunze changed the title ~~Create adr-016-validator-consensus-key-rotation.md~~ ADR 016: Validator consensus key rotation Oct 23, 2019

fedekunze added T: ADR An issue or PR relating to an architectural decision record R4R labels Oct 23, 2019

alexanderbez suggested changes Oct 23, 2019

View reviewed changes

Hyung-bharvest and others added 9 commits October 24, 2019 10:40

typo line 10

8706fe8

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

typo line 26

df3524c

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

typo line 25

d0a8030

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

typo line 27

352cac4

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

typo line 29

65c1f40

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

remove rotation fee

4181be9

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

add ADR 016 in ADR Table of Contents

a73be81

remove pruning and further implementation part

922fe9d

add a step on procedure about HSM and KMS users

1a62db4

alexanderbez reviewed Oct 24, 2019

View reviewed changes

docs/architecture/adr-016-validator-consensus-key-rotation.md Outdated Show resolved Hide resolved

alexanderbez reviewed Oct 24, 2019

View reviewed changes

docs/architecture/adr-016-validator-consensus-key-rotation.md Outdated Show resolved Hide resolved

Merge branch 'master' into master

d7e9c53

Hyung-bharvest and others added 2 commits October 25, 2019 12:40

RotateValConsensusKey -> MsgRotateConsPubkey

afffccd

Co-Authored-By: Alexander Bezobchuk <alexanderbez@users.noreply.github.com>

Add workflow

7708e5e

alexanderbez reviewed Oct 28, 2019

View reviewed changes

alexanderbez and others added 3 commits October 28, 2019 11:39

Merge branch 'master' into master

953a8f8

Add checking MaxConsPubKeyRotations to workflow

4c932ee

Merge branch 'master' into master

889e382

alexanderbez requested a review from fedekunze December 10, 2019 16:58

fedekunze approved these changes Dec 10, 2019

View reviewed changes

Merge branch 'master' into master

21f710a

Hyung-bharvest and others added 4 commits December 11, 2019 13:08

add suggested default value for genesis parameters

0b65286

add suggested default value for genesis parameters

all additional features will be implemented on staking module

897a2da

all additional features will be implemented on `staking` module

Merge branch 'master' into master

744b2f1

Merge branch 'master' into master

523a493

zmanian requested changes Dec 12, 2019

View reviewed changes

AdityaSripal reviewed Dec 13, 2019

View reviewed changes

Update docs/architecture/adr-016-validator-consensus-key-rotation.md

f00e565

Co-Authored-By: Aditya <adityasripal@gmail.com>

Hyung-bharvest added 2 commits December 16, 2019 17:00

add "key rotation costs related to LCD and IBC" in considerations and…

4b81ab4

….,.. - add "key rotation costs related to LCD and IBC" in considerations - fix `KeyRotationFee` formula.

fix KeyRotationFee formula

f190c9d

Hyung-bharvest and others added 2 commits December 16, 2019 17:04

Merge branch 'master' into master

a4119f6

Merge branch 'master' into master

c39d3a2

alexanderbez requested a review from zmanian December 16, 2019 22:21

zmanian approved these changes Jan 5, 2020

View reviewed changes

Merge branch 'master' into master

aa8b632

alexanderbez merged commit 0e62442 into cosmos:master Jan 5, 2020

Hyung-bharvest mentioned this pull request Jan 11, 2020

EPIC: validator consensus key rotation #5231

Closed

defihyung mentioned this pull request Mar 29, 2021

ADR 016: Validator Consensus Key Rotation #9016

Closed

9 tasks

ValarDragon reviewed Apr 12, 2021

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADR 016: Validator consensus key rotation #5233

ADR 016: Validator consensus key rotation #5233

Hyung-bharvest commented Oct 23, 2019 •

edited by alexanderbez

Loading

alexanderbez left a comment

Hyung-bharvest commented Oct 24, 2019

alexanderbez commented Oct 24, 2019

alexanderbez left a comment •

edited

Loading

fedekunze left a comment

codecov bot commented Dec 11, 2019 •

edited

Loading

zmanian left a comment

Hyung-bharvest commented Dec 13, 2019 •

edited

Loading

AdityaSripal Dec 13, 2019

Hyung-bharvest Dec 14, 2019

AdityaSripal Dec 13, 2019

Hyung-bharvest Dec 14, 2019

zmanian commented Dec 16, 2019

Hyung-bharvest commented Dec 16, 2019 •

edited

Loading

zmanian commented Dec 16, 2019

Hyung-bharvest commented Dec 16, 2019

alexanderbez commented Dec 16, 2019

Hyung-bharvest commented Dec 30, 2019

Hyung-bharvest commented Jan 5, 2020

ValarDragon Apr 12, 2021

ADR 016: Validator consensus key rotation #5233

ADR 016: Validator consensus key rotation #5233

Conversation

Hyung-bharvest commented Oct 23, 2019 • edited by alexanderbez Loading

alexanderbez left a comment

Choose a reason for hiding this comment

Hyung-bharvest commented Oct 24, 2019

alexanderbez commented Oct 24, 2019

alexanderbez left a comment • edited Loading

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2019 • edited Loading

Codecov Report

zmanian left a comment

Choose a reason for hiding this comment

Hyung-bharvest commented Dec 13, 2019 • edited Loading

AdityaSripal Dec 13, 2019

Choose a reason for hiding this comment

Hyung-bharvest Dec 14, 2019

Choose a reason for hiding this comment

AdityaSripal Dec 13, 2019

Choose a reason for hiding this comment

Hyung-bharvest Dec 14, 2019

Choose a reason for hiding this comment

zmanian commented Dec 16, 2019

Hyung-bharvest commented Dec 16, 2019 • edited Loading

zmanian commented Dec 16, 2019

Hyung-bharvest commented Dec 16, 2019

alexanderbez commented Dec 16, 2019

Hyung-bharvest commented Dec 30, 2019

Hyung-bharvest commented Jan 5, 2020

ValarDragon Apr 12, 2021

Choose a reason for hiding this comment

Hyung-bharvest commented Oct 23, 2019 •

edited by alexanderbez

Loading

alexanderbez left a comment •

edited

Loading

codecov bot commented Dec 11, 2019 •

edited

Loading

Hyung-bharvest commented Dec 13, 2019 •

edited

Loading

Hyung-bharvest commented Dec 16, 2019 •

edited

Loading