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

Add client support for the lighthouse/peers route #4937

Open
wants to merge 8 commits into
base: unstable
Choose a base branch
from

Conversation

jmcph4
Copy link
Member

@jmcph4 jmcph4 commented Nov 20, 2023

Issue Addressed

N/A

Proposed Changes

This PR implements the client-side of the (non-standard) lighthouse/peers HTTP API route. To achieve this, the following changes have been made:

  • Addition of a new crate, serde_utils, under common. This is to handle the Deserialize logic for Instants and exists alongside other utility crates like clap_utils.
  • Addition of the Deserialize derive macro to various types under lighthouse_network (as this is where the majority of peer-related types exist)
  • Addition of the get_lighthouse_peers function alongside the existing eth2 API methods

Additional Info

This functionality is desirable as it allows downstream crates to contact the extended HTTP API programmatically. Desire for such functionality has already been alluded to in the codebase:

https://github.com/sigp/lighthouse/blob/stable/common/eth2/src/lighthouse.rs#L415-L421

@jmcph4 jmcph4 self-assigned this Nov 20, 2023
@jmcph4 jmcph4 added enhancement New feature or request work-in-progress PR is a work-in-progress HTTP-API labels Nov 20, 2023
@@ -0,0 +1,10 @@
[package]
name = "serde_utils"
Copy link
Member

Choose a reason for hiding this comment

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

We have an external package (ethereum_serde_utils) which defines a crate called serde_utils. I think we could either put this code there, give it a different crate name to avoid confusion, or put the Instant serialisation as a module in the lighthouse_network crate.

I think my preference is for the latter, as the Instant stuff is not really Ethereum-specific, and seems a bit too hacky (using SystemTime::now) to put in ethereum_serde_utils. Similarly, for future serde utils, if we deem them Ethereum-relevant we can put them in the extern crate, whereas if they're Lighthouse-specific we can keep them in-tree. Wdyt?

Copy link
Member Author

Choose a reason for hiding this comment

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

My preference is the former, although I understand your point about lack of Ethereum specificity (and concur). My issue with moving it to lighthouse_network is that, in my opinion, that crate is already too busy and this busyness complicates downstream use of the crate outside of Lighthouse. I already have some changes that I need to open a (separate) PR for that address some of this.

and seems a bit too hacky

I also agree here, but my desire for full client-side implementation of the HTTP API overrides this. This certainly isn't a nice solution but my feeling at the moment is that having missing routes is less nice than even this.

Copy link
Member

Choose a reason for hiding this comment

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

Ok cool. I'm happy with putting it in ethereum_serde_utils. I thought it might break WASM compatibility (which seems to be something people want from our crates) but it seems it will compile fine and just panic on WASM, which is fine as it's completely optional (rust-lang/rust#48564).

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wow, hadn't considered that WASM quirk before!

@AgeManning
Copy link
Member

Just bumping this. Anyone know the current state? @michaelsproul @jmcph4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request HTTP-API work-in-progress PR is a work-in-progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants