[Merged by Bors] - Improve HTTP API error messages + tweaks by michaelsproul · Pull Request #4595 · sigp/lighthouse

michaelsproul · 2023-08-10T02:50:12Z

Issue Addressed

Closes #3404 (mostly)

Proposed Changes

Remove all uses of Warp's and_then (which backtracks) in favour of then (which doesn't).
Bump the priority of the POST method for v2/blocks to P0. Publishing a block needs to happen quickly.
Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging [Merged by Bors] - Use BeaconProcessor for API requests #4462 and [Merged by Bors] - Support SSZ request body for POST /beacon/blinded_blocks endpoints (v1 & v2) #4504/[Merged by Bors] - add ssz support in request body for /beacon/blocks endpoints (v1 & v2) #4479.
Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped.

Additional Info

I've tested this manually and can confirm that we no longer get the dreaded Unsupported endpoint version errors for queries like:

$ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq
{
  "code": 400,
  "message": "BAD_REQUEST: WeakSubjectivityConflict",
  "stacktraces": []
}

$ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq
{
  "code": 400,
  "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)",
  "stacktraces": []
}

$ curl "http://localhost:5052/eth/v2/validator/blocks/7067595"
{"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]}

However, I can still trigger it by leaving off the Content-Type. We can re-test this aspect with #4575.

michaelsproul · 2023-08-10T02:51:18Z

beacon_node/http_api/src/task_spawner.rs

    process_fn: BlockingOrAsync,
    rx: oneshot::Receiver<T>,
-) -> Result<T, Response> {
+) -> Result<T, warp::Rejection> {


@paulhauner I refactored this to return a Rejection so that it can be plumbed through to spawn_async_with_rejection_no_conversion without being silently dropped.

beacon_node/http_api/src/lib.rs

beacon_node/http_api/src/task_spawner.rs

jimmygchen

Nice!! Looks good 👍

paulhauner

This looks good to me, it fixes some nice bugs.

From our chat with @jxs I got the impression that using Rejection as a way to return an error to the client was not the author's intention with warp. I.e., the author expects us to use Err(Rejection) to indicate that another route should be used internally and Ok(Response) to return a 500/400/etc to the client.

I can see that our current approach of using Rejection for client-facing errors does actually work, so I'm not opposed to going all-in on it. I wonder if you'd considered the inverse; replacing our Result<_, Rejection> with a new Result<_, HttpError> where HttpError impl Into<Response>?

Or perhaps my understanding of how warp wants to be used is not correct.

beacon_node/http_api/src/lib.rs

michaelsproul · 2023-08-14T01:02:09Z

I wonder if you'd considered the inverse; replacing our Result<, Rejection> with a new Result<, HttpError> where HttpError impl Into?

Yeah I think removing all usages of Rejection eventually could be good. I left it in there for now just to avoid churn. I think even if we did have a new type we'd still need to use then instead of and_then, and do the mapping from Result<_, HttpError> to response ourselves, so it should look pretty similar to this, just with a different type in the convert_rejection func

Thanks for the review!

michaelsproul · 2023-08-14T02:16:58Z

bors r+

## Issue Addressed Closes #3404 (mostly) ## Proposed Changes - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging #4462 and #4504/#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. ## Additional Info I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with #4575.

bors · 2023-08-14T03:15:51Z

Build failed (retrying...):

release-tests-windows

## Issue Addressed Closes #3404 (mostly) ## Proposed Changes - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging #4462 and #4504/#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. ## Additional Info I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with #4575.

bors · 2023-08-14T04:49:47Z

Pull request successfully merged into unstable.

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

Closes sigp#3404 (mostly) - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging sigp#4462 and sigp#4504/sigp#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with sigp#4575.

Improve HTTP API error messages + tweaks

828d6ee

michaelsproul added ready-for-review The code is ready for review HTTP-API v4.4.1 ETA August 2023 labels Aug 10, 2023

michaelsproul commented Aug 10, 2023

View reviewed changes

This was referenced Aug 10, 2023

Counterintuitive warp error for Unsupported endpoint version #3404

Closed

Fix HTTP error handling in VC API #4597

Closed

jimmygchen reviewed Aug 10, 2023

View reviewed changes

beacon_node/http_api/src/task_spawner.rs Show resolved Hide resolved

Docs

7c04d90

michaelsproul mentioned this pull request Aug 10, 2023

[Merged by Bors] - Don't kill SSE stream if channel fills up #4500

Closed

1 task

jimmygchen approved these changes Aug 10, 2023

View reviewed changes

eserilev mentioned this pull request Aug 10, 2023

Assume Content-Type is json for endpoints that require json #4575

Merged

paulhauner approved these changes Aug 14, 2023

View reviewed changes

beacon_node/http_api/src/lib.rs Show resolved Hide resolved

michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Aug 14, 2023

bors bot changed the title ~~Improve HTTP API error messages + tweaks~~ [Merged by Bors] - Improve HTTP API error messages + tweaks Aug 14, 2023

bors bot closed this Aug 14, 2023

michaelsproul deleted the warp-no-backtrack branch August 14, 2023 05:25

protocolwhisper mentioned this pull request Aug 14, 2023

(fix): HTTP error handling in VC API #4606

Closed

michaelsproul mentioned this pull request Aug 15, 2023

Unexpected Unsupported endpoint version on REST API #4620

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Improve HTTP API error messages + tweaks#4595

[Merged by Bors] - Improve HTTP API error messages + tweaks#4595
michaelsproul wants to merge 2 commits intosigp:unstablefrom
michaelsproul:warp-no-backtrack

michaelsproul commented Aug 10, 2023 •

edited

Loading

Uh oh!

michaelsproul Aug 10, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimmygchen left a comment

Uh oh!

paulhauner left a comment •

edited

Loading

Uh oh!

Uh oh!

michaelsproul commented Aug 14, 2023

Uh oh!

michaelsproul commented Aug 14, 2023

Uh oh!

bors bot commented Aug 14, 2023

Uh oh!

bors bot commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

michaelsproul commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue Addressed

Proposed Changes

Additional Info

Uh oh!

michaelsproul Aug 10, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimmygchen left a comment

Choose a reason for hiding this comment

Uh oh!

paulhauner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelsproul commented Aug 14, 2023

Uh oh!

michaelsproul commented Aug 14, 2023

Uh oh!

bors bot commented Aug 14, 2023

Uh oh!

bors bot commented Aug 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

michaelsproul commented Aug 10, 2023 •

edited

Loading

paulhauner left a comment •

edited

Loading