-
Notifications
You must be signed in to change notification settings - Fork 231
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
Check conten-type and return 415 if not supported by route #6321
Comments
According to currently released version of Beacon-API specification https://ethereum.github.io/beacon-APIs/#/Validator/getAttesterDuties there is no such error as 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). |
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.
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. |
If this is desirable, agree with @cheatfate, this should be standardized in the beacon API.
As is happening here. But so far it appears that it has not achieved consensus. |
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 |
@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. |
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 |
I have opened a PR on the spec to clarify this behavior |
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 bodyAs 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)
The text was updated successfully, but these errors were encountered: