refactor(p2p): read a NodeRecord's fields through one set of helpers

`admit` and `parse_enr` each re-derived the IPv4-over-IPv6 preference and the
secp256k1-to-libp2p key decode. Both answer "who does this record belong to, and
where do we reach them", so letting the two drift would mean the bootnode parser
and the admission filter disagreeing about the same ENR. They now share
`read_ip`/`read_public_key`, next to the `read_quic_port` they already shared.

Three smaller things in the same pass, none of them behaviour changes:

`forget_discovered_peer` was the only inherent `P2PServer` method defined outside
`lib.rs`; every other submodule reaches the actor through a free function taking
`&mut P2PServer`, so `grep 'impl P2PServer'` no longer missed part of its
mutating surface.

The dial loop cloned the peer-table ref and the filter on every tick but only
used them when refilling an empty candidate queue, so the clones now happen
under that condition.

Discovery items nothing outside the crate names drop to `pub(crate)`. Only
`DiscoverySpawnConfig`, `DiscoveryError` and `DEFAULT_DISCOVERY_TARGET_PEERS`
cross into `bin/ethlambda`; the rest read as API with no consumer.

Author: MegaRedHand

crates/net/p2p/src/discovery/admission.rs

@@ -18,24 +18,26 @@
 //! ([`rank_by_uncovered_subnets`]).
 
 use std::collections::HashSet;
-use std::net::IpAddr;
 
 use ethrex_p2p::peer_filter::PeerFilter;
 use ethrex_p2p::types::NodeRecord;
 use libp2p::{Multiaddr, PeerId};
 use libssz::SszDecode;
 use tracing::debug;
 
-use super::enr::{ATTNETS_ENR_KEY, ETH2_ENR_KEY, EnrForkId, read_quic_port, subnets_from_attnets};
+use super::enr::{
+    ATTNETS_ENR_KEY, ETH2_ENR_KEY, EnrForkId, read_ip, read_public_key, read_quic_port,
+    subnets_from_attnets,
+};
 use crate::quic_multiaddr;
 
 /// A peer that passed admission and is ready to dial.
 #[derive(Debug, Clone, PartialEq)]
