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

Implement SSZ responses in the light client REST APIs #3836

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from

Conversation

zah
Copy link
Member

@zah zah commented Jul 2, 2022

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.

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.
Copy link
Contributor

@etan-status etan-status left a 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?

beacon_chain/rpc/rest_beacon_api.nim Show resolved Hide resolved
of jsonResponseType:
RestApiResponse.jsonResponse(updates)
of sszResponseType:
RestApiResponse.sszResponse(updates.asSszList(MAX_CLIENT_UPDATES))
Copy link
Contributor

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)

beacon_chain/rpc/rest_utils.nim Show resolved Hide resolved
@etan-status
Copy link
Contributor

LC events are in rest_event_api.nim, but it seems there is no SSZ support there in general.

@github-actions
Copy link

github-actions bot commented Jul 2, 2022

Unit Test Results

     12 files  ±0     857 suites  ±0   1h 10m 26s ⏱️ + 7m 16s
1 712 tests ±0  1 660 ✔️ ±0    52 💤 ±0  0 ±0 
9 944 runs  ±0  9 816 ✔️ ±0  128 💤 ±0  0 ±0 

Results for commit f6ac177. ± Comparison against base commit 9eb1a3e.

@arnetheduck
Copy link
Member

the risk of overimplementing unspecced stuff is that the specced stuff later becomes incompatible .. because this is not in the spec, right?

@etan-status
Copy link
Contributor

Well, the LC specific endpoints are /eth/v0 or _v0 (events), there is no adopted spec so far.
ethereum/beacon-APIs#181

@etan-status
Copy link
Contributor

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.

@etan-status
Copy link
Contributor

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.

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.

None yet

3 participants