Check conten-type and return 415 if not supported by route

[nflaig]

Describe the bug

Noticed that Nimbus BN does not check the content type of the request body and just tries to deserialize it, causing strange errors.

E.g. Error on Lodestar when trying to call getAttesterDuties with SSZ request body

[vc-06-geth-nimbus-lodestar] May-28 22:37:20.092[]                error: Failed to download attester duties epoch=0 - getAttesterDuties failed with status 400: Invalid validator's index value(s)
[vc-06-geth-nimbus-lodestar] Error: getAttesterDuties failed with status 400: Invalid validator's index value(s)

As per spec, this route does not require to support SSZ request bodies and by default Lodestar will be using JSON but it would still be nice if Nimbus could return a 415 error as this would allow to gradually support SSZ for more routes as the client can retry the request with JSON body and cache for this route that SSZ is not supported, only sending JSON in subsequent requests.

To Reproduce

I've been using Kurtosis to run the tests (with custom lodestar image)

participants:
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: lodestar
    cl_image: nflaig/lodestar:ssz-api
    vc_type: nimbus
    vc_image: statusim/nimbus-validator-client:amd64-latest
    vc_extra_params:
      - --doppelganger-detection=off
    count: 1
  - el_type: geth
    el_image: ethereum/client-go:stable
    cl_type: nimbus
    cl_image: statusim/nimbus-eth2:amd64-latest
    vc_type: lodestar
    vc_image: nflaig/lodestar:ssz-api
    count: 1

[cheatfate]

According to currently released version of Beacon-API specification https://ethereum.github.io/beacon-APIs/#/Validator/getAttesterDuties there is no such error as 415 as well as there is only application/json encoded body support. What version of specification you are using?

From my point of view Nimbus behavior is absolutely correct, because you are trying to supply it with incorrect validator indices, so you got correct response of 400 (Invalid request).

[nflaig]

According to currently released version of Beacon-API specification https://ethereum.github.io/beacon-APIs/#/Validator/getAttesterDuties there is no such error as 415 as well as there is only application/json encoded body support. What version of specification you are using?

As noted in the issue, it is not defined in the spec but it would allow clients to use ssz by default if they wanna implement it. This is also just general good behavior of any server api, not specific to beacon api per se.

From my point of view Nimbus behavior is absolutely correct, because you are trying to supply it with incorrect validator indices, so you got correct response of 400 (Invalid request).

I wouldn't say the response is incorrect but in general, if a more specific http status code (415 in this case) can be used instead of 400 it should be preferred.

The goal of this is really to incrementally increase ssz support for more routes, servers returning 415 if they don't support the content type is kinda required for that, see related issue https://github.com/ethereum/beacon-APIs/issues/250.

The alternative which we use now is to default to json for all routes that do not support ssz as per spec and provide a CLI flag to override that behavior, but that means a user has to do it manually, and it probably won't be possible in any multi node setup.

If you currently have to implement this per route instead of having a generic handler for all routes I can see that this is quite a lot of effort to implement and maintain.

I have just been running interop tests with ssz with all clients and opened those issues as currently only Teku and Lodestar consistently return 415, and Lighthouse just opened a PR for it. There is no rush on this, this is really just to get an overview of current state and get visibility on this / improve beacon api server behavior.

[tersec]

If this is desirable, agree with @cheatfate, this should be standardized in the beacon API.

The goal of this is really to incrementally increase ssz support for more routes, servers returning 415 if they don't support the content type is kinda required for that, see related issue ethereum/beacon-APIs#250.

As is happening here. But so far it appears that it has not achieved consensus.

[nflaig]

this should be standardized in the beacon API.

makes sense, we might wanna document this more centrally on what are expectations of a well-behaved beacon-api server

could extend what's already documented here which already has some notes about expected headers

    All requests by default send and receive JSON, and as such should have either or both of the "Content-Type: application/json"
    and "Accept: application/json" headers.  In addition, some requests can return data in the SSZ format.  To indicate that SSZ
    data is required in response to a request the header "Accept: application/octet-stream" should be sent.  Note that only a subset
    of requests can respond with data in SSZ format; these are noted in each individual request.

And then get feedback in the spec PR if it's feasible for everyone to implement

[tersec]

@nflaig Is this issue effectively resolved then, as far as Nimbus's implementation is concerned, or are there still things required to resolve it?

If/when https://github.com/ethereum/beacon-APIs/issues/250 is merged, this can be revisited/reopened.

[nflaig]

@nflaig Is this issue effectively resolved then, as far as Nimbus's implementation is concerned

If not every client supports this it will mean we can't enable ssz requests by default for all routes but that's not a big issue.

I still think it would be better error handling to return a 415 and improve content type validation but it's not well-defined as per spec so can consider this closed.

Closing this, better to discuss on spec-related issue / PRs

[nflaig]

I have opened a PR on the spec to clarify this behavior

• https://github.com/ethereum/beacon-APIs/pull/457