Skip to content

Comments

Add network feature to eth2#8558

Merged
mergify[bot] merged 8 commits intosigp:unstablefrom
macladson:eth2-network-feature
Feb 12, 2026
Merged

Add network feature to eth2#8558
mergify[bot] merged 8 commits intosigp:unstablefrom
macladson:eth2-network-feature

Conversation

@macladson
Copy link
Member

Proposed Changes

This reverts some of the changes from #8524 by adding back the typed network endpoints with an optional network feature. Without the network feature, these endpoints (and associated dependencies) will not be built.

This means the enr, multiaddr and libp2p-identity dependencies have returned but are now optional

@macladson macladson added code-quality dependencies Pull requests that update a dependency file work-in-progress PR is a work-in-progress labels Dec 9, 2025
@macladson macladson added ready-for-review The code is ready for review and removed work-in-progress PR is a work-in-progress labels Dec 18, 2025
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

I'm not sure about this one.

Do we have any downstream users of this crate that would need the networking types to be the right ones and not strings?

Copy link
Member

@jxs jxs left a comment

Choose a reason for hiding this comment

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

I'm not sure about this one.

Do we have any downstream users of this crate that would need the networking types to be the right ones and not strings?

I was the one bringing this up in the previous PR to Mac. Since we’re using a typed language, it would be better to rely on the available types instead of Strings where possible right? 😅

Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

Sorry, just realised that this would not matter for beacon_node build because we already compile the networking crates for it.

We need to gate the get_node_identity function too though, otherwise no feature compilation would fail with

error[E0412]: cannot find type `IdentityData` in this scope
    --> common/eth2/src/lib.rs:1939:69
     |
1939 |     pub async fn get_node_identity(&self) -> Result<GenericResponse<IdentityData>, Error> {
     |                                                                     ^^^^^^^^^^^^ not found in this scope
     |
note: found an item that was configured out
    --> common/eth2/src/types.rs:568:12
     |
 566 | #[cfg(feature = "network")]
     |       ------------------- the item is gated behind the `network` feature

Comment on lines 1943 to 1945
/// `GET node/identity`
#[cfg(feature = "network")]
pub async fn get_node_identity(&self) -> Result<GenericResponse<IdentityData>, Error> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah nice catch, fixed here

@pawanjay176 pawanjay176 added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 12, 2026
@mergify mergify bot added the queued label Feb 12, 2026
@mergify
Copy link

mergify bot commented Feb 12, 2026

Merge Queue Status

Rule: default


This pull request spent 30 minutes 20 seconds in the queue, including 29 minutes 10 seconds running CI.

Required conditions to merge
  • check-success=local-testnet-success
  • check-success=test-suite-success

mergify bot added a commit that referenced this pull request Feb 12, 2026
@mergify mergify bot merged commit 036ba1f into sigp:unstable Feb 12, 2026
36 checks passed
@mergify mergify bot removed the queued label Feb 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code-quality dependencies Pull requests that update a dependency file ready-for-merge This PR is ready to merge.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants