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

Make API friendly to block explorers #702

Merged
merged 31 commits into from Dec 19, 2019
Merged

Make API friendly to block explorers #702

merged 31 commits into from Dec 19, 2019

Conversation

paulhauner
Copy link
Member

@paulhauner paulhauner commented Dec 11, 2019

Issue Addressed

Proposed Changes

Point-by-point addressing of @ppratscher's requests:

The blocks object should also include the block root hash alongside the state root hash and parent root hash

This has been done, see /beacon/block.

Currently I could not find any call that returns the current balance of a validator, it would be great if it is included in the get_beacon_validators response alongside the effective balance

The /beacon/validators endpoint now includes the balance.

The get_beacon_validators should include the validator index alongside the validator public key

The /beacon/validators endpoint now includes the validator index.

Add a get_beacon_validators_duties that returns the duties of all validators for a given epoch, otherwise we will have to call get_beacon_validator_duties with all public keys which might be not that performant (but I haven't tested this up to now)

The /beacon/validators/all endpoint serves this request. It's returning quite quickly on my end (40ms).

In order to de-aggregate attestations and associate an attestation with the included validators we will need the beacon committees for each epoch. I couldn't find any call that returns the committees by epoch

The /beacon/committees endpoint should fulfill this request.

In order to show participation statistics for each epoch add a call that shows the eligible ether, voted ether and the participation rate for each epoch

The consensus/global_votes endpoint should serve this request.

  • eligible ether = previous_epoch_active_gwei
  • voted ether = previous_epoch_attesting_gwei
  • participation rate = voted ether / eligible ether (I didn't see the need to include this in the endpoint)

Notes

This is a work-in-progress.

@paulhauner paulhauner added the work-in-progress label Dec 11, 2019
@paulhauner paulhauner self-assigned this Dec 11, 2019
@paulhauner
Copy link
Member Author

@paulhauner paulhauner commented Dec 12, 2019

@ppratscher, I have finished implementing the endpoints you were after and I have addressed each point in the top of this PR.

I will move onto unit testing next, however I have tested the endpoints by hand. If you are eager, feel free to start building off this branch. Otherwise, feel free to wait until it has been merged to master.

@peterbitfly
Copy link

@peterbitfly peterbitfly commented Dec 12, 2019

@paulhauner thanks a lot for your efforts! we will be starting next week on Monday to add a compatibility layer to beaconcha.in in order to support lighthouse. Will report back in case we run into any issues.

@paulhauner paulhauner added ready-for-review and removed work-in-progress labels Dec 13, 2019
@paulhauner paulhauner marked this pull request as ready for review Dec 13, 2019
@paulhauner paulhauner added this to the v0.2.0 milestone Dec 15, 2019
@paulhauner paulhauner removed this from the v0.2.0 milestone Dec 15, 2019
@paulhauner paulhauner added this to the v0.1.1 milestone Dec 15, 2019
@pscott
Copy link
Contributor

@pscott pscott commented Dec 15, 2019

Just read the docs in this PR, everything looked good to me! Disappointed I couldn't find a single typo... :p

@michaelsproul michaelsproul self-requested a review Dec 15, 2019
Copy link
Member

@michaelsproul michaelsproul left a comment

Looks good overall, would just like to discuss some of the points raised about the /consensus end point.


### Returns

Returns an object containing a single [`BeaconBlock`](https://github.com/ethereum/eth2.0-specs/blob/v0.9.2/specs/core/0_beacon-chain.md#beaconblock) and it's signed root.
Copy link
Member

@michaelsproul michaelsproul Dec 16, 2019

Choose a reason for hiding this comment

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

its ;)

book/src/http_consensus.md Show resolved Hide resolved
- `previous_epoch_active_gwei`: as above, but during the previous epoch.
- `current_epoch_attesting_gwei`: the total staked gwei that had one or more
attestations included in a block during the current epoch (multiple
attestations by the same validator does not increase this figure).
Copy link
Member

@michaelsproul michaelsproul Dec 16, 2019

Choose a reason for hiding this comment

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

does not -> do not?

figure must be equal to or less than `current_epoch_attesting_gwei`.
- `previous_epoch_attesting_gwei`: see `current_epoch_attesting_gwei`.
- `previous_epoch_target_attesting_gwei`: see `current_epoch_target_attesting_gwei`.
- `previous_epoch_head_attesting_gwei`: see `previous_epoch_head_attesting_gwei`.
Copy link
Member

@michaelsproul michaelsproul Dec 16, 2019

Choose a reason for hiding this comment

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

I guess this should read "see current_epoch_head_attesting_gwei", and current_epoch_attesting_gwei should have a description above. Would be nice to arrange all the current_ fields into one block of bullet points, and follow them with all the "see above" previous_ fields.

book/src/http_consensus.md Show resolved Hide resolved

## `/network/peers`

Requests the beacon node for one `MultiAddr` for each connected peer.
Copy link
Member

@michaelsproul michaelsproul Dec 16, 2019

Choose a reason for hiding this comment

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

Gramatically awkward, maybe:

Requests one MultiAddr for each peer connected to the beacon node


## `network/enr`

Requests the beacon node for it's listening `ENR` address.
Copy link
Member

@michaelsproul michaelsproul Dec 16, 2019

Choose a reason for hiding this comment

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

its

Method | GET
JSON Encoding | Object
Query Parameters | `epoch`
Copy link
Member

@pawanjay176 pawanjay176 Dec 16, 2019

Choose a reason for hiding this comment

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

This should be a POST method with query params None.

### Parameters

Requires the `epoch` (`Epoch`) query parameter to determine which epoch will be
considered the current epoch.
Copy link
Member

@pawanjay176 pawanjay176 Dec 16, 2019

Choose a reason for hiding this comment

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

I think this should be a RequestBody with epoch and pubkeys as parameters.

@michaelsproul michaelsproul added waiting-on-author and removed ready-for-review labels Dec 18, 2019
@peterbitfly
Copy link

@peterbitfly peterbitfly commented Dec 18, 2019

Thanks a lot for your efforts. Today we completed the integration of the lighthouse rpc api into our block explorer. Must of the calls work as expected, we just found 1 oddity:

For some states calling /beacon/validators/all returns Unable to read pubkey cache: PubkeyCacheIncomplete { cache_len: 0, registry_len: 16384 }

An example would be retrieving the validators with the state root of slot 192:
http://localhost:5052/beacon/validators/all?state_root=0xdf0b538dbf80d2a1d1933b80052a740e1a4f07297f83c8948650977ecf09d418

@paulhauner
Copy link
Member Author

@paulhauner paulhauner commented Dec 18, 2019

For some states calling /beacon/validators/all returns Unable to read pubkey cache: PubkeyCacheIncomplete { cache_len: 0, registry_len: 16384 }

Thanks @ppratscher, I know exactly what this is and will fix it.

@paulhauner
Copy link
Member Author

@paulhauner paulhauner commented Dec 19, 2019

All comments have been addressed :)

@paulhauner paulhauner added ready-for-review and removed waiting-on-author labels Dec 19, 2019
Copy link
Member

@michaelsproul michaelsproul left a comment

Looks good!

@michaelsproul michaelsproul added ready-to-merge and removed ready-for-review labels Dec 19, 2019
@paulhauner paulhauner merged commit 251aea6 into master Dec 19, 2019
7 checks passed
@AgeManning AgeManning deleted the bitfly branch Jan 3, 2020
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.

5 participants