nimbus-eth2 icon indicating copy to clipboard operation
nimbus-eth2 copied to clipboard

Implement SSZ responses in the light client REST APIs

Open zah opened this issue 3 years ago • 6 comments

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.

zah avatar Jul 02 '22 13:07 zah

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

etan-status avatar Jul 02 '22 15:07 etan-status

Unit Test Results

     12 files  ±0     857 suites  ±0   1h 10m 26s :stopwatch: + 7m 16s 1 712 tests ±0  1 660 :heavy_check_mark: ±0    52 :zzz: ±0  0 :x: ±0  9 944 runs  ±0  9 816 :heavy_check_mark: ±0  128 :zzz: ±0  0 :x: ±0 

Results for commit f6ac1774. ± Comparison against base commit 9eb1a3ef.

github-actions[bot] avatar Jul 02 '22 16:07 github-actions[bot]

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

arnetheduck avatar Jul 03 '22 11:07 arnetheduck

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

etan-status avatar Jul 03 '22 15:07 etan-status

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 avatar Jul 03 '22 15:07 etan-status

SSZ is now supported as of https://github.com/ethereum/beacon-APIs/pull/247 and implemented in Nimbus in https://github.com/status-im/nimbus-eth2/pull/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.

etan-status avatar Oct 03 '22 21:10 etan-status