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 support to builder validator registration using ssz #9065

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lucassaldanha
Copy link
Member

PR Description

Implement support for SSZ requests when registering validators via the Builder endpoint. Logic based on this spec ethereum/builder-specs#110.

Since we don't have a way to know if the relay supports SSZ, our best bet is to try SSZ and if it fails fallback into JSON. However, we do not want to keep trying SSZ for every request after a failure so we are adding a backoff logic (24h).

Fixed Issue(s)

fixes #9003

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

Comment on lines 206 to 254
+ " is not a supported milestone for the builder rest api. Milestones >= Bellatrix are supported."));
+ " is not a supported milestone for the builder rest api. Milestones >= Bellatrix are "
+ "supported."));
Copy link
Contributor

Choose a reason for hiding this comment

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

could be unchanged really...

Comment on lines 312 to 315
assertThat(mockWebServer.getRequestCount()).isEqualTo(3);
verifyPostRequestSsz("/eth/v1/builder/validators", signedValidatorRegistrations);
verifyPostRequestJson("/eth/v1/builder/validators", signedValidatorRegistrationsRequest);
verifyPostRequestSsz("/eth/v1/builder/validators", signedValidatorRegistrations);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we should probably make a tiny change here to show that the first set are verified and the second set is just the ssz call... we basically show that with the asserts above but it'd be clearer if this last check is just seeing 1 request, and just ssz.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I can move the early assertions closer to where we exercise the request to make them a bit more "sequential".

Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM just nits.

Comment on lines +135 to +137
} else {
return SafeFuture.completedFuture(response);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

you could avoid else here ;-)

Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

I think what is missing is a test checking that the fallback doesn't happen on non-http errors, like timeout or connection errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add SSZ suport to validatorRegistration builder endpoint
3 participants