Skip to content
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

POST endpoints for validators and validator_balances #367

Merged
merged 4 commits into from
Nov 7, 2023

Conversation

michaelsproul
Copy link
Collaborator

Some API users want to fetch large chunks of the validator set, and currently do so using the id param in the query string for GET /eth/v1/beacon/states/{state_id}/validators. However, query strings are usually subject to length limits on both the client and server side. For Lighthouse, we can't extend the length limit without forking our HTTP server (see sigp/lighthouse#4333 (comment)).

To remedy this situation, this PR introduces a POST variant of the endpoint which is semantically identical, but passes the IDs and statuses in the request body. This is preferable to a GET request with body, as GETs with bodies do not conform to HTTP standards.

Currently, the only alternatives are sub-optimal:

  • Make lots of small requests, which is slow on clients that load full states, particularly if done in parallel (see Implement POST validators/validator_balances APIs sigp/lighthouse#4872).
  • Make a single request for the entire validator set. This has the downside of consuming a large amount of memory, and generating useless work on clients that are capable of loading just the relevant parts of a BeaconState.

I don't think this is a client-engineering problem, because designing a database around its ability to load partial states is not likely to be space-efficient. The most space-efficient schemes known currently use state diffs, which don't (usually) provide the ability to be partially applied. Lighthouse's tree-states release track uses a variant of the state diffs described by Anton Nashatyrev here.

Hacking query string limits across clients is also unlikely to provide a long-term fix, as every new HTTP client that interacts with the beacon API may impose its own length-limit that breaks compatibility.

@jclapis
Copy link

jclapis commented Oct 23, 2023

+1 to this, Rocket Pool uses this and runs into the limitation on GET request strings all the time. We have to limit our requests to 600 validators per call to get around the size limit, which ends up requiring around 50 parallel calls to get all of our validators in the set on Mainnet. When we call it for a state beyond the head, it does tend to slow things down. If we had a POST route where we could put all of the validators in, we could do it with a single call and probably remove a lot of the DB thrashing the clients do.

@rolfyone
Copy link
Collaborator

I'd be happy for this, i do wonder if endpoints like this should support ssz or at least gzip compression, as the response would compress very well...

@michaelsproul
Copy link
Collaborator Author

@rolfyone Agree we should pursue SSZ. I think there's broad support for it in #333, just waiting for a PR to the spec.

@cheatfate
Copy link

GET /eth/v1/beacon/states/{state_id}/validator_balances has same problem with id param used for filtering. So probably it should be also extended with POST call.

@rolfyone
Copy link
Collaborator

rolfyone commented Nov 5, 2023

can we add this to changelog then I'll approve and merge...

@michaelsproul michaelsproul changed the title POST /eth/v1/beacon/states/{state_id}/validators POST endpoints for validators and validator_balances Nov 5, 2023
@michaelsproul
Copy link
Collaborator Author

Updated the changelog and added a POST for validator_balances per @cheatfate's suggestion

Copy link
Collaborator

@dapplion dapplion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strict improvement, looking forward to adopt in Lodestar

Copy link
Collaborator

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - thanks everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants