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 SSZ support to builder api #104

Merged
merged 3 commits into from
Dec 19, 2024
Merged

Conversation

nflaig
Copy link
Contributor

@nflaig nflaig commented Jul 9, 2024

This is a proposal to add SSZ support to builder API in a backward compatible way without requiring to bump the apis to v2.

It outlines what is expected from the client and how a well-behaved server should handle different content types, headers, and use appropriate error codes to allow the client to handle the error.

Notable changes are

  • submitBlindedBlock allows to SSZ encoding for both request and response body
  • getHeader allows SSZ encoding of response body
  • Define error codes for unsupported media types (406, 415), those are not mandatory but it would be good if servers adopt these over time to allow clients to implement specific error handling
  • Adds Eth-Consensus-Version to responses, only required if response body is SSZ encoded but preferably should be added to all responses eventually
  • Add notes on how a well-behaved client and server implementation should work

Since we can't know if a builder implemented the changes as per spec, the client can determine if SSZ is supported by requesting an SSZ encoded response in the getHeader request. Preferably this should be done setting a Accept header with multiple entries and quality values like Accept: application/octet-stream;q=1.0,application/json;q=0.9 to give the builder the option to send a JSON response.

Related to #53 and #54

@metachris
Copy link
Contributor

In support of this change. It's nicely backwards-compatible, doesn't add significant risk, decreases latency and could simplify CL flows.

@ltitanb
Copy link

ltitanb commented Aug 20, 2024

Full support, it's a great improvement over the existing API and should be particularly useful for larger blocks. Maybe worth adding an example payload for SSZ as well?

@nflaig
Copy link
Contributor Author

nflaig commented Aug 21, 2024

Maybe worth adding an example payload for SSZ as well?

I gave this a try but it has it's limitations since you can't display binary data on the explorer. The solution seems to be to use external examples so we could add ssz to existing examples and reference the files in the ssz examples.

However, there seems to be an issue swagger-api/swagger-ui#5433, the examples won't be displayed on the explorer, it would be great if it would show an option to download the example file but it only shows no example available.

image

There might be a fix for this at some point like swagger-api/swagger-js#2013. For now could do something like here nflaig/builder-specs@ssz-support...nflaig:builder-specs:ssz-examples with the caveat that it won't be available on the explorer.

@barnabasbusa
Copy link
Member

Would it make sense to mark the json api deprecated with this same PR? That way we can push to get json removed in the next fork.

@nflaig
Copy link
Contributor Author

nflaig commented Oct 17, 2024

That way we can push to get json removed in the next fork.

In my opinion, having JSON as an option is a good thing, it can be quite useful for debugging.

If this reduces complexity significantly for some implementation it could be considered, however in Lodestar for example it does not make a difference at all as we have to keep JSON support for beacon-api anyways.

@eserilev
Copy link

Im in support of this. In Lighthouse its not a problem for us to support both JSON and SSZ

types/http.yaml Show resolved Hide resolved
@nflaig
Copy link
Contributor Author

nflaig commented Nov 23, 2024

Lodestar supports this in the latest release (v1.23.0) in case anyone needs a client to test with.

Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

generally looks good!

had one question about adding a 415 code to the headers endpoint

@ralexstokes ralexstokes merged commit 875882d into ethereum:main Dec 19, 2024
3 checks passed
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.

6 participants