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

fix validator set proposer priorities in light client provider #5307

Merged
merged 4 commits into from
Aug 31, 2020

Conversation

cmwaters
Copy link
Contributor

Description

Created ValidatorSetFromExistingValidators which restores a validator set (without incrementing priorities) from an array of validators.

Closes: #5306

@erikgrinaker is it possible to see if this fixes your problem

@cmwaters cmwaters added the C:light Component: Light label Aug 28, 2020
@codecov
Copy link

codecov bot commented Aug 28, 2020

Codecov Report

Merging #5307 into master will increase coverage by 1.75%.
The diff coverage is 59.60%.

@@            Coverage Diff             @@
##           master    #5307      +/-   ##
==========================================
+ Coverage   60.83%   62.59%   +1.75%     
==========================================
  Files         203      258      +55     
  Lines       20526    27171    +6645     
==========================================
+ Hits        12488    17009    +4521     
- Misses       6961     8686    +1725     
- Partials     1077     1476     +399     
Impacted Files Coverage Δ
blockchain/v2/processor_context.go 63.15% <0.00%> (ø)
cmd/tendermint/commands/run_node.go 0.00% <0.00%> (ø)
config/toml.go 65.95% <ø> (ø)
consensus/replay_stubs.go 59.57% <0.00%> (-1.30%) ⬇️
crypto/crypto.go 0.00% <ø> (ø)
crypto/sr25519/pubkey.go 44.82% <ø> (ø)
evidence/pool.go 66.49% <ø> (-0.40%) ⬇️
evidence/reactor.go 61.20% <ø> (+0.52%) ⬆️
evidence/services.go 66.66% <ø> (ø)
evidence/verify.go 82.00% <ø> (ø)
... and 118 more

@erikgrinaker
Copy link
Contributor

Great, thanks! I'll give it a try when I'm able.

@erikgrinaker
Copy link
Contributor

This appears to give the correct light client results. However, I'm still seeing state sync consensus failures due to proposer problems, so I'm going to dig into it a bit further before approving this.

@cmwaters
Copy link
Contributor Author

Ok. Has the output from the script you wrote here: #5295 (comment) changed at all?

@erikgrinaker
Copy link
Contributor

Yes, I'm getting the right priorities, so it appears to fix the immediate problem. Not sure why I'm still seeing consensus failures.

Copy link
Contributor

@erikgrinaker erikgrinaker left a comment

Choose a reason for hiding this comment

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

There was an additional off-by-one issue elsewhere, have state sync working again now. Thanks for fixing this!

@erikgrinaker
Copy link
Contributor

PS: this should have a changelog entry

@cmwaters cmwaters merged commit 8670786 into master Aug 31, 2020
@cmwaters cmwaters deleted the callum/fix-provider branch August 31, 2020 10:47
@@ -966,6 +984,21 @@ func ValidatorSetFromProto(vp *tmproto.ValidatorSet) (*ValidatorSet, error) {
return vals, vals.ValidateBasic()
}

// ValidatorSetFromExistingValidators takes an existing array of validators and rebuilds
// the exact same validator set that corresponds to it without changing the proposer priority or power
func ValidatorSetFromExistingValidators(valz []*Validator) (*ValidatorSet, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can't we combine this func with NewValidatorSet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in what way? have a check to see if the proposer priority is not nil for the validators? and if so then follow the logic of this function instead

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 think my reason for having a separate function is that I wanted to show the clear distinction between the concept of making a validator set from new validators and making a validator set from existing validators. I can change this if you prefer going for simplicity

Copy link
Contributor

Choose a reason for hiding this comment

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

have a check to see if the proposer priority is not nil for the validators? and if so then follow the logic of this function instead

yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I can make the change in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:light Component: Light
Projects
None yet
Development

Successfully merging this pull request may close these issues.

light: provider uses NewValidatorSet to aggregate validators which shifts proposer priority
3 participants