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

Validator client builder support #10749

Merged
merged 112 commits into from
Jun 6, 2022

Conversation

james-prysm
Copy link
Contributor

@james-prysm james-prysm commented May 24, 2022

What type of PR is this?

Feature
related to #10593
What does this PR do? Why is it needed?

This PR builds on top of #10669

This PR provides capabilities for users to define fee recipients and gaslimit information on their corresponding validator public keys and send them to the beacon node, which will in turn call the builder APIs. These user-defined configurations are important for custom builders to distribute funds correctly and are signed by the validator key for authenticity. The information used here is similar to the prepare_beacon_proposer APIs which are sent to the execution engine, the register validator API proxies to the builder APIs.

Please refer to the corresponding PRs for additional information
ethereum/builder-specs#2
https://github.com/ethereum/beacon-APIs/pull/209/files

includes

  • deprecates fee-recipient-config-file and url flags
  • introduces new validator-proposer-settings-file and url flags
  • updates internal naming from fee recipient to validator proposer setting
  • yaml config parsing support for validator proposer settings
  • introduce gas limit to config file
  • builds register validator request
  • calls register validator beacon API from validator client
  • update unit tests

does not include

  • register validator beacon API implementation internally ( gRPC)
  • register validator beacon API externally ( REST)
  • caching register validator
  • dynamic gas limit ( does not calculate previous block gaslimit and compare with default gas limit or current gas limit setting)
  • gas limit range warnings ( if the gas limit is too high or too low)

@@ -980,7 +983,8 @@ func (v *validator) UpdateProposerSettings(ctx context.Context, km keymanager.IK
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 {
return errors.Wrap(err, fmt.Sprintf("failed to register validator for custom builder for %s", hexutil.Encode(request.Pubkey)))
// log the error and keep going
log.Warnf("failed to register validator for custom builder for %s, error: %v ", hexutil.Encode(request.Pubkey), err)
Copy link
Contributor

@kasey kasey Jun 6, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to address this, lt me know if this works , unit test added

if len(failedRegistrationPubkeys) != 0 {
return errors.Errorf("Register Validator requests failed for the following publickeys: %v", failedRegistrationPubkeys)
if failedRegistrationCount != 0 {
return errors.Errorf("Register validator requests failed %v times out of %v requests", failedRegistrationCount, len(registerValidatorRequests))
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: it's nice to use the typed string formatters (eg %d for numbers, %s for strings) rather than %v where possible, because linters understand them and will complain when types are misaligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated

kasey
kasey previously approved these changes Jun 6, 2022
@james-prysm james-prysm self-assigned this Jun 6, 2022
@prylabs-bulldozer prylabs-bulldozer bot merged commit db687bf into develop Jun 6, 2022
@delete-merged-branch delete-merged-branch bot deleted the validator-client-builder-support branch June 6, 2022 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Api related tasks docs-required Documentation update will be required for the PR or issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants