diff --git a/CLAUDE.md b/CLAUDE.md index a4b14c91..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) +- **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 @@ -283,8 +284,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 @@ -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 ff12119d..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); @@ -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..757e2ebd 100644 --- a/crates/net/p2p/src/discovery/admission.rs +++ b/crates/net/p2p/src/discovery/admission.rs @@ -27,15 +27,18 @@ 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; +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, - pub(crate) addr: Multiaddr, + /// 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, /// 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 = read_tcp_port(pairs); + 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)); @@ -158,7 +170,7 @@ fn admit( Ok(DiscoveredPeer { peer_id, - addr: quic_multiaddr(ip, quic_port, peer_id), + addrs: dial_addrs(ip, quic_port, tcp_port, peer_id), subnets, }) } @@ -264,7 +276,7 @@ mod tests { fn for_test(subnets: Vec) -> Self { Self { peer_id: PeerId::random(), - addr: Multiaddr::empty(), + addrs: vec![Multiaddr::empty()], subnets, } } @@ -279,11 +291,57 @@ 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_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"); + 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] fn rejects_a_peer_with_no_eth2_entry() { let record = record_with(|pairs| set_quic(pairs, 9001)); @@ -317,23 +375,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..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 @@ -8,10 +8,12 @@ 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}; 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. @@ -105,14 +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. - if !server.swarm_handle.dial_accepted(candidate.addr).await { - return; + // 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(); + // 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 f7f0a55d..f97dbc9f 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 @@ -21,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}; @@ -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,13 @@ impl LocalEnrParams { fn local_pairs(&self) -> NodeRecordPairs { let mut pairs = NodeRecordPairs { udp_port: Some(self.discovery_port), - tcp_port: None, + // 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() { @@ -160,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) } @@ -211,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::*; @@ -224,6 +272,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,17 +340,76 @@ 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)); } + /// 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 ecaaf63d..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. @@ -76,6 +78,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, @@ -117,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 { @@ -140,6 +147,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, }; @@ -176,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, @@ -228,6 +237,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..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, @@ -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; @@ -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,13 +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, - pub(crate) bootnode_addrs: HashMap, + /// 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) @@ -293,6 +313,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") @@ -302,42 +328,89 @@ 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 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); 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(); + } + // 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_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 - // 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.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.clone()) + .map_err(|source| SwarmBuildError::Listen { + transport: "TCP", + addr: tcp_addr, + source, + })?; // Subscribe to block topic (all nodes) let block_topic = block_topic(); @@ -480,7 +553,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 +618,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 +709,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 +729,7 @@ async fn handle_swarm_event( trace!( %peer_id, %direction, + %transport, peer_count, our_finalized_slot, our_head_slot, @@ -664,7 +744,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 +887,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 +912,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 +932,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 +943,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 +977,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 +988,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 = read_tcp_port(pairs); + 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 none of quic, tcp, or udp".to_string()); } Ok(Bootnode { ip, quic_port, + tcp_port, udp_port, public_key: public_key.into(), }) @@ -921,9 +1015,45 @@ fn parse_enr(enr_str: &str) -> Result { // --- Utility functions --- -/// The address of a libp2p QUIC listener, as both dial paths spell it: static -/// bootnodes in [`build_swarm`] and discovered peers in +/// Every address worth trying for one peer, from whichever of its two ports are +/// present. +/// +/// 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(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)); + } + addrs +} + +/// 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. @@ -936,6 +1066,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 +1086,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 +1132,141 @@ 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"); + } + + /// 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(); @@ -1152,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(), @@ -1174,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 d9cbffcb..481f7e65 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,10 +21,15 @@ pub enum SwarmCommand { data: Vec, }, Dial { - addr: Multiaddr, - /// Callback reporting whether the swarm accepted the dial. `None` when - /// the caller does not need to know. - accepted_tx: Option>, + /// 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 what the swarm did with the dial. `None` when the + /// caller does not need to know. + outcome_tx: Option>, }, SendRequest { peer: PeerId, @@ -39,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, @@ -52,40 +86,42 @@ 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, - accepted_tx: None, + opts, + 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, addr: Multiaddr) -> 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 { - addr, - accepted_tx: Some(tx), + opts, + 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. @@ -181,13 +217,16 @@ fn execute_command(swarm: &mut libp2p::Swarm, cmd: SwarmCommand) { .inspect_err(|err| debug!(%err, "Swarm adapter: publish failed")) .ok(); } - SwarmCommand::Dial { addr, accepted_tx } => { - let accepted = swarm - .dial(addr) - .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 8513fe51..1eeff4d7 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 @@ -45,11 +47,34 @@ 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 | +| `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 | +`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. + +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 @@ -64,7 +89,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 +101,18 @@ 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` 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 coverage. A peer advertising no `attnets` is ranked last but never dropped. @@ -87,25 +124,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.