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/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 { 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.