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

Basic Beacon API #3

Merged
merged 2 commits into from
Dec 10, 2019
Merged

Basic Beacon API #3

merged 2 commits into from
Dec 10, 2019

Conversation

paulhauner
Copy link
Contributor

@paulhauner paulhauner commented Aug 22, 2019

What

Adds a markdown document describing a basic beacon API that provides block, state, head and network information.

Why

In the short term, clients need to interoperate and it would be helpful if they implemented standard, human-readable APIs so we don't need to go scrounging through logs to find simple things. Also, so people can build tools that can assess our interoperability foo.

In the long term, it'd be great if clients standardize upon API functionality to help make a unified developer ecosystem. Maybe this document can be the basis for this standardization effort.

Details

What this is:

  • Testing the waters to try and find a good format that is:
    • Easily maintainable (i.e., easy to read and modify the source).
    • Not going to require lots of work whenever the spec data structures change.
    • Well defined enough to allow someone to build a HTTP-wrapper library from it.
    • Not completely HTTP-centric; sets a standard that can be used for other transports (e.g., gRPC).
    • Acknowledges that HTTP seems to be the preferred API among clients.
    • Provides easy, curl-able examples.
    • Small enough to go from nothing to something with minimal contention.

What this is not:

  • Optimized for data transmission (e.g., if you want the finalized checkpoint you need to download the whole state).
  • The full suite of useful API endpoints (e.g., there's no shuffling endpoints).

Use cases

Using this API, we can perform these tasks:

  • $ curl /network/enr and copy-paste it into a --bootnodes CLI flag on another node.
  • $ curl /network/peer_id to get the node's libp2p PeerId.
  • $ curl /network/peers to see if a node is peered with some other node (comparing peer ids).
  • Check that some nodes agree on the same chain at some slot using /beacon/block?slot=42 and comparing the returned root fields.
  • Check that two nodes share the same finalized root by using /beacon/head to learn the state root at the head of the chain, then use /beacon/state to read the finalized checkpoints.

Additionally, I hope this API can be used during network testing/analysis with tools that detect network connectivity (connected peers, etc) and canonical chain info (head, blocks at slots, etc).

Notes

  • This API does not allow for requesting multiple blocks or states. I left it out for simplicity, thinking we can add it later. I could be persuaded to add it now, if it's widely desired.
  • This API allows a useful method we've implemented in Lighthouse, where you "bootstrap" a node by pointing it at another node's HTTP interface and download the genesis state and peer connection info.

@paulhauner paulhauner mentioned this pull request Aug 22, 2019
@paulhauner
Copy link
Contributor Author

paulhauner commented Aug 22, 2019

As mentioned by @terencechain in #2, Prysm have been doing lots of work on the API too. This is great and I've been using it as a reference for this.

After chatting on Discord, I thought it might be useful if I listed points where this PR differs from the Prysm docs. That way we can see if I've made some errors in my decision making (likely).

I reference mainly api.prylabs.net instead of ethereumapis because the former is in HTTP-style and the latter is more gRPC focused.

Get blocks/states by epoch

I think it's ambiguous as to whether or not querying by epoch should return slots_per_epoch blocks, or the first block in the epoch (e.g., a finalized checkpoint).

I don't think this query is strictly necessary and that the typing in our API would be clearer without it.

Can't get multiple blocks/states

Trying to keep this PR small. There's a bit of complexity in getting multiple blocks (e.g., handling skip slots, insufficient history, etc). If it's a must have immediately, I'd propose we do that in a separate PR following this one.

/eth/v1aplha1 prefix

I'm not sure of the value of the /eth/ prefix. It seems to me that having /beacon is enough without an /eth/ prefix. I might be missing something though.

WRT to the v1alpha1, versioning makes sense. I figure we can define versioning in a later PR, however I'm happy to do it now if desired.

chainhead method replaced by head

My idea was to keep the head method small for now and say that reading the finalized checkpoints is a state-reading optimization for a future PR (e.g., provide a /state/finalized_checkpoint method so you don't need to download a whole state). Perhaps I'm being too minimal?

@paulhauner
Copy link
Contributor Author

Also, #1 is still open so we could decide that this markdown format is a bad idea.

@prestonvanloon
Copy link
Contributor

Get blocks/states by epoch

Filed that note here: https://github.com/prysmaticlabs/ethereumapis/issues/23
Replace the epoch filter and slot filter with a slot-range filter sounds like a great idea to me.

Can't get multiple blocks/states

If you are looking for a long lasting API, I urge you to reconsider. What is the use case where users would only want one block/state, aside from debugging?

We removed state fetches from our data API as it was going to be far too expensive to serve on average. Maybe the methods you are suggesting are under a /debug service which can be conditionally enabled in some deployments.

/eth/v1aplha1 prefix

The decision for this was largely influenced by the idea what github.com/prysmaticlabs/ethereumapis could be a large collection of any API that applied in the ethereum ecosystem. Layer 2 solutions, public dApps, anything with an API. So, we prefix with the general domain eth and the versioning v1alpha.

In the context of this repository where it is only Ethereum 2.0 APIs, it isn't necessary.
The versioning prefix, we took inspiration from this guide: https://cloud.google.com/apis/design/versioning
We haven't written out the version migration pattern, but likely v1beta1 would launch with phase 0 then we finalize it at some point after running in production to v1

chainhead method replaced by head

I don't see how this is better to make multiple requests for the head information. I suppose if you only wanted a subset of the payload then you could use GraphQL or some response filter.

@paulhauner
Copy link
Contributor Author

Thanks for the response!

If you are looking for a long lasting API, I urge you to reconsider. What is the use case where users would only want one block/state, aside from debugging?

I'm totally happy to make a new PR straight after this that describes that.

Are your urging me to include it in this PR?

The versioning prefix, we took inspiration from this guide: https://cloud.google.com/apis/design/versioning

Cool. Do you think we should version the /beacon and /network endpoints individually, or group them all under one version?

I don't see how this is better to make multiple requests for the head information.

Yeah, I'm down to add more info to /beacon/head. You've changed my mind.

@cemozerr
Copy link

Great work @paulhauner! I'm going through, implementing this API for Artemis right now. For the "/beacon/head" path, do you think it's necessary to return beacon state root? IMO, since beacon blocks already include the state root, that might be a bit redundant. Also, the Store uses beacon block roots to index the corresponding state in time.

@djrtwo
Copy link
Contributor

djrtwo commented Oct 31, 2019

I'm keen to get some basics merged here. Where do things stand @paulhauner. Any thing to update/modify before an initial merge?

@cemozerr
Copy link

cemozerr commented Nov 1, 2019

To add to my previous comment, in the "/beacon/state" path, would it not be better to index the beacon state using a beacon block root hash tree root, since that how we index beacon state's in the Store object?

@jrhea
Copy link

jrhea commented Nov 2, 2019

@paulhauner is it worth adding epoch in addition to slot (for each response) considering that someone could be running minimal constants?

@paulhauner
Copy link
Contributor Author

@cemozerr: Great work @paulhauner! I'm going through, implementing this API for Artemis right now. For the "/beacon/head" path, do you think it's necessary to return beacon state root? IMO, since beacon blocks already include the state root, that might be a bit redundant. Also, the Store uses beacon block roots to index the corresponding state in time.

Thank you! I agree that it's not maximally minimal (lol) to return the beacon state root, however it is convenient for users. @prestonvanloon touched upon this in the "chainhead method replaced by head" part of his comment. I ended up capitulating to Preston's idea that including things like state_root in the /head route will make life easier for API consumers and it seems to come at little cost for us developers.

@cemozerr: To add to my previous comment, in the "/beacon/state" path, would it not be better to index the beacon state using a beacon block root hash tree root, since that how we index beacon state's in the Store object?

I assume the Store object is the one referenced in fork choice?

I think it may be confusing to users if we index BeaconState by the root of a BeaconBlock, as it deviates from the easy-to-comprehend concept of "index Thing by root(Thing)".

My gut feeling is that keying BeaconState by a BeaconBlock root would be shaping the API to suit a particular description of the fork choice algorithm, instead of shaping it to make life easier for API consumers. I would understand if the specification was heavily biased towards keying by block root (instead of state root) but as far as I can tell, the Store is only used to describe a fork choice implementation and that implementation is not "production ready" (it's slow) and therefore unlikely to be used in the long term.

@jrhea: is it worth adding epoch in addition to slot (for each response) considering that someone could be running minimal constants?

I could be wrong, but I think you're suggesting to permit querying for items via epoch, as well as slot and root? Sorry if I misunderstood.

If so, I remember chatting to @prestonvanloon about this. I noted that when querying by epoch it's not clear if you should get SLOTS_PER_EPOCH items or the item at the first slot of the epoch. IIRC we agreed that the benefits from querying by epoch don't outweigh the potential confusion.

@djrtwo: I'm keen to get some basics merged here. Where do things stand @paulhauner. Any thing to update/modify before an initial merge?

I think the main thing to consider is #1. Since @mpetrunic is pushing forward with the Swagger API, perhaps this markdown version is just duplicate effort?

If we do want to go ahead with this, I think all I need to do is add more info to the /head endpoint (as per @prestonvanloon's comments, but perhaps not if @cemozerr disagrees).

@jrhea
Copy link

jrhea commented Nov 4, 2019

@paulhauner thanks for the reply.

I could be wrong, but I think you're suggesting to permit querying for items via epoch, as well as slot and root? Sorry if I misunderstood.

Actually, I was suggesting adding epoch to the response. Agreed that it would be confusing to query by epoch, but there is some value to knowing the epoch of the response bc slots_per_epoch is different for minimal and main net configs

@mpetrunic
Copy link
Contributor

mpetrunic commented Nov 4, 2019

Do we wanna follow more restful endpoints like:

  • beacon/blocks/head
  • beacon/blocks/{root}
  • beacon/slots/{slot}
    ?

https://restfulapi.net/resource-naming/

@paulhauner
Copy link
Contributor Author

paulhauner commented Nov 6, 2019

Actually, I was suggesting adding epoch to the response. Agreed that it would be confusing to query by epoch, but there is some value to knowing the epoch of the response bc slots_per_epoch is different for minimal and main net configs

Oh yeah I'm with you now. We had this problem on Lighthouse, we had to expose a spec/slots_per_epoch endpoint to resolve it.

I think my preference would be to define the config.yaml as a struct which can be passed over the wire. We do something along these lines (same information, different struct) for Lighthouse, which ensures that the validator client works on the same constants as the beacon node. Thoughts?

EDIT (accidental submission):

Do we wanna follow more restful endpoints like:

I don't have strong feelings on this. My understanding was that query parameters are more common in the wild, but I'm not an active "web developer" so I could be wrong. I also like query params because it's easy to reason about them as a hashmap and it makes routing a tiny bit easier. Like I said though, no strong feelings on the topic.

@mkalinin
Copy link

I am in favor of conformance with validator API and using RESTful endpoints like @mpetrunic mentioned. This is very common practice in the web. Usually, query string contains optional parameters of the request while parameter in the path makes it obligatory which is straightforward when resources are requested by their identifiers.

@djrtwo
Copy link
Contributor

djrtwo commented Dec 10, 2019

Merging to get the base in. We can work from here.

@djrtwo djrtwo merged commit 2717ab1 into ethereum:master Dec 10, 2019
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.

7 participants