-
Notifications
You must be signed in to change notification settings - Fork 242
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
Implement SSZ responses in the light client REST APIs #3836
Conversation
Other changes: * More DRY encoding of the Nimbus content type preference. * Switch if/elif/else to exhaustive case statements to guard the code better from future changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding SSZ support! This was indeed still missing.
The nim-stew
bump intentional?
of jsonResponseType: | ||
RestApiResponse.jsonResponse(updates) | ||
of sszResponseType: | ||
RestApiResponse.sszResponse(updates.asSszList(MAX_CLIENT_UPDATES)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updates may be from different fork digests in the future, i.e., each update
may become a ForkedLightClientUpdate
. The response should be future proofed for that, similar to the libp2p protocol which already did this.
See ethereum/beacon-APIs#181 (comment)
And, originally, point 3.3 from ethereum/consensus-specs#2802 (comment)
LC events are in |
the risk of overimplementing unspecced stuff is that the specced stuff later becomes incompatible .. because this is not in the spec, right? |
Well, the LC specific endpoints are |
The problem with the fork digest also exists for other endpoints, but for those only an individual object is returned in the response, so heuristics can be applied such as peeking the first 8 bytes for slot number etc, or try which deserializations succeed and which don't (that sounds error prone but seems to work well in practice for existing use). What's new in the LC spec is that there is a way to request entire ranges of updates, whereas for the blocks the REST API only allows single block download per request. Requiring a light client to perform multiple round trips to be consistent with this existing behaviour is kinda dumb (also considering that LC updates are much smaller than blocks, so the roundtrips add significant overhead), so a new solution to exchange SSZ lists of potentially different objects over REST should be designed. Of course, this only starts mattering once there actually are different objects, but at the very least the mechanism should be future-proof to that. For the other endpoints (finality / optimistic updates), they also have the problem of not including fork context with them, but at the very least there it can be argued that there is existing prior art of just attempting deserializations / heuristics which seems to be acceptable for now. For JSON response, the problem is way smaller as JSON does not enforce a specific memory layout, e.g. if the slot is moved in some version the client can still find it and infer fork digest from it. But for SSZ, the payload does not itself describe where each property is located. |
SSZ is now supported as of ethereum/beacon-APIs#247 and implemented in Nimbus in #4213 Note that the event stream uses SSE which is a text encoding, so only supports JSON. This is in line with the other routes. This PR here still has unrelated adjustments that may still be useful. Suggest untying them from the LC related parts if still relevant. |
Other changes:
the code better from future changes.