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 format for block production. #205

Merged
merged 8 commits into from
May 5, 2022

Conversation

rolfyone
Copy link
Collaborator

Block production and signed block consumption should allow SSZ encoding to help us reduce overheads on for api consumers.

Fallbacks will likely remain json for clients anyway if they're not reading the headers, but if they do read the headers then there's big performance gains to be had in returning / consuming SSZ.

At a later time it may be worth also accepting SSZ on other endpoints also.

Block production and signed block consumption should allow SSZ encoding to help us reduce overheads on for api consumers.

Fallbacks will likely remain json for clients anyway if they're not reading the headers, but if they do read the headers then there's big performance gains to be had in returning / consuming SSZ.

At a later time it may be worth also accepting SSZ on other endpoints also.
apis/beacon/blocks/blinded_blocks.yaml Outdated Show resolved Hide resolved
apis/beacon/blocks/blocks.yaml Outdated Show resolved Hide resolved
apis/beacon/blocks/blinded_blocks.yaml Outdated Show resolved Hide resolved
apis/beacon/blocks/blocks.yaml Outdated Show resolved Hide resolved
@rolfyone
Copy link
Collaborator Author

the linter doesn't like me but if i try to insert schema somehow the swagger UI breaks... so i might leave it in this working state :)

@mcdee
Copy link
Contributor

mcdee commented Apr 23, 2022

I don't think that it would be a terrible thing to say that an Accept header is required for all requests. Having a fallback position of sending JSON seems easy, but it does provide potential confusion as to what to do if a client states that it only accepts an octet stream, and the server responds with JSON because that's all it provides for that request. Perhaps being explicit is better, to the point where if a server doesn't handle any types in the Accept header it returns a 415.

That said, we're already in a situation where we have an implicit default of JSON so it wouldn't come without some upgrade issues.

@ajsutton
Copy link
Contributor

I don't think that it would be a terrible thing to say that an Accept header is required for all requests. Having a fallback position of sending JSON seems easy, but it does provide potential confusion as to what to do if a client states that it only accepts an octet stream, and the server responds with JSON because that's all it provides for that request. Perhaps being explicit is better, to the point where if a server doesn't handle any types in the Accept header it returns a 415.

Yeah I can see the thinking here, but it would a) break backwards compatibility for everyone and b) make using the API with curl significantly harder. On the other hand a client doing the weird thing of strictly requiring SSZ can handle that easily by just checking the returned Content-Type and treating the request as failed if it isn't JSON.

It's also much more common in HTTP to have a default representation that's returned when the Accept header doesn't match than it is to reject the request.

@rolfyone
Copy link
Collaborator Author

This should be ready for re-review @mpetrunic but let me know if you wanted more changes?

@mpetrunic
Copy link
Collaborator

This should be ready for re-review @mpetrunic but let me know if you wanted more changes?

Fine by me. @mcdee @ajsutton ?

@arnetheduck
Copy link
Contributor

Do we even need to specify which endpoints must support SSZ? ie clients can simply specify multiple values in http-accept (both json and ssz, with p= priorities) to show preference - then we can make it such that all requests potentially support SSZ but all must support json.

@mcdee
Copy link
Contributor

mcdee commented May 1, 2022

@arnetheduck yes we could do that, although from memory not all beacon nodes currently support multi-value Accept headers. And it would be nice to have a minimal number of endpoints that are required to support SSZ, purely down to the size of the response if they only support JSON.

@arnetheduck
Copy link
Contributor

arnetheduck commented May 2, 2022

not all beacon nodes currently support

And it would be nice to have a minimal number of endpoints that are required to support SSZ,

I consider these to be quality-of-implementation points or optimizations really - if we say the json encoding is canonical/non-optional, I would even go as far as putting a table on top of the spec describing the (possible) encodings and not say anything more in each endpoint. I guess the post requests are the only ones where we need to specify MUST:s, since negotiation is more difficult.

One more format I've been contemplating is snappy-framed compressed encoding using application/x-snappy-framed, same as is used in the libp2p spec - this allows clients to store compressed data in the database/on-disk for historical blocks, and just stream..

@rolfyone
Copy link
Collaborator Author

rolfyone commented May 2, 2022

IMO this is optional, but of high value. We can't make it compulsory because old endpoints that already exist don't do it.

If clients can pick this up, there are gains to be had internally and externally, because clients such as our own validator client can save effort encoding/decoding, and also other products consuming and evaluating can gain the benefits of not having to json encode/decode.
I'm adding the option to teku, but obviously if other clients don't pick it up then it becomes much more limited value to 3rd parties, because they can't reasonably expect to get ssz back if only 1 client implements it, so it seems like an obvious thing to declare the intent in the API spec.

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.

5 participants