-pub struct DiscoveredPeer {
-    pub peer_id: PeerId,
-    pub addr: Multiaddr,
+pub(crate) struct DiscoveredPeer {
+    pub(crate) peer_id: PeerId,
+    pub(crate) addr: Multiaddr,
     /// Attestation subnets the peer advertises in `attnets`.
-    pub subnets: Vec<u64>,
+    pub(crate) subnets: Vec<u64>,
 }
 
 /// Why a discovered peer was turned away.
@@ -44,7 +46,7 @@ pub struct DiscoveredPeer {
 /// record, so a peer that adds a `quic` entry or gains an address through
 /// discv5's IP voting is reconsidered without restarting the process.
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
-pub enum RejectReason {
+pub(crate) enum RejectReason {
     /// No `eth2` entry, or one that does not decode. Cannot be our network.
     MissingForkId,
     /// On a different network.
@@ -73,7 +75,7 @@ pub struct LeanFilter {
 }
 
 impl LeanFilter {
-    pub fn new(fork_id: EnrForkId, attestation_committee_count: u64) -> Self {
+    pub(crate) fn new(fork_id: EnrForkId, attestation_committee_count: u64) -> Self {
         Self {
             fork_id,
             attestation_committee_count,
@@ -88,7 +90,7 @@ impl LeanFilter {
     /// `expect` because the two are only guaranteed to agree while the record is
     /// unchanged, and the peer table hands out clones: a caller that reaches
     /// this with an arbitrary record should get nothing to dial, not a panic.
-    pub fn dial_target(&self, record: &NodeRecord) -> Option<DiscoveredPeer> {
+    pub(crate) fn dial_target(&self, record: &NodeRecord) -> Option<DiscoveredPeer> {
         admit(record, &self.fork_id, self.attestation_committee_count).ok()
     }
 }
@@ -144,17 +146,10 @@ fn admit(
 
     let quic_port = read_quic_port(record).ok_or(RejectReason::NoQuicPort)?;
 
-    let public_key_bytes = pairs.secp256k1.ok_or(RejectReason::BadPublicKey)?;
-    let public_key =
-        libp2p::identity::secp256k1::PublicKey::try_from_bytes(public_key_bytes.as_bytes())
-            .map_err(|_| RejectReason::BadPublicKey)?;
+    let public_key = read_public_key(pairs).ok_or(RejectReason::BadPublicKey)?;
     let peer_id = PeerId::from_public_key(&libp2p::identity::PublicKey::from(public_key));
 
-    let ip = pairs
-        .ip
-        .map(IpAddr::from)
-        .or_else(|| pairs.ip6.map(IpAddr::from))
-        .ok_or(RejectReason::MissingAddress)?;
+    let ip = read_ip(pairs).ok_or(RejectReason::MissingAddress)?;
 
     let subnets = pairs
         .extra(ATTNETS_ENR_KEY)
@@ -173,7 +168,7 @@ fn admit(
 ///
 /// A candidate advertising no subnets scores zero and sorts last, but is never
 /// dropped: with few peers, any peer is better than none.
-pub fn rank_by_uncovered_subnets(candidates: &mut [DiscoveredPeer], covered: &HashSet<u64>) {
+pub(crate) fn rank_by_uncovered_subnets(candidates: &mut [DiscoveredPeer], covered: &HashSet<u64>) {
     candidates.sort_by_key(|candidate| {
         std::cmp::Reverse(
             candidate
@@ -192,7 +187,7 @@ mod tests {
     use ethrex_p2p::utils::public_key_from_signing_key;
     use libssz::SszEncode;
     use std::collections::HashSet;
-    use std::net::Ipv4Addr;
+    use std::net::{IpAddr, Ipv4Addr};
 
     use super::super::enr::{FAR_FUTURE_EPOCH, QUIC_ENR_KEY, encode_attnets};
 

crates/net/p2p/src/discovery/dial.rs

@@ -44,37 +44,37 @@ impl DiscoveryState {
     }
 }
 
-impl P2PServer {
-    /// Drop a peer's discovery bookkeeping.
-    ///
-    /// Called from both teardown paths — a connection that closed and a dial
-    /// that never established — so the map cannot outlive the peers in it and
-    /// [`covered_subnets`] cannot credit a subnet to someone who left. A no-op
-    /// when discovery is off.
-    pub(crate) fn forget_discovered_peer(&mut self, peer_id: &PeerId) {
-        if let Some(discovery) = self.discovery.as_mut() {
-            discovery.peer_attnets.remove(peer_id);
-        }
+/// Drop a peer's discovery bookkeeping.
+///
+/// Called from both teardown paths — a connection that closed and a dial that
+/// never established — so the map cannot outlive the peers in it and
+/// [`covered_subnets`] cannot credit a subnet to someone who left. A no-op when
+/// discovery is off.
+pub(crate) fn forget_discovered_peer(server: &mut P2PServer, peer_id: &PeerId) {
+    if let Some(discovery) = server.discovery.as_mut() {
+        discovery.peer_attnets.remove(peer_id);
     }
 }
 
 /// One tick of the dial loop. A no-op when discovery is disabled.
 pub(crate) async fn dial_tick(server: &mut P2PServer) {
     // Snapshot what the refill needs before any `.await`, so no borrow of
     // `server.discovery` has to live across the async boundary. Both are handle
-    // clones: an actor ref and two `Copy` fields.
+    // clones: an actor ref and two `Copy` fields, taken only when a refill is
+    // actually due rather than on every tick that just drains the queue.
     let Some(discovery) = server.discovery.as_ref() else {
         return;
     };
     if server.connected_peers.len() >= discovery.target_peers {
         return;
     }
-    let peer_table = discovery.peer_table.clone();
-    let filter = discovery.filter.clone();
-    let mut admitted = if discovery.candidates.is_empty() {
-        draw_candidates(&peer_table, &filter).await
-    } else {
-        Vec::new()
+    let refill = discovery
+        .candidates
+        .is_empty()
+        .then(|| (discovery.peer_table.clone(), discovery.filter.clone()));
+    let mut admitted = match refill {
+        Some((peer_table, filter)) => draw_candidates(&peer_table, &filter).await,
+        None => Vec::new(),
     };
 
     let Some(discovery) = server.discovery.as_mut() else {

crates/net/p2p/src/discovery/enr.rs

@@ -43,15 +43,15 @@ pub(crate) const FAR_FUTURE_EPOCH: u64 = u64::MAX;
 /// The `eth2` ENR entry: SSZ, 16 bytes, byte-identical to the beacon-chain
 /// `ENRForkID` container.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, SszEncode, SszDecode)]
-pub struct EnrForkId {
-    pub fork_digest: [u8; 4],
-    pub next_fork_version: [u8; 4],
-    pub next_fork_epoch: u64,
+pub(crate) struct EnrForkId {
+    pub(crate) fork_digest: [u8; 4],
+    pub(crate) next_fork_version: [u8; 4],
+    pub(crate) next_fork_epoch: u64,
 }
 
 impl EnrForkId {
     /// This node's fork id. Constant for the lifetime of the process.
-    pub fn local() -> Self {
+    pub(crate) fn local() -> Self {
         Self {
             fork_digest: fork_digest(),
             next_fork_version: NEXT_FORK_VERSION,
@@ -171,6 +171,31 @@ pub(crate) fn build_local_enr(params: &LocalEnrParams) -> Result<NodeRecord, Dis
         .map_err(DiscoveryError::BuildEnr)
 }
 
+/// The address a record advertises, preferring IPv4 when it carries both.
+///
+/// `None` for a record with neither `ip` nor `ip6`, which names no host to
+/// reach. Shared with the bootnode parser so both readers agree on which family
+/// wins.
+pub(crate) fn read_ip(pairs: &NodeRecordPairs) -> Option<IpAddr> {
+    pairs
+        .ip
+        .map(IpAddr::from)
+        .or_else(|| pairs.ip6.map(IpAddr::from))
+}
+
+/// The `secp256k1` entry as a libp2p key, or `None` when absent or not a valid
+/// compressed point.
+///
+/// libp2p derives the peer id from this key, so the bootnode parser and the
+/// admission filter must decode it the same way or they would disagree about who
+/// a record belongs to.
+pub(crate) fn read_public_key(
+    pairs: &NodeRecordPairs,
+) -> Option<libp2p::identity::secp256k1::PublicKey> {
+    let bytes = pairs.secp256k1?;
+    libp2p::identity::secp256k1::PublicKey::try_from_bytes(bytes.as_bytes()).ok()
+}
+
 /// The advertised libp2p QUIC port, if it is one we could dial.
 ///
 /// `None` covers an absent entry, an encoding `extra_int` cannot read (including

crates/net/p2p/src/discovery/mod.rs

@@ -80,9 +80,11 @@ pub struct DiscoverySpawnConfig {
 /// What the P2P actor needs from a running discovery server.
 pub struct DiscoveryHandle {
     pub peer_table: PeerTable,
-    /// This node's ENR as an `enr:`-prefixed string, for logs and the RPC
-    /// identity endpoint. Reflects startup state; discv5 may bump the sequence
-    /// number later if PONG voting changes our external IP.
+    /// This node's ENR as an `enr:`-prefixed string. `spawn_discovery` already
+    /// logs it; this copy is what the tests assert the published record against,
+    /// and what a future RPC identity endpoint would read. Reflects startup
+    /// state; discv5 may bump the sequence number later if PONG voting changes
+    /// our external IP.
     pub local_enr: String,
     /// The admission policy the peer table judges records with, kept so the dial
     /// loop can apply the same rules when it turns a contact into a dial target.

crates/net/p2p/src/lib.rs

@@ -37,8 +37,8 @@ use tracing::{debug, info, trace, warn};
 use crate::{
     discovery::{
         DISCOVERY_DIAL_INTERVAL, DiscoveryError, DiscoverySpawnConfig,
-        dial::{DiscoveryState, dial_tick},
-        enr::read_quic_port,
+        dial::{DiscoveryState, dial_tick, forget_discovered_peer},
+        enr::{read_ip, read_public_key, read_quic_port},
         spawn_discovery,
     },
     gossipsub::{
@@ -681,7 +681,7 @@ async fn handle_swarm_event(
             };
             if num_established == 0 {
                 server.connected_peers.remove(&peer_id);
-                server.forget_discovered_peer(&peer_id);
+                forget_discovered_peer(server, &peer_id);
                 let peer_count = server.connected_peers.len();
                 metrics::notify_peer_disconnected(
                     server.resolve_node_name(Some(&peer_id)),
@@ -723,24 +723,23 @@ async fn handle_swarm_event(
             );
             warn!(?peer_id, %error, "Outgoing connection error");
 
-            // A dial that never establishes ends up here rather than in
-            // `ConnectionClosed`, so this is the only place a peer we dialed but
-            // never connected to can be forgotten.
             if let Some(pid) = peer_id {
-                server.forget_discovered_peer(&pid);
-            }
-
-            // Schedule redial if this was a bootnode
-            if let Some(pid) = peer_id
-                && server.bootnode_addrs.contains_key(&pid)
-                && !server.connected_peers.contains(&pid)
-            {
-                send_after(
-                    Duration::from_secs(PEER_REDIAL_INTERVAL_SECS),
-                    ctx.clone(),
-                    p2p_protocol::RetryPeerRedial { peer_id: pid },
-                );
-                info!(%pid, "Scheduled bootnode redial after connection error");
+                // A dial that never establishes ends up here rather than in
+                // `ConnectionClosed`, so this is the only place a peer we dialed
+                // but never connected to can be forgotten.
+                forget_discovered_peer(server, &pid);
+
+                // Schedule redial if this was a bootnode
+                if server.bootnode_addrs.contains_key(&pid)
+                    && !server.connected_peers.contains(&pid)
+                {
+                    send_after(
+                        Duration::from_secs(PEER_REDIAL_INTERVAL_SECS),
+                        ctx.clone(),
+                        p2p_protocol::RetryPeerRedial { peer_id: pid },
+                    );
+                    info!(%pid, "Scheduled bootnode redial after connection error");
+                }
             }
         }
         SwarmEvent::IncomingConnectionError { peer_id, error, .. } => {
@@ -862,19 +861,10 @@ fn parse_enr(enr_str: &str) -> Result<Bootnode, String> {
     // `build_swarm` skip it when it picks static dial targets.
     let quic_port = read_quic_port(&record);
 
-    let public_key_bytes = pairs
-        .secp256k1
-        .ok_or_else(|| "node record missing public key".to_string())?;
-    let public_key =
-        libp2p::identity::secp256k1::PublicKey::try_from_bytes(public_key_bytes.as_bytes())
-            .map_err(|err| format!("bad secp256k1 key: {err}"))?;
-
-    // Prefer IPv4 if both are present.
-    let ip = pairs
-        .ip
-        .map(IpAddr::from)
-        .or_else(|| pairs.ip6.map(IpAddr::from))
-        .ok_or_else(|| "node record missing IP address".to_string())?;
+    let public_key = read_public_key(pairs)
+        .ok_or_else(|| "node record missing or malformed public key".to_string())?;
+
+    let ip = read_ip(pairs).ok_or_else(|| "node record missing IP address".to_string())?;
 
     // `quic` and `udp` are independently optional, but a record with neither is
     // reachable by nothing we speak: it can be neither dialed nor seeded. Drop