search

attestantio / go-eth2-client

Apache License 2.0
102 stars 59 forks source link

Opt-in to debug endpoint usage in `Validators` #100

Closed moshe-blox closed 5 months ago

moshe-blox commented 5 months ago

The Beacon API spec says that the debug endpoints are:

Set of endpoints to debug chain and shouldn't be exposed publicly.

For that reason, Prysm disables them by default, and since the Validators method leverages the debug endpoint to retrieve large validator sets, it's likely to fail in many setups.

I believe that go-eth2-client should either check that these endpoints are available as part of client creation, or allow the user to opt-in to using them.

This PR implements the latter, since I suspect that requesting the state at every connection may be too heavy.

@mcdee If you think opting-in at the client-level is preferable to method-level, I wouldn't mind refactoring that.

mcdee commented 5 months ago

The text from the spec differs a little from reality nowadays, in that the state endpoint is generally recognized as useful. There was discussion about moving it outside of the debug namespace due to this, but it was ultimately decided to leave it there to avoid the potential confusion of moving the endpoint around (see https://github.com/ethereum/beacon-APIs/pull/357 for details).

I would suggest that given the linked conversation it would make sense for prysm to default to enabling the debug endpoints. All other beacon nodes provide these without additional flags. Having what amounts to a specific flag for the current release of prysm isn't something I'm interested in adding to the client, especially given the performance savings when obtaining large numbers of validators via state.

moshe-blox commented 5 months ago

@mcdee I agree regarding Prysm, though I think it would've been safer to wait until it's no longer a debug route before making it the default behavior.

With that said, after checking your claim, BeaconState is indeed so much faster! (~13s vs. ~55s with 5k pubkeys)

We're likely gonna add a preflight check by calling BeaconState on boot, so that our node fails early and alerts the user to enable the Prysm flag, so we don't need this PR anymore.

Thanks 🙏