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

Conversation

Hyung-bharvest
Copy link
Contributor

@Hyung-bharvest Hyung-bharvest commented Oct 23, 2019

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)

@fedekunze fedekunze changed the title Create adr-016-validator-consensus-key-rotation.md ADR 016: Validator consensus key rotation Oct 23, 2019
@fedekunze fedekunze added T: ADR An issue or PR relating to an architectural decision record R4R labels Oct 23, 2019
Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

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

Hyung-bharvest and others added 9 commits October 24, 2019 10:40
@Hyung-bharvest
Copy link
Contributor Author

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

done!

@alexanderbez
Copy link
Contributor

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

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

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

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 :)

Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

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

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

@codecov
Copy link

codecov bot commented Dec 11, 2019

Codecov Report

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

Impacted file tree graph

@@            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

Copy link
Member

@zmanian zmanian left a comment

Choose a reason for hiding this comment

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

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

Hyung-bharvest commented Dec 13, 2019

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

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

}
```

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

@zmanian
Copy link
Member

zmanian commented Dec 16, 2019

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

Hyung-bharvest commented Dec 16, 2019

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

zmanian commented Dec 16, 2019

How do you like the suggestion?

Perfect.

….,..

- add "key rotation costs related to LCD and IBC" in considerations
- fix `KeyRotationFee` formula.
@Hyung-bharvest
Copy link
Contributor Author

How do you like the suggestion?

Perfect.

done!

@alexanderbez
Copy link
Contributor

@zmanian mind giving an ACK?

@Hyung-bharvest
Copy link
Contributor Author

How do you like the suggestion?

Perfect.

@zmanian Could you confirm the update? Thank you!

@Hyung-bharvest
Copy link
Contributor Author

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

- 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
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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T: ADR An issue or PR relating to an architectural decision record
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants