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

nflaig · 2024-05-08T12:23:32Z

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

nazarhussain

There is a lot to review further but sharing my initial review.

My major concern upto this point is the the lack of response codes from the routes definitions. Now use will never know any endpoint can response with 200, 206, 503, or 404.

We are putting alls calls in a promise box and saying it would be either fullfil or fail. But in case of failure user don't know what could be expected status codes.

This was a major information missing in earlier implementation and we added that in last refactor. I don't think loosing that context is a worthwhile.

nazarhussain · 2024-05-22T12:56:20Z

packages/api/src/utils/client/method.ts

+}
+
+export function createApiClientMethods<Es extends Record<string, Endpoint>>(
+ definitions: RouteDefinitions<Es>,


Use either routesDefinitions or routes instead of just definitions for clarity. The word definitions does is not understandable in context this function is used.

We use the term "definitions" throughout the api package for route definitions, the function only has 1 line of code and we have the type name in the same line, should be clear enough.

Would prefer to keep as is for consistency

nazarhussain · 2024-05-22T12:58:02Z

packages/api/src/utils/client/method.ts

+): ApiClientMethods<Es> {
+ return mapValues(definitions, (definition, operationId) => {
+ return createApiClientMethod(definition, client, operationId as string);
+ }) as unknown as ApiClientMethods<Es>;


Anywhere you have to use unknown as that highlights some hidden type complexity which may lead to problem in future. As it's all new code written so better to identify and address such points right now.

I agree we should try to avoid using unknown as, I previously looked at this already and gave it another attempt today but I don't see what's the issue, the types look good to me, seems to be some issue how mapValues remaps the types, the type error is pretty strange Type 'Es[keyof Es]' is not comparable to type 'Es[K]'

We have the same type cast on unstable branch

lodestar/packages/api/src/utils/client/client.ts

Line 124 in c635098

}) as unknown as ApiWithExtraOpts<Api>;

My guess is there is some issue with the typing of mapValues that makes the translation incorrect but not sure how to fix it right now, maybe you could take a look and help with that. If it's related to mapValues type might be better to fix on unstable branch first

nazarhussain · 2024-05-22T12:59:18Z

packages/api/src/beacon/client/beacon.ts

- return client.getBlockV2(blockId, format);
- },
- };
+export function getClient(config: ChainForkConfig, httpClient: IHttpClient): ApiClientMethods<Endpoints> {


The method name and the return type is not matching. We asked for a client and return type says it's returning client methods. Would be nicer to align the type names with the functions where these are used.

Updated return type name to better align with method name

nazarhussain · 2024-05-22T13:02:32Z

packages/api/src/beacon/client/debug.ts

- } as ApiClientResponse<{[HttpStatusCode.OK]: Uint8Array}>;
- }
- return client.getState(stateId, format);
+ getState(args, init) {


Does the second argument are request options or initialization options?

second arg is RequestInit & LodestarExtras

nazarhussain · 2024-05-22T13:04:14Z

packages/api/src/beacon/client/events.ts

 eventSource.close();
+ onClose?.();
+ signal.removeEventListener("abort", close);


As it's one time listener, so need to remove. This way you can use this listener as inline similars to all others used in this file.

Are you suggestion to just remove the line

signal.removeEventListener("abort", close);

or anything else in addition to that?

nazarhussain · 2024-05-22T13:18:37Z

packages/api/src/beacon/routes/beacon/pool.ts


-export type Api = {
+type AttestationList = ValueOf<typeof AttestationListType>;


This type is same as phase0.Attestation[] then why to declare with more complex patterns?

I think the idea is to always match the ssz type by deriving the ts type from it. In this specific case, I think it's even better if we stick to phase0.Attestation[] as for electra we will have to change it to allForks.Attestation[] and construct the ssz type dynamically based on the fork.

I am honestly in favor of changing it as well as you suggested, and remove all -List suffixed type unless those are more broadly used (outside routes file).

Those were added by @wemeetagain, let's get his thoughts first

I think the idea is to always match the ssz type by deriving the ts type from it.

Yeah that was the idea. This is how its done in our types package, and my thought was that we may want to move all these types defined in the api over to our types package.

nazarhussain · 2024-05-22T13:18:52Z

packages/api/src/beacon/routes/beacon/pool.ts

 */
- getPoolProposerSlashings(): Promise<ApiClientResponse<{[HttpStatusCode.OK]: {data: phase0.ProposerSlashing[]}}>>;
+ getPoolProposerSlashings: Endpoint<
+ //


Suggested change

//

same as above

nazarhussain · 2024-05-22T13:22:36Z

packages/api/src/beacon/routes/beacon/pool.ts

- submitPoolBlsToExecutionChange(
- blsToExecutionChange: capella.SignedBLSToExecutionChange[]
- ): Promise<ApiClientResponse<{[HttpStatusCode.OK]: void}, HttpStatusCode.BAD_REQUEST>>;
+ submitPoolBLSToExecutionChange: Endpoint<


I think we should defer changing Bls to BLS from this PR and do it project wise later.

This rename is required to make the routes testable against the spec which is critical for this PR to ensure correctness.

I don't feel strongly about renaming this, it's just required here to match spec operationId.

We could open an issue to do a follow-up renaming of other parts in the code after this PR is merged.

nazarhussain · 2024-05-22T13:31:21Z

packages/api/src/beacon/routes/node.ts

+ req: {
+ writeReq: ({syncingStatus}) => ({query: {syncing_status: syncingStatus}}),
+ parseReq: ({query}) => ({syncingStatus: query.syncing_status}),
+ schema: {query: {syncing_status: Schema.Uint}},


Uint have different range but this parameter have different integer range 100-599.

This is the same as on unstable, Schema.Unit will currently be translated to {type: "integer", minimum: 0};, we can add more accurate / ranged-based schema validation for integer values but this is the only API as far as I know that requires it.

We currently already enforce that the range is valid in server implementation

lodestar/packages/beacon-node/src/api/impl/node/index.ts

Lines 72 to 73 in 3121363

if (syncingStatus != null && (syncingStatus < 100 || syncingStatus > 599)) {

throw new ApiError(400, `Invalid syncing status code: ${syncingStatus}`);

nazarhussain · 2024-05-22T13:35:45Z

packages/api/src/beacon/routes/node.ts

+ url: "/eth/v1/node/syncing",
+ method: "GET",
+ req: EmptyRequestCodec,
+ resp: JsonOnlyResponseCodec,


When we say our API have full SSZ support then it we should not have JsonOnlyResponseCodec expect the lodestar namespace.

the node namespace has several response schemas that don't cleanly map to ssz types.

Future steps would be to update the spec to better align to ssz everywhere.
If we require ALL endpoints to support SSZ before landing this feature, then we will likely never land this feature.

When we say our API have full SSZ support

This is not the claim of this PR, but it provides first-class support for ssz on the api. What we can claim though is that we have full ssz support on all performance relevant apis, that includes mostly beacon and validator required apis.

If we require ALL endpoints to support SSZ before landing this feature, then we will likely never land this feature.

I don't even think this is practically useful. A lot of apis are not performance sensitive or mostly consumed by clients that do not even supports ssz, think dappnode querying the peer count.

I am not saying we shouldn't push for more apis to support ssz but there are limitations to it's usefulness in most of those cases.

nflaig · 2024-05-23T20:47:15Z

My major concern upto this point is the the lack of response codes from the routes definitions. Now use will never know any endpoint can response with 200, 206, 503, or 404.

We are putting alls calls in a promise box and saying it would be either fullfil or fail. But in case of failure user don't know what could be expected status codes.

This was a major information missing in earlier implementation and we added that in last refactor. I don't think loosing that context is a worthwhile.

We could add this by extending the Endpoint type but I honestly kinda happy that those are removed.

The reasons why I think those were not great are

not correctness checked against spec, e.g. getHeader claims it only returns 200 while the spec also has 204 defined (the spec also has no 404 while our types do...)
I have only found a single case in the code where we actually use the status code directly
we don't enforce those status codes anywhere to ensure implementation correctness, in current unstable, it's not even possible to set the status code from the server impl (although we can do that now)
the information communicated by the status code types is not very detailed, e.g. it says what status codes can be returned but not why those might be returned, making it hardly actionable without looking at the spec which contains more details

I personally don't think we should add status codes to each endpoint for reasons listed above but if we feel strongly about it could still be done.

nflaig · 2024-05-24T08:00:23Z

Request times from validator client point of view stable (on the left) vs. feat1 (on the right)

Last 7 days, rate interval 1d

getAttesterDuties

getProposerDuties

prepareBeaconCommitteeSubnet

produceAttestationData

submitPoolAttestations

produceBlockV3

publishBlockV2

wemeetagain and others added 30 commits October 30, 2023 16:39

Add createApiClientMethods

6f66e29

Start server impl

6a40e14

Add status code

863d536

Add route creation

3536dc6

Support ssz or value application responses

2a078cb

Simplify meta toHeaders

4b7c051

Clean up server status code

aeabcd6

Add refactored HttpClient

a0d5af7

Consistently rename params to args

677660b

Update getHealth example, no nested objects

a9a1f2c

Add headers to request

51b6656

Decouple header parsing from wire format, 415 error handling

70daf28

Rename supported media types const

92797bf

Update 415 error handling

4b7e26b

Uppercase header names in error messages

0ddc43c

Move functions around, delete old types and utilities

c47dfdc

Remove more

83cec8a

Refactor beacon block route definitions

f3b3f94

Simplify ApiResponse types

00b215c

Add sszTest example

e99ed79

Add beacon pool route definitions

b47369b

Add beacon state route definitions

f372000

Add remaining beacon route definitions

db81879

Add config route definitions

0f03542

Add debug route definitions

2d80b97

Add events route description

5e5feda

Add lightclient route definitions

3ecf5fb

Merge branch 'unstable' into cayman/ssz-api

ec2d09b

Flatten function params

94379d7

Type safety for optional params in write / parse req

44dd312

nflaig requested a review from a team as a code owner May 17, 2024 13:32

philknows modified the milestone: v1.19.0 May 21, 2024

nazarhussain assigned nflaig May 22, 2024

nazarhussain requested changes May 22, 2024

View reviewed changes

nflaig added 4 commits May 23, 2024 21:20

Do not set default value for context

40e5467

Update getClient return type to better align with method name

613f803

Consistent pattern to get route definitions

a667575

Merge branch 'unstable' into cayman/ssz-api

26d3017

Dedupe api client type for builder and keymanager

efaecc7

nflaig added 9 commits May 24, 2024 18:59

Fix fallback logic if server returns http error

bde4299

Update head event error logging

a1af1ce

Merge branch 'unstable' into cayman/ssz-api

8606807

Retry 415 errors with JSON and cache SSZ not supported

6a11417

Use fetch spy to assert call times

1512a5e

Update comment

7a8503b

Update getLightClientUpdatesByRange endpoint meta

3f44119

Do not forward ssz bytes of blinded block to publishBlock

28133d7

Fix lightclient e2e tests

a073e44

nflaig mentioned this pull request May 28, 2024

Check conten-type and return 415 if not supported by route sigp/lighthouse#5855

Open

nflaig added 6 commits May 28, 2024 22:22

Version header in publishBlock api is optional

17808bf

Reduce type duplication

0085dcf

Add option to override request init in route definition

6e1b7f5

Add JsonOnlyResp codec

5925900

Validate boolean str value from headers

07713f0

Document default wire formats

7876937

nflaig mentioned this pull request May 29, 2024

Lodestar CL < > Nimbus VC incompatibility #6634

Open

nflaig requested a review from nazarhussain May 29, 2024 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nflaig commented May 8, 2024 •

edited

nazarhussain left a comment

nazarhussain May 22, 2024

nflaig May 23, 2024

nazarhussain May 22, 2024

nflaig May 23, 2024

nazarhussain May 22, 2024

nflaig May 23, 2024

nazarhussain May 22, 2024

wemeetagain May 22, 2024

nazarhussain May 22, 2024

nflaig May 23, 2024

nazarhussain May 22, 2024

nflaig May 23, 2024

wemeetagain May 24, 2024

nazarhussain May 22, 2024

nflaig May 23, 2024

nazarhussain May 22, 2024

nflaig May 23, 2024 •

edited

nazarhussain May 22, 2024

nflaig May 23, 2024

nazarhussain May 22, 2024

wemeetagain May 22, 2024

nflaig May 23, 2024

nflaig commented May 23, 2024 •

edited

nflaig commented May 24, 2024


		export type Api = {
		type AttestationList = ValueOf<typeof AttestationListType>;

	if (syncingStatus != null && (syncingStatus < 100 \|\| syncingStatus > 599)) {
	throw new ApiError(400, `Invalid syncing status code: ${syncingStatus}`);

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

Are you sure you want to change the base?

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

Conversation

nflaig commented May 8, 2024 • edited

Motivation

Description

BREAKING CHANGES

Client initialization

Request with value

Request without value

Passing arguments

TODO

nazarhussain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nflaig May 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nflaig commented May 23, 2024 • edited

nflaig commented May 24, 2024

getAttesterDuties

getProposerDuties

prepareBeaconCommitteeSubnet

produceAttestationData

submitPoolAttestations

produceBlockV3

publishBlockV2

nflaig commented May 8, 2024 •

edited

nflaig May 23, 2024 •

edited

nflaig commented May 23, 2024 •

edited