-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Conversation
Codecov Report
@@ 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
|
Great, thanks! I'll give it a try when I'm able. |
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. |
Ok. Has the output from the script you wrote here: #5295 (comment) changed at all? |
Yes, I'm getting the right priorities, so it appears to fix the immediate problem. Not sure why I'm still seeing consensus failures. |
There was a problem hiding this 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!
PS: this should have a changelog entry |
@@ -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) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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