Modularize or Simplify the eth2 crate

[AgeManning]

Description

The common/eth2 crate appears to import the whole world. Other crates that depend on eth2 are compiling and building libp2p and a whole host of other things.

It would be great if we could significantly reduce the dependency chain here. Either by splitting up some of the structs or removing unnecessary imports.

[dapplion]

I remember asking myself this when doing some PoC a long time ago that depended on common/eth2

[dapplion]

The dependency on libp2p is only for these types

use lighthouse_network::{types::SyncState, PeerInfo};
use lighthouse_network::{ConnectionDirection, Enr, Multiaddr, PeerConnectionStatus};
use lighthouse_network::PeerId;

[hopinheimer]

hi @AgeManning do you think I can swoop in on this?

[dapplion]

From an offline chat with @AgeManning we suggested to:

• Return the serialized ENR as a String to avoid the dependency on discv5
• Import PeerID from its own crate
• Move the misc types SyncState, ConnectionDirection, etc to their own crate of only types and make it a common dependency of lighthouse_network and common/eth2

[SkandaBhat]

If nobody else is working on this, can I take a crack at this? @AgeManning @dapplion

[AgeManning]

Hey @SkandaBhat - Sorry we have been away at a conference.

@hopinheimer are you still looking into this? Or should @SkandaBhat have a crack?

[hopinheimer]

Hey @AgeManning sorry just reached back from the conference.

hi @SkandaBhat please feel free to pick this up I haven't started work on it.

[SkandaBhat]

Sure thanks! @AgeManning please feel free to assign this to me! I am available this week and can create smaller PRs towards this issue.

[Gua00va]

From an offline chat with @AgeManning we suggested to:

• Return the serialized ENR as a String to avoid the dependency on discv5
• Import PeerID from its own crate
• Move the misc types SyncState, ConnectionDirection, etc to their own crate of only types and make it a common dependency of lighthouse_network and common/eth2

@dapplion for the 3rd point, should a new crate be created or should I put it in an existing one?

[dapplion]

for the 3rd point, should a new crate be created or should I put it in an existing one?

You can add a new crate network_types

[dknopik]

Hey @SkandaBhat and @Gua00va, thanks for picking up this issue!

We are currently modularizing some parts of the Lighthouse code base for use in Anchor, and as the eth2 crate is rather central we depend on it getting modularized soon. How is your progress? Let me know if I should take a look at some work in progress :)

[SkandaBhat]

Hey @dknopik I should have a draft by Tuesday next week, does that work?

[dknopik]

@SkandaBhat Sounds great, thanks!

[dknopik]

Hey @SkandaBhat, I already got started a bit today, see PR #6680. Feel free to improve on it if you have some ideas!

[SkandaBhat]

Unfortunately got held up with something else last week @dknopik, I'll check your PR today and see if I have anything I can improve!

[PoulavBhowmick03]

Can I hop onto this? I would love to discuss the other portions that can be simplified and worked on @AgeManning

[AgeManning]

Hey @PoulavBhowmick03 - Sorry I've been away for the last 2 weeks. I think @dknopik might be able to chime in on where we are at with this, as I think we've had a few additions towards this issue since I created it.

I'll let him chime in.

[dknopik]

Hey @PoulavBhowmick03, thanks for your interest in helping out!

In #6680 we already got rid of the worst dependencies, but there is still room for improvement. Certain dependencies, e.g. eth2_keystore, enr, proto_array and slashing_protection are pretty large and only required for very few requests. Unfortunately, I don't think we can fully get rid of them easily, but using Cargo features, we could at least disable them for users that do not require the few functions that use these deps. So you could help out by checking which dependencies are only required by few functions, grouping them in a meaningful way, and introducing crate features that disable them.

Alternatively (or additionally), you could explore making the dependencies themselves slimmer using features. eth2 mostly only uses these dependencies for deserialising into the types contained in them, so it might be possible in some cases to get rid of some transitive dependencies through feature flags.

Let me know if you have any questions!

[PoulavBhowmick03]

Hey @PoulavBhowmick03, thanks for your interest in helping out!

In #6680 we already got rid of the worst dependencies, but there is still room for improvement. Certain dependencies, e.g. eth2_keystore, enr, proto_array and slashing_protection are pretty large and only required for very few requests. Unfortunately, I don't think we can fully get rid of them easily, but using Cargo features, we could at least disable them for users that do not require the few functions that use these deps. So you could help out by checking which dependencies are only required by few functions, grouping them in a meaningful way, and introducing crate features that disable them.

Alternatively (or additionally), you could explore making the dependencies themselves slimmer using features. eth2 mostly only uses these dependencies for deserialising into the types contained in them, so it might be possible in some cases to get rid of some transitive dependencies through feature flags.

Let me know if you have any questions!

I see, so currently what you plan to do is to disable those certain dependencies for users who don't need them in the reqd functions or perhaps use feature flags to get rid of these. i would love to look a little into it to get a better idea. any kickstart for this?

[dknopik]

@PoulavBhowmick03 Sure! Let's take eth2_keystore as an example. Let me walk you through my approach.

Searching through the eth2 crate for eth2_keystore reveals that it is used in lighthouse_vc/types.rs and lighthouse_vc/std_types.rs. This is very good - these modules contain Lighthouse specific API, and are actually already feature gated: https://github.com/sigp/lighthouse/blob/39eb8145f89e11e3fd3a6c9c3d1031e8772ac491/common/eth2/src/lib.rs#L10-L13

So for this dependency, it's as easy as marking the dependency as optional and enabling it if the lighthouse feature of the crate is enabled. That being said, as this PR is all about modularizing further, it might be a good idea to instead introduce a lighthouse-vc feature, allowing users to pick only lighthouse if they want to avoid that dependency.

There is no hard rule or requirement how granular we want these features to be. I think disabling singular functions is too granular. But we also do not want to be forced to import a lot of unnecessary crates if the grouping is too coarse. Natural feature boundaries might be the top-level URL fragments of the API endpoints, i.e. beacon, node, etc., but I am not sure if that makes sense. So I suggest:

1. Take a look at the other dependencies I mentioned above, identify where they are used
2. Try to group the functions in a way that allows us to make dependencies optional
3. Check the other dependencies with cargo tree - if there are crates with a lot of sub-dependencies, it might make sense to figure out if they can be made optional as well. For ubiquitous dependencies such as types this is likely to be impossible though.

Let me know if I can clarify further!

[PoulavBhowmick03]

@PoulavBhowmick03 Sure! Let's take eth2_keystore as an example. Let me walk you through my approach.

Searching through the eth2 crate for eth2_keystore reveals that it is used in lighthouse_vc/types.rs and lighthouse_vc/std_types.rs. This is very good - these modules contain Lighthouse specific API, and are actually already feature gated:

lighthouse/common/eth2/src/lib.rs

Lines 10 to 13 in 39eb814

#[cfg(feature = "lighthouse")] pub mod lighthouse; #[cfg(feature = "lighthouse")] pub mod lighthouse_vc; So for this dependency, it's as easy as marking the dependency as optional and enabling it if the lighthouse feature of the crate is enabled. That being said, as this PR is all about modularizing further, it might be a good idea to instead introduce a lighthouse-vc feature, allowing users to pick only lighthouse if they want to avoid that dependency.

There is no hard rule or requirement how granular we want these features to be. I think disabling singular functions is too granular. But we also do not want to be forced to import a lot of unnecessary crates if the grouping is too coarse. Natural feature boundaries might be the top-level URL fragments of the API endpoints, i.e. beacon, node, etc., but I am not sure if that makes sense. So I suggest:

1. Take a look at the other dependencies I mentioned above, identify where they are used
2. Try to group the functions in a way that allows us to make dependencies optional
3. Check the other dependencies with cargo tree - if there are crates with a lot of sub-dependencies, it might make sense to figure out if they can be made optional as well. For ubiquitous dependencies such as types this is likely to be impossible though.

Let me know if I can clarify further!

Thank you for this ! Will be going over it and the other crates as well

[michaelsproul]

Are we still waiting on more improvements here, or can we considered this handled for now?

Covered by this PR?

• https://github.com/sigp/lighthouse/pull/6680

[dknopik]

@michaelsproul

IMO, we can still do better by allowing to disable certain request via features as laid out in my conversation with Poulav above. This would be nice for other sigp projects relying on eth2. But yeah, that PR got rid of the worst stuff.

[michaelsproul]

Ok cool, will leave this open for integration into the new dependency revamp