Validator client builder support #10749
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -980,18 +980,17 @@ func (v *validator) PushProposerSettings(ctx context.Context, km keymanager.IKey | |
| return err | ||
| } | ||
| log.Infoln("Successfully prepared beacon proposer with fee recipient to validator index mapping.") | ||
| failedRegistrationCount := 0 | ||
| for _, request := range registerValidatorRequests { | ||
| // calls beacon API but used for custom builders | ||
| if err := SubmitValidatorRegistration(ctx, v.validatorClient, v.node, km.Sign, request); err != nil { | ||
| // log the error and keep going | ||
| failedRegistrationCount++ | ||
| log.Warnf("failed to register validator for custom builder for %s, error: %v ", hexutil.Encode(request.Pubkey), err) | ||
|
There was a problem hiding this comment. I thought the plan was to make all requests (so that a bad request doesn't ruin it for the rest), but at the end of the method return an error if any of the requests failed. @terencechain if some of the requests fail, should this method return an error? There was a problem hiding this comment. Updated to address this, lt me know if this works , unit test added |
||
| } | ||
| } | ||
| if failedRegistrationCount != 0 { | ||
| return errors.Errorf("Register validator requests failed %v times out of %v requests", failedRegistrationCount, len(registerValidatorRequests)) | ||
|
There was a problem hiding this comment. nitpick: it's nice to use the typed string formatters (eg |
||
| } | ||
| log.Infoln("Successfully submitted builder validator registration settings for custom builders.") | ||
| return nil | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Is this running in a separate goroutine or blocking an unrelated thread? If the latter, should we consider running this in the background in its own goroutine on a timer?
Should we retry failures? What is the upper bound on the number of requests we could do here - maybe we should consider adding a context deadline?
Should the entire batch fail upon any individual request failing? The one validator that is due to be proposer in the next epoch could be at the end of the list and be not updated due to a previous unimportant validator registration failing. It would be cool to order the list so that proposers come first, but failing that I think we should put some thought into retries and how to deal with partial failures. For instance we could log the individual failures at debug log level, and at the end of the batch return an error that indicates
x out of y validator registrations failed(maybe a different error message with the pubkey if just one failed).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.
when it runs per epoch I have it running in its own go routine you can see that in the runner.go file line 190. otherwise it runs when status updates and on startup which are not on their on go routine.
I think those are all possible future improvements, with it being set every6.4 minutes and on startup as well I wonder if that's 100% needed at this moment. I can make a note on the tracking of this feature but I don't think it should be part of this particular PR.
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.
Are we building a prototype for integration testing, or is this something we want to be usable by end-users once this is merged? If it's a prototype then I agree that we can punt on resiliency, but I think accounting for retries and partial failures are basic requirements we need in place before this is used in production. I'll defer to @terencechain on clarifying the goal.