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

Stream Duties Client Implementation #5867

Merged
merged 213 commits into from Jun 18, 2020
Merged

Stream Duties Client Implementation #5867

merged 213 commits into from Jun 18, 2020

Conversation

rauljordan
Copy link
Contributor

@rauljordan rauljordan commented May 15, 2020

What type of PR is this?

Feature

What does this PR do? Why is it needed?

This PR is the follow-up to #5685, which now uses the server-side stream to update the validator client and call out to this function. This PR also does a few amends to the server-side implementation, where there were a few mismatches in requirements.

A design document for this feature explaining its background, rationale, and implementation is located here.

Other notes for review

  • Ensure runtime works as expected (passes e2e, chain can move and finalize normally)
  • The stream is disconnected temporarily, validator reconnects stream
  • Chain start event, does the validator have assignments prior to the start of slot 0 / slot 1?
  • A validator receives their assignments with ample time to produce a block on the first slot of an epoch. I.e. are assignments eagerly sent to the validator?
  • Needs feature flag

@rauljordan rauljordan requested a review from a team as a code owner May 15, 2020 16:07
@rauljordan rauljordan self-assigned this May 15, 2020
@rauljordan rauljordan added the Ready For Review A pull request ready for code review label May 15, 2020
@rauljordan rauljordan removed the Ready For Review A pull request ready for code review label May 15, 2020
@rauljordan rauljordan added the Ready For Review A pull request ready for code review label May 15, 2020
validator/client/validator.go Outdated Show resolved Hide resolved
validator/client/validator_duties.go Outdated Show resolved Hide resolved
validator/client/validator_duties_test.go Outdated Show resolved Hide resolved
@@ -0,0 +1,121 @@
package metrics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to review, just consolidated metrics into a shared folder

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

Let's
1.) add streaming flags as part of e2e test
2.) add streaming flags to dev

}

func handleAssignmentError(err error, slot uint64) {
if errCode, ok := status.FromError(err); ok && errCode.Code() == codes.NotFound {
Copy link
Member

Choose a reason for hiding this comment

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

NotFound automatically means validator is not yet assigned?

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 so, it comes from this server-side error:

if !ok {
return nil, status.Errorf(codes.NotFound, "Could not find validator index for public key %#x", pubkeyBytes)
}

Copy link
Member

Choose a reason for hiding this comment

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

The concern here is anyone can update server side with additional error NotFound. This will be unknowingly impacted

validator/client/streaming/service_test.go Outdated Show resolved Hide resolved
type Validator interface {
Done()
WaitForChainStart(ctx context.Context) error
WaitForSync(ctx context.Context) error
Copy link
Member

Choose a reason for hiding this comment

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

Why two different methods for sync ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's always been there. It's gated behind a feature flag that Ivan created to simplify the validator runtime but we've never enabled it, so both code paths still exist

@rauljordan rauljordan merged commit 7067c84 into master Jun 18, 2020
@delete-merged-branch delete-merged-branch bot deleted the client-stream-duties branch June 18, 2020 18:30
@rauljordan rauljordan mentioned this pull request Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

5 participants