-
Notifications
You must be signed in to change notification settings - Fork 995
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
Conversation
… into client-stream-duties
@@ -0,0 +1,121 @@ | |||
package metrics |
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.
No need to review, just consolidated metrics into a shared folder
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.
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 { |
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.
NotFound
automatically means validator is not yet assigned?
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 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)
}
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.
The concern here is anyone can update server side with additional error NotFound
. This will be unknowingly impacted
Co-authored-by: terence tsao <terence@prysmaticlabs.com>
type Validator interface { | ||
Done() | ||
WaitForChainStart(ctx context.Context) error | ||
WaitForSync(ctx context.Context) 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.
Why two different methods for sync ?
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.
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
What type of PR is this?
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