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

feat: provide first-class ssz support on api #6749

Merged
merged 292 commits into from
Jun 10, 2024
Merged

Conversation

nflaig
Copy link
Member

@nflaig nflaig commented May 8, 2024

Motivation

Initial discussion around this has started in #5128 to provide first-class support for SSZ in both request and response bodies but besides that, there are plenty of other shortcomings in the way we currently define routes and handle server / client responses. In a lot of cases, Lodestar was not following the spec (e.g. not setting all required headers) causing incompatibilities with other clients. The main reason for this is because our API spec tests were incomplete, headers were not checked at all or if we support SSZ on all required routes as per spec, and addressing those shortcomings requires workarounds like overriding handlers on both the client and server.

Description

This PR changes the structure of how routes are defined and makes it trivial to add SSZ support to any route. The only exception are routes with data payloads that cannot be represented as a SSZ type due to lack of JSON mapping, meaning it is not possible to translate the JSON payload into a SSZ-serialized payload.

The second major change is how we handle requests and responses on the server and client. While previously if you would request a state as SSZ from the server, the client would only be able to handle that format and if the server didn't support it, the request would fail. Now, we use content type negotiation between client and server which works based on headers and status codes to allow a more flexible and robust handling of requests and responses.

The third redesign in this PR is related to the API client, it is now possible to provide options like HTTP timeout per request and the response object is much more complete and self-contained, e.g. it's no longer needed to call ApiError.assert(...) everywhere (more examples below). This refactoring allows for further extensions in the future like a per route strategy for sending requests to fallback nodes and picking responses based on metadata, e.g. pick the most profitable block from all connected beacon nodes.

BREAKING CHANGES

While there is are no breaking changes to the validator client and beacon node, the client exported from @lodestar/api package changed significantly, and there are minor other changes to exported types but those are not as relevant for most consumers.

These are the most notable changes

Client initialization

before

const api = getClient({urls, getAbortSignal: () => opts.abortController.signal}, {config, logger});

after

const api = getClient({urls, globalInit: {signal: opts.abortController.signal}}, {config, logger});

Request with value

before

const res = await api.beacon.getGenesis();
ApiError.assert(res, "Can not fetch genesis data from beacon node");
const genesis = res.response.data;

after (.value() will assert if request was successful and throw detailed error)

const genesis = (await api.beacon.getGenesis()).value()

Request without value

before

ApiError.assert(await beaconClient.submitPoolVoluntaryExit(signedVoluntaryExit));

after (response object is self-contained, no utility like ApiError.assert is required anymore)

(await beaconClient.submitPoolVoluntaryExit({signedVoluntaryExit})).assertOk();

Passing arguments

before

api.validator.produceAttestationData(committeeIndex, slot)

after (args are passed as object, it is now possible to pass per request options, like timeout)

api.validator.produceAttestationData({committeeIndex, slot}, {timeoutMs: 4000})

TODO


Closes #5128
Closes #5244
Closes #5826
Closes #6110
Closes #6564
Closes #6634

@nflaig nflaig requested a review from nazarhussain June 3, 2024 12:56
nazarhussain
nazarhussain previously approved these changes Jun 3, 2024
Copy link

codecov bot commented Jun 5, 2024

Codecov Report

Attention: Patch coverage is 92.37816% with 416 lines in your changes missing coverage. Please review.

Project coverage is 62.77%. Comparing base (f6d3bce) to head (907f221).

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #6749      +/-   ##
============================================
+ Coverage     62.19%   62.77%   +0.58%     
============================================
  Files           571      580       +9     
  Lines         60099    61306    +1207     
  Branches       1978     2116     +138     
============================================
+ Hits          37376    38486    +1110     
- Misses        22680    22781     +101     
+ Partials         43       39       -4     

nazarhussain
nazarhussain previously approved these changes Jun 7, 2024
@nflaig
Copy link
Member Author

nflaig commented Jun 7, 2024

Metrics from feat3 in addition to what's already in #6749 (comment), most notable difference if of course in the REST API latency, especially if we look at it from VC point of view as it includes latency over the wire, and not just serialization / deserialization.

image

image

Other metrics look good to me as well (no regressions), or even slightly better across the board (lower gc, eventloop lag, etc.)

Copy link
Member

@wemeetagain wemeetagain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
Thanks for doing the hard work of pushing this to completion

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meta-breaking-change Introduces breaking changes to DB, Validator, Beacon Node, or CLI interfaces. Handle with care!
Projects
None yet
4 participants