From 4e5270bba591281dfb942de858441aef44f0a2f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:32:13 -0300 Subject: [PATCH 1/3] feat(p2p): accept TCP alongside QUIC, and advertise it A peer that advertises a `quic` entry nothing answers on leaves a QUIC-only node with no second address to try: the dial times out and the peer is never reached. Add a TCP transport (noise + yamux) alongside QUIC, bound to the same port number as the existing QUIC listener, so libp2p can race a peer's `tcp` address in the same dial attempt when its `quic` does not answer. TCP and UDP are separate namespaces, so one `--gossipsub-port` still names both listeners. Admission now accepts a peer advertising `quic`, `tcp`, or both, and orders the resulting dial list QUIC first so it stays the preferred path; it rejects only when neither is present, which renames `NoQuicPort` to `NoDialableTransport`. `SwarmCommand::Dial` and `SwarmHandle::dial` carry a full `DialOpts` (peer id plus address list) rather than a single `Multiaddr`, since that list is what makes the per-dial race possible. Static bootnode dialing, bootnode redialing and the discovery dial loop all go through it. `Bootnode` gains a `tcp_port`, read from the ENR under the same "a `0` port is absent" rule the other two follow, and `as_discovery_node` now hands ethrex the real port instead of the hardcoded `0`. The local ENR advertises `tcp` too, at the same port number, so a peer applying the same rule we do can reach us over the transport that just landed. That also clears lighthouse's discovery predicate, which requires `enr.tcp4().is_some() || enr.tcp6().is_some()` on top of the `fork_digest` comparison; the lean digest is still the cross-client dummy, so a beacon-chain client rejects us on that instead. "Peer connected" gains a `transport` field, read off the connection's own multiaddr rather than off which address we dialed, so a live run can show which path actually carried it. The libp2p fork already exposes what this needs: a `.with_tcp(...)` insertion ahead of `.with_quic()` in the SwarmBuilder chain, no manifest change. Tests: admission cases for tcp-only, quic-only, both (QUIC first), and each transport at port 0 with the other absent; a new test builds two real swarms through `build_swarm` and proves they complete a TCP connection end to end, so a regression to QUIC-only hangs to the timeout rather than passing. --- bin/ethlambda/src/main.rs | 3 + crates/net/p2p/src/discovery/admission.rs | 118 +++++++++-- crates/net/p2p/src/discovery/dial.rs | 9 +- crates/net/p2p/src/discovery/enr.rs | 36 ++-- crates/net/p2p/src/discovery/mod.rs | 5 + crates/net/p2p/src/lib.rs | 232 ++++++++++++++++++---- crates/net/p2p/src/swarm_adapter.rs | 23 ++- 7 files changed, 344 insertions(+), 82 deletions(-) diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index ff12119d..d4788e55 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -318,6 +318,9 @@ async fn run_node(options: NodeOptions) -> eyre::Result<()> { bind_ip: p2p_socket.ip(), discovery_port: options.discovery.port, quic_port: p2p_socket.port(), + // Same port number as `quic_port`: TCP and UDP are separate namespaces, + // so `build_swarm` binds both without a collision. + tcp_port: p2p_socket.port(), subscription_subnets: subscribed_subnets, attestation_committee_count, bootnodes, diff --git a/crates/net/p2p/src/discovery/admission.rs b/crates/net/p2p/src/discovery/admission.rs index 51524ec6..7f058d8c 100644 --- a/crates/net/p2p/src/discovery/admission.rs +++ b/crates/net/p2p/src/discovery/admission.rs @@ -29,13 +29,16 @@ 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; +use crate::{quic_multiaddr, tcp_multiaddr}; /// A peer that passed admission and is ready to dial. #[derive(Debug, Clone, PartialEq)] pub(crate) struct DiscoveredPeer { pub(crate) peer_id: PeerId, - pub(crate) addr: Multiaddr, + /// Dial targets, QUIC first then TCP, built from whichever of the two ports + /// the record actually advertises. Never empty: [`admit`] rejects a record + /// with neither. + pub(crate) addrs: Vec, /// Attestation subnets the peer advertises in `attnets`. pub(crate) subnets: Vec, } @@ -51,9 +54,10 @@ pub(crate) enum RejectReason { MissingForkId, /// On a different network. ForkDigestMismatch, - /// Discoverable over discv5, but advertises no dialable libp2p QUIC port - /// (see [`read_quic_port`] for what that folds together). - NoQuicPort, + /// Discoverable over discv5, but advertises no dialable transport: neither a + /// libp2p QUIC port nor a libp2p TCP port (see [`read_quic_port`] for what + /// "dialable" folds together; a `0` port is treated as absent either way). + NoDialableTransport, /// No `secp256k1` entry, or one that is not a valid key. BadPublicKey, /// Neither `ip` nor `ip6`. @@ -144,7 +148,15 @@ fn admit( ); } - let quic_port = read_quic_port(record).ok_or(RejectReason::NoQuicPort)?; + // Mainnet peers widely advertise a `quic` entry that does not answer, so + // accepting TCP as well is what keeps a dial from timing out with nowhere to + // fall back to. A `0` port is absent for either transport: it RLP-decodes + // the same way an absent entry does, and is undialable regardless. + let quic_port = read_quic_port(record); + let tcp_port = pairs.tcp_port.filter(|port| *port != 0); + if quic_port.is_none() && tcp_port.is_none() { + return Err(RejectReason::NoDialableTransport); + } let public_key = read_public_key(pairs).ok_or(RejectReason::BadPublicKey)?; let peer_id = PeerId::from_public_key(&libp2p::identity::PublicKey::from(public_key)); @@ -156,9 +168,19 @@ fn admit( .map(|bits| subnets_from_attnets(&bits, attestation_committee_count)) .unwrap_or_default(); + // QUIC first, so it stays the preferred path when a peer offers both and + // libp2p races the list. + let mut addrs = Vec::with_capacity(2); + if let Some(port) = quic_port { + addrs.push(quic_multiaddr(ip, port, peer_id)); + } + if let Some(port) = tcp_port { + addrs.push(tcp_multiaddr(ip, port, peer_id)); + } + Ok(DiscoveredPeer { peer_id, - addr: quic_multiaddr(ip, quic_port, peer_id), + addrs, subnets, }) } @@ -264,7 +286,7 @@ mod tests { fn for_test(subnets: Vec) -> Self { Self { peer_id: PeerId::random(), - addr: Multiaddr::empty(), + addrs: vec![Multiaddr::empty()], subnets, } } @@ -279,8 +301,52 @@ mod tests { let peer = admit_record(&record).expect("accepted"); assert_eq!(peer.subnets, vec![2, 5]); assert_eq!( - peer.addr.to_string(), - format!("/ip4/127.0.0.1/udp/9001/quic-v1/p2p/{}", peer.peer_id) + peer.addrs, + vec![ + format!("/ip4/127.0.0.1/udp/9001/quic-v1/p2p/{}", peer.peer_id) + .parse() + .unwrap() + ] + ); + } + + #[test] + fn accepts_a_tcp_only_peer() { + // Every published mainnet beacon-chain bootnode looks like this: `tcp` + // and `udp`, no `quic`. Before TCP support this was `NoQuicPort`. + let record = record_with(|pairs| { + set_eth2(pairs, EnrForkId::local()); + pairs.tcp_port = Some(9001); + }); + let peer = admit_record(&record).expect("accepted"); + assert_eq!( + peer.addrs, + vec![ + format!("/ip4/127.0.0.1/tcp/9001/p2p/{}", peer.peer_id) + .parse() + .unwrap() + ] + ); + } + + #[test] + fn accepts_a_peer_with_both_transports_and_orders_quic_first() { + let record = record_with(|pairs| { + set_eth2(pairs, EnrForkId::local()); + set_quic(pairs, 9001); + pairs.tcp_port = Some(9002); + }); + let peer = admit_record(&record).expect("accepted"); + assert_eq!( + peer.addrs, + vec![ + format!("/ip4/127.0.0.1/udp/9001/quic-v1/p2p/{}", peer.peer_id) + .parse() + .unwrap(), + format!("/ip4/127.0.0.1/tcp/9002/p2p/{}", peer.peer_id) + .parse() + .unwrap(), + ] ); } @@ -317,23 +383,43 @@ mod tests { } #[test] - fn rejects_a_peer_with_no_quic_port() { - // Reachable by discv5 but not over our only transport. + fn rejects_a_peer_with_no_quic_or_tcp_port() { + // Reachable by discv5 but over neither transport we speak. let record = record_with(|pairs| set_eth2(pairs, EnrForkId::local())); - assert_eq!(admit_record(&record), Err(RejectReason::NoQuicPort)); + assert_eq!( + admit_record(&record), + Err(RejectReason::NoDialableTransport) + ); } #[test] - fn rejects_a_peer_with_a_quic_port_of_zero() { + fn rejects_a_peer_with_a_quic_port_of_zero_and_no_tcp() { // A port of 0 is undialable, and this is also how an absent entry // decodes (left-padded to 0u16), so it must hit the same reason as - // `rejects_a_peer_with_no_quic_port` rather than sail through as + // `rejects_a_peer_with_no_quic_or_tcp_port` rather than sail through as // "accepted" with an unusable `/udp/0/quic-v1` multiaddr. let record = record_with(|pairs| { set_eth2(pairs, EnrForkId::local()); set_quic(pairs, 0); }); - assert_eq!(admit_record(&record), Err(RejectReason::NoQuicPort)); + assert_eq!( + admit_record(&record), + Err(RejectReason::NoDialableTransport) + ); + } + + #[test] + fn rejects_a_peer_with_a_tcp_port_of_zero_and_no_quic() { + // `tcp: 0` decodes the same way an absent entry does, exactly as + // `quic: 0` does, so it must reach the same rejection. + let record = record_with(|pairs| { + set_eth2(pairs, EnrForkId::local()); + pairs.tcp_port = Some(0); + }); + assert_eq!( + admit_record(&record), + Err(RejectReason::NoDialableTransport) + ); } #[test] diff --git a/crates/net/p2p/src/discovery/dial.rs b/crates/net/p2p/src/discovery/dial.rs index 641c602f..f8d223f5 100644 --- a/crates/net/p2p/src/discovery/dial.rs +++ b/crates/net/p2p/src/discovery/dial.rs @@ -8,6 +8,7 @@ use std::collections::{HashMap, HashSet, VecDeque}; use ethrex_p2p::peer_table::{PeerTable, PeerTableServerProtocol as _}; use libp2p::PeerId; +use libp2p::swarm::dial_opts::DialOpts; use tracing::info; use super::admission::{DiscoveredPeer, LeanFilter, rank_by_uncovered_subnets}; @@ -109,7 +110,13 @@ pub(crate) async fn dial_tick(server: &mut P2PServer) { // synchronously — already connected, already dialing, no addresses, denied — // raises no `OutgoingConnectionError`, so `forget_discovered_peer` would // never run and the entry would outlive the process's interest in it. - if !server.swarm_handle.dial_accepted(candidate.addr).await { + // One `DialOpts` carrying every address, not one dial per address: libp2p + // races them within the attempt, which is what lets a live TCP address + // rescue a peer whose advertised QUIC port does not answer. + let opts = DialOpts::peer_id(candidate.peer_id) + .addresses(candidate.addrs) + .build(); + if !server.swarm_handle.dial_accepted(opts).await { return; } metrics::inc_discovered_peers_dialed(); diff --git a/crates/net/p2p/src/discovery/enr.rs b/crates/net/p2p/src/discovery/enr.rs index f7f0a55d..820f07d5 100644 --- a/crates/net/p2p/src/discovery/enr.rs +++ b/crates/net/p2p/src/discovery/enr.rs @@ -3,14 +3,16 @@ //! The entry set follows the beacon-chain phase0 p2p spec's discovery domain: //! //! ```text -//! id, ip, udp=, quic=, secp256k1, +//! id, ip, udp=, quic=, tcp=, +//! secp256k1, //! eth2 = SSZ(ENRForkID) //! attnets = subscribed attestation subnet bitfield //! ``` //! -//! There is deliberately no `tcp` entry. The spec defines it as the libp2p TCP -//! listening port and makes it optional; ethlambda speaks QUIC only, so -//! advertising one would invite a dial that cannot succeed. +//! `tcp` is the spec's own entry for the libp2p TCP listening port: ethlambda +//! binds one alongside QUIC (see `crates/net/p2p/src/lib.rs`'s `build_swarm`), +//! on the same port number, so advertising it is what lets a peer whose +//! advertised `quic` does not answer still reach us. //! //! Lean defines no fork schedule and its fork digest is a compile-time constant //! rather than a genesis-derived value, so every field of [`EnrForkId`] is @@ -113,6 +115,10 @@ pub(crate) struct LocalEnrParams { pub(crate) discovery_port: u16, /// UDP port the libp2p QUIC transport is bound to. pub(crate) quic_port: u16, + /// TCP port the libp2p TCP transport is bound to. The same port number as + /// [`Self::quic_port`]: TCP and UDP are separate namespaces, so `build_swarm` + /// binds both without a collision. + pub(crate) tcp_port: u16, pub(crate) subscription_subnets: HashSet, pub(crate) attestation_committee_count: u64, } @@ -120,22 +126,19 @@ pub(crate) struct LocalEnrParams { impl LocalEnrParams { /// The `Node` ethrex's discovery server takes as its local identity. /// - /// `tcp_port` is 0, which ethrex reads as "no TCP listener" and omits from - /// the record. + /// `tcp_port` is the real port the libp2p TCP transport is bound to, now + /// that ethlambda has one. pub(crate) fn local_node(&self) -> Node { Node::new( self.ip, self.discovery_port, - 0, + self.tcp_port, public_key_from_signing_key(&self.signer), ) } /// The full entry set this node advertises. /// - /// `tcp_port` is left unset rather than zero: `from_pairs` takes the entry - /// set verbatim, so "no TCP listener" is spelled by the entry's absence. - /// /// The three consensus entries go through `set_extra`/`set_extra_int`, /// which pick the RLP codec once. Encoding them by hand is the trap that /// helper exists for: a bare `Vec` hits the generic `Vec` impl and @@ -145,7 +148,7 @@ impl LocalEnrParams { fn local_pairs(&self) -> NodeRecordPairs { let mut pairs = NodeRecordPairs { udp_port: Some(self.discovery_port), - tcp_port: None, + tcp_port: Some(self.tcp_port), ..Default::default() }; match self.ip.to_canonical() { @@ -224,6 +227,7 @@ mod tests { ip: IpAddr::from(Ipv4Addr::LOCALHOST), discovery_port: 9010, quic_port: 9001, + tcp_port: 9001, subscription_subnets: HashSet::from([1u64, 4]), attestation_committee_count: 8, }) @@ -291,13 +295,17 @@ mod tests { } #[test] - fn local_enr_advertises_udp_and_quic_but_no_tcp() { + fn local_enr_advertises_udp_quic_and_tcp() { + // Inverts what this test used to pin: ethlambda now binds a TCP + // transport alongside QUIC (see `build_swarm`), so the ENR must + // advertise all three ports rather than omitting `tcp`. let record = build(); let pairs = record.pairs(); assert_eq!(pairs.udp_port, Some(9010)); assert_eq!( - pairs.tcp_port, None, - "ethlambda has no TCP listener, so it must not advertise one" + pairs.tcp_port, + Some(9001), + "ethlambda now has a TCP listener and must advertise it" ); assert_eq!(read_quic_port(&record), Some(9001)); } diff --git a/crates/net/p2p/src/discovery/mod.rs b/crates/net/p2p/src/discovery/mod.rs index ecaaf63d..859aec5b 100644 --- a/crates/net/p2p/src/discovery/mod.rs +++ b/crates/net/p2p/src/discovery/mod.rs @@ -76,6 +76,9 @@ pub struct DiscoverySpawnConfig { pub bind_ip: IpAddr, pub discovery_port: u16, pub quic_port: u16, + /// TCP port the libp2p TCP transport is bound to. Same port number as + /// `quic_port`, since TCP and UDP are separate namespaces. + pub tcp_port: u16, pub subscription_subnets: HashSet, pub attestation_committee_count: u64, pub bootnodes: Vec, @@ -140,6 +143,7 @@ pub async fn spawn_discovery( ip: advertise_ip, discovery_port: bound.port(), quic_port: config.quic_port, + tcp_port: config.tcp_port, subscription_subnets: config.subscription_subnets, attestation_committee_count: config.attestation_committee_count, }; @@ -228,6 +232,7 @@ mod tests { bind_ip: IpAddr::from(Ipv4Addr::LOCALHOST), discovery_port, quic_port: 9001, + tcp_port: 9001, subscription_subnets: HashSet::from([0u64]), attestation_committee_count: 4, bootnodes: Vec::new(), diff --git a/crates/net/p2p/src/lib.rs b/crates/net/p2p/src/lib.rs index dd74bc1e..be54e2ad 100644 --- a/crates/net/p2p/src/lib.rs +++ b/crates/net/p2p/src/lib.rs @@ -22,7 +22,7 @@ use libp2p::{ identity::{Keypair, PublicKey, secp256k1}, multiaddr::Protocol, request_response::{self, OutboundRequestId}, - swarm::{NetworkBehaviour, SwarmEvent}, + swarm::{NetworkBehaviour, SwarmEvent, dial_opts::DialOpts}, }; use sha2::Digest; use spawned_concurrency::actor; @@ -219,7 +219,9 @@ pub struct BuiltSwarm { pub(crate) attestation_committee_count: u64, pub(crate) block_topic: libp2p::gossipsub::IdentTopic, pub(crate) aggregation_topic: libp2p::gossipsub::IdentTopic, - pub(crate) bootnode_addrs: HashMap, + /// Dial targets per bootnode, QUIC first then TCP. Empty entries are never + /// inserted; see [`bootnode_dial_addrs`]. + pub(crate) bootnode_addrs: HashMap>, } /// Build and configure the libp2p swarm, dial bootnodes, subscribe to topics. @@ -293,6 +295,12 @@ pub fn build_swarm( let mut swarm = libp2p::SwarmBuilder::with_existing_identity(identity) .with_tokio() + .with_tcp( + libp2p::tcp::Config::default().nodelay(true), + libp2p::noise::Config::new, + libp2p::yamux::Config::default, + ) + .expect("failed to add TCP transport to swarm") .with_quic() .with_behaviour(|_| behavior) .expect("failed to add behaviour to swarm") @@ -303,41 +311,50 @@ pub fn build_swarm( .build(); let local_peer_id = *swarm.local_peer_id(); let mut bootnode_addrs = HashMap::new(); - let mut quic_less_bootnodes = 0usize; + let mut undialable_bootnodes = 0usize; for bootnode in config.bootnodes { let peer_id = PeerId::from_public_key(&bootnode.public_key); if peer_id == local_peer_id { continue; } - // Discovery-only seed: reachable over discv5, but with no QUIC port - // there is nothing for the swarm to dial. - let Some(quic_port) = bootnode.quic_port else { - quic_less_bootnodes += 1; - debug!(%peer_id, ip = %bootnode.ip, "Bootnode advertises no quic port, discv5 seed only"); + let addrs = bootnode_dial_addrs(&bootnode, peer_id); + if addrs.is_empty() { + // Discovery-only seed: reachable over discv5, but with no QUIC or + // TCP port there is nothing for the swarm to dial. + undialable_bootnodes += 1; + debug!(%peer_id, ip = %bootnode.ip, "Bootnode advertises no dialable transport, discv5 seed only"); continue; - }; - let addr = quic_multiaddr(bootnode.ip, quic_port, peer_id); - bootnode_addrs.insert(peer_id, addr.clone()); - swarm.dial(addr).unwrap(); + } + bootnode_addrs.insert(peer_id, addrs.clone()); + swarm + .dial(DialOpts::peer_id(peer_id).addresses(addrs).build()) + .unwrap(); } // Every skip above is individually unremarkable and logged at `debug`, but a // list that produces no dial target at all leaves the node isolated unless - // discovery is on, which is worth one line at `warn`. A beacon-chain - // bootstrap list is exactly this shape: `tcp` and `udp`, never `quic`. - if bootnode_addrs.is_empty() && quic_less_bootnodes > 0 { + // discovery is on, which is worth one line at `warn`. + if bootnode_addrs.is_empty() && undialable_bootnodes > 0 { warn!( - quic_less_bootnodes, - "No bootnode advertises a quic port, so nothing will be dialed statically; \ + undialable_bootnodes, + "No bootnode advertises a quic or tcp port, so nothing will be dialed statically; \ peering depends entirely on discv5 discovery" ); } - let addr = Multiaddr::empty() + let quic_addr = Multiaddr::empty() .with(config.listening_socket.ip().into()) .with(Protocol::Udp(config.listening_socket.port())) .with(Protocol::QuicV1); swarm - .listen_on(addr) - .expect("failed to bind gossipsub listening address"); + .listen_on(quic_addr) + .expect("failed to bind gossipsub QUIC listening address"); + // Same port number as the QUIC listener above: TCP and UDP are separate + // namespaces, so this cannot collide with it. + let tcp_addr = Multiaddr::empty() + .with(config.listening_socket.ip().into()) + .with(Protocol::Tcp(config.listening_socket.port())); + swarm + .listen_on(tcp_addr) + .expect("failed to bind gossipsub TCP listening address"); // Subscribe to block topic (all nodes) let block_topic = block_topic(); @@ -480,7 +497,7 @@ pub struct P2PServer { pub(crate) pending_root_requests: HashMap, pub(crate) outbound_requests: HashMap, pub(crate) range_sync_state: Option, - bootnode_addrs: HashMap, + bootnode_addrs: HashMap>, node_names: HashMap, /// Set when discovery is enabled. `None` disables the dial loop entirely. @@ -545,9 +562,10 @@ impl P2PServer { return; } - if let Some(addr) = self.bootnode_addrs.get(&peer_id) { + if let Some(addrs) = self.bootnode_addrs.get(&peer_id) { trace!(%peer_id, "Redialing disconnected bootnode"); - self.swarm_handle.dial(addr.clone()); + self.swarm_handle + .dial(DialOpts::peer_id(peer_id).addresses(addrs.clone()).build()); } } @@ -635,6 +653,11 @@ async fn handle_swarm_event( .. } => { let direction = connection_direction(&endpoint); + // Read off the connection's own address rather than which one we + // dialed: with both QUIC and TCP offered, libp2p races every + // address in a dial and may connect over either. This is the + // field that answers "did the TCP fallback actually help". + let transport = transport_label(endpoint.get_remote_address()); if num_established.get() == 1 { server.connected_peers.insert(peer_id); let peer_count = server.connected_peers.len(); @@ -650,6 +673,7 @@ async fn handle_swarm_event( trace!( %peer_id, %direction, + %transport, peer_count, our_finalized_slot, our_head_slot, @@ -664,7 +688,7 @@ async fn handle_swarm_event( ) .await; } else { - trace!(%peer_id, %direction, "Added peer connection"); + trace!(%peer_id, %direction, %transport, "Added peer connection"); } } SwarmEvent::ConnectionClosed { @@ -807,16 +831,23 @@ pub struct Bootnode { pub(crate) ip: IpAddr, /// The libp2p QUIC port, when the ENR advertises one. /// - /// `None` for records that are discv5-reachable but speak no transport we - /// have: every beacon-chain bootnode published today advertises `tcp` and - /// `udp` but no `quic`. Such a bootnode still seeds the discv5 routing - /// table; it just is never dialed statically. + /// `None` for a record that does not advertise one. See + /// [`Bootnode::tcp_port`] for the other transport that can still make such + /// a record dialable: every beacon-chain bootnode published today is + /// exactly that case, `tcp` and `udp` but no `quic`. pub(crate) quic_port: Option, + /// The libp2p TCP port, when the ENR advertises one. + /// + /// `None` for the ENRs lean-quickstart generates today, which carry only + /// `ip`/`quic`/`secp256k1`. Every published mainnet beacon-chain bootnode + /// carries this instead of `quic`, which is what makes them statically + /// dialable now that the swarm speaks both transports. + pub(crate) tcp_port: Option, /// The discv5 UDP port, when the ENR advertises one. /// /// `None` for the ENRs lean-quickstart generates today, which carry only /// `ip`/`quic`/`secp256k1`. Such a bootnode is still dialed statically over - /// QUIC; it just cannot seed the discv5 routing table. + /// QUIC or TCP; it just cannot seed the discv5 routing table. pub(crate) udp_port: Option, pub(crate) public_key: PublicKey, } @@ -825,7 +856,9 @@ impl Bootnode { /// This bootnode as a discv5 seed, or `None` when its ENR advertises no /// `udp` port and it therefore cannot be reached by discovery. /// - /// `tcp_port` is 0: ethrex reads that as "no TCP listener". + /// `tcp_port` carries this bootnode's real advertised TCP port when it has + /// one, now that ethlambda dials TCP too; it is `0` only when the ENR + /// advertises none, which ethrex reads as "no TCP listener". pub(crate) fn as_discovery_node(&self) -> Option { let udp_port = self.udp_port?; // libp2p and ethrex hold the same key in different representations: @@ -843,7 +876,7 @@ impl Bootnode { Some(ethrex_p2p::types::Node::new( self.ip, udp_port, - 0, + self.tcp_port.unwrap_or(0), ethrex_common::H512::from_slice(&uncompressed[1..]), )) } @@ -854,8 +887,8 @@ impl Bootnode { /// Records that cannot be decoded, or that lack an IP, a public key or any /// dialable port at all, are skipped with a warning rather than aborting /// startup: one malformed entry in the bootnode file should not stop the node -/// from booting. A record carrying only one of `quic` and `udp` is kept, since -/// each is useful on its own. +/// from booting. A record carrying only one of `quic`, `tcp` and `udp` is kept, +/// since each is useful on its own. pub fn parse_enrs(enrs: Vec) -> Vec { let configured = enrs.len(); let bootnodes: Vec = enrs @@ -888,10 +921,10 @@ fn parse_enr(enr_str: &str) -> Result { let record = NodeRecord::decode(&decoded).map_err(|err| format!("RLP decode failed: {err}"))?; let pairs = record.pairs(); - // A record with no dialable `quic` entry is not an error: it is - // discv5-reachable but speaks no transport we have, which is exactly what - // every beacon-chain bootnode looks like. Keep it as a discovery seed and let - // `build_swarm` skip it when it picks static dial targets. + // A record with no dialable `quic` entry is not an error: it may still + // advertise `tcp`, and even a record with neither is worth keeping as a + // discv5 seed. Keep it and let `build_swarm` skip it when it picks static + // dial targets. let quic_port = read_quic_port(&record); // An explicit `udp: 0` is no more reachable than a `quic: 0`, which @@ -899,21 +932,26 @@ fn parse_enr(enr_str: &str) -> Result { // with a contact on a port nothing listens on. let udp_port = pairs.udp_port.filter(|port| *port != 0); + // Same rule for `tcp`, the transport every published beacon-chain bootnode + // advertises and none of them pairs with a `quic` entry. + let tcp_port = pairs.tcp_port.filter(|port| *port != 0); + 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 - // it here rather than carry a contact that no code path can ever use. - if quic_port.is_none() && udp_port.is_none() { - return Err("node advertises neither a quic nor a udp port".to_string()); + // `quic`, `tcp` and `udp` are independently optional, but a record with none + // of them is reachable by nothing we speak: it can be neither dialed nor + // seeded. Drop it here rather than carry a contact no code path can use. + if quic_port.is_none() && tcp_port.is_none() && udp_port.is_none() { + return Err("node advertises neither a quic, tcp nor a udp port".to_string()); } Ok(Bootnode { ip, quic_port, + tcp_port, udp_port, public_key: public_key.into(), }) @@ -921,6 +959,21 @@ fn parse_enr(enr_str: &str) -> Result { // --- Utility functions --- +/// Dial targets for a static bootnode, QUIC first then TCP. +/// +/// Empty when the record advertises neither, which is a discv5-only seed: it can +/// still answer FINDNODE, but there is nothing for the swarm to dial. +pub(crate) fn bootnode_dial_addrs(bootnode: &Bootnode, peer_id: PeerId) -> Vec { + let mut addrs = Vec::with_capacity(2); + if let Some(quic_port) = bootnode.quic_port { + addrs.push(quic_multiaddr(bootnode.ip, quic_port, peer_id)); + } + if let Some(tcp_port) = bootnode.tcp_port { + addrs.push(tcp_multiaddr(bootnode.ip, tcp_port, peer_id)); + } + addrs +} + /// The address of a libp2p QUIC listener, as both dial paths spell it: static /// bootnodes in [`build_swarm`] and discovered peers in /// [`admission::admit`](discovery::admission). @@ -936,6 +989,18 @@ pub(crate) fn quic_multiaddr(ip: IpAddr, quic_port: u16, peer_id: PeerId) -> Mul .expect("a freshly built multiaddr carries no p2p component") } +/// The address of a libp2p TCP listener, the fallback path for a peer whose +/// advertised `quic` entry does not answer. +/// +/// Infallible for the same reason [`quic_multiaddr`] is. +pub(crate) fn tcp_multiaddr(ip: IpAddr, tcp_port: u16, peer_id: PeerId) -> Multiaddr { + Multiaddr::empty() + .with(ip.into()) + .with(Protocol::Tcp(tcp_port)) + .with_p2p(peer_id) + .expect("a freshly built multiaddr carries no p2p component") +} + fn connection_direction(endpoint: &libp2p::core::ConnectedPoint) -> &'static str { if endpoint.is_dialer() { "outbound" @@ -944,6 +1009,22 @@ fn connection_direction(endpoint: &libp2p::core::ConnectedPoint) -> &'static str } } +/// "quic" or "tcp", read off which protocol the connection's own multiaddr +/// carries. `"unknown"` is unreachable in practice: every address this swarm +/// ever connects over came from one of the two transports it was built with, +/// but a swarm event is not proof of that, so this stays total rather than +/// panicking on a shape it does not expect. +fn transport_label(addr: &Multiaddr) -> &'static str { + for protocol in addr.iter() { + match protocol { + Protocol::Quic | Protocol::QuicV1 => return "quic", + Protocol::Tcp(_) => return "tcp", + _ => {} + } + } + "unknown" +} + fn compute_message_id(message: &libp2p::gossipsub::Message) -> libp2p::gossipsub::MessageId { const MESSAGE_DOMAIN_INVALID_SNAPPY: [u8; 4] = [0x00, 0x00, 0x00, 0x00]; const MESSAGE_DOMAIN_VALID_SNAPPY: [u8; 4] = [0x01, 0x00, 0x00, 0x00]; @@ -974,6 +1055,73 @@ mod tests { PeerId::from_public_key(&Keypair::generate_ed25519().public()) } + /// Proves the TCP transport `build_swarm` now adds actually completes a + /// connection end to end, rather than only compiling. Builds two real + /// swarms via the production entry point (port `0`, so this cannot collide + /// with a running node or a sibling test), learns the first swarm's TCP + /// listen address off its own `NewListenAddr` event, dials it from the + /// second swarm, and polls both until each reports `ConnectionEstablished`. + /// A regression to QUIC-only, or a misconfigured TCP transport, hangs here + /// until the timeout rather than racing to a false positive. + #[tokio::test] + async fn two_swarms_connect_over_tcp() { + fn build(node_key_byte: u8) -> BuiltSwarm { + build_swarm(SwarmConfig { + node_key: vec![node_key_byte; 32], + bootnodes: Vec::new(), + listening_socket: "127.0.0.1:0".parse().expect("valid socket"), + validator_ids: Vec::new(), + attestation_committee_count: 1, + subscription_subnets: HashSet::new(), + }) + .expect("swarm builds") + } + + let mut dialer = build(1); + let mut listener = build(2); + + // Both a QUIC and a TCP `NewListenAddr` arrive for `listener`; only the + // TCP one is wanted here. + let listener_tcp_addr = loop { + if let SwarmEvent::NewListenAddr { address, .. } = + listener.swarm.select_next_some().await + && address.iter().any(|p| matches!(p, Protocol::Tcp(_))) + { + break address + .with_p2p(listener.local_peer_id) + .expect("failed to add peer ID to multiaddr"); + } + }; + + dialer + .swarm + .dial(listener_tcp_addr) + .expect("dial is accepted"); + + let (mut dialer_connected, mut listener_connected) = (false, false); + let both_connect = async { + while !(dialer_connected && listener_connected) { + tokio::select! { + event = dialer.swarm.select_next_some() => { + if let SwarmEvent::ConnectionEstablished { endpoint, .. } = event { + assert_eq!(transport_label(endpoint.get_remote_address()), "tcp"); + dialer_connected = true; + } + } + event = listener.swarm.select_next_some() => { + if let SwarmEvent::ConnectionEstablished { endpoint, .. } = event { + assert_eq!(transport_label(endpoint.get_remote_address()), "tcp"); + listener_connected = true; + } + } + } + } + }; + tokio::time::timeout(Duration::from_secs(10), both_connect) + .await + .expect("both swarms must connect over TCP within the timeout"); + } + #[test] fn range_sync_state_merges_new_peer_ranges() { let first_peer = random_peer(); diff --git a/crates/net/p2p/src/swarm_adapter.rs b/crates/net/p2p/src/swarm_adapter.rs index d9cbffcb..16f438f7 100644 --- a/crates/net/p2p/src/swarm_adapter.rs +++ b/crates/net/p2p/src/swarm_adapter.rs @@ -2,10 +2,10 @@ use std::collections::HashMap; use std::time::Duration; use libp2p::{ - Multiaddr, PeerId, StreamProtocol, + PeerId, StreamProtocol, futures::StreamExt, request_response::{self, OutboundRequestId}, - swarm::SwarmEvent, + swarm::{SwarmEvent, dial_opts::DialOpts}, }; use tokio::{sync::mpsc, time::MissedTickBehavior}; use tracing::{debug, error}; @@ -21,7 +21,12 @@ pub enum SwarmCommand { data: Vec, }, Dial { - addr: Multiaddr, + /// Carries the full set of addresses worth trying for one dial attempt + /// (a peer's QUIC and TCP ports both, say): libp2p races every address + /// in a single `DialOpts` rather than treating each as a separate + /// attempt, which is what lets a live TCP address rescue a dial whose + /// advertised QUIC port does not answer. + opts: DialOpts, /// Callback reporting whether the swarm accepted the dial. `None` when /// the caller does not need to know. accepted_tx: Option>, @@ -52,11 +57,11 @@ impl SwarmHandle { .inspect_err(|_| debug!("Swarm adapter closed, cannot publish")); } - pub fn dial(&self, addr: Multiaddr) { + pub fn dial(&self, opts: DialOpts) { let _ = self .cmd_tx .send(SwarmCommand::Dial { - addr, + opts, accepted_tx: None, }) .inspect_err(|_| debug!("Swarm adapter closed, cannot dial")); @@ -72,12 +77,12 @@ impl SwarmHandle { /// /// A `true` only means the dial was queued: success or failure still arrives /// later as `ConnectionEstablished` or `OutgoingConnectionError`. - pub async fn dial_accepted(&self, addr: Multiaddr) -> bool { + pub async fn dial_accepted(&self, opts: DialOpts) -> bool { let (tx, rx) = tokio::sync::oneshot::channel(); if self .cmd_tx .send(SwarmCommand::Dial { - addr, + opts, accepted_tx: Some(tx), }) .is_err() @@ -181,9 +186,9 @@ fn execute_command(swarm: &mut libp2p::Swarm, cmd: SwarmCommand) { .inspect_err(|err| debug!(%err, "Swarm adapter: publish failed")) .ok(); } - SwarmCommand::Dial { addr, accepted_tx } => { + SwarmCommand::Dial { opts, accepted_tx } => { let accepted = swarm - .dial(addr) + .dial(opts) .inspect_err(|err| debug!(%err, "Swarm adapter: dial failed")) .is_ok(); if let Some(tx) = accepted_tx { From 295cec1ef50e850b8dbbd81df012f43ff61e122f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Mon, 31 Aug 2026 15:32:56 -0300 Subject: [PATCH 2/3] docs: document the TCP transport discovery.md: the ENR entry table lists `tcp` alongside `quic` and `udp`, with a note on why they share a port number and what advertising `tcp` buys against lighthouse's discovery predicate. The "which peers get dialed" rule accepts either transport, a paragraph explains that a dial carries every address at once rather than one per retry, and the bootnode table drops the old "quic absent implies seed-only" framing since `tcp` can now carry a bootnode too. CLAUDE.md: the Networking section's transport line mentions TCP, and the discovery bullets track the new ENR entry and admission rule. --- CLAUDE.md | 6 +++--- docs/discovery.md | 45 ++++++++++++++++++++++++++++++++------------- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index a4b14c91..3c4f7e05 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -273,7 +273,7 @@ actual_slot = finalized_slot + 1 + relative_index ## Networking (libp2p) ### Protocols -- **Transport**: QUIC over UDP (TLS 1.3) +- **Transport**: QUIC over UDP (TLS 1.3), plus TCP (noise + yamux) on the same port number as a fallback: a peer whose advertised `quic` doesn't answer can still be reached over TCP, and libp2p races both addresses within one dial - **Gossipsub**: Blocks + Attestations (snappy raw compression) - Topic: `/leanconsensus/{fork_digest}/{block|aggregation|attestation_N}/ssz_snappy` - `fork_digest` is a 4-byte hex string (no `0x` prefix); currently the dummy `12345678` agreed across clients @@ -283,8 +283,8 @@ actual_slot = finalized_slot + 1 + relative_index ### Peer Discovery (discv5, opt-in) - Off by default; `--discovery.enable` plus `--discovery.port` (own UDP socket, must differ from `--gossipsub-port`) - Reuses ethrex's `DiscoveryServer` + `PeerTable` with discv4 disabled; `spawn` takes the prepared lean ENR, so the record ethrex serves is the one we report -- ENR follows the beacon phase0 spec: `ip`/`udp`/`quic`/`secp256k1`/`eth2`/`attnets` -- Admission mirrors lighthouse: `eth2.fork_digest` must match, `next_fork_*` may differ, `quic` entry required. Handed to the peer table as `LeanFilter: PeerFilter`, so records are judged on arrival, not at dial time; a reject is re-judged on a higher-`seq` ENR +- ENR follows the beacon phase0 spec: `ip`/`udp`/`quic`/`tcp`/`secp256k1`/`eth2`/`attnets` +- Admission mirrors lighthouse: `eth2.fork_digest` must match, `next_fork_*` may differ, a `quic` or `tcp` entry required. Handed to the peer table as `LeanFilter: PeerFilter`, so records are judged on arrival, not at dial time; a reject is re-judged on a higher-`seq` ENR - Candidates ranked by uncovered attestation subnets. See [`docs/discovery.md`](docs/discovery.md) ### Retry Strategy on Block Requests diff --git a/docs/discovery.md b/docs/discovery.md index 8513fe51..0e3db6ca 100644 --- a/docs/discovery.md +++ b/docs/discovery.md @@ -24,7 +24,9 @@ ethlambda --discovery.enable | `--discovery.target-peers` | `200` | Connected-peer count above which dialing stops | `--discovery.port` and `--gossipsub-port` (default `9001`, libp2p QUIC) are both -UDP and so cannot share a port. The defaults are one apart, so `--discovery.enable` +UDP and so cannot share a port. `--gossipsub-port` also binds a libp2p TCP +listener on the same number, which collides with neither: TCP and UDP are +separate namespaces. The defaults are one apart, so `--discovery.enable` works on its own; overriding either onto the other is rejected at startup. The discv5 socket always binds the wildcard `0.0.0.0`, since that is where we @@ -46,10 +48,19 @@ The layout follows the discovery domain of the beacon-chain | `ip` | `--discovery.advertise-ip`, or the bind address (`0.0.0.0`) if unset | | `udp` | `--discovery.port` | | `quic` | `--gossipsub-port`, the libp2p QUIC listener | +| `tcp` | `--gossipsub-port`, the libp2p TCP listener | | `secp256k1` | compressed public key from `--node-key` | | `eth2` | SSZ `ENRForkID`, 16 bytes | | `attnets` | subscribed attestation subnet bitfield | +`tcp` and `quic` share the same port number: TCP and UDP are separate +namespaces, so `build_swarm` binds both without a collision. Advertising both +is what lets a peer whose `quic` port does not answer still reach this node +over TCP. It also gets us past lighthouse's discovery predicate, which requires +`enr.tcp4().is_some() || enr.tcp6().is_some()` on top of the spec's +`fork_digest` comparison; the lean fork digest is still the cross-client dummy +`0x12345678`, so a beacon-chain client rejects us on that instead. + The local ENR is logged once at startup. This same record is handed to ethrex's `DiscoveryServer`, so it is what answers @@ -64,7 +75,7 @@ A discovered peer is admitted only if: - its ENR carries a decodable `eth2` entry, **and** - that entry's `fork_digest` equals ours, **and** -- it advertises a `quic` port. +- it advertises a `quic` port, a `tcp` port, or both. A differing `next_fork_version` or `next_fork_epoch` is *not* grounds for rejection: the spec permits connecting to a peer that is incompatible with an @@ -76,6 +87,10 @@ No rejection is final: the peer table runs the filter again as soon as the peer publishes a higher-`seq` ENR, so a node that adds a `quic` entry, or gains an address through discv5's IP voting, is reconsidered without a restart. +A peer's dial list carries every address it advertises, QUIC first then TCP. +libp2p races every address in one dial attempt, so a peer whose `quic` does not +answer can still connect over `tcp` without a separate retry. + Admitted peers are ranked by how many attestation subnets they advertise that no currently connected peer covers, so discovery preferentially fills gaps in subnet coverage. A peer advertising no `attnets` is ranked last but never dropped. @@ -87,25 +102,29 @@ nothing in ethrex's peer table or discv5's own pacing enforces it (see ## Bootnodes -The two entries a bootnode ENR can carry are read independently, because they +The three entries a bootnode ENR can carry are read independently, because they answer different questions: | Entry | Absent means | | --- | --- | -| `quic` | Not dialed statically by `build_swarm`; discv5 seed only | -| `udp` | Not seeded into the discv5 routing table; static dial target only | - -Neither absence is an error, and a record carrying just one of them is still -kept. The ENRs `lean-quickstart` generates today carry `ip`/`quic`/`secp256k1` -and no `udp`, so they stay reachable but contribute nothing to discovery. Every -beacon-chain bootnode published today is the mirror image: a `udp` port but no -`quic`, usable as a discv5 seed but never dialed. A record with neither is -dropped with a warning, as is one missing an `ip` or a `secp256k1` key. +| `quic` | Not part of the static dial list over QUIC | +| `tcp` | Not part of the static dial list over TCP | +| `udp` | Not seeded into the discv5 routing table | + +A bootnode is dropped only when it has none of the three: neither transport to +dial nor a `udp` port to seed discv5 from. Any other combination is kept, +including one with only `quic`, only `tcp`, only `udp`, or any pair. The ENRs +`lean-quickstart` generates today carry `ip`/`quic`/`secp256k1` and no `udp`, +so they stay reachable but contribute nothing to discovery. A beacon-chain +bootnode is close to the mirror image, `udp` and `tcp` but no `quic`, and the +`tcp` entry is what now makes it statically dialable rather than a discv5 seed +only. A record missing an `ip` or a `secp256k1` key is dropped regardless of +its transports. The ENR a node logs at startup is only useful to a peer if that node was started with a real `--discovery.advertise-ip`. Copying an ENR built from the default `0.0.0.0` into another node's bootnode -list produces a `udp`/`quic` target that cannot be dialed, since `0.0.0.0` +list produces a `udp`/`quic`/`tcp` target that cannot be dialed, since `0.0.0.0` names no reachable host. Set `--discovery.advertise-ip` before pointing other nodes at this one's ENR: `127.0.0.1` on a local devnet, or the host's public address otherwise. From 39692cff8b1806057adfdbb9ab50d50531f78d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1s=20Gr=C3=BCner?= <47506558+MegaRedHand@users.noreply.github.com> Date: Wed, 2 Sep 2026 11:44:09 -0300 Subject: [PATCH 3/3] fix(p2p): harden the TCP transport against review findings The switch from a single `Multiaddr` to `DialOpts::peer_id` changed the dial condition from `Always` to the default `DisconnectedAndNotDialing`, and two call sites were written as if it had not. `build_swarm` dialed under `.unwrap()`, so a bootnode file naming one peer twice (the same ENR pasted twice, or an old and a new record for one secp256k1 key, both decoding to one `PeerId`) aborted the node on the second entry: the swarm refuses that dial synchronously with `DialPeerConditionFalse`. It now dedups by peer id, merging the two entries' address lists into one dial, and a refused dial warns rather than panics. The discovery dial loop returned early on any refusal, having already popped the candidate and marked it tried, so the `peer_attnets` insert was skipped and `covered_subnets` under-counted a peer that does connect. The refusals want opposite handling, so the swarm adapter now reports which one it was: `AlreadyInProgress` has a terminal event coming and is safe to record, `Unreachable` has none and would leak. Both listeners were bound with `.expect()`. `build_swarm` already returned a `Result`, so they are typed errors now, and the CLI rejects the clashes that cause them before anything binds: `--gossipsub-port` against `--api-port` and `--metrics-port`, legal while the swarm bound UDP only, and `0` under `--discovery.enable`, which would publish a record naming neither of the two real OS-assigned ports. The ENR writer also drops a `0` port, matching every reader's rule that `0` means absent. "QUIC first, so it stays the preferred path" was wrong. The pinned fork's `ConcurrentDial` pushes up to `dial_concurrency_factor` addresses into one `FuturesUnordered` and takes whichever handshake finishes first, and the default factor exceeds the two addresses a lean peer can offer, so list order decides nothing. That race is what the transport is for, so it stays; the claim is gone from the code, the docs and the test that asserted it, replaced by the cost it actually carries (two handshakes per dial, both ends). The ENR entry set changed without its sequence number moving, so a peer that stayed up across an in-place upgrade would keep the tcp-less record forever: ethrex's WHOAREYOU responder omits the record when `enr_seq` matches, and the peer table only accepts a strictly higher seq. `LOCAL_ENR_SEQ` replaces ethrex's `INITIAL_ENR_SEQ`, with a compile-time floor and a table recording which entry set each value stands for. Also: one `dial_addrs` helper behind both dial paths, so what counts as dialable cannot diverge between them, plus a `read_tcp_port` sibling to `read_quic_port` holding the port-0 rule in one place; `EXPOSE 9001/tcp`, so a container does not advertise a `tcp` entry nothing outside can reach; and the stale doc comments in `discovery/mod.rs` that still described a QUIC-only client with untouched bootnode dialing. --- CLAUDE.md | 5 +- Dockerfile | 7 +- bin/ethlambda/src/cli.rs | 94 ++++++++- bin/ethlambda/src/main.rs | 2 +- crates/net/p2p/src/discovery/admission.rs | 46 ++--- crates/net/p2p/src/discovery/dial.rs | 27 ++- crates/net/p2p/src/discovery/enr.rs | 108 +++++++++- crates/net/p2p/src/discovery/mod.rs | 17 +- crates/net/p2p/src/lib.rs | 229 +++++++++++++++++++--- crates/net/p2p/src/swarm_adapter.rs | 72 +++++-- docs/discovery.md | 32 ++- 11 files changed, 526 insertions(+), 113 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 3c4f7e05..649e4bbc 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -273,7 +273,8 @@ actual_slot = finalized_slot + 1 + relative_index ## Networking (libp2p) ### Protocols -- **Transport**: QUIC over UDP (TLS 1.3), plus TCP (noise + yamux) on the same port number as a fallback: a peer whose advertised `quic` doesn't answer can still be reached over TCP, and libp2p races both addresses within one dial +- **Transport**: QUIC over UDP (TLS 1.3), plus TCP (noise + yamux) on the same port number as a fallback: a peer whose advertised `quic` doesn't answer can still be reached over TCP, and libp2p races both addresses within one dial (list order confers no preference; the default `dial_concurrency_factor` starts both handshakes) + - Binding TCP puts `--gossipsub-port` in the HTTP servers' namespace, so it must now differ from `--api-port`/`--metrics-port` too. `NodeOptions::validate_ports` rejects every clash before anything binds - **Gossipsub**: Blocks + Attestations (snappy raw compression) - Topic: `/leanconsensus/{fork_digest}/{block|aggregation|attestation_N}/ssz_snappy` - `fork_digest` is a 4-byte hex string (no `0x` prefix); currently the dummy `12345678` agreed across clients @@ -314,7 +315,7 @@ GENESIS_VALIDATORS: - All genesis state fields (checkpoints, justified_slots, etc.) initialize to zero/empty defaults - Matches Ream/Zeam format — no extra state fields in the config file -**Bootnodes:** ENR records (Base64-encoded, RLP decoded for QUIC port + secp256k1 pubkey) +**Bootnodes:** ENR records (Base64-encoded, RLP decoded for the `quic`/`tcp`/`udp` ports + secp256k1 pubkey). A `0` port means absent everywhere, on read and on write. `build_swarm` dedups by `PeerId`: two entries naming one key merge into a single dial ## Testing diff --git a/Dockerfile b/Dockerfile index 61b7cd89..df228ea1 100644 --- a/Dockerfile +++ b/Dockerfile @@ -77,7 +77,12 @@ COPY LICENSE ./ # 9000/tcp, 9000/udp - P2P networking (discv5 when --discovery.enable) # 9001/udp - libp2p QUIC connections +# 9001/tcp - libp2p TCP (noise + yamux) connections, the fallback transport # 5052 - API RPC # 5054 - Prometheus metrics -EXPOSE 9000/tcp 9000/udp 9001/udp 5052 5054 +# +# The swarm binds both protocols on 9001 and the ENR advertises both, so a `tcp` +# entry with no mapping behind it is worse than none: every peer racing the +# address list burns a connect timeout on a black-holed port. +EXPOSE 9000/tcp 9000/udp 9001/udp 9001/tcp 5052 5054 ENTRYPOINT ["/usr/local/bin/ethlambda"] diff --git a/bin/ethlambda/src/cli.rs b/bin/ethlambda/src/cli.rs index c203a611..675f0011 100644 --- a/bin/ethlambda/src/cli.rs +++ b/bin/ethlambda/src/cli.rs @@ -21,10 +21,15 @@ pub(crate) struct NodeOptions { /// Directory containing per-validator XMSS keys (e.g., hash-sig-keys/). #[arg(long)] pub(crate) hash_sig_keys_dir: PathBuf, - /// UDP port for the libp2p QUIC listener. + /// Port for the libp2p listeners: UDP for QUIC and TCP for the noise+yamux + /// fallback, both on this same number. + /// + /// TCP and UDP are separate namespaces, so one number names both. It must + /// still differ from every other port the node binds: `--discovery.port` + /// (also UDP), and `--api-port`/`--metrics-port` (also TCP). /// /// Defaults one above `--discovery.port` so that `--discovery.enable` works - /// on its own: both are UDP sockets and cannot share a port. + /// on its own. #[arg(long, default_value = "9001")] pub(crate) gossipsub_port: u16, #[arg(long, default_value = "127.0.0.1")] @@ -160,11 +165,16 @@ pub(crate) struct DiscoveryConfig { } impl NodeOptions { - /// Reject a discovery port that collides with the QUIC port. + /// Reject port assignments that cannot all bind, before anything binds. /// - /// Both are UDP. Without this the collision surfaces at bind time as an - /// opaque `EADDRINUSE` on whichever socket loses the race. - pub(crate) fn validate_discovery(&self) -> eyre::Result<()> { + /// There are two clashes to catch, on two protocols. `--discovery.port` and + /// `--gossipsub-port` are both UDP. `--gossipsub-port` also binds TCP for + /// the noise+yamux listener, which puts it in the same namespace as the + /// HTTP servers: sharing that number with `--api-port` was legal while the + /// swarm bound UDP only, and is now a real collision. Without these checks + /// either surfaces at bind time as an opaque `EADDRINUSE` on whichever + /// socket loses the race. + pub(crate) fn validate_ports(&self) -> eyre::Result<()> { if self.discovery.enable && self.discovery.port == self.gossipsub_port { eyre::bail!( "--discovery.port ({}) must differ from --gossipsub-port ({}): \ @@ -173,6 +183,28 @@ impl NodeOptions { self.gossipsub_port ); } + // A discovery-enabled node publishes `quic` and `tcp` entries naming + // this port. Port 0 asks the OS to pick, so the two listeners land on + // different real ports and the record advertises neither of them: a + // peer reading it finds nothing dialable. + if self.discovery.enable && self.gossipsub_port == 0 { + eyre::bail!( + "--gossipsub-port 0 cannot be used with --discovery.enable: the \ + advertised ENR would name port 0, which no peer can dial" + ); + } + for (flag, port) in [ + ("--api-port", self.api_port), + ("--metrics-port", self.metrics_port), + ] { + if port == self.gossipsub_port { + eyre::bail!( + "{flag} ({port}) must differ from --gossipsub-port ({}): the \ + libp2p swarm binds TCP on that port as well as UDP", + self.gossipsub_port + ); + } + } Ok(()) } } @@ -255,16 +287,58 @@ mod tests { let options = parse(&["--discovery.enable"]); assert_ne!(options.discovery.port, options.gossipsub_port); - assert!(options.validate_discovery().is_ok()); + assert!(options.validate_ports().is_ok()); } #[test] - fn colliding_ports_are_rejected_only_when_discovery_is_enabled() { + fn colliding_udp_ports_are_rejected_only_when_discovery_is_enabled() { let ports = ["--gossipsub-port", "9000", "--discovery.port", "9000"]; let mut enabled = ports.to_vec(); enabled.push("--discovery.enable"); - assert!(parse(&ports).validate_discovery().is_ok()); - assert!(parse(&enabled).validate_discovery().is_err()); + assert!(parse(&ports).validate_ports().is_ok()); + assert!(parse(&enabled).validate_ports().is_err()); + } + + /// Unlike the UDP clash above, this one does not depend on discovery: the + /// swarm binds TCP either way, so an HTTP port sharing the number always + /// loses one of the two listeners. + #[test] + fn an_http_port_sharing_the_gossipsub_port_is_rejected() { + // A port no default claims, so only the flag under test collides and + // the message can be checked for naming it. + const SHARED: &str = "9100"; + + for flag in ["--api-port", "--metrics-port"] { + let err = parse(&["--gossipsub-port", SHARED, flag, SHARED]) + .validate_ports() + .expect_err("a TCP clash with an HTTP port must be rejected"); + assert!( + err.to_string().contains(flag), + "the message must name the offending flag, got: {err}" + ); + } + } + + /// `--api-port` and `--metrics-port` sharing one number is supported (the + /// RPC crate merges the routers onto a single listener), so the TCP check + /// must not sweep that up. + #[test] + fn api_and_metrics_may_share_a_port() { + let options = parse(&["--api-port", "5052", "--metrics-port", "5052"]); + + assert!(options.validate_ports().is_ok()); + } + + /// Port 0 leaves the two listeners on different OS-assigned ports, so the + /// one number the ENR publishes describes neither. + #[test] + fn gossipsub_port_zero_is_rejected_with_discovery() { + assert!(parse(&["--gossipsub-port", "0"]).validate_ports().is_ok()); + assert!( + parse(&["--gossipsub-port", "0", "--discovery.enable"]) + .validate_ports() + .is_err() + ); } } diff --git a/bin/ethlambda/src/main.rs b/bin/ethlambda/src/main.rs index d4788e55..57c282e4 100644 --- a/bin/ethlambda/src/main.rs +++ b/bin/ethlambda/src/main.rs @@ -117,7 +117,7 @@ fn init_benchmark_logging() -> eyre::Result<()> { #[cfg_attr(not(feature = "shadow-integration"), tokio::main)] #[cfg_attr(feature = "shadow-integration", tokio::main(flavor = "current_thread"))] async fn run_node(options: NodeOptions) -> eyre::Result<()> { - options.validate_discovery()?; + options.validate_ports()?; #[cfg(feature = "shadow-integration")] init_shadow_cost(&options.shadow); diff --git a/crates/net/p2p/src/discovery/admission.rs b/crates/net/p2p/src/discovery/admission.rs index 7f058d8c..757e2ebd 100644 --- a/crates/net/p2p/src/discovery/admission.rs +++ b/crates/net/p2p/src/discovery/admission.rs @@ -27,15 +27,15 @@ use tracing::debug; use super::enr::{ ATTNETS_ENR_KEY, ETH2_ENR_KEY, EnrForkId, read_ip, read_public_key, read_quic_port, - subnets_from_attnets, + read_tcp_port, subnets_from_attnets, }; -use crate::{quic_multiaddr, tcp_multiaddr}; +use crate::dial_addrs; /// A peer that passed admission and is ready to dial. #[derive(Debug, Clone, PartialEq)] pub(crate) struct DiscoveredPeer { pub(crate) peer_id: PeerId, - /// Dial targets, QUIC first then TCP, built from whichever of the two ports + /// Every dial target for this peer, built from whichever of the two ports /// the record actually advertises. Never empty: [`admit`] rejects a record /// with neither. pub(crate) addrs: Vec, @@ -153,7 +153,7 @@ fn admit( // fall back to. A `0` port is absent for either transport: it RLP-decodes // the same way an absent entry does, and is undialable regardless. let quic_port = read_quic_port(record); - let tcp_port = pairs.tcp_port.filter(|port| *port != 0); + let tcp_port = read_tcp_port(pairs); if quic_port.is_none() && tcp_port.is_none() { return Err(RejectReason::NoDialableTransport); } @@ -168,19 +168,9 @@ fn admit( .map(|bits| subnets_from_attnets(&bits, attestation_committee_count)) .unwrap_or_default(); - // QUIC first, so it stays the preferred path when a peer offers both and - // libp2p races the list. - let mut addrs = Vec::with_capacity(2); - if let Some(port) = quic_port { - addrs.push(quic_multiaddr(ip, port, peer_id)); - } - if let Some(port) = tcp_port { - addrs.push(tcp_multiaddr(ip, port, peer_id)); - } - Ok(DiscoveredPeer { peer_id, - addrs, + addrs: dial_addrs(ip, quic_port, tcp_port, peer_id), subnets, }) } @@ -330,24 +320,26 @@ mod tests { } #[test] - fn accepts_a_peer_with_both_transports_and_orders_quic_first() { + fn accepts_a_peer_with_both_transports_and_offers_both_addresses() { + // Both addresses must reach the dial, and nothing here pins their + // order: libp2p races them within one attempt and takes whichever + // handshake finishes first, so position confers no preference (see + // `dial_addrs`). let record = record_with(|pairs| { set_eth2(pairs, EnrForkId::local()); set_quic(pairs, 9001); pairs.tcp_port = Some(9002); }); let peer = admit_record(&record).expect("accepted"); - assert_eq!( - peer.addrs, - vec![ - format!("/ip4/127.0.0.1/udp/9001/quic-v1/p2p/{}", peer.peer_id) - .parse() - .unwrap(), - format!("/ip4/127.0.0.1/tcp/9002/p2p/{}", peer.peer_id) - .parse() - .unwrap(), - ] - ); + let expected: HashSet = HashSet::from([ + format!("/ip4/127.0.0.1/udp/9001/quic-v1/p2p/{}", peer.peer_id) + .parse() + .unwrap(), + format!("/ip4/127.0.0.1/tcp/9002/p2p/{}", peer.peer_id) + .parse() + .unwrap(), + ]); + assert_eq!(peer.addrs.iter().cloned().collect::>(), expected); } #[test] diff --git a/crates/net/p2p/src/discovery/dial.rs b/crates/net/p2p/src/discovery/dial.rs index f8d223f5..6e45d7b3 100644 --- a/crates/net/p2p/src/discovery/dial.rs +++ b/crates/net/p2p/src/discovery/dial.rs @@ -1,4 +1,4 @@ -//! The dial loop: turn what discv5 found into libp2p QUIC connections. +//! The dial loop: turn what discv5 found into libp2p connections. //! //! Runs as a `P2PServer` tick every [`DISCOVERY_DIAL_INTERVAL`], drawing //! candidates from the ethrex peer table, ranking them by subnet coverage, and @@ -13,6 +13,7 @@ use tracing::info; use super::admission::{DiscoveredPeer, LeanFilter, rank_by_uncovered_subnets}; use super::{DISCOVERY_CANDIDATE_BATCH, DiscoveryHandle}; +use crate::swarm_adapter::DialOutcome; use crate::{P2PServer, metrics}; /// Everything the dial loop needs from a running discovery server. @@ -106,20 +107,30 @@ pub(crate) async fn dial_tick(server: &mut P2PServer) { subnets = ?candidate.subnets, "Dialing discovered peer" ); - // Record the peer only once the swarm has taken the dial. A dial it rejects - // synchronously — already connected, already dialing, no addresses, denied — - // raises no `OutgoingConnectionError`, so `forget_discovered_peer` would - // never run and the entry would outlive the process's interest in it. // One `DialOpts` carrying every address, not one dial per address: libp2p // races them within the attempt, which is what lets a live TCP address // rescue a peer whose advertised QUIC port does not answer. let opts = DialOpts::peer_id(candidate.peer_id) .addresses(candidate.addrs) .build(); - if !server.swarm_handle.dial_accepted(opts).await { - return; + // The candidate has already been popped and marked tried in the peer table, + // so this is the only chance to record its subnets: whatever happens here, + // it will not be offered again. Which makes the refusal the swarm gives back + // decide whether recording them is right. + match server.swarm_handle.dial_outcome(opts).await { + // Nothing in flight and nothing coming, so `forget_discovered_peer` + // would never run: recording the subnets here would leave + // `covered_subnets` counting a peer we never reach. + DialOutcome::Unreachable => return, + // A dial to this peer is already in flight, from an earlier tick or from + // the static bootnode path in `build_swarm`. Recording is still right: + // that attempt has a terminal event coming, which tears the entry down. + // Skipping it is what would drift, and permanently — the peer connects, + // covers subnets, and `covered_subnets` never counts them, so the dial + // loop keeps hunting for coverage it already has. + DialOutcome::AlreadyInProgress => {} + DialOutcome::Queued => metrics::inc_discovered_peers_dialed(), } - metrics::inc_discovered_peers_dialed(); if let Some(discovery) = server.discovery.as_mut() { discovery .peer_attnets diff --git a/crates/net/p2p/src/discovery/enr.rs b/crates/net/p2p/src/discovery/enr.rs index 820f07d5..f97dbc9f 100644 --- a/crates/net/p2p/src/discovery/enr.rs +++ b/crates/net/p2p/src/discovery/enr.rs @@ -23,7 +23,7 @@ use std::collections::HashSet; use std::net::IpAddr; use ethlambda_types::constants::FORK_DIGEST; -use ethrex_p2p::types::{INITIAL_ENR_SEQ, Node, NodeRecord, NodeRecordPairs}; +use ethrex_p2p::types::{Node, NodeRecord, NodeRecordPairs}; use ethrex_p2p::utils::public_key_from_signing_key; use libssz::SszEncode; use libssz_derive::{SszDecode, SszEncode}; @@ -148,7 +148,13 @@ impl LocalEnrParams { fn local_pairs(&self) -> NodeRecordPairs { let mut pairs = NodeRecordPairs { udp_port: Some(self.discovery_port), - tcp_port: Some(self.tcp_port), + // A `0` here is spelled by the entry's absence, matching what + // `read_tcp_port` and `read_quic_port` make of one on the way in. + // The ports come from configuration rather than from the bound + // listeners, so `--gossipsub-port 0` reaches this with a `0` that + // describes neither of the two real OS-assigned ports; publishing + // it would advertise a transport nothing answers on. + tcp_port: Some(self.tcp_port).filter(|port| *port != 0), ..Default::default() }; match self.ip.to_canonical() { @@ -163,14 +169,41 @@ impl LocalEnrParams { let attnets = encode_attnets(&self.subscription_subnets, self.attestation_committee_count); pairs.set_extra(ATTNETS_ENR_KEY, attnets); pairs.set_extra(ETH2_ENR_KEY, EnrForkId::local().to_ssz()); - pairs.set_extra_int(QUIC_ENR_KEY, self.quic_port.into()); + if self.quic_port != 0 { + pairs.set_extra_int(QUIC_ENR_KEY, self.quic_port.into()); + } pairs } } +/// Sequence number this node signs its ENR at. +/// +/// **Bump this whenever [`LocalEnrParams::local_pairs`] changes which entries it +/// publishes.** A record is identified by (node id, seq), so two ethlambda +/// versions advertising different entry sets under one seq are indistinguishable +/// to a peer: ethrex's WHOAREYOU responder omits the record when the requester's +/// `enr_seq` already matches, and the peer table only accepts a record whose seq +/// is strictly higher than the one it holds. A peer that stayed up across an +/// in-place upgrade would keep the old entry set forever. +/// +/// ethrex re-signs at a higher seq of its own whenever discv5's IP voting moves +/// the advertised address, so this is a floor rather than the only value a +/// record is ever seen at. +/// +/// | Value | Entry set | +/// | --- | --- | +/// | 1 | `id`, `ip`, `udp`, `quic`, `secp256k1`, `eth2`, `attnets` | +/// | 2 | the above plus `tcp` | +const LOCAL_ENR_SEQ: u64 = 2; + +/// The `tcp`-less entry set was published at seq 1, so anything at or below it +/// is a record peers may already hold under a different set. Checked here rather +/// than in a test: a wrong value must not compile. +const _: () = assert!(LOCAL_ENR_SEQ > 1); + /// Build and sign this node's ENR. pub(crate) fn build_local_enr(params: &LocalEnrParams) -> Result { - NodeRecord::from_pairs(INITIAL_ENR_SEQ, ¶ms.signer, params.local_pairs()) + NodeRecord::from_pairs(LOCAL_ENR_SEQ, ¶ms.signer, params.local_pairs()) .map_err(DiscoveryError::BuildEnr) } @@ -214,6 +247,18 @@ pub(crate) fn read_quic_port(record: &NodeRecord) -> Option { .filter(|port| *port != 0) } +/// The advertised libp2p TCP port, if it is one we could dial. +/// +/// `tcp` is a first-class entry rather than an `extra`, so an absent one is +/// already `None`; the filter is for the literal `0`, which decodes the same way +/// an absent entry does and names nothing dialable either. Same answer as +/// [`read_quic_port`] gives for `quic`, and it is deliberately the only place +/// that rule is spelled: both dial paths and the ENR writer go through it, so a +/// `0` cannot mean "absent" on one side and "port zero" on the other. +pub(crate) fn read_tcp_port(pairs: &NodeRecordPairs) -> Option { + pairs.tcp_port.filter(|port| *port != 0) +} + #[cfg(test)] mod tests { use super::*; @@ -310,6 +355,61 @@ mod tests { assert_eq!(read_quic_port(&record), Some(9001)); } + /// The advertised ports come from configuration, not from the bound + /// listeners, so a `--gossipsub-port 0` reaches the writer as a literal `0` + /// that names neither of the two real OS-assigned ports. Every reader treats + /// `0` as absent, so the writer must not emit it: the alternative is a + /// record that satisfies lighthouse's `tcp4().is_some()` predicate while our + /// own `admit` rejects it as `NoDialableTransport`. + #[test] + fn local_enr_omits_a_zero_quic_and_tcp_port() { + let record = build_local_enr(&LocalEnrParams { + signer: secp256k1::SecretKey::new(&mut rand::rngs::OsRng), + ip: IpAddr::from(Ipv4Addr::LOCALHOST), + discovery_port: 9010, + quic_port: 0, + tcp_port: 0, + subscription_subnets: HashSet::from([1u64]), + attestation_committee_count: 8, + }) + .expect("ENR builds"); + + assert_eq!( + record.pairs().tcp_port, + None, + "a tcp: 0 must not be emitted" + ); + assert!( + record.pairs().extra(QUIC_ENR_KEY).is_none(), + "a quic: 0 must not be emitted" + ); + // The discovery port is unaffected: `spawn_discovery` binds first and + // passes the real bound port, so a 0 never reaches here. + assert_eq!(record.pairs().udp_port, Some(9010)); + } + + #[test] + fn read_tcp_port_treats_zero_as_absent() { + // Same answer `read_quic_port` gives for `quic: 0`, so the two dial + // paths and the ENR writer cannot disagree about what `0` means. + let mut pairs = NodeRecordPairs::default(); + assert_eq!(read_tcp_port(&pairs), None, "absent"); + pairs.tcp_port = Some(0); + assert_eq!(read_tcp_port(&pairs), None, "explicit zero"); + pairs.tcp_port = Some(9001); + assert_eq!(read_tcp_port(&pairs), Some(9001)); + } + + /// A peer identifies a record by (node id, seq) and only accepts a strictly + /// higher seq, so an entry-set change under an unchanged seq is invisible to + /// every peer that already holds the old record. The floor itself is a + /// compile-time check next to the constant; what this pins is that the + /// builder actually signs at it, rather than at ethrex's `INITIAL_ENR_SEQ`. + #[test] + fn the_built_enr_carries_the_local_seq() { + assert_eq!(build().seq, LOCAL_ENR_SEQ); + } + #[test] fn local_enr_carries_the_fork_id() { let record = build(); diff --git a/crates/net/p2p/src/discovery/mod.rs b/crates/net/p2p/src/discovery/mod.rs index 859aec5b..1db21b3f 100644 --- a/crates/net/p2p/src/discovery/mod.rs +++ b/crates/net/p2p/src/discovery/mod.rs @@ -3,7 +3,9 @@ //! ethrex's `DiscoveryServer` runs discv5-only on its own UDP socket and writes //! what it finds into an ethrex `PeerTable`. ethlambda's `P2PServer` polls that //! table, applies the spec checks in [`admission`], and dials the survivors over -//! libp2p QUIC. Static bootnode dialing is untouched. +//! libp2p. Both dial paths, discovered peers and static bootnodes alike, hand +//! the swarm every address a peer advertises (`quic` and `tcp`) in one attempt +//! and let libp2p race them. //! //! See `docs/discovery.md` for the operator-facing description. @@ -120,8 +122,10 @@ pub struct DiscoveryHandle { /// (ask the OS for a free port) still produces an ENR advertising the real /// bound port rather than the literal 0, which would be undialable. /// -/// Only bootnodes whose ENR advertises a `udp` port can seed discv5; the rest -/// are still dialed statically by `build_swarm`. +/// Only bootnodes whose ENR advertises a `udp` port can seed discv5. That is a +/// separate question from whether `build_swarm` dials one statically, which +/// turns on its `quic`/`tcp` entries: a bootnode can do both, either, or +/// neither. pub async fn spawn_discovery( config: DiscoverySpawnConfig, ) -> Result { @@ -180,9 +184,10 @@ pub async fn spawn_discovery( // The record we hand over is the same one `enr_url` above reported, so what // ethrex answers discv5 queries with carries the consensus entries (`eth2`, - // `attnets`, `quic`) and a lean peer applying our own admission rules to it - // admits us. ethrex re-signs it under `params.signer` whenever IP voting - // bumps the sequence number, keeping the extra entries. + // `attnets`, `quic`) and the `tcp` port of the fallback transport, and a + // lean peer applying our own admission rules to it admits us. ethrex + // re-signs it under `params.signer` whenever IP voting bumps the sequence + // number, keeping the extra entries. DiscoveryServer::spawn( local_node, local_record, diff --git a/crates/net/p2p/src/lib.rs b/crates/net/p2p/src/lib.rs index be54e2ad..6c1366cd 100644 --- a/crates/net/p2p/src/lib.rs +++ b/crates/net/p2p/src/lib.rs @@ -1,5 +1,5 @@ use std::{ - collections::{HashMap, HashSet}, + collections::{HashMap, HashSet, hash_map::Entry}, net::{IpAddr, SocketAddr}, ops::Range, time::Duration, @@ -38,7 +38,7 @@ use crate::{ discovery::{ DISCOVERY_DIAL_INTERVAL, DiscoveryError, DiscoverySpawnConfig, dial::{DiscoveryState, dial_tick, forget_discovered_peer}, - enr::{read_ip, read_public_key, read_quic_port}, + enr::{read_ip, read_public_key, read_quic_port, read_tcp_port}, spawn_discovery, }, gossipsub::{ @@ -219,15 +219,33 @@ pub struct BuiltSwarm { pub(crate) attestation_committee_count: u64, pub(crate) block_topic: libp2p::gossipsub::IdentTopic, pub(crate) aggregation_topic: libp2p::gossipsub::IdentTopic, - /// Dial targets per bootnode, QUIC first then TCP. Empty entries are never + /// Every dial target per bootnode; see [`dial_addrs`]. Empty entries are never /// inserted; see [`bootnode_dial_addrs`]. pub(crate) bootnode_addrs: HashMap>, } +/// Why [`build_swarm`] could not produce a usable swarm. +/// +/// Both listeners are fatal rather than best-effort. Carrying on after a failed +/// TCP bind would leave the node advertising a `tcp` entry nothing answers, +/// which is the failure this transport exists to remove, inverted. The +/// configuration cases are caught before anything binds (`validate_ports` in +/// the CLI), so reaching this means the port is genuinely taken. +#[derive(Debug, thiserror::Error)] +pub enum SwarmBuildError { + #[error("failed to bind the gossipsub {transport} listener on {addr}: {source}")] + Listen { + transport: &'static str, + addr: Multiaddr, + #[source] + source: libp2p::TransportError, + }, + #[error("failed to subscribe to a gossipsub topic: {0}")] + Subscription(#[from] libp2p::gossipsub::SubscriptionError), +} + /// Build and configure the libp2p swarm, dial bootnodes, subscribe to topics. -pub fn build_swarm( - config: SwarmConfig, -) -> Result { +pub fn build_swarm(config: SwarmConfig) -> Result { let gossipsub_config = libp2p::gossipsub::ConfigBuilder::default() // d .mesh_n(8) @@ -310,7 +328,7 @@ pub fn build_swarm( }) .build(); let local_peer_id = *swarm.local_peer_id(); - let mut bootnode_addrs = HashMap::new(); + let mut bootnode_addrs: HashMap> = HashMap::new(); let mut undialable_bootnodes = 0usize; for bootnode in config.bootnodes { let peer_id = PeerId::from_public_key(&bootnode.public_key); @@ -325,10 +343,40 @@ pub fn build_swarm( debug!(%peer_id, ip = %bootnode.ip, "Bootnode advertises no dialable transport, discv5 seed only"); continue; } - bootnode_addrs.insert(peer_id, addrs.clone()); + // One dial per peer id, not one per file entry. Two entries can name a + // single peer: the same ENR pasted twice, or an old and a new record + // for one secp256k1 key. `DialOpts::peer_id` dials under the default + // `DisconnectedAndNotDialing` condition, so the second attempt is + // refused while the first is still in flight, and `parse_enrs` does not + // dedup. Merging the address lists dials once with everything the + // duplicate entries offered between them. + match bootnode_addrs.entry(peer_id) { + Entry::Occupied(mut known) => { + debug!( + %peer_id, + ip = %bootnode.ip, + "Bootnode list names this peer more than once, merging its addresses" + ); + let merged = known.get_mut(); + for addr in addrs { + if !merged.contains(&addr) { + merged.push(addr); + } + } + continue; + } + Entry::Vacant(unknown) => { + unknown.insert(addrs.clone()); + } + } + // A refused dial is not fatal: the entry stays in `bootnode_addrs`, so + // the redial path picks the peer up. Unwrapping here would abort a node + // over a bootnode file that is merely redundant. swarm .dial(DialOpts::peer_id(peer_id).addresses(addrs).build()) - .unwrap(); + .unwrap_or_else(|err| { + warn!(%peer_id, %err, "Swarm refused the initial bootnode dial"); + }); } // Every skip above is individually unremarkable and logged at `debug`, but a // list that produces no dial target at all leaves the node isolated unless @@ -345,16 +393,24 @@ pub fn build_swarm( .with(Protocol::Udp(config.listening_socket.port())) .with(Protocol::QuicV1); swarm - .listen_on(quic_addr) - .expect("failed to bind gossipsub QUIC listening address"); + .listen_on(quic_addr.clone()) + .map_err(|source| SwarmBuildError::Listen { + transport: "QUIC", + addr: quic_addr, + source, + })?; // Same port number as the QUIC listener above: TCP and UDP are separate // namespaces, so this cannot collide with it. let tcp_addr = Multiaddr::empty() .with(config.listening_socket.ip().into()) .with(Protocol::Tcp(config.listening_socket.port())); swarm - .listen_on(tcp_addr) - .expect("failed to bind gossipsub TCP listening address"); + .listen_on(tcp_addr.clone()) + .map_err(|source| SwarmBuildError::Listen { + transport: "TCP", + addr: tcp_addr, + source, + })?; // Subscribe to block topic (all nodes) let block_topic = block_topic(); @@ -934,7 +990,7 @@ fn parse_enr(enr_str: &str) -> Result { // Same rule for `tcp`, the transport every published beacon-chain bootnode // advertises and none of them pairs with a `quic` entry. - let tcp_port = pairs.tcp_port.filter(|port| *port != 0); + let tcp_port = read_tcp_port(pairs); let public_key = read_public_key(pairs) .ok_or_else(|| "node record missing or malformed public key".to_string())?; @@ -945,7 +1001,7 @@ fn parse_enr(enr_str: &str) -> Result { // of them is reachable by nothing we speak: it can be neither dialed nor // seeded. Drop it here rather than carry a contact no code path can use. if quic_port.is_none() && tcp_port.is_none() && udp_port.is_none() { - return Err("node advertises neither a quic, tcp nor a udp port".to_string()); + return Err("node advertises none of quic, tcp, or udp".to_string()); } Ok(Bootnode { @@ -959,24 +1015,45 @@ fn parse_enr(enr_str: &str) -> Result { // --- Utility functions --- -/// Dial targets for a static bootnode, QUIC first then TCP. +/// Every address worth trying for one peer, from whichever of its two ports are +/// present. /// -/// Empty when the record advertises neither, which is a discv5-only seed: it can -/// still answer FINDNODE, but there is nothing for the swarm to dial. -pub(crate) fn bootnode_dial_addrs(bootnode: &Bootnode, peer_id: PeerId) -> Vec { +/// Empty when neither is, which is a discv5-only seed: it can still answer +/// FINDNODE, but there is nothing for the swarm to dial. +/// +/// The order is not a preference. libp2p pushes up to `dial_concurrency_factor` +/// of these into one `FuturesUnordered` and takes whichever handshake finishes +/// first; the default factor is larger than this list can ever be, so both +/// transports are always attempted and the position here decides nothing. That +/// race is the point: a peer advertising a `quic` port nothing answers still +/// connects over `tcp` without waiting out a connect timeout first. +/// +/// Shared by both dial paths, so a change to what counts as dialable cannot +/// apply to static bootnodes and discovered peers differently: static bootnodes +/// in [`build_swarm`] and discovered peers in +/// [`admission::admit`](discovery::admission). +pub(crate) fn dial_addrs( + ip: IpAddr, + quic_port: Option, + tcp_port: Option, + peer_id: PeerId, +) -> Vec { let mut addrs = Vec::with_capacity(2); - if let Some(quic_port) = bootnode.quic_port { - addrs.push(quic_multiaddr(bootnode.ip, quic_port, peer_id)); + if let Some(port) = quic_port { + addrs.push(quic_multiaddr(ip, port, peer_id)); } - if let Some(tcp_port) = bootnode.tcp_port { - addrs.push(tcp_multiaddr(bootnode.ip, tcp_port, peer_id)); + if let Some(port) = tcp_port { + addrs.push(tcp_multiaddr(ip, port, peer_id)); } addrs } -/// The address of a libp2p QUIC listener, as both dial paths spell it: static -/// bootnodes in [`build_swarm`] and discovered peers in -/// [`admission::admit`](discovery::admission). +/// Dial targets for a static bootnode. See [`dial_addrs`]. +pub(crate) fn bootnode_dial_addrs(bootnode: &Bootnode, peer_id: PeerId) -> Vec { + dial_addrs(bootnode.ip, bootnode.quic_port, bootnode.tcp_port, peer_id) +} + +/// The address of a libp2p QUIC listener, as [`dial_addrs`] spells it. /// /// Infallible: `with_p2p` only rejects a multiaddr that already carries a `p2p` /// component, and this one is built fresh. @@ -1122,6 +1199,74 @@ mod tests { .expect("both swarms must connect over TCP within the timeout"); } + /// A bootnode file naming one peer twice must not abort the node. + /// + /// `DialOpts::peer_id` dials under the default `DisconnectedAndNotDialing` + /// condition, so a second dial while the first is in flight comes back as + /// `Err(DialPeerConditionFalse)` synchronously. Two file entries decode to + /// one `PeerId` whenever they share a secp256k1 key: the same ENR pasted + /// twice, or an old and a new record for one node. `parse_enrs` does not + /// dedup, so `build_swarm` has to, and it merges the address lists rather + /// than dropping whichever entry came second. + /// + /// The QUIC-only condition (`From`) this replaced was `Always`, + /// which is why the duplicate went unnoticed before. + #[tokio::test] + async fn a_bootnode_named_twice_is_dialed_once_with_both_addresses() { + let key = secp256k1::Keypair::generate(); + let public_key: PublicKey = key.public().clone().into(); + let peer_id = PeerId::from_public_key(&public_key); + let ip = IpAddr::from(Ipv4Addr::new(203, 0, 113, 1)); + // Two records for one key: the first advertising QUIC only, the second + // having since added TCP. Neither port is listening, which is fine — + // the dial only has to be *taken*. + let bootnodes = vec![ + Bootnode { + ip, + quic_port: Some(9001), + tcp_port: None, + udp_port: Some(9000), + public_key: public_key.clone(), + }, + Bootnode { + ip, + quic_port: Some(9001), + tcp_port: Some(9001), + udp_port: Some(9000), + public_key, + }, + ]; + + let built = build_swarm(SwarmConfig { + node_key: vec![7u8; 32], + bootnodes, + listening_socket: "127.0.0.1:0".parse().expect("valid socket"), + validator_ids: Vec::new(), + attestation_committee_count: 1, + subscription_subnets: HashSet::new(), + }) + .expect("a duplicated bootnode entry must not fail the build"); + + assert_eq!( + built.bootnode_addrs.len(), + 1, + "the two entries name one peer, so they must collapse to one dial target" + ); + let addrs = built + .bootnode_addrs + .get(&peer_id) + .expect("the bootnode is tracked under its peer id"); + let expected: HashSet = HashSet::from([ + quic_multiaddr(ip, 9001, peer_id), + tcp_multiaddr(ip, 9001, peer_id), + ]); + assert_eq!( + addrs.iter().cloned().collect::>(), + expected, + "the merged list must carry every address the duplicate entries offered" + ); + } + #[test] fn range_sync_state_merges_new_peer_ranges() { let first_peer = random_peer(); @@ -1300,13 +1445,16 @@ mod tests { } #[test] - fn parse_enrs_keeps_a_quic_less_record_as_a_discovery_seed() { + fn parse_enrs_keeps_a_quic_less_record_and_dials_it_over_tcp() { // Some nodes advertise `tcp` and `udp` but no `quic`, so requiring // `quic` here would drop the entire mainnet bootstrap list and leave - // discv5 with nothing to seed from. Such a record is kept, with - // `quic_port: None` telling `build_swarm` not to dial it. + // discv5 with nothing to seed from. Now that TCP is a transport we + // speak, such a record is not merely kept as a seed: the one that + // carries `tcp` becomes a static dial target too, over that port alone. // - // The two ENRs are from eth-clients/mainnet's `bootstrap_nodes.yaml`. + // The two ENRs are from eth-clients/mainnet's `bootstrap_nodes.yaml`, + // and they differ in exactly the way that matters here: the first + // advertises `tcp` and the second does not. let enrs = vec![ "enr:-Iu4QLm7bZGdAt9NSeJG0cEnJohWcQTQaI9wFLu3Q7eHIDfrI4cwtzvEW3F3VbG9XdFXlrHyFGeXPn9snTCQJ9bnMRABgmlkgnY0gmlwhAOTJQCJc2VjcDI1NmsxoQIZdZD6tDYpkpEfVo5bgiU8MGRjhcOmHGD2nErK0UKRrIN0Y3CCIyiDdWRwgiMo".to_string(), "enr:-Le4QPUXJS2BTORXxyx2Ia-9ae4YqA_JWX3ssj4E_J-3z1A-HmFGrU8BpvpqhNabayXeOZ2Nq_sbeDgtzMJpLLnXFgAChGV0aDKQtTA_KgEAAAAAIgEAAAAAAIJpZIJ2NIJpcISsaa0Zg2lwNpAkAIkHAAAAAPA8kv_-awoTiXNlY3AyNTZrMaEDHAD2JKYevx89W0CcFJFiskdcEzkH_Wdv9iW42qLK79ODdWRwgiMohHVkcDaCI4I".to_string(), @@ -1322,6 +1470,27 @@ mod tests { assert_eq!(bootnode.udp_port, Some(9000)); assert!(bootnode.as_discovery_node().is_some()); } + + // What the TCP transport changed: the first record's `tcp` entry is now + // a dial target, where before it produced no address and the bootnode + // was a discv5 seed and nothing more. + assert_eq!(bootnodes[0].tcp_port, Some(9000)); + let peer_id = PeerId::from_public_key(&bootnodes[0].public_key); + assert_eq!( + bootnode_dial_addrs(&bootnodes[0], peer_id), + vec![ + format!("/ip4/3.147.37.0/tcp/9000/p2p/{peer_id}") + .parse::() + .unwrap() + ], + "a tcp-only bootnode is dialed over tcp alone" + ); + + // The second advertises neither, so it stays a seed with nothing to + // dial: this is the case the `warn` in `build_swarm` counts. + assert_eq!(bootnodes[1].tcp_port, None); + let seed_only = PeerId::from_public_key(&bootnodes[1].public_key); + assert!(bootnode_dial_addrs(&bootnodes[1], seed_only).is_empty()); } #[test] diff --git a/crates/net/p2p/src/swarm_adapter.rs b/crates/net/p2p/src/swarm_adapter.rs index 16f438f7..481f7e65 100644 --- a/crates/net/p2p/src/swarm_adapter.rs +++ b/crates/net/p2p/src/swarm_adapter.rs @@ -27,9 +27,9 @@ pub enum SwarmCommand { /// attempt, which is what lets a live TCP address rescue a dial whose /// advertised QUIC port does not answer. opts: DialOpts, - /// Callback reporting whether the swarm accepted the dial. `None` when - /// the caller does not need to know. - accepted_tx: Option>, + /// Callback reporting what the swarm did with the dial. `None` when the + /// caller does not need to know. + outcome_tx: Option>, }, SendRequest { peer: PeerId, @@ -44,6 +44,35 @@ pub enum SwarmCommand { }, } +/// What the swarm did with a dial, as far as a caller's bookkeeping cares. +/// +/// The distinction matters because the two refusals want opposite handling. A +/// dial refused because one is already in flight has a terminal event coming +/// (`ConnectionEstablished` then eventually `ConnectionClosed`, or +/// `OutgoingConnectionError`), so per-peer bookkeeping recorded now is still +/// torn down later. A dial nothing will ever come of has no such event, so +/// recording anything would leak. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum DialOutcome { + /// The swarm took the dial. A terminal event will follow. + Queued, + /// Refused because this peer is already connected or already being dialed. + /// That other attempt still produces a terminal event. + AlreadyInProgress, + /// Refused with nothing in flight and nothing to come: no address to dial, a + /// behaviour denied it, the peer is us, or the adapter is gone. + Unreachable, +} + +impl From<&libp2p::swarm::DialError> for DialOutcome { + fn from(err: &libp2p::swarm::DialError) -> Self { + match err { + libp2p::swarm::DialError::DialPeerConditionFalse(_) => Self::AlreadyInProgress, + _ => Self::Unreachable, + } + } +} + #[derive(Clone)] pub struct SwarmHandle { cmd_tx: mpsc::UnboundedSender, @@ -62,35 +91,37 @@ impl SwarmHandle { .cmd_tx .send(SwarmCommand::Dial { opts, - accepted_tx: None, + outcome_tx: None, }) .inspect_err(|_| debug!("Swarm adapter closed, cannot dial")); } - /// Dial and report whether the swarm took the dial. + /// Dial and report what the swarm did with it. /// /// `Swarm::dial` rejects some dials synchronously — `LocalPeerId`, /// `NoAddresses`, `Denied`, and `DialPeerConditionFalse` (already connected, /// or already dialing) — and those produce **no** `OutgoingConnectionError` /// event. A caller that keeps per-dial bookkeeping has to know, or nothing - /// will ever tear that bookkeeping down. `false` also covers a dead adapter. + /// will ever tear that bookkeeping down, and it has to know *which* refusal + /// it was: see [`DialOutcome`]. /// - /// A `true` only means the dial was queued: success or failure still arrives - /// later as `ConnectionEstablished` or `OutgoingConnectionError`. - pub async fn dial_accepted(&self, opts: DialOpts) -> bool { + /// [`DialOutcome::Queued`] only means the dial was taken: success or failure + /// still arrives later as `ConnectionEstablished` or + /// `OutgoingConnectionError`. + pub async fn dial_outcome(&self, opts: DialOpts) -> DialOutcome { let (tx, rx) = tokio::sync::oneshot::channel(); if self .cmd_tx .send(SwarmCommand::Dial { opts, - accepted_tx: Some(tx), + outcome_tx: Some(tx), }) .is_err() { debug!("Swarm adapter closed, cannot dial"); - return false; + return DialOutcome::Unreachable; } - rx.await.unwrap_or(false) + rx.await.unwrap_or(DialOutcome::Unreachable) } /// Send a request and return the assigned OutboundRequestId. @@ -186,13 +217,16 @@ fn execute_command(swarm: &mut libp2p::Swarm, cmd: SwarmCommand) { .inspect_err(|err| debug!(%err, "Swarm adapter: publish failed")) .ok(); } - SwarmCommand::Dial { opts, accepted_tx } => { - let accepted = swarm - .dial(opts) - .inspect_err(|err| debug!(%err, "Swarm adapter: dial failed")) - .is_ok(); - if let Some(tx) = accepted_tx { - let _ = tx.send(accepted); + SwarmCommand::Dial { opts, outcome_tx } => { + let outcome = match swarm.dial(opts) { + Ok(()) => DialOutcome::Queued, + Err(err) => { + debug!(%err, "Swarm adapter: dial failed"); + DialOutcome::from(&err) + } + }; + if let Some(tx) = outcome_tx { + let _ = tx.send(outcome); } } SwarmCommand::SendRequest { diff --git a/docs/discovery.md b/docs/discovery.md index 0e3db6ca..1eeff4d7 100644 --- a/docs/discovery.md +++ b/docs/discovery.md @@ -47,8 +47,8 @@ The layout follows the discovery domain of the beacon-chain | `id` | `v4` | | `ip` | `--discovery.advertise-ip`, or the bind address (`0.0.0.0`) if unset | | `udp` | `--discovery.port` | -| `quic` | `--gossipsub-port`, the libp2p QUIC listener | -| `tcp` | `--gossipsub-port`, the libp2p TCP listener | +| `quic` | `--gossipsub-port`, the libp2p QUIC listener; omitted when `0` | +| `tcp` | `--gossipsub-port`, the libp2p TCP listener; omitted when `0` | | `secp256k1` | compressed public key from `--node-key` | | `eth2` | SSZ `ENRForkID`, 16 bytes | | `attnets` | subscribed attestation subnet bitfield | @@ -61,6 +61,20 @@ over TCP. It also gets us past lighthouse's discovery predicate, which requires `fork_digest` comparison; the lean fork digest is still the cross-client dummy `0x12345678`, so a beacon-chain client rejects us on that instead. +Both ports come from configuration rather than from the bound listeners, so a +`--gossipsub-port 0` would name neither of the two real OS-assigned ports. +Startup rejects that combination when discovery is enabled, and the writer omits +a `0` either way, matching every reader's rule that `0` means absent. + +Because the entry set is what a peer caches, the record carries a sequence +number (`LOCAL_ENR_SEQ`) that has to be bumped whenever that set changes. A peer +identifies a record by (node id, seq) and accepts a replacement only at a +strictly higher seq — ethrex's WHOAREYOU responder does not even send the record +when the requester's `enr_seq` already matches — so two ethlambda versions +publishing different entries under one seq are indistinguishable to any peer +that stayed up across the upgrade. Adding `tcp` bumped it; the constant's own +doc comment records which entry set each value stands for. + The local ENR is logged once at startup. This same record is handed to ethrex's `DiscoveryServer`, so it is what answers @@ -87,9 +101,17 @@ No rejection is final: the peer table runs the filter again as soon as the peer publishes a higher-`seq` ENR, so a node that adds a `quic` entry, or gains an address through discv5's IP voting, is reconsidered without a restart. -A peer's dial list carries every address it advertises, QUIC first then TCP. -libp2p races every address in one dial attempt, so a peer whose `quic` does not -answer can still connect over `tcp` without a separate retry. +A peer's dial list carries every address it advertises, `quic` and `tcp` both, +in one dial attempt. libp2p races them: it starts up to `dial_concurrency_factor` +handshakes at once and keeps whichever completes first, dropping the other. So a +peer whose `quic` port does not answer still connects over `tcp` with no separate +retry and no connect timeout waited out first. + +The list order is not a preference, and nothing should be read into it: the +default concurrency factor exceeds the two addresses a lean peer can offer, so +both are always attempted. The cost of that is the thing to know, since it is +paid on every dial rather than only on a failure: two sockets and two handshakes +per peer, on both ends, until one wins. Admitted peers are ranked by how many attestation subnets they advertise that no currently connected peer covers, so discovery preferentially fills gaps in subnet