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

Treat the validator updates from the application as an unordered set #3181

Closed
ancazamfir opened this issue Jan 21, 2019 · 6 comments
Closed
Labels
C:abci Component: Application Blockchain Interface C:consensus Component: Consensus T:breaking Type: Breaking Change T:bug Type Bug (Confirmed)
Milestone

Comments

@ancazamfir
Copy link
Contributor

When the application updates the validator set it calls updateValidators() with an 'updates' parameter, a list of changes that is then parsed in order, one entry at a time, making updates for the corresponding validator. i.e. the list is treated as a transaction list.
For example [add v1, rem v2, add v3] is different than [rem v2, add v1, add v3] and the call with one will produce different results than with the other.
This is an issue for gaia that requires these updates to be instead interpreted as a set.

@ancazamfir ancazamfir changed the title Treat the updates from the application as an unordered set Treat the validator updates from the application as an unordered set Jan 21, 2019
@ebuchman ebuchman added T:bug Type Bug (Confirmed) C:abci Component: Application Blockchain Interface C:consensus Component: Consensus protocol labels Jan 22, 2019
@ebuchman ebuchman added this to the v0.30.0 milestone Jan 22, 2019
@kevlubkcm
Copy link
Contributor

sounds related to #3073

@cwgoes
Copy link
Contributor

cwgoes commented Jan 24, 2019

Proposal for atomic application:

  • First apply all updates to the validator set, without setting priorities. Track a list of addresses of newly added validators.
  • Calculate the total voting power of the new validator set.
  • Set the priorities of all newly added validators to -C * total voting power (presently C = 1.125).
  • Normalize priorities if necessary & subtract the average.

Also we should consider if we need C = 1.125 after this change.

@ancazamfir
Copy link
Contributor Author

Proposal for atomic application:
First apply all updates to the validator set, without setting priorities. Track a list of addresses of newly added validators.

all updates except the validator removals

Calculate the total voting power of the new validator set.
Set the priorities of all newly added validators to -C * total voting power (presently C = 1.125).

do validator removals here

Normalize priorities if necessary & subtract the average.
Also we should consider if we need C = 1.125 after this change.

@melekes melekes added the T:breaking Type: Breaking Change label Jan 28, 2019
@ancazamfir
Copy link
Contributor Author

pls. see pull request #3222

@ancazamfir
Copy link
Contributor Author

ancazamfir commented Feb 4, 2019

Seems like NewValidatorSet(valList) does not check for duplicates in the validator list valList and we end up with a validator "set" including same validator multiple times. I'm not sure if all checks are in place when running in a real setup so valList does not contain duplicates. Will check.
This also needs fix it in validator_set.go

@ebuchman
Copy link
Contributor

ebuchman commented Feb 8, 2019

Merged! Thanks @ancazamfir ! Will add follow up notes to #3166

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:abci Component: Application Blockchain Interface C:consensus Component: Consensus T:breaking Type: Breaking Change T:bug Type Bug (Confirmed)
Projects
None yet
Development

No branches or pull requests

5 participants