Check conten-type and return 415 if not supported by route about nimbus-eth2 HOT 6 CLOSED

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.

from nimbus-eth2.

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).

from nimbus-eth2.

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 ethereum/beacon-APIs#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.

from nimbus-eth2.

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

from nimbus-eth2.

@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 ethereum/beacon-APIs#250 is merged, this can be revisited/reopened.

from nimbus-eth2.

@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

from nimbus-eth2.