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

Add more validation to AllValidatorsAreExited #7755

Merged
merged 3 commits into from
Nov 10, 2020

Conversation

prestonvanloon
Copy link
Member

@prestonvanloon prestonvanloon commented Nov 9, 2020

What type of PR is this?

Other

What does this PR do? Why is it needed?

I think this method, which defaults to "true", can be error prone.
I have added a few checks:

  • Return false when no keys are provided. "All keys are exited" cannot be true if there are no keys.
  • Return false and an error if the server only partially responds to the request. The validator should not assume a key is exited if the server did not respond with a status for that key.

Which issues(s) does this PR fix?

Related: #7707

Other notes for review

I did not test at runtime.

This method is used here:

allExited, err := v.AllValidatorsAreExited(ctx)
if err != nil {
log.WithError(err).Error("Could not check if validators are exited")
}
if allExited {
log.Info("All validators are exited, no more work to perform...")
continue
}

In the event, that there is some erroneous failure, it will only log a message rather than stopping the active validators from performing work.

@prestonvanloon prestonvanloon requested a review from a team as a code owner November 9, 2020 20:25
@prestonvanloon prestonvanloon added Bug Something isn't working Enhancement New feature or request Ready For Review A pull request ready for code review labels Nov 9, 2020
@codecov
Copy link

codecov bot commented Nov 9, 2020

Codecov Report

Merging #7755 (ae1bb94) into master (742808c) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #7755   +/-   ##
=======================================
  Coverage   62.16%   62.16%           
=======================================
  Files         429      429           
  Lines       30292    30296    +4     
=======================================
+ Hits        18831    18834    +3     
- Misses       8531     8532    +1     
  Partials     2930     2930           

@prylabs-bulldozer prylabs-bulldozer bot merged commit ce75b2f into master Nov 10, 2020
@delete-merged-branch delete-merged-branch bot deleted the better-all-validators-exited branch November 10, 2020 11:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Enhancement New feature or request Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants