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

Allow SSZ encoding in addition to JSON encoding (for request and response payloads) #53

Open
metachris opened this issue Oct 7, 2022 · 19 comments

Comments

@metachris
Copy link
Contributor

metachris commented Oct 7, 2022

I've benchmarked encoding and decoding of signed-blinded-beacon-block payloads with SSZ vs JSON: flashbots/go-boost-utils#50

SSZ seems 40-50x faster:

BenchmarkJSONvsSSZEncoding/Encode_JSON-10         	   18561	     62939 ns/op	   50754 B/op	     274 allocs/op
BenchmarkJSONvsSSZEncoding/Encode_SSZ-10          	  855457	      1368 ns/op	    9472 B/op	       1 allocs/op
BenchmarkJSONvsSSZEncoding/Decode_JSON-10         	    7621	    153292 ns/op	   30433 B/op	     613 allocs/op
BenchmarkJSONvsSSZEncoding/Decode_SSZ-10          	  276904	      3968 ns/op	   10116 B/op	     152 allocs/op

This would be particularly relevant for getPayload calls, because of the payload size and timing constraints (but perhaps also interesting for getHeader and registerValidator).


Each getPayload call currently does 8 JSON encoding and decoding steps (if we include mev-boost in the mix):

  1. BN encodes the getPayload request body (signed blinded beacon block)
  2. mev-boost decodes the getPayload request body
  3. mev-boost encodes the getPayload request body
  4. relay decodes the getPayload request body
  5. relay encodes the getPayload response body (execution payload)
  6. mev-boost decodes the getPayload response body
  7. mev-boost encodes the getPayload response body
  8. BN decodes the getPayload response body

Considering 20-40ms per coding on average, that's up to 200-300ms JSON latency (or more).

Sending the data SSZ encoded could reliably shave 200-250ms off each getPayload roundtrip.


I propose we add SSZ encoded payloads as first-class citizens.

Questions:

  • How can this work with relays that didn't (yet) support SSZ encoded payloads? 🤔
    • How can we make this backward compatible, so it will also work if a given relay didn't yet upgrade?
    • Should the BN just try to send the getPayload with SSZ body, and if the relay responds with an error then try with JSON again? Or should it send an accept-encoding: ssz header to getHeader, and if it receives the header in SSZ encoding then use SSZ for getPayload too, otherwise fall back to JSON?
@metachris metachris changed the title Allow SSZ encoding in addition to JSON encoding for all the requests Allow SSZ encoding in addition to JSON encoding Oct 7, 2022
@metachris metachris changed the title Allow SSZ encoding in addition to JSON encoding Allow SSZ encoding in addition to JSON encoding (for request and response payloads) Oct 7, 2022
@ralexstokes
Copy link
Member

in general I think this would be great to add ASAP!

How can this work with relays that didn't (yet) support SSZ encoded payloads?

can always just make a v2 of the API that supports either codec via the header (this is how the beacon-apis do it) and leave the v1 as-is

otherwise, we update the v1 to have current behavior w/ no header, and then also add support for Content-Type: application/json or Content-Type: application/ssz based on what the user wants -- if a caller requests something a relay cannot provide they just respond with a HTTP 400 error

@metachris
Copy link
Contributor Author

Thinking about it, upgrading v1 to allow for it seems kind of straight forward this way:

  1. beacon-node calls getHeader with request header accept-encoding: application/ssz
    1. If response is SSZ encoded, then it uses SSZ for getPayload
    2. If response is JSON encoded, then use JSON for getPayload

Should be easy to notify other release before this change gets out, for them to add least make sure they can handle it correctly and not error out (even if not using SSZ).

@metachris
Copy link
Contributor Author

@dapplion
Copy link

dapplion commented Oct 8, 2022

@metachris Could you expand the benchmarks to different block sizes within what's observed in mainnet?

Agree that would be great to add this asap. No strong opinions on whether to bump to v2 or not

@tersec
Copy link
Contributor

tersec commented Oct 8, 2022

Agree with @dapplion that this would be a good addition.

@mcdee
Copy link

mcdee commented Oct 8, 2022

A few notes on this after investigating this for the beacon APIs:

  • for requests that read data, the relay should support Accept headers with q parameters such as "application/octet-stream;q=1,application/json;q=0.9" for best compatibility, as it allows them to pick and choose if they want to send back SSZ or JSON depending on their own ability. Clients can check the Content-type header in the response to know the format of the data they have received
  • for requests that write data, the simplest system is to have clients attempt a call with a Content-type of "application/octet-stream" and if the server responds with a 415 error to fall back to "application/json"

This won't require any changes to API versioning, if it is assumes that a lack of a Content-type header implies "application/json", and a lack of Accept header the same.

@metachris
Copy link
Contributor Author

metachris commented Oct 9, 2022

Indeed, there's no official ssz mime type: https://www.iana.org/assignments/media-types/media-types.xhtml - and it seems that application/octet-stream is the most applicable (unless we go non-standard with application/ssz).

Here's more about the q parameter and allowing multiple encodings: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept-Encoding

It seems it would be okay to add to the current v1, as long as the beacon nodes keep track of the payload encoding the relay sends in the getHeader response, for use in the subsequent getPayload. Because relays might just error out if they receive a getPayload request with SSZ encoded body, no matter what's the request headers.

@Ruteri
Copy link

Ruteri commented Oct 10, 2022

I would also love to see this for builder submissions.

@tbenr
Copy link

tbenr commented Oct 10, 2022

This is the reason we have rest API instead of JSON-RPC on the builder. I'm strongly in favour of this!

  1. beacon-node calls getHeader with request header accept-encoding: application/ssz
    1. If response is SSZ encoded, then it uses SSZ for getPayload
    2. If response is JSON encoded, then use JSON for getPayload

I think that the 415 flow as @mcdee sayd must be the standard (and the easiest way is to implement a fallback mechanism) we already implemented it in beacon API when VC posts signed blocks. To avoid additional systematic additional latency during rollout, clients should remember that they falledback to json and not try to post ssz again.

To remove even the first attempt latency, a nice to have would be, as you're saying, that CLs (and mev-boost) switch their rest clients to json as long as they receive a json response from getHeader.

@metachris
Copy link
Contributor Author

Sidenote: would the Accept header be more appropriate than Accept-Encoding? Accept expresses the preference in mime-type, not algorithm:

@tbenr
Copy link

tbenr commented Oct 10, 2022

Sidenote: would the Accept header be more appropriate than Accept-Encoding? Accept expresses the preference in mime-type, not algorithm:

Yep, it is Accept to me.

@benhenryhunter
Copy link

Sounds like the headers is a more proper way to do things but I'm pro v2 endpoints from a relay implementation point of view. I think it would be easier to debug the implementation and more clear to users of mev-boost that there is a difference in the behavior of the endpoint.

@metachris
Copy link
Contributor Author

metachris commented Oct 10, 2022

If going with v2, we could also allow SSZ encoding for validator registrations, which could speed up the processing a whole lot. This wouldn't work with v1 in a backwards-compatible manner (without some workarounds on the BN).

@mcdee
Copy link

mcdee commented Oct 10, 2022

This is a change to the server rather than the endpoints, so I'd really rather not see it resulting in incrementing versions.

It's possible for clients to work out what encodings servers accept with a little bit of back-and-forth, and as long as they persist that information over the lifetime of the connection it shouldn't result in much in terms of additional overhead.

@StefanBratanov
Copy link
Contributor

StefanBratanov commented Oct 11, 2022

In my opinion it would be best to be consistent with the current beacon-APIs https://ethereum.github.io/beacon-APIs/ and the way the support for SSZ is described for the endpoints (basically support application/octet-stream for requests and responses)

In terms of backwards compatibility, if clients (CLs and mev-boost) don't change anything, everything will work same as before since all requests default to json anyways.

If the same clients start sending ssz requests and expect ssz responses, they should handle:

  • 415 errors when there is a request body and Content-Type : application/octet-stream is not supported (try again with json)
  • no ssz response when excepting ssz response Accept: application/octet-stream (fallback to json deserialize if ssz deserialization fails)

For mev-boost it would be good to mark a relay as ssz-not-supported and immediately use json for further requests. (with some expiry maybe since the relay could upgrade)

For CLs, I suppose it is better to have a configuration option to use ssz or not for the builder interface, since they only talk to one component and can switch it on/off depending on what is supported.

@metachris
Copy link
Contributor Author

metachris commented Oct 12, 2022

For CLs, I suppose it is better to have a configuration option to use ssz or not for the builder interface, since they only talk to one component and can switch it on/off depending on what is supported.

What's the argument for having a switch, rather than simply checking for SSZ support on getHeader and using it by default if supported?

For mev-boost it would be good to mark a relay as ssz-not-supported and immediately use json for further requests. (with some expiry maybe since the relay could upgrade)

I don't think mev-boost should do transformations on the requests. It seems preferable to have mev-boost continue to be just proxying the original calls. The logic with detecting SSZ support on getHeader seems solid enough.

@StefanBratanov
Copy link
Contributor

StefanBratanov commented Oct 12, 2022

What's the argument for having a switch, rather than simply checking for SSZ support on getHeader and using it by default if supported?

Maybe an user would want to only use the json flow and not ssz, so having the choice is more user-friendly. Not super important though since it is implementation details, which each client will handle in their preferred way.

I don't think mev-boost should do transformations on the requests. It seems preferable to have mev-boost continue to be just proxying the original calls. The logic with detecting SSZ support on getHeader seems solid enough.

Yeah, it seems solid enough to be fair as long as mev-boost honors the Accept header (json or octet-stream)

@terencechain
Copy link
Contributor

Related: ethereum/beacon-APIs#250

@avalonche
Copy link
Contributor

bump on this issue, with the introduction of blobs seeing client timeouts in the devnets when max blob payloads are returned to the beacon node.

"error":"write tcp : write: broken pipe","level":"error","msg":"Couldn't write response"

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

No branches or pull requests