From bc823e3ebe2d7a6018185f1136a501e934adc5eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Tue, 4 Nov 2025 13:21:43 -0500 Subject: [PATCH 01/40] rename PunchMe to ReachOut --- quinn-proto/src/connection/mod.rs | 2 +- quinn-proto/src/connection/stats.rs | 4 +-- quinn-proto/src/frame.rs | 42 ++++++++++++++--------------- 3 files changed, 24 insertions(+), 24 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index be4fc6b66..4f905885b 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4389,7 +4389,7 @@ impl Connection { Frame::AddAddress(_addr) => { // TODO(@divma): handle } - Frame::PunchMeNow(_frame) => { + Frame::ReachOut(_frame) => { // TODO(@divma): handle } Frame::RemoveAddress(_frame) => { diff --git a/quinn-proto/src/connection/stats.rs b/quinn-proto/src/connection/stats.rs index 895dd500e..1462f72a9 100644 --- a/quinn-proto/src/connection/stats.rs +++ b/quinn-proto/src/connection/stats.rs @@ -71,7 +71,7 @@ pub struct FrameStats { pub paths_blocked: u64, pub path_cids_blocked: u64, pub add_address: u64, - pub punch_me_now: u64, + pub reach_out: u64, pub remove_address: u64, } @@ -131,7 +131,7 @@ impl FrameStats { self.path_cids_blocked = self.path_cids_blocked.saturating_add(1) } Frame::AddAddress(_) => self.add_address = self.add_address.saturating_add(1), - Frame::PunchMeNow(_) => self.punch_me_now = self.punch_me_now.saturating_add(1), + Frame::ReachOut(_) => self.reach_out = self.reach_out.saturating_add(1), Frame::RemoveAddress(_) => self.remove_address = self.remove_address.saturating_add(1), } } diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 54bfc90a1..94afcdfb0 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -153,8 +153,8 @@ frame_types! { // NAT TRAVERSAL ADD_IPV4_ADDRESS = 0x3d7e90, ADD_IPV6_ADDRESS = 0x3d7e91, - PUNCH_IPV4_ADDR = 0x3d7e92, - PUNCH_IPV6_ADDR = 0x3d7e93, + REACH_OUT_AT_IPV4 = 0x3d7e92, + REACH_OUT_AT_IPV6 = 0x3d7e93, REMOVE_ADDRESS = 0x3d7e94, } @@ -195,7 +195,7 @@ pub(crate) enum Frame { PathsBlocked(PathsBlocked), PathCidsBlocked(PathCidsBlocked), AddAddress(AddAddress), - PunchMeNow(PunchMeNow), + ReachOut(ReachOut), RemoveAddress(RemoveAddress), } @@ -247,7 +247,7 @@ impl Frame { PathsBlocked(_) => FrameType::PATHS_BLOCKED, PathCidsBlocked(_) => FrameType::PATH_CIDS_BLOCKED, AddAddress(ref frame) => frame.get_type(), - PunchMeNow(ref frame) => frame.get_type(), + ReachOut(ref frame) => frame.get_type(), RemoveAddress(_) => self::RemoveAddress::TYPE, } } @@ -985,10 +985,10 @@ impl Iter { let add_address = AddAddress::read(&mut self.bytes, is_ipv6)?; Frame::AddAddress(add_address) } - FrameType::PUNCH_IPV4_ADDR | FrameType::PUNCH_IPV6_ADDR => { - let is_ipv6 = ty == FrameType::PUNCH_IPV6_ADDR; - let punch_me = PunchMeNow::read(&mut self.bytes, is_ipv6)?; - Frame::PunchMeNow(punch_me) + FrameType::REACH_OUT_AT_IPV4 | FrameType::REACH_OUT_AT_IPV6 => { + let is_ipv6 = ty == FrameType::REACH_OUT_AT_IPV6; + let reach_out = ReachOut::read(&mut self.bytes, is_ipv6)?; + Frame::ReachOut(reach_out) } FrameType::REMOVE_ADDRESS => { Frame::RemoveAddress(RemoveAddress::read(&mut self.bytes)?) @@ -1564,12 +1564,12 @@ impl AddAddress { } } -/// Conjuction of the information contained in the punch me now frames -/// ([`FrameType::PUNCH_IPV4_ADDR`], [`FrameType::PUNCH_IPV6_ADDR`]) +/// Conjuction of the information contained in the reach out frames +/// ([`FrameType::REACH_OUT_AT_IPV4`], [`FrameType::REACH_OUT_AT_IPV6`]) #[derive(Debug, PartialEq, Eq, Clone)] // TODO(@divma): remove. Beg the draft people for a better name #[allow(dead_code)] -pub(crate) struct PunchMeNow { +pub(crate) struct ReachOut { /// The sequence number of the NAT Traversal attempts // TODO(@divma): type assumed, spec is un-spec-ific pub(crate) round: VarInt, @@ -1583,7 +1583,7 @@ pub(crate) struct PunchMeNow { // TODO(@divma): remove #[allow(dead_code)] -impl PunchMeNow { +impl ReachOut { /// Smallest number of bytes this type of frame is guaranteed to fit within pub(crate) const SIZE_BOUND: usize = Self { round: VarInt::MAX, @@ -1609,9 +1609,9 @@ impl PunchMeNow { /// Get the [`FrameType`] for this frame pub(crate) const fn get_type(&self) -> FrameType { if self.ip.is_ipv6() { - FrameType::PUNCH_IPV6_ADDR + FrameType::REACH_OUT_AT_IPV6 } else { - FrameType::PUNCH_IPV4_ADDR + FrameType::REACH_OUT_AT_IPV4 } } @@ -1644,7 +1644,7 @@ impl PunchMeNow { /// Read the frame contents from the buffer /// /// Should only be called when the frame type has been identified as - /// [`FrameType::PUNCH_IPV4_ADDR`] or [`FrameType::PUNCH_IPV6_ADDR`]. + /// [`FrameType::REACH_OUT_AT_IPV4`] or [`FrameType::REACH_OUT_AT_IPV6`]. pub(crate) fn read(bytes: &mut R, is_ipv6: bool) -> coding::Result { let round = bytes.get()?; let paired_with = bytes.get()?; @@ -1976,18 +1976,18 @@ mod test { /// Test that encoding and decoding [`AddAddress`] produces the same result #[test] - fn test_punch_me_now_roundrip() { - let punch_me = PunchMeNow { + fn test_reach_out_roundrip() { + let reach_out = ReachOut { round: VarInt(42), paired_with: VarInt(24), ip: std::net::Ipv6Addr::LOCALHOST.into(), port: 4242, }; - let mut buf = Vec::with_capacity(punch_me.size()); - punch_me.write(&mut buf); + let mut buf = Vec::with_capacity(reach_out.size()); + reach_out.write(&mut buf); assert_eq!( - punch_me.size(), + reach_out.size(), buf.len(), "expected written bytes and actual size differ" ); @@ -1995,7 +1995,7 @@ mod test { let mut decoded = frames(buf); assert_eq!(decoded.len(), 1); match decoded.pop().expect("non empty") { - Frame::PunchMeNow(decoded) => assert_eq!(decoded, punch_me), + Frame::ReachOut(decoded) => assert_eq!(decoded, reach_out), x => panic!("incorrect frame {x:?}"), } } From 7785350cb23981cd708530e44f3c4e532a031bc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Tue, 4 Nov 2025 14:33:46 -0500 Subject: [PATCH 02/40] use new codes for transport parameter and frames --- quinn-proto/src/frame.rs | 12 ++++++------ quinn-proto/src/transport_parameters.rs | 11 ++++++----- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 94afcdfb0..8173410be 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -150,12 +150,12 @@ frame_types! { MAX_PATH_ID = 0x15228c0c, PATHS_BLOCKED = 0x15228c0d, PATH_CIDS_BLOCKED = 0x15228c0e, - // NAT TRAVERSAL - ADD_IPV4_ADDRESS = 0x3d7e90, - ADD_IPV6_ADDRESS = 0x3d7e91, - REACH_OUT_AT_IPV4 = 0x3d7e92, - REACH_OUT_AT_IPV6 = 0x3d7e93, - REMOVE_ADDRESS = 0x3d7e94, + // IROH'S NAT TRAVERSAL + ADD_IPV4_ADDRESS = 0x3d7f90, + ADD_IPV6_ADDRESS = 0x3d7f91, + REACH_OUT_AT_IPV4 = 0x3d7f92, + REACH_OUT_AT_IPV6 = 0x3d7f93, + REMOVE_ADDRESS = 0x3d7f94, } const STREAM_TYS: RangeInclusive = RangeInclusive::new(0x08, 0x0f); diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index fdedf20e2..41cb7091c 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -414,7 +414,7 @@ impl TransportParameters { w.write(val); } } - TransportParameterId::NatTraversal => { + TransportParameterId::IrohNatTraversal => { if let Some(val) = self.nat_traversal { w.write_var(id as u64); w.write_var(val.size() as u64); @@ -546,7 +546,7 @@ impl TransportParameters { params.initial_max_path_id = Some(value); } - TransportParameterId::NatTraversal => { + TransportParameterId::IrohNatTraversal => { if params.nat_traversal.is_some() { return Err(Error::Malformed); } @@ -731,8 +731,9 @@ pub(crate) enum TransportParameterId { // https://datatracker.ietf.org/doc/html/draft-ietf-quic-multipath InitialMaxPathId = 0x0f739bbc1b666d0c, - // https://www.ietf.org/archive/id/draft-seemann-quic-nat-traversal-02.html - NatTraversal = 0x3d7e9f0bca12fea6, + // inspired by https://www.ietf.org/archive/id/draft-seemann-quic-nat-traversal-02.html, + // simplified to iroh's needs + IrohNatTraversal = 0x3d7f91120401, } impl TransportParameterId { @@ -761,7 +762,7 @@ impl TransportParameterId { Self::MinAckDelayDraft07, Self::ObservedAddr, Self::InitialMaxPathId, - Self::NatTraversal, + Self::IrohNatTraversal, ]; } From d20cc0b57b2425aaafabfdbb434aead06a01ab9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Tue, 4 Nov 2025 14:51:14 -0500 Subject: [PATCH 03/40] update frame fields --- quinn-proto/src/connection/paths.rs | 2 +- quinn-proto/src/frame.rs | 26 +++++++++++++------------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index 0047e2ee9..467e09bd7 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -41,7 +41,7 @@ impl PathId { pub const ZERO: Self = Self(0); /// The number of bytes this [`PathId`] uses when encoded as a [`VarInt`] - pub(crate) fn size(&self) -> usize { + pub(crate) const fn size(&self) -> usize { VarInt(self.0 as u64).size() } diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 8173410be..f085524b3 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -1567,14 +1567,14 @@ impl AddAddress { /// Conjuction of the information contained in the reach out frames /// ([`FrameType::REACH_OUT_AT_IPV4`], [`FrameType::REACH_OUT_AT_IPV6`]) #[derive(Debug, PartialEq, Eq, Clone)] -// TODO(@divma): remove. Beg the draft people for a better name +// TODO(@divma): remove #[allow(dead_code)] pub(crate) struct ReachOut { /// The sequence number of the NAT Traversal attempts - // TODO(@divma): type assumed, spec is un-spec-ific pub(crate) round: VarInt, - /// The sequence number of the address that was paired with this address - pub(crate) paired_with: VarInt, + /// The [`PathId`] that will be used to send challenges. This same id should be used by the + /// server. + pub(crate) path_id: PathId, /// Address to use pub(crate) ip: IpAddr, /// Port to use with this address @@ -1587,7 +1587,7 @@ impl ReachOut { /// Smallest number of bytes this type of frame is guaranteed to fit within pub(crate) const SIZE_BOUND: usize = Self { round: VarInt::MAX, - paired_with: VarInt::MAX, + path_id: PathId::MAX, ip: IpAddr::V6(std::net::Ipv6Addr::LOCALHOST), port: u16::MAX, } @@ -1595,12 +1595,12 @@ impl ReachOut { pub(crate) const fn new( round: VarInt, - paired_with: VarInt, + path_id: PathId, local_addr: std::net::SocketAddr, ) -> Self { Self { round, - paired_with, + path_id, ip: local_addr.ip(), port: local_addr.port(), } @@ -1619,17 +1619,17 @@ impl ReachOut { pub(crate) const fn size(&self) -> usize { let type_size = VarInt(self.get_type().0).size(); let round_bytes = self.round.size(); - let paired_with_bytes = self.paired_with.size(); + let path_id_bytes = self.path_id.size(); let ip_bytes = if self.ip.is_ipv6() { 16 } else { 4 }; let port_bytes = 2; - type_size + round_bytes + paired_with_bytes + ip_bytes + port_bytes + type_size + round_bytes + path_id_bytes + ip_bytes + port_bytes } /// Unconditionally write this frame to `buf` pub(crate) fn write(&self, buf: &mut W) { buf.write(self.get_type()); buf.write(self.round); - buf.write(self.paired_with); + buf.write(self.path_id); match self.ip { IpAddr::V4(ipv4_addr) => { buf.write(ipv4_addr); @@ -1647,7 +1647,7 @@ impl ReachOut { /// [`FrameType::REACH_OUT_AT_IPV4`] or [`FrameType::REACH_OUT_AT_IPV6`]. pub(crate) fn read(bytes: &mut R, is_ipv6: bool) -> coding::Result { let round = bytes.get()?; - let paired_with = bytes.get()?; + let path_id = bytes.get()?; let ip = if is_ipv6 { IpAddr::V6(bytes.get()?) } else { @@ -1656,7 +1656,7 @@ impl ReachOut { let port = bytes.get()?; Ok(Self { round, - paired_with, + path_id, ip, port, }) @@ -1979,7 +1979,7 @@ mod test { fn test_reach_out_roundrip() { let reach_out = ReachOut { round: VarInt(42), - paired_with: VarInt(24), + path_id: PathId(24), ip: std::net::Ipv6Addr::LOCALHOST.into(), port: 4242, }; From 4422853c3c77b54d074975dbb15fc2308cf0860e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 00:05:46 -0500 Subject: [PATCH 04/40] add the beginnings of nat traversal state --- quinn-proto/src/connection/iroh_hp.rs | 101 ++++++++++++++++++++++++++ quinn-proto/src/connection/mod.rs | 7 ++ quinn-proto/src/frame.rs | 6 +- 3 files changed, 113 insertions(+), 1 deletion(-) create mode 100644 quinn-proto/src/connection/iroh_hp.rs diff --git a/quinn-proto/src/connection/iroh_hp.rs b/quinn-proto/src/connection/iroh_hp.rs new file mode 100644 index 000000000..0563725fe --- /dev/null +++ b/quinn-proto/src/connection/iroh_hp.rs @@ -0,0 +1,101 @@ +use std::{ + collections::hash_map::Entry, + net::{IpAddr, SocketAddr}, +}; + +use rustc_hash::FxHashMap; + +use crate::{VarInt, frame::RemoveAddress}; + +use super::frame::AddAddress; + +/// Maximum number of addresses to handle, applied both to local and remote addresses. +// TODO(@divma): consider making this a config option +const MAX_ADDRESSES: usize = 20; + +/// Errors that the nat traversal state might encounter. +pub(crate) enum Error { + // An endpoint (local or remote) tried to add too many addresses to their advertised set + TooManyAddresses, +} + +/// State kept for Iroh's nat traversal +#[derive(Debug, Default)] +pub(crate) struct State { + /// Candidate addresses the remote server reports as potentially reachable, to use for nat + /// traversal attempts. + remote_addresses: FxHashMap, + + /// Candidate addresses the local client reports as potentially reachable, to use for nat + /// traversal attempts. + local_addresses: FxHashMap<(IpAddr, u16), VarInt>, + // The next id to use for local addresses sent to the client + next_local_addr_id: VarInt, +} + +impl State { + /// Add a local address to use for nat traversal + /// + /// When this endpoint is the server within the connection, these addresses will be sent to the + /// client in add address frames. For clients, these addresses will be sent in reach out frames + pub(crate) fn add_local_address(&mut self, address: SocketAddr) -> Result { + let address = (address.ip(), address.port()); + let allow_new = self.local_addresses.len() < MAX_ADDRESSES; + match self.local_addresses.entry(address) { + Entry::Occupied(occupied_entry) => Ok(*occupied_entry.get()), + Entry::Vacant(vacant_entry) if allow_new => { + let id = self.next_local_addr_id; + self.next_local_addr_id = self.next_local_addr_id.saturating_add(1u8); + vacant_entry.insert(id); + Ok(id) + } + _ => Err(Error::TooManyAddresses), + } + } + + /// Removes a local address from the advertised set for nat traversal + /// + /// When this endpoint is the server, removed addresses must be reported with remove address + /// frames. Clients will simply stop reporting these addresses in reach out frames. + pub(crate) fn remove_local_address(&mut self, address: SocketAddr) -> Option { + self.local_addresses.remove(&(address.ip(), address.port())) + } + + /// Adds an address to the remote set + /// + /// On success returns whether the address was new to the set. It will error when the set has + /// no capacity for the address. + pub(crate) fn add_remote_address(&mut self, add_addr: AddAddress) -> Result { + let AddAddress { seq_no, ip, port } = add_addr; + let address = (ip, port); + let allow_new = self.remote_addresses.len() < MAX_ADDRESSES; + match self.remote_addresses.entry(seq_no) { + Entry::Occupied(mut occupied_entry) => { + let old_value = occupied_entry.insert(address); + // The value might be different. This should not happen, but we assume that the new + // address is more recent than the previous, and thus worth updating + Ok(address != old_value) + } + Entry::Vacant(vacant_entry) if allow_new => { + vacant_entry.insert(address); + Ok(true) + } + _ => Err(Error::TooManyAddresses), + } + } + + /// Removes an address from the remote set + /// + /// Returns whether the address was present. + pub(crate) fn remove_remote_address(&mut self, remove_addr: RemoveAddress) -> bool { + self.remote_addresses.remove(&remove_addr.seq_no).is_some() + } + + /// Checks that a received remote address is valid + /// + /// An address is valid as long as it does not change the value of a known address id + pub(crate) fn check_remote_address(&self, add_addr: AddAddress) -> bool { + let existing = self.remote_addresses.get(&add_addr.seq_no); + existing.is_none() || existing == Some(&add_addr.ip_port()) + } +} diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 4f905885b..d1f8c102f 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -66,6 +66,8 @@ mod paths; pub use paths::{ClosedPath, PathEvent, PathId, PathStatus, RttEstimator}; use paths::{PathData, PathState}; +mod iroh_hp; + pub(crate) mod qlog; mod send_buffer; @@ -299,6 +301,8 @@ pub struct Connection { // TODO(flub): Make this a more efficient data structure. Like ranges of abandoned // paths. Or a set together with a minimum. Or something. abandoned_paths: FxHashSet, + + iroh_hp: Option, } impl Connection { @@ -437,6 +441,9 @@ impl Connection { remote_max_path_id: PathId::ZERO, max_path_id_with_cids: PathId::ZERO, abandoned_paths: Default::default(), + + // iroh's nat traversal + iroh_hp: None, }; if path_validated { this.on_path_validated(PathId::ZERO); diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index f085524b3..5aa4ef2dc 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -1560,7 +1560,11 @@ impl AddAddress { /// Give the [`SocketAddr`] encoded in the frame pub(crate) fn socket_addr(&self) -> SocketAddr { - (self.ip, self.port).into() + self.ip_port().into() + } + + pub(crate) fn ip_port(&self) -> (IpAddr, u16) { + (self.ip, self.port) } } From 6a484f90fb0a22762bbf3228e81645da989a1429 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 00:27:30 -0500 Subject: [PATCH 05/40] init state when the extension is negotiated --- quinn-proto/src/connection/iroh_hp.rs | 15 +++++++++++++-- quinn-proto/src/connection/mod.rs | 14 ++++++++++++++ 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/connection/iroh_hp.rs b/quinn-proto/src/connection/iroh_hp.rs index 0563725fe..1344d2339 100644 --- a/quinn-proto/src/connection/iroh_hp.rs +++ b/quinn-proto/src/connection/iroh_hp.rs @@ -20,7 +20,7 @@ pub(crate) enum Error { } /// State kept for Iroh's nat traversal -#[derive(Debug, Default)] +#[derive(Debug)] pub(crate) struct State { /// Candidate addresses the remote server reports as potentially reachable, to use for nat /// traversal attempts. @@ -29,8 +29,10 @@ pub(crate) struct State { /// Candidate addresses the local client reports as potentially reachable, to use for nat /// traversal attempts. local_addresses: FxHashMap<(IpAddr, u16), VarInt>, - // The next id to use for local addresses sent to the client + /// The next id to use for local addresses sent to the client next_local_addr_id: VarInt, + /// Max concurrent address validations to perform + max_concurrent_path_validations: u64, } impl State { @@ -98,4 +100,13 @@ impl State { let existing = self.remote_addresses.get(&add_addr.seq_no); existing.is_none() || existing == Some(&add_addr.ip_port()) } + + pub(crate) fn new(VarInt(max_concurrent_path_validations): VarInt) -> Self { + Self { + remote_addresses: Default::default(), + local_addresses: Default::default(), + next_local_addr_id: Default::default(), + max_concurrent_path_validations, + } + } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index d1f8c102f..eb38b5952 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5252,6 +5252,20 @@ impl Connection { debug!(initial_max_path_id=%local_max_path_id.min(remote_max_path_id), "multipath negotiated"); } + if let (Some(local_max_hp_validations), Some(remote_max_hp_validations)) = ( + self.config.get_nat_traversal_concurrency_limit(), + params.nat_traversal, + ) { + let max_concurrent_path_validations = + local_max_hp_validations.min(remote_max_hp_validations); + self.iroh_hp = Some(iroh_hp::State::new(max_concurrent_path_validations)); + + debug!( + %max_concurrent_path_validations, + "iroh hole punching negotiated" + ); + } + self.peer_params = params; let peer_max_udp_payload_size = u16::try_from(self.peer_params.max_udp_payload_size.into_inner()).unwrap_or(u16::MAX); From 0bf83287581252ca17f0acbf0f6854c6ee6b7fa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 00:56:52 -0500 Subject: [PATCH 06/40] move the client side api away to deal with the error just once --- quinn-proto/src/connection/iroh_hp.rs | 62 +++++++++++++++++++-------- quinn-proto/src/connection/mod.rs | 10 ++++- 2 files changed, 51 insertions(+), 21 deletions(-) diff --git a/quinn-proto/src/connection/iroh_hp.rs b/quinn-proto/src/connection/iroh_hp.rs index 1344d2339..6c2bac789 100644 --- a/quinn-proto/src/connection/iroh_hp.rs +++ b/quinn-proto/src/connection/iroh_hp.rs @@ -5,9 +5,10 @@ use std::{ use rustc_hash::FxHashMap; -use crate::{VarInt, frame::RemoveAddress}; - -use super::frame::AddAddress; +use crate::{ + Side, VarInt, + frame::{AddAddress, RemoveAddress}, +}; /// Maximum number of addresses to handle, applied both to local and remote addresses. // TODO(@divma): consider making this a config option @@ -15,8 +16,11 @@ const MAX_ADDRESSES: usize = 20; /// Errors that the nat traversal state might encounter. pub(crate) enum Error { - // An endpoint (local or remote) tried to add too many addresses to their advertised set + /// An endpoint (local or remote) tried to add too many addresses to their advertised set TooManyAddresses, + /// The operation is now allowed for this endpoint's connection side + // TODO(@divma): ignoring this part for now + WrongConnectionSide, } /// State kept for Iroh's nat traversal @@ -25,7 +29,6 @@ pub(crate) struct State { /// Candidate addresses the remote server reports as potentially reachable, to use for nat /// traversal attempts. remote_addresses: FxHashMap, - /// Candidate addresses the local client reports as potentially reachable, to use for nat /// traversal attempts. local_addresses: FxHashMap<(IpAddr, u16), VarInt>, @@ -33,6 +36,13 @@ pub(crate) struct State { next_local_addr_id: VarInt, /// Max concurrent address validations to perform max_concurrent_path_validations: u64, + /// Local connection side + side: Side, +} + +/// Nat traversal api exclusive to clients +pub(crate) struct ClientSide<'a> { + state: &'a mut State, } impl State { @@ -63,6 +73,26 @@ impl State { self.local_addresses.remove(&(address.ip(), address.port())) } + pub(crate) fn client_side(&mut self) -> Result, Error> { + if self.side.is_client() { + Ok(ClientSide { state: self }) + } else { + Err(Error::WrongConnectionSide) + } + } + + pub(crate) fn new(VarInt(max_concurrent_path_validations): VarInt, side: Side) -> Self { + Self { + remote_addresses: Default::default(), + local_addresses: Default::default(), + next_local_addr_id: Default::default(), + max_concurrent_path_validations, + side, + } + } +} + +impl<'a> ClientSide<'a> { /// Adds an address to the remote set /// /// On success returns whether the address was new to the set. It will error when the set has @@ -70,8 +100,8 @@ impl State { pub(crate) fn add_remote_address(&mut self, add_addr: AddAddress) -> Result { let AddAddress { seq_no, ip, port } = add_addr; let address = (ip, port); - let allow_new = self.remote_addresses.len() < MAX_ADDRESSES; - match self.remote_addresses.entry(seq_no) { + let allow_new = self.state.remote_addresses.len() < MAX_ADDRESSES; + match self.state.remote_addresses.entry(seq_no) { Entry::Occupied(mut occupied_entry) => { let old_value = occupied_entry.insert(address); // The value might be different. This should not happen, but we assume that the new @@ -90,23 +120,17 @@ impl State { /// /// Returns whether the address was present. pub(crate) fn remove_remote_address(&mut self, remove_addr: RemoveAddress) -> bool { - self.remote_addresses.remove(&remove_addr.seq_no).is_some() + self.state + .remote_addresses + .remove(&remove_addr.seq_no) + .is_some() } /// Checks that a received remote address is valid /// - /// An address is valid as long as it does not change the value of a known address id + /// An address is valid as long as it does not change the value of a known address id. pub(crate) fn check_remote_address(&self, add_addr: AddAddress) -> bool { - let existing = self.remote_addresses.get(&add_addr.seq_no); + let existing = self.state.remote_addresses.get(&add_addr.seq_no); existing.is_none() || existing == Some(&add_addr.ip_port()) } - - pub(crate) fn new(VarInt(max_concurrent_path_validations): VarInt) -> Self { - Self { - remote_addresses: Default::default(), - local_addresses: Default::default(), - next_local_addr_id: Default::default(), - max_concurrent_path_validations, - } - } } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index eb38b5952..b012933e7 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4393,7 +4393,10 @@ impl Connection { )); } } - Frame::AddAddress(_addr) => { + Frame::AddAddress(addr) => { + if let Some(hp_state) = self.iroh_hp.as_mut() { + // hp_state. + } // TODO(@divma): handle } Frame::ReachOut(_frame) => { @@ -5258,7 +5261,10 @@ impl Connection { ) { let max_concurrent_path_validations = local_max_hp_validations.min(remote_max_hp_validations); - self.iroh_hp = Some(iroh_hp::State::new(max_concurrent_path_validations)); + self.iroh_hp = Some(iroh_hp::State::new( + max_concurrent_path_validations, + self.side(), + )); debug!( %max_concurrent_path_validations, From e2b4f1c379a7ef336d1c923de79460a1c9dcbebe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 01:43:22 -0500 Subject: [PATCH 07/40] handle removed and added frames --- quinn-proto/src/connection/iroh_hp.rs | 31 ++++++++++----- quinn-proto/src/connection/mod.rs | 56 ++++++++++++++++++++++++--- 2 files changed, 72 insertions(+), 15 deletions(-) diff --git a/quinn-proto/src/connection/iroh_hp.rs b/quinn-proto/src/connection/iroh_hp.rs index 6c2bac789..8932ce55d 100644 --- a/quinn-proto/src/connection/iroh_hp.rs +++ b/quinn-proto/src/connection/iroh_hp.rs @@ -15,14 +15,21 @@ use crate::{ const MAX_ADDRESSES: usize = 20; /// Errors that the nat traversal state might encounter. +#[derive(Debug)] pub(crate) enum Error { /// An endpoint (local or remote) tried to add too many addresses to their advertised set TooManyAddresses, /// The operation is now allowed for this endpoint's connection side - // TODO(@divma): ignoring this part for now WrongConnectionSide, } +// TODO(@divma): unclear to me what these events are useful for\ +#[derive(Debug)] +pub(crate) enum Event { + AddressAdded(SocketAddr), + AddressRemoved(SocketAddr), +} + /// State kept for Iroh's nat traversal #[derive(Debug)] pub(crate) struct State { @@ -95,9 +102,12 @@ impl State { impl<'a> ClientSide<'a> { /// Adds an address to the remote set /// - /// On success returns whether the address was new to the set. It will error when the set has - /// no capacity for the address. - pub(crate) fn add_remote_address(&mut self, add_addr: AddAddress) -> Result { + /// On success returns the address if it was new to the set. It will error when the set has no + /// capacity for the address. + pub(crate) fn add_remote_address( + &mut self, + add_addr: AddAddress, + ) -> Result, Error> { let AddAddress { seq_no, ip, port } = add_addr; let address = (ip, port); let allow_new = self.state.remote_addresses.len() < MAX_ADDRESSES; @@ -106,11 +116,11 @@ impl<'a> ClientSide<'a> { let old_value = occupied_entry.insert(address); // The value might be different. This should not happen, but we assume that the new // address is more recent than the previous, and thus worth updating - Ok(address != old_value) + Ok((address != old_value).then_some(address.into())) } Entry::Vacant(vacant_entry) if allow_new => { vacant_entry.insert(address); - Ok(true) + Ok(Some(address.into())) } _ => Err(Error::TooManyAddresses), } @@ -119,17 +129,20 @@ impl<'a> ClientSide<'a> { /// Removes an address from the remote set /// /// Returns whether the address was present. - pub(crate) fn remove_remote_address(&mut self, remove_addr: RemoveAddress) -> bool { + pub(crate) fn remove_remote_address( + &mut self, + remove_addr: RemoveAddress, + ) -> Option { self.state .remote_addresses .remove(&remove_addr.seq_no) - .is_some() + .map(Into::into) } /// Checks that a received remote address is valid /// /// An address is valid as long as it does not change the value of a known address id. - pub(crate) fn check_remote_address(&self, add_addr: AddAddress) -> bool { + pub(crate) fn check_remote_address(&self, add_addr: &AddAddress) -> bool { let existing = self.state.remote_addresses.get(&add_addr.seq_no); existing.is_none() || existing == Some(&add_addr.ip_port()) } diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index b012933e7..c37bd3f42 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4394,17 +4394,59 @@ impl Connection { } } Frame::AddAddress(addr) => { - if let Some(hp_state) = self.iroh_hp.as_mut() { - // hp_state. + let Some(hp_state) = self.iroh_hp.as_mut() else { + return Err(TransportError::PROTOCOL_VIOLATION( + "received ADD_ADDRESS frame when iroh's nat traversal was not negotiated", + )); + }; + + let Ok(mut client_state) = hp_state.client_side() else { + return Err(TransportError::PROTOCOL_VIOLATION( + "client sent ADD_ADDRESS frame", + )); + }; + + if !client_state.check_remote_address(&addr) { + // if the address is not valid we flag it, but update anyway + warn!(?addr, "server sent ilegal ADD_ADDRESS frame"); + } + + match client_state.add_remote_address(addr.clone()) { + Ok(maybe_added) => { + if let Some(added) = maybe_added { + self.events.push_back(Event::NatTraversal( + iroh_hp::Event::AddressAdded(added), + )); + } + } + Err(e) => { + warn!(?e, "failed to add remote address") + } + } + } + Frame::RemoveAddress(addr) => { + let Some(hp_state) = self.iroh_hp.as_mut() else { + return Err(TransportError::PROTOCOL_VIOLATION( + "received REMOVE_ADDRESS frame when iroh's nat traversal was not negotiated", + )); + }; + + let Ok(mut client_state) = hp_state.client_side() else { + return Err(TransportError::PROTOCOL_VIOLATION( + "client sent REMOVE_ADDRESS frame", + )); + }; + + if let Some(removed_addr) = client_state.remove_remote_address(addr.clone()) { + self.events + .push_back(Event::NatTraversal(iroh_hp::Event::AddressRemoved( + removed_addr, + ))); } - // TODO(@divma): handle } Frame::ReachOut(_frame) => { // TODO(@divma): handle } - Frame::RemoveAddress(_frame) => { - // TODO(@divma): handle - } } } @@ -5955,6 +5997,8 @@ pub enum Event { DatagramsUnblocked, /// (Multi)Path events Path(PathEvent), + /// Iroh's nat traversal events + NatTraversal(iroh_hp::Event), } impl From for Event { From b77d06658432d25955e31fadcd4fe307b62103a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 12:53:42 -0500 Subject: [PATCH 08/40] make some types public --- quinn-proto/src/connection/iroh_hp.rs | 13 +++++++++---- quinn/src/connection.rs | 14 ++++++++++++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/connection/iroh_hp.rs b/quinn-proto/src/connection/iroh_hp.rs index 8932ce55d..ec7d3e1c1 100644 --- a/quinn-proto/src/connection/iroh_hp.rs +++ b/quinn-proto/src/connection/iroh_hp.rs @@ -15,17 +15,22 @@ use crate::{ const MAX_ADDRESSES: usize = 20; /// Errors that the nat traversal state might encounter. -#[derive(Debug)] -pub(crate) enum Error { +#[derive(Debug, thiserror::Error)] +pub enum Error { /// An endpoint (local or remote) tried to add too many addresses to their advertised set + #[error("Tried to add too many addresses to their advertised set")] TooManyAddresses, - /// The operation is now allowed for this endpoint's connection side + /// The operation is not allowed for this endpoint's connection side + #[error("Not allowed for this endpoint's connection side")] WrongConnectionSide, + /// The extension was not negotiated + #[error("Iroh's nat traversal was not negotiated")] + ExtensionNotNegotiated, } // TODO(@divma): unclear to me what these events are useful for\ #[derive(Debug)] -pub(crate) enum Event { +pub enum Event { AddressAdded(SocketAddr), AddressRemoved(SocketAddr), } diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 66c83d3b4..085076030 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -818,6 +818,17 @@ impl Connection { let conn = self.0.state.lock("is_multipath_enabled"); conn.inner.is_multipath_negotiated() } + + /// Registers one or more addresses at which this endpoint is reachable + /// + /// When the NAT traversal extension is negotiated, servers send these addresses to clients in + /// `ADD_ADDRESS` frames. This allows clients to obtain server address candidates to initiate + /// NAT traversal attempts. Clients provide their own reachable addresses in `REACH_OUT` frames + /// when [`Self::initiate_nat_traversal`] is called. + pub fn add_nat_traversal_addresses(&self, addresses: &[SocketAddr]) -> Result<(), ()> { + // TODO(@divma): here + Ok(()) + } } pin_project! { @@ -1395,6 +1406,9 @@ impl State { Path(evt @ PathEvent::RemoteStatus { .. }) => { self.path_events.send(evt).ok(); } + NatTraversal(_event) => { + // TODO(@divma): handle event + } } } } From abd30792bd3d1336668448271c5c69c64d32c4c5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 12:55:32 -0500 Subject: [PATCH 09/40] move iroh_hp out of connection --- quinn-proto/src/connection/mod.rs | 3 +-- quinn-proto/src/{connection => }/iroh_hp.rs | 0 quinn-proto/src/lib.rs | 2 ++ 3 files changed, 3 insertions(+), 2 deletions(-) rename quinn-proto/src/{connection => }/iroh_hp.rs (100%) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index c37bd3f42..364457c22 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -27,6 +27,7 @@ use crate::{ congestion::Controller, crypto::{self, KeyPair, Keys, PacketKey}, frame::{self, Close, Datagram, FrameStruct, NewToken, ObservedAddr}, + iroh_hp, packet::{ FixedLengthConnectionIdParser, Header, InitialHeader, InitialPacket, LongType, Packet, PacketNumber, PartialDecode, SpaceId, @@ -66,8 +67,6 @@ mod paths; pub use paths::{ClosedPath, PathEvent, PathId, PathStatus, RttEstimator}; use paths::{PathData, PathState}; -mod iroh_hp; - pub(crate) mod qlog; mod send_buffer; diff --git a/quinn-proto/src/connection/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs similarity index 100% rename from quinn-proto/src/connection/iroh_hp.rs rename to quinn-proto/src/iroh_hp.rs diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index 87d8366bb..86d10c058 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -103,6 +103,8 @@ mod address_discovery; mod token_memory_cache; pub use token_memory_cache::TokenMemoryCache; +pub mod iroh_hp; + #[cfg(feature = "arbitrary")] use arbitrary::Arbitrary; From 12fe8a04788d35d19fdc92e748981097ecf7ea5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 13:31:55 -0500 Subject: [PATCH 10/40] thread adding addresses --- quinn-proto/src/connection/mod.rs | 19 +++++++++++++++++++ quinn/src/connection.rs | 11 +++++++---- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 364457c22..8812de84d 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5682,6 +5682,25 @@ impl Connection { None } } + + /// Add addresses the local endpoint believes are reachable for nat traversal + /// + /// If adding any address fails, an error is returned. Previous addresses might have been + /// added. + // TODO(@divma): this combined api has the issue that an error does not mean nothing was done + pub fn add_nat_traversal_addresses( + &mut self, + addresses: &[SocketAddr], + ) -> Result<(), iroh_hp::Error> { + let hp_state = self + .iroh_hp + .as_mut() + .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; + for address in addresses { + hp_state.add_local_address(*address)?; + } + Ok(()) + } } impl fmt::Debug for Connection { diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 085076030..2d2c47606 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -28,7 +28,7 @@ use crate::{ }; use proto::{ ConnectionError, ConnectionHandle, ConnectionStats, Dir, EndpointEvent, PathError, PathEvent, - PathId, PathStatus, Side, StreamEvent, StreamId, congestion::Controller, + PathId, PathStatus, Side, StreamEvent, StreamId, congestion::Controller, iroh_hp, }; /// In-progress connection attempt future @@ -825,9 +825,12 @@ impl Connection { /// `ADD_ADDRESS` frames. This allows clients to obtain server address candidates to initiate /// NAT traversal attempts. Clients provide their own reachable addresses in `REACH_OUT` frames /// when [`Self::initiate_nat_traversal`] is called. - pub fn add_nat_traversal_addresses(&self, addresses: &[SocketAddr]) -> Result<(), ()> { - // TODO(@divma): here - Ok(()) + pub fn add_nat_traversal_addresses( + &self, + addresses: &[SocketAddr], + ) -> Result<(), iroh_hp::Error> { + let conn = self.0.state.lock("add_nat_traversal_addresses"); + conn.inner.add_nat_traversal_addresses(addresses) } } From fd756af1ce0298964bcdd0ef49b09a05ae4362f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 13:57:59 -0500 Subject: [PATCH 11/40] thread removing addresses --- quinn-proto/src/connection/mod.rs | 19 ++++++++++++++++++- quinn/src/connection.rs | 19 ++++++++++++++++++- 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 8812de84d..262284ba4 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5683,7 +5683,7 @@ impl Connection { } } - /// Add addresses the local endpoint believes are reachable for nat traversal + /// Add addresses the local endpoint considers are reachable for nat traversal /// /// If adding any address fails, an error is returned. Previous addresses might have been /// added. @@ -5701,6 +5701,23 @@ impl Connection { } Ok(()) } + + /// Removes an address the endpoing no longer considers reachable for nat traversal + /// + /// Addresses not present in the set will be silently ignored. + pub fn remove_nat_traversal_addresses( + &mut self, + addresses: &[SocketAddr], + ) -> Result<(), iroh_hp::Error> { + let hp_state = self + .iroh_hp + .as_mut() + .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; + for address in addresses { + hp_state.remove_local_address(*address); + } + Ok(()) + } } impl fmt::Debug for Connection { diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 2d2c47606..406fe6607 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -829,7 +829,24 @@ impl Connection { &self, addresses: &[SocketAddr], ) -> Result<(), iroh_hp::Error> { - let conn = self.0.state.lock("add_nat_traversal_addresses"); + let mut conn = self.0.state.lock("add_nat_traversal_addresses"); + conn.inner.add_nat_traversal_addresses(addresses) + } + + /// Removes one or more addresses from the set of addresses at which this endpoint is reachable + /// + /// When the NAT traversal extension is negotiated, servers send address removals to + /// clients in `REMOVE_ADDRESS` frames. This allows clients to stop using outdated + /// server address candidates that are no longer valid for NAT traversal. + /// + /// For clients, removed addresses will no longer be advertised in `REACH_OUT` frames. + /// + /// Addresses not present in the set will be silently ignored. + pub fn remove_nat_traversal_addresses( + &self, + addresses: &[SocketAddr], + ) -> Result<(), iroh_hp::Error> { + let mut conn = self.0.state.lock("add_nat_traversal_addresses"); conn.inner.add_nat_traversal_addresses(addresses) } } From c71a428852ce39d3a39163da51edd8760d076410 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 6 Nov 2025 14:34:51 -0500 Subject: [PATCH 12/40] include adding and removing frames in retransmission data --- quinn-proto/src/connection/mod.rs | 19 ++++++++++++++++--- quinn-proto/src/connection/spaces.rs | 11 +++++++++++ quinn-proto/src/iroh_hp.rs | 5 +++-- 3 files changed, 30 insertions(+), 5 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 262284ba4..517948613 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5692,12 +5692,17 @@ impl Connection { &mut self, addresses: &[SocketAddr], ) -> Result<(), iroh_hp::Error> { + let is_server = self.side().is_server(); let hp_state = self .iroh_hp .as_mut() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; - for address in addresses { - hp_state.add_local_address(*address)?; + + for &address in addresses { + let added = hp_state.add_local_address(address)?; + if is_server { + self.spaces[SpaceId::Data].pending.add_address.insert(added); + } } Ok(()) } @@ -5709,12 +5714,20 @@ impl Connection { &mut self, addresses: &[SocketAddr], ) -> Result<(), iroh_hp::Error> { + let is_server = self.side().is_server(); let hp_state = self .iroh_hp .as_mut() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; for address in addresses { - hp_state.remove_local_address(*address); + if let Some(removed) = hp_state.remove_local_address(*address) { + if is_server { + self.spaces[SpaceId::Data] + .pending + .remove_address + .insert(removed); + } + } } Ok(()) } diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 96b91118e..06b6e6a15 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -2,6 +2,7 @@ use std::{ cmp, collections::{BTreeMap, BTreeSet, VecDeque}, mem, + net::IpAddr, ops::{Bound, Index, IndexMut}, }; @@ -551,6 +552,12 @@ pub struct Retransmits { pub(super) path_status: BTreeSet, /// If a PATH_CIDS_BLOCKED frame needs to be sent for a path pub(super) path_cids_blocked: Vec, + + // Nat traversal data + /// Addresses to report in `ADD_ADDRESS` frames + pub(super) add_address: BTreeSet, + /// Address IDs to remove in `REMOVE_ADDRESS` frames + pub(super) remove_address: BTreeSet, } impl Retransmits { @@ -574,6 +581,8 @@ impl Retransmits { && self.path_status.is_empty() && !self.max_path_id && !self.paths_blocked + && self.add_address.is_empty() + && self.remove_address.is_empty() } } @@ -600,6 +609,8 @@ impl ::std::ops::BitOrAssign for Retransmits { self.path_abandon.append(&mut rhs.path_abandon); self.max_path_id |= rhs.max_path_id; self.paths_blocked |= rhs.paths_blocked; + self.add_address.extend(rhs.add_address.iter()); + self.remove_address.extend(rhs.remove_address.iter()); } } diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index ec7d3e1c1..c1e3a8365 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -62,7 +62,7 @@ impl State { /// /// When this endpoint is the server within the connection, these addresses will be sent to the /// client in add address frames. For clients, these addresses will be sent in reach out frames - pub(crate) fn add_local_address(&mut self, address: SocketAddr) -> Result { + pub(crate) fn add_local_address(&mut self, address: SocketAddr) -> Result { let address = (address.ip(), address.port()); let allow_new = self.local_addresses.len() < MAX_ADDRESSES; match self.local_addresses.entry(address) { @@ -71,7 +71,8 @@ impl State { let id = self.next_local_addr_id; self.next_local_addr_id = self.next_local_addr_id.saturating_add(1u8); vacant_entry.insert(id); - Ok(id) + // NOTE for ipv6 addresses this cleans up fields not relevant to the protocol + Ok(address.into()) } _ => Err(Error::TooManyAddresses), } From 0cce3f98c2d1c3c8bec10c2fb6d520ee7de2b22f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Sun, 9 Nov 2025 23:23:31 -0500 Subject: [PATCH 13/40] Send ADD_ADDRESS and REMOVE_ADDRESS frames --- quinn-proto/src/connection/mod.rs | 23 +++++++++++++++---- quinn-proto/src/connection/spaces.rs | 16 +++++++++----- quinn-proto/src/frame.rs | 12 ++++------ quinn-proto/src/iroh_hp.rs | 33 ++++++++++++++++++++++------ 4 files changed, 59 insertions(+), 25 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 517948613..08b6e592d 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5172,6 +5172,23 @@ impl Connection { self.stats.frame_tx.stream += sent.stream_frames.len() as u64; } + // TODO(@divma): check if we need to do path exclusive filters + while space_id == SpaceId::Data && frame::AddAddress::SIZE_BOUND <= buf.remaining_mut() { + if let Some(added_address) = space.pending.add_address.pop_last() { + added_address.write(buf); + } else { + break; + } + } + + while space_id == SpaceId::Data && frame::RemoveAddress::SIZE_BOUND <= buf.remaining_mut() { + if let Some(removed_address) = space.pending.remove_address.pop_last() { + removed_address.write(buf); + } else { + break; + } + } + sent } @@ -5692,17 +5709,15 @@ impl Connection { &mut self, addresses: &[SocketAddr], ) -> Result<(), iroh_hp::Error> { - let is_server = self.side().is_server(); let hp_state = self .iroh_hp .as_mut() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; for &address in addresses { - let added = hp_state.add_local_address(address)?; - if is_server { + if let Some(added) = hp_state.add_local_address(address)? { self.spaces[SpaceId::Data].pending.add_address.insert(added); - } + }; } Ok(()) } diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 06b6e6a15..9975e59a6 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -2,7 +2,6 @@ use std::{ cmp, collections::{BTreeMap, BTreeSet, VecDeque}, mem, - net::IpAddr, ops::{Bound, Index, IndexMut}, }; @@ -13,7 +12,11 @@ use tracing::{error, trace}; use super::{PathId, assembler::Assembler}; use crate::{ Dir, Duration, Instant, SocketAddr, StreamId, TransportError, TransportErrorCode, VarInt, - connection::StreamsState, crypto::Keys, frame, packet::SpaceId, range_set::ArrayRangeSet, + connection::StreamsState, + crypto::Keys, + frame::{self, AddAddress, RemoveAddress}, + packet::SpaceId, + range_set::ArrayRangeSet, shared::IssuedCid, }; @@ -555,9 +558,9 @@ pub struct Retransmits { // Nat traversal data /// Addresses to report in `ADD_ADDRESS` frames - pub(super) add_address: BTreeSet, + pub(super) add_address: BTreeSet, /// Address IDs to remove in `REMOVE_ADDRESS` frames - pub(super) remove_address: BTreeSet, + pub(super) remove_address: BTreeSet, } impl Retransmits { @@ -609,8 +612,9 @@ impl ::std::ops::BitOrAssign for Retransmits { self.path_abandon.append(&mut rhs.path_abandon); self.max_path_id |= rhs.max_path_id; self.paths_blocked |= rhs.paths_blocked; - self.add_address.extend(rhs.add_address.iter()); - self.remove_address.extend(rhs.remove_address.iter()); + self.add_address.extend(rhs.add_address.iter().copied()); + self.remove_address + .extend(rhs.remove_address.iter().copied()); } } diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 5aa4ef2dc..7e0796248 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -1478,7 +1478,7 @@ impl PathBackup { /// Conjuction of the information contained in the add address frames /// ([`FrameType::ADD_IPV4_ADDRESS`], [`FrameType::ADD_IPV6_ADDRESS`]). -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Copy, Clone, PartialOrd, Ord)] // TODO(@divma): remove #[allow(dead_code)] pub(crate) struct AddAddress { @@ -1502,12 +1502,8 @@ impl AddAddress { } .size(); - pub(crate) const fn new(remote: std::net::SocketAddr, seq_no: VarInt) -> Self { - Self { - ip: remote.ip(), - port: remote.port(), - seq_no, - } + pub(crate) const fn new((ip, port): (IpAddr, u16), seq_no: VarInt) -> Self { + Self { ip, port, seq_no } } /// Get the [`FrameType`] for this frame. @@ -1673,7 +1669,7 @@ impl ReachOut { } /// Frame signaling an address is no longer being advertised -#[derive(Debug, PartialEq, Eq, Clone)] +#[derive(Debug, PartialEq, Eq, Copy, Clone, PartialOrd, Ord)] // TODO(@divma): remove #[allow(dead_code)] pub(crate) struct RemoveAddress { diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index c1e3a8365..a5bc4b97a 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -58,21 +58,31 @@ pub(crate) struct ClientSide<'a> { } impl State { - /// Add a local address to use for nat traversal + /// Adds a local address to use for nat traversal /// /// When this endpoint is the server within the connection, these addresses will be sent to the /// client in add address frames. For clients, these addresses will be sent in reach out frames - pub(crate) fn add_local_address(&mut self, address: SocketAddr) -> Result { + /// when nat traversal attempts are initiated. + /// + /// If a frame should be sent, it is returned. + pub(crate) fn add_local_address( + &mut self, + address: SocketAddr, + ) -> Result, Error> { let address = (address.ip(), address.port()); let allow_new = self.local_addresses.len() < MAX_ADDRESSES; + let is_server = self.side.is_server(); match self.local_addresses.entry(address) { - Entry::Occupied(occupied_entry) => Ok(*occupied_entry.get()), + Entry::Occupied(_) => Ok(None), Entry::Vacant(vacant_entry) if allow_new => { let id = self.next_local_addr_id; self.next_local_addr_id = self.next_local_addr_id.saturating_add(1u8); vacant_entry.insert(id); - // NOTE for ipv6 addresses this cleans up fields not relevant to the protocol - Ok(address.into()) + if is_server { + Ok(Some(AddAddress::new(address, id))) + } else { + Ok(None) + } } _ => Err(Error::TooManyAddresses), } @@ -82,8 +92,17 @@ impl State { /// /// When this endpoint is the server, removed addresses must be reported with remove address /// frames. Clients will simply stop reporting these addresses in reach out frames. - pub(crate) fn remove_local_address(&mut self, address: SocketAddr) -> Option { - self.local_addresses.remove(&(address.ip(), address.port())) + /// + /// If a frame should be sent, it is returned. + pub(crate) fn remove_local_address(&mut self, address: SocketAddr) -> Option { + let id = self + .local_addresses + .remove(&(address.ip(), address.port()))?; + if self.side.is_server() { + Some(RemoveAddress::new(id)) + } else { + None + } } pub(crate) fn client_side(&mut self) -> Result, Error> { From 9dab34f6295ac2ab5e85c065162c565b55f7030d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Sun, 9 Nov 2025 23:29:54 -0500 Subject: [PATCH 14/40] Add how to query the local nat traversal addresses. --- quinn-proto/src/connection/mod.rs | 9 +++++++++ quinn-proto/src/iroh_hp.rs | 8 ++++++++ quinn/src/connection.rs | 6 ++++++ 3 files changed, 23 insertions(+) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 08b6e592d..93cb1c53f 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5746,6 +5746,15 @@ impl Connection { } Ok(()) } + + /// Get the currently advertised nat traversal addresses + pub fn get_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { + let hp_state = self + .iroh_hp + .as_ref() + .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; + Ok(hp_state.get_nat_traversal_addresses()) + } } impl fmt::Debug for Connection { diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index a5bc4b97a..d3ff611c7 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -122,6 +122,14 @@ impl State { side, } } + + pub(crate) fn get_nat_traversal_addresses(&self) -> Vec { + self.local_addresses + .keys() + .copied() + .map(Into::into) + .collect() + } } impl<'a> ClientSide<'a> { diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 406fe6607..69dcb1705 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -849,6 +849,12 @@ impl Connection { let mut conn = self.0.state.lock("add_nat_traversal_addresses"); conn.inner.add_nat_traversal_addresses(addresses) } + + /// Get the currently advertised nat traversal addresses + pub fn get_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { + let conn = self.0.state.lock("add_nat_traversal_addresses"); + conn.inner.get_nat_traversal_addresses() + } } pin_project! { From 38d73d0fefa6db2c684764877b56688c4e85d06d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Mon, 10 Nov 2025 00:20:42 -0500 Subject: [PATCH 15/40] Thread server updates about addresses --- quinn/src/connection.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 69dcb1705..556bbcc69 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -492,6 +492,15 @@ impl Connection { self.0.state.lock("path_events").path_events.subscribe() } + /// A broadcast receiver of [`iroh_hp::Event`]s for updates about server addresses + pub fn nat_traversal_updates(&self) -> tokio::sync::broadcast::Receiver { + self.0 + .state + .lock("nat_traversal_updates") + .nat_traversal_updates + .subscribe() + } + /// Wait for the connection to be closed for any reason /// /// Despite the return type's name, closed connections are often not an error condition at the @@ -1114,6 +1123,7 @@ impl ConnectionRef { send_buffer: Vec::new(), buffered_transmit: None, observed_external_addr: watch::Sender::new(None), + nat_traversal_updates: tokio::sync::broadcast::channel(32).0, }), shared: Shared::default(), })) @@ -1252,6 +1262,7 @@ pub(crate) struct State { /// Our last external address reported by the peer. When multipath is enabled, this will be the /// last report across all paths. pub(crate) observed_external_addr: watch::Sender>, + pub(crate) nat_traversal_updates: tokio::sync::broadcast::Sender, } impl State { @@ -1432,8 +1443,8 @@ impl State { Path(evt @ PathEvent::RemoteStatus { .. }) => { self.path_events.send(evt).ok(); } - NatTraversal(_event) => { - // TODO(@divma): handle event + NatTraversal(update) => { + self.nat_traversal_updates.send(evt).ok(); } } } From 6dc2ba1c40a7fa473827669a3ff1e28ca1d3cf7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Mon, 10 Nov 2025 01:14:30 -0500 Subject: [PATCH 16/40] Boilerplate threading the nat traversal round call --- quinn-proto/src/connection/mod.rs | 8 ++++++++ quinn-proto/src/iroh_hp.rs | 8 ++++++++ quinn/src/connection.rs | 7 +++++++ 3 files changed, 23 insertions(+) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 93cb1c53f..4f9959a4e 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5755,6 +5755,14 @@ impl Connection { .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; Ok(hp_state.get_nat_traversal_addresses()) } + + pub fn initiate_nat_traversal_round(&self) -> Result<(), iroh_hp::Error> { + let hp_state = self + .iroh_hp + .as_ref() + .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; + hp_state.initiate_nat_traversal_round() + } } impl fmt::Debug for Connection { diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index d3ff611c7..5c884c6f5 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -130,6 +130,14 @@ impl State { .map(Into::into) .collect() } + + pub(crate) fn initiate_nat_traversal_round(&self) -> Result<(), Error> { + if self.side.is_server() { + return Err(Error::WrongConnectionSide); + } + // TODO(@divma): here + Ok(()) + } } impl<'a> ClientSide<'a> { diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 556bbcc69..5b684f986 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -864,6 +864,13 @@ impl Connection { let conn = self.0.state.lock("add_nat_traversal_addresses"); conn.inner.get_nat_traversal_addresses() } + + //// Initiate a nat traversal round + // TODO(@divma): improve docs when things are more clear un my head + pub fn initiate_nat_traversal_round(&self) -> Result<(), iroh_hp::Error> { + let conn = self.0.state.lock("initiate_nat_traversal_round"); + conn.inner.initiate_nat_traversal_round() + } } pin_project! { From 5bc97ea4a77baa11e0cb46d1a60567c1d73cdfb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Tue, 11 Nov 2025 15:22:02 -0500 Subject: [PATCH 17/40] Remove the path_id from the ReachOut frame --- quinn-proto/src/frame.rs | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 7e0796248..2fb93c92c 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -1572,9 +1572,6 @@ impl AddAddress { pub(crate) struct ReachOut { /// The sequence number of the NAT Traversal attempts pub(crate) round: VarInt, - /// The [`PathId`] that will be used to send challenges. This same id should be used by the - /// server. - pub(crate) path_id: PathId, /// Address to use pub(crate) ip: IpAddr, /// Port to use with this address @@ -1587,20 +1584,14 @@ impl ReachOut { /// Smallest number of bytes this type of frame is guaranteed to fit within pub(crate) const SIZE_BOUND: usize = Self { round: VarInt::MAX, - path_id: PathId::MAX, ip: IpAddr::V6(std::net::Ipv6Addr::LOCALHOST), port: u16::MAX, } .size(); - pub(crate) const fn new( - round: VarInt, - path_id: PathId, - local_addr: std::net::SocketAddr, - ) -> Self { + pub(crate) const fn new(round: VarInt, local_addr: std::net::SocketAddr) -> Self { Self { round, - path_id, ip: local_addr.ip(), port: local_addr.port(), } @@ -1619,17 +1610,15 @@ impl ReachOut { pub(crate) const fn size(&self) -> usize { let type_size = VarInt(self.get_type().0).size(); let round_bytes = self.round.size(); - let path_id_bytes = self.path_id.size(); let ip_bytes = if self.ip.is_ipv6() { 16 } else { 4 }; let port_bytes = 2; - type_size + round_bytes + path_id_bytes + ip_bytes + port_bytes + type_size + round_bytes + ip_bytes + port_bytes } /// Unconditionally write this frame to `buf` pub(crate) fn write(&self, buf: &mut W) { buf.write(self.get_type()); buf.write(self.round); - buf.write(self.path_id); match self.ip { IpAddr::V4(ipv4_addr) => { buf.write(ipv4_addr); @@ -1647,19 +1636,13 @@ impl ReachOut { /// [`FrameType::REACH_OUT_AT_IPV4`] or [`FrameType::REACH_OUT_AT_IPV6`]. pub(crate) fn read(bytes: &mut R, is_ipv6: bool) -> coding::Result { let round = bytes.get()?; - let path_id = bytes.get()?; let ip = if is_ipv6 { IpAddr::V6(bytes.get()?) } else { IpAddr::V4(bytes.get()?) }; let port = bytes.get()?; - Ok(Self { - round, - path_id, - ip, - port, - }) + Ok(Self { round, ip, port }) } /// Give the [`SocketAddr`] encoded in the frame @@ -1979,7 +1962,6 @@ mod test { fn test_reach_out_roundrip() { let reach_out = ReachOut { round: VarInt(42), - path_id: PathId(24), ip: std::net::Ipv6Addr::LOCALHOST.into(), port: 4242, }; From eed7d682f08f73096afa4ed2c6f61b518e5ed266 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Wed, 12 Nov 2025 23:46:16 -0500 Subject: [PATCH 18/40] add retransmission fields for reach out frames --- quinn-proto/src/connection/spaces.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 9975e59a6..be93b4116 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -2,6 +2,7 @@ use std::{ cmp, collections::{BTreeMap, BTreeSet, VecDeque}, mem, + net::IpAddr, ops::{Bound, Index, IndexMut}, }; @@ -561,6 +562,8 @@ pub struct Retransmits { pub(super) add_address: BTreeSet, /// Address IDs to remove in `REMOVE_ADDRESS` frames pub(super) remove_address: BTreeSet, + /// Round and local addresses to advertise in `REACH_OUT` frames + pub(super) reach_out: Option<(VarInt, Vec<(IpAddr, u16)>)>, } impl Retransmits { @@ -586,6 +589,7 @@ impl Retransmits { && !self.paths_blocked && self.add_address.is_empty() && self.remove_address.is_empty() + && self.reach_out.is_none() } } @@ -615,6 +619,14 @@ impl ::std::ops::BitOrAssign for Retransmits { self.add_address.extend(rhs.add_address.iter().copied()); self.remove_address .extend(rhs.remove_address.iter().copied()); + if let Some((rhs_round, _)) = rhs.reach_out.as_ref() { + let maybe_lhs_round = self.reach_out.as_ref().map(|(round, _addresses)| *round); + match maybe_lhs_round { + Some(lhs_round) if *rhs_round > lhs_round => self.reach_out = rhs.reach_out.clone(), + None => self.reach_out = rhs.reach_out.clone(), + _ => {} + } + } } } From ed1dfea1cf063e7256c1054ddf78fa6868519d23 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 13 Nov 2025 13:16:22 -0500 Subject: [PATCH 19/40] Send the reach out frames, probe server addresses --- quinn-proto/src/connection/mod.rs | 65 +++++++++++++++++++++++++++++-- quinn-proto/src/iroh_hp.rs | 64 ++++++++++++++++++++++++++++-- quinn/src/connection.rs | 11 +++--- 3 files changed, 128 insertions(+), 12 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 4f9959a4e..5553e8312 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5756,12 +5756,71 @@ impl Connection { Ok(hp_state.get_nat_traversal_addresses()) } - pub fn initiate_nat_traversal_round(&self) -> Result<(), iroh_hp::Error> { + /// Initiates a new nat traversal round + /// + /// A nat traversal round involves advertising the client's local addresses in `REACH_OUT` + /// frames, and initiating probing of the known remote addresses. When a new round is + /// initiated, the previous one is cancelled, and paths that have not been opened are closed. + /// + /// Returns the server addresses that are now being probed. + pub fn initiate_nat_traversal_round( + &mut self, + now: Instant, + ) -> Result, iroh_hp::Error> { let hp_state = self .iroh_hp - .as_ref() + .as_mut() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; - hp_state.initiate_nat_traversal_round() + let iroh_hp::NatTraversalRound { + new_round, + reach_out_at, + addresses_to_probe, + prev_round_path_ids, + } = hp_state.initiate_nat_traversal_round()?; + + self.spaces[SpaceId::Data].pending.reach_out = Some((new_round, reach_out_at)); + + for path_id in prev_round_path_ids { + // TODO(@divma): this sounds reasonable but we need if this actually works for the + // purposes of the protocol + let validated = self + .path(path_id) + .map(|path| path.validated) + .unwrap_or(false); + + if !validated { + let _ = + self.close_path(now, path_id, TransportErrorCode::APPLICATION_ABANDON.into()); + } + } + + let mut err = None; + + let mut path_ids = Vec::with_capacity(addresses_to_probe.len()); + let mut probed_addresses = Vec::with_capacity(addresses_to_probe.len()); + for address in addresses_to_probe { + let remote: SocketAddr = address.into(); + match self.open_path_ensure(remote, PathStatus::Backup, now) { + Ok((path_id, path_was_known)) if !path_was_known => { + path_ids.push(path_id); + probed_addresses.push(remote); + } + Ok((path_id, _)) => { + trace!(%path_id, %remote,"nat traversal: path existed for remote") + } + Err(e) => { + debug!(%remote, %e,"nat traversal: failed to probe remote"); + err.get_or_insert(e); + } + } + } + + let hp_state = self.iroh_hp.as_mut().expect("previously validated"); + hp_state + .set_round_path_ids(path_ids) + .expect("connection side validated"); + + Ok(probed_addresses) } } diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index 5c884c6f5..d5e3a971e 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -6,7 +6,7 @@ use std::{ use rustc_hash::FxHashMap; use crate::{ - Side, VarInt, + PathId, Side, VarInt, frame::{AddAddress, RemoveAddress}, }; @@ -26,10 +26,27 @@ pub enum Error { /// The extension was not negotiated #[error("Iroh's nat traversal was not negotiated")] ExtensionNotNegotiated, + /// Not enough addresses to complete the operation + #[error("Not enough addresses")] + NotEnoughAddresses, + /// Nat traversal attempt failed due to a multipath error + #[error("Failed to establish paths {0}")] + Multipath(super::PathError), +} + +pub(crate) struct NatTraversalRound { + /// Sequence number to use for the new reach out frames + pub(crate) new_round: VarInt, + /// Addresses to use to send reach out frames + pub(crate) reach_out_at: Vec<(IpAddr, u16)>, + /// Remotes to probe by attempting to open new paths + pub(crate) addresses_to_probe: Vec<(IpAddr, u16)>, + /// [`PathId`]s of the cancelled round + pub(crate) prev_round_path_ids: Vec, } // TODO(@divma): unclear to me what these events are useful for\ -#[derive(Debug)] +#[derive(Debug, Clone)] pub enum Event { AddressAdded(SocketAddr), AddressRemoved(SocketAddr), @@ -47,9 +64,18 @@ pub(crate) struct State { /// The next id to use for local addresses sent to the client next_local_addr_id: VarInt, /// Max concurrent address validations to perform + // TODO(@divma): opening paths might not be a good idea after all max_concurrent_path_validations: u64, /// Local connection side side: Side, + /// Current nat holepunching round + /// + /// Clients initiate hole punching rounds and are thus responsible for incrementing the count. + /// Servers keep track of the client's most recent round and cancel probing related to previous + /// rounds. + round: VarInt, + /// [`PathId`]s used to probe remotes assigned to this round + round_path_ids: Vec, } /// Nat traversal api exclusive to clients @@ -120,6 +146,8 @@ impl State { next_local_addr_id: Default::default(), max_concurrent_path_validations, side, + round: Default::default(), + round_path_ids: Default::default(), } } @@ -131,11 +159,39 @@ impl State { .collect() } - pub(crate) fn initiate_nat_traversal_round(&self) -> Result<(), Error> { + /// Initiates a new nat traversal round + /// + /// A nat traversal round involves advertising the client's local addresses in `REACH_OUT` + /// frames, and initiating probing of the known remote addresses. When a new round is + /// initiated, the previous one is cancelled, and paths that have not been opened should be + /// closed. + pub(crate) fn initiate_nat_traversal_round(&mut self) -> Result { if self.side.is_server() { return Err(Error::WrongConnectionSide); } - // TODO(@divma): here + + if self.local_addresses.is_empty() || self.remote_addresses.is_empty() { + return Err(Error::NotEnoughAddresses); + } + + let prev_round_path_ids = std::mem::replace(&mut self.round_path_ids, Default::default()); + self.round = self.round.saturating_add(1u8); + + Ok(NatTraversalRound { + new_round: self.round, + reach_out_at: self.local_addresses.keys().copied().collect(), + addresses_to_probe: self.remote_addresses.values().copied().collect(), + prev_round_path_ids, + }) + } + + /// Add a [`PathId`] as part of the current attempts to create paths based on the server's + /// advertised addresses. + pub(crate) fn set_round_path_ids(&mut self, path_ids: Vec) -> Result<(), Error> { + if self.side.is_server() { + return Err(Error::WrongConnectionSide); + } + self.round_path_ids = path_ids; Ok(()) } } diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 5b684f986..5ca1a4bb1 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -866,10 +866,11 @@ impl Connection { } //// Initiate a nat traversal round - // TODO(@divma): improve docs when things are more clear un my head - pub fn initiate_nat_traversal_round(&self) -> Result<(), iroh_hp::Error> { - let conn = self.0.state.lock("initiate_nat_traversal_round"); - conn.inner.initiate_nat_traversal_round() + /// + pub fn initiate_nat_traversal_round(&self) -> Result, iroh_hp::Error> { + let mut conn = self.0.state.lock("initiate_nat_traversal_round"); + let now = conn.runtime.now(); + conn.inner.initiate_nat_traversal_round(now) } } @@ -1451,7 +1452,7 @@ impl State { self.path_events.send(evt).ok(); } NatTraversal(update) => { - self.nat_traversal_updates.send(evt).ok(); + self.nat_traversal_updates.send(update).ok(); } } } From 86aea3e5cc99fa15e29d01de2c21b2208067653e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 13 Nov 2025 14:22:20 -0500 Subject: [PATCH 20/40] fix docs --- quinn/src/connection.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 5ca1a4bb1..2018a6a55 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -833,7 +833,7 @@ impl Connection { /// When the NAT traversal extension is negotiated, servers send these addresses to clients in /// `ADD_ADDRESS` frames. This allows clients to obtain server address candidates to initiate /// NAT traversal attempts. Clients provide their own reachable addresses in `REACH_OUT` frames - /// when [`Self::initiate_nat_traversal`] is called. + /// when [`Self::initiate_nat_traversal_round`] is called. pub fn add_nat_traversal_addresses( &self, addresses: &[SocketAddr], @@ -865,8 +865,13 @@ impl Connection { conn.inner.get_nat_traversal_addresses() } - //// Initiate a nat traversal round + /// Initiates a new nat traversal round /// + /// A nat traversal round involves advertising the client's local addresses in `REACH_OUT` + /// frames, and initiating probing of the known remote addresses. When a new round is + /// initiated, the previous one is cancelled, and paths that have not been opened are closed. + /// + /// Returns the server addresses that are now being probed. pub fn initiate_nat_traversal_round(&self) -> Result, iroh_hp::Error> { let mut conn = self.0.state.lock("initiate_nat_traversal_round"); let now = conn.runtime.now(); From b73cf6a5d041bd3b248ef37e4b7721c156a792cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 13 Nov 2025 15:21:43 -0500 Subject: [PATCH 21/40] Send the reach out frames --- quinn-proto/src/connection/mod.rs | 16 ++++++++++++++++ quinn-proto/src/frame.rs | 8 ++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 5553e8312..3d0b67e12 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4687,6 +4687,22 @@ impl Connection { self.stats.frame_tx.handshake_done.saturating_add(1); } + // TODO(@divma): path explusive considerations + if let Some((round, addresses)) = space.pending.reach_out.as_mut() { + while let Some(local_addr) = addresses.pop() { + let reach_out = frame::ReachOut::new(*round, local_addr); + if buf.remaining_mut() > reach_out.size() { + reach_out.write(buf); + } else { + addresses.push(local_addr); + break; + } + } + if addresses.is_empty() { + space.pending.reach_out = None; + } + } + // OBSERVED_ADDR if !path_exclusive_only && space_id == SpaceId::Data diff --git a/quinn-proto/src/frame.rs b/quinn-proto/src/frame.rs index 2fb93c92c..54f04cd48 100644 --- a/quinn-proto/src/frame.rs +++ b/quinn-proto/src/frame.rs @@ -1589,12 +1589,8 @@ impl ReachOut { } .size(); - pub(crate) const fn new(round: VarInt, local_addr: std::net::SocketAddr) -> Self { - Self { - round, - ip: local_addr.ip(), - port: local_addr.port(), - } + pub(crate) const fn new(round: VarInt, (ip, port): (IpAddr, u16)) -> Self { + Self { round, ip, port } } /// Get the [`FrameType`] for this frame From 01b4fd3324b35e68578bd3118e20e1cf81f0679f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Fri, 14 Nov 2025 16:14:15 -0500 Subject: [PATCH 22/40] add fields to queue pending server-side challenges --- quinn-proto/src/connection/spaces.rs | 28 ++++++++++++++++++++++------ quinn-proto/src/iroh_hp.rs | 4 ++++ 2 files changed, 26 insertions(+), 6 deletions(-) diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index e021ff957..e0de609d8 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -564,6 +564,8 @@ pub struct Retransmits { pub(super) remove_address: BTreeSet, /// Round and local addresses to advertise in `REACH_OUT` frames pub(super) reach_out: Option<(VarInt, Vec<(IpAddr, u16)>)>, + /// Round and remote addresses to which `PATH_CHALLENGE`s need to be sent + pub(super) challenges: Option<(VarInt, Vec<(IpAddr, u16)>)>, } impl Retransmits { @@ -590,6 +592,7 @@ impl Retransmits { && self.add_address.is_empty() && self.remove_address.is_empty() && self.reach_out.is_none() + && self.challenges.is_none() } } @@ -619,13 +622,26 @@ impl ::std::ops::BitOrAssign for Retransmits { self.add_address.extend(rhs.add_address.iter().copied()); self.remove_address .extend(rhs.remove_address.iter().copied()); - if let Some((rhs_round, _)) = rhs.reach_out.as_ref() { - let maybe_lhs_round = self.reach_out.as_ref().map(|(round, _addresses)| *round); - match maybe_lhs_round { - Some(lhs_round) if *rhs_round > lhs_round => self.reach_out = rhs.reach_out.clone(), - None => self.reach_out = rhs.reach_out.clone(), - _ => {} + // if there are two rounds, prefer the most recent reach out set + let lhs_round = self.reach_out.as_ref().map(|(round, _)| *round); + let rhs_round = rhs.reach_out.as_ref().map(|(round, _)| *round); + match (lhs_round, rhs_round) { + (None, Some(_)) => self.reach_out = rhs.reach_out.clone(), + (Some(lhs_round), Some(rhs_round)) if rhs_round > lhs_round => { + self.reach_out = rhs.reach_out.clone() } + _ => {} + } + + // if there are two rounds, prefer the most recent pending challenges set + let lhs_round = self.challenges.as_ref().map(|(round, _)| *round); + let rhs_round = rhs.challenges.as_ref().map(|(round, _)| *round); + match (lhs_round, rhs_round) { + (None, Some(_)) => self.challenges = rhs.challenges.clone(), + (Some(lhs_round), Some(rhs_round)) if rhs_round > lhs_round => { + self.challenges = rhs.challenges.clone() + } + _ => {} } } } diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index d5e3a971e..365b5fd70 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -76,6 +76,9 @@ pub(crate) struct State { round: VarInt, /// [`PathId`]s used to probe remotes assigned to this round round_path_ids: Vec, + /// Challenges sent by servers to validate client addresses without attempting to open + /// multipath paths + challenges: FxHashMap, } /// Nat traversal api exclusive to clients @@ -148,6 +151,7 @@ impl State { side, round: Default::default(), round_path_ids: Default::default(), + challenges: Default::default(), } } From 49b750d5f2e20e148bed3248bf010d27140b1037 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Mon, 17 Nov 2025 16:24:36 -0500 Subject: [PATCH 23/40] Return the remotely advertised nat traversal addresses instead --- quinn-proto/src/connection/mod.rs | 6 +++--- quinn-proto/src/iroh_hp.rs | 13 +++++++++---- quinn/src/connection.rs | 10 +++++----- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 6f2f1b10e..f1e762572 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5808,13 +5808,13 @@ impl Connection { Ok(()) } - /// Get the currently advertised nat traversal addresses - pub fn get_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { + /// Get the currently advertised nat traversal addresses by the server + pub fn get_remote_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { let hp_state = self .iroh_hp .as_ref() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; - Ok(hp_state.get_nat_traversal_addresses()) + hp_state.get_remote_nat_traversal_addresses() } /// Initiates a new nat traversal round diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index 365b5fd70..2372826f5 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -155,12 +155,17 @@ impl State { } } - pub(crate) fn get_nat_traversal_addresses(&self) -> Vec { - self.local_addresses - .keys() + pub(crate) fn get_remote_nat_traversal_addresses(&self) -> Result, Error> { + if !self.side.is_client() { + return Err(Error::WrongConnectionSide); + } + + Ok(self + .remote_addresses + .values() .copied() .map(Into::into) - .collect() + .collect()) } /// Initiates a new nat traversal round diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index 627bad00d..a99f9af41 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -878,14 +878,14 @@ impl Connection { &self, addresses: &[SocketAddr], ) -> Result<(), iroh_hp::Error> { - let mut conn = self.0.state.lock("add_nat_traversal_addresses"); + let mut conn = self.0.state.lock("remove_nat_traversal_addresses"); conn.inner.add_nat_traversal_addresses(addresses) } - /// Get the currently advertised nat traversal addresses - pub fn get_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { - let conn = self.0.state.lock("add_nat_traversal_addresses"); - conn.inner.get_nat_traversal_addresses() + /// Get the currently advertised nat traversal addresses by the server + pub fn get_remote_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { + let conn = self.0.state.lock("get_remote_nat_traversal_addresses"); + conn.inner.get_remote_nat_traversal_addresses() } /// Initiates a new nat traversal round From 366dec68a059c700c36d956b38f606c8aebb7462 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Mon, 17 Nov 2025 20:28:25 -0500 Subject: [PATCH 24/40] modify the meaning of the transport parameter --- quinn-proto/src/config/transport.rs | 41 ++++++++-------- quinn-proto/src/connection/mod.rs | 62 +++++++++++++++++++------ quinn-proto/src/iroh_hp.rs | 26 +++++++---- quinn-proto/src/transport_parameters.rs | 32 ++++++------- 4 files changed, 97 insertions(+), 64 deletions(-) diff --git a/quinn-proto/src/config/transport.rs b/quinn-proto/src/config/transport.rs index a2313832f..d2de9ac0a 100644 --- a/quinn-proto/src/config/transport.rs +++ b/quinn-proto/src/config/transport.rs @@ -1,4 +1,8 @@ -use std::{fmt, num::NonZeroU32, sync::Arc}; +use std::{ + fmt, + num::{NonZeroU8, NonZeroU32}, + sync::Arc, +}; #[cfg(feature = "qlog")] use std::{io, sync::Mutex, time::Instant}; @@ -15,7 +19,7 @@ use crate::{ /// When multipath is required and has not been explicitly enabled, this value will be used for /// [`TransportConfig::max_concurrent_multipath_paths`]. const DEFAULT_CONCURRENT_MULTIPATH_PATHS_WHEN_ENABLED_: NonZeroU32 = { - match NonZeroU32::new(4) { + match NonZeroU32::new(12) { Some(v) => v, None => panic!("to enable multipath this must be positive, which clearly it is"), } @@ -78,7 +82,7 @@ pub struct TransportConfig { pub(crate) default_path_max_idle_timeout: Option, pub(crate) default_path_keep_alive_interval: Option, - pub(crate) nat_traversal_concurrency_limit: Option, + pub(crate) max_remote_nat_traversal_addresses: Option, pub(crate) qlog_sink: QlogSink, } @@ -443,18 +447,19 @@ impl TransportConfig { .map(Into::into) } - /// Sets the maximum number of concurrent nat traversal attempts to initiate as a client, or to - /// allow as a server. + /// Sets the maximum number of nat traversal addresses this endpoint allows the remote to + /// advertise /// - /// Setting this to any nonzero value will enable the Nat Traversal Extension for QUIC, - /// see + /// Setting this to any nonzero value will enable Iroh's holepunching, losely based in the Nat + /// Traversal Extension for QUIC, see + /// /// /// This implementation expects the multipath extension to be enabled as well. if not yet /// enabled via [`Self::max_concurrent_multipath_paths`], a default value of /// [`DEFAULT_CONCURRENT_MULTIPATH_PATHS_WHEN_ENABLED`] will be used. - pub fn set_max_nat_traversal_concurrent_attempts(&mut self, max_concurrent: u32) -> &mut Self { - self.nat_traversal_concurrency_limit = NonZeroU32::new(max_concurrent); - if max_concurrent != 0 && self.max_concurrent_multipath_paths.is_none() { + pub fn set_max_remote_nat_traversal_addresses(&mut self, max_addresses: u8) -> &mut Self { + self.max_remote_nat_traversal_addresses = NonZeroU8::new(max_addresses); + if max_addresses != 0 && self.max_concurrent_multipath_paths.is_none() { self.max_concurrent_multipath_paths( DEFAULT_CONCURRENT_MULTIPATH_PATHS_WHEN_ENABLED_.get(), ); @@ -462,14 +467,6 @@ impl TransportConfig { self } - /// Gets the maximum number of concurrent attempts for nat traversal - /// - /// If this is `Some`, the value is guaranteed to be non zero. - pub fn get_nat_traversal_concurrency_limit(&self) -> Option { - self.nat_traversal_concurrency_limit - .map(|non_zero| VarInt::from_u32(non_zero.get())) - } - /// qlog capture configuration to use for a particular connection #[cfg(feature = "qlog")] pub fn qlog_stream(&mut self, stream: Option) -> &mut Self { @@ -526,7 +523,7 @@ impl Default for TransportConfig { default_path_keep_alive_interval: None, // nat traversal disabled by default - nat_traversal_concurrency_limit: None, + max_remote_nat_traversal_addresses: None, qlog_sink: QlogSink::default(), } @@ -565,7 +562,7 @@ impl fmt::Debug for TransportConfig { max_concurrent_multipath_paths, default_path_max_idle_timeout, default_path_keep_alive_interval, - nat_traversal_concurrency_limit, + max_remote_nat_traversal_addresses, qlog_sink, } = self; let mut s = fmt.debug_struct("TransportConfig"); @@ -610,8 +607,8 @@ impl fmt::Debug for TransportConfig { default_path_keep_alive_interval, ) .field( - "nat_traversal_concurrency_limit", - nat_traversal_concurrency_limit, + "max_remote_nat_traversal_addresses", + max_remote_nat_traversal_addresses, ); if cfg!(feature = "qlog") { s.field("qlog_stream", &qlog_sink.is_enabled()); diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index f1e762572..c6367bccf 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5364,6 +5364,7 @@ impl Connection { } self.ack_frequency.peer_max_ack_delay = get_max_ack_delay(¶ms); + let mut multipath_enabled = None; if let (Some(local_max_path_id), Some(remote_max_path_id)) = ( self.config.get_initial_max_path_id(), params.initial_max_path_id, @@ -5371,24 +5372,55 @@ impl Connection { // multipath is enabled, register the local and remote maximums self.local_max_path_id = local_max_path_id; self.remote_max_path_id = remote_max_path_id; - debug!(initial_max_path_id=%local_max_path_id.min(remote_max_path_id), "multipath negotiated"); + let initial_max_path_id = local_max_path_id.min(remote_max_path_id); + debug!(%initial_max_path_id, "multipath negotiated"); + multipath_enabled = Some(initial_max_path_id); } - if let (Some(local_max_hp_validations), Some(remote_max_hp_validations)) = ( - self.config.get_nat_traversal_concurrency_limit(), - params.nat_traversal, - ) { - let max_concurrent_path_validations = - local_max_hp_validations.min(remote_max_hp_validations); - self.iroh_hp = Some(iroh_hp::State::new( - max_concurrent_path_validations, - self.side(), - )); + if let Some((max_locally_allowed_remote_addresses, max_remotely_allowed_remote_addresses)) = + self.config + .max_remote_nat_traversal_addresses + .zip(params.max_remote_nat_traversal_addresses) + { + if let Some(max_initial_paths) = + multipath_enabled.map(|path_id| path_id.saturating_add(1u8)) + { + let max_local_addresses = max_remotely_allowed_remote_addresses.get(); + let max_remote_addresses = max_locally_allowed_remote_addresses.get(); + self.iroh_hp = Some(iroh_hp::State::new( + max_remote_addresses, + max_local_addresses, + self.side(), + )); + debug!( + %max_remote_addresses, %max_local_addresses, + "iroh hole punching negotiated" + ); - debug!( - %max_concurrent_path_validations, - "iroh hole punching negotiated" - ); + match self.side() { + Side::Client => { + if max_initial_paths.as_u32() < max_remote_addresses as u32 + 1 { + // in this case the client might try to open `max_remote_addresses` new + // paths, but the current multipath configuration will not allow it + warn!(%max_initial_paths, %max_remote_addresses, "local client configuration might cause nat traversal issues") + } else if max_local_addresses as u64 + > params.active_connection_id_limit.into_inner() + { + // the server allows us to send at most `params.active_connection_id_limit` + // but they might need at least `max_local_addresses` to effectively send + // `PATH_CHALLENGE` frames to each advertised local address + warn!(%max_local_addresses, remote_cid_limit=%params.active_connection_id_limit.into_inner(), "remote server configuration might cause nat traversal issues") + } + } + Side::Server => { + if (max_initial_paths.as_u32() as u64) < crate::LOC_CID_COUNT { + warn!(%max_initial_paths, local_cid_limit=%crate::LOC_CID_COUNT, "local server configuration might cause nat traversal issues") + } + } + } + } else { + debug!("iroh nat traversal enabled for both endpoints, but multipath is missing") + } } self.peer_params = params; diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index 2372826f5..378da1a09 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -10,9 +10,9 @@ use crate::{ frame::{AddAddress, RemoveAddress}, }; -/// Maximum number of addresses to handle, applied both to local and remote addresses. -// TODO(@divma): consider making this a config option -const MAX_ADDRESSES: usize = 20; +/// Maximum number of addresses to handle, applied both to local and remote addresses, regardless +/// of configuration parameters +const MAX_ADDRESSES: u8 = 20; /// Errors that the nat traversal state might encounter. #[derive(Debug, thiserror::Error)] @@ -55,6 +55,14 @@ pub enum Event { /// State kept for Iroh's nat traversal #[derive(Debug)] pub(crate) struct State { + /// Max number of remote addresses we allow + /// + /// This is set by the local endpoint. + max_remote_addresses: usize, + /// Max number of local addresses allowed + /// + /// This is set by the remote endpoint. + max_local_addresses: usize, /// Candidate addresses the remote server reports as potentially reachable, to use for nat /// traversal attempts. remote_addresses: FxHashMap, @@ -63,9 +71,6 @@ pub(crate) struct State { local_addresses: FxHashMap<(IpAddr, u16), VarInt>, /// The next id to use for local addresses sent to the client next_local_addr_id: VarInt, - /// Max concurrent address validations to perform - // TODO(@divma): opening paths might not be a good idea after all - max_concurrent_path_validations: u64, /// Local connection side side: Side, /// Current nat holepunching round @@ -99,7 +104,7 @@ impl State { address: SocketAddr, ) -> Result, Error> { let address = (address.ip(), address.port()); - let allow_new = self.local_addresses.len() < MAX_ADDRESSES; + let allow_new = self.local_addresses.len() < self.max_local_addresses; let is_server = self.side.is_server(); match self.local_addresses.entry(address) { Entry::Occupied(_) => Ok(None), @@ -142,16 +147,17 @@ impl State { } } - pub(crate) fn new(VarInt(max_concurrent_path_validations): VarInt, side: Side) -> Self { + pub(crate) fn new(max_remote_addresses: u8, max_local_addresses: u8, side: Side) -> Self { Self { remote_addresses: Default::default(), local_addresses: Default::default(), next_local_addr_id: Default::default(), - max_concurrent_path_validations, side, round: Default::default(), round_path_ids: Default::default(), challenges: Default::default(), + max_remote_addresses: max_remote_addresses.min(MAX_ADDRESSES).into(), + max_local_addresses: max_local_addresses.min(MAX_ADDRESSES).into(), } } @@ -216,7 +222,7 @@ impl<'a> ClientSide<'a> { ) -> Result, Error> { let AddAddress { seq_no, ip, port } = add_addr; let address = (ip, port); - let allow_new = self.state.remote_addresses.len() < MAX_ADDRESSES; + let allow_new = self.state.remote_addresses.len() < self.state.max_remote_addresses; match self.state.remote_addresses.entry(seq_no) { Entry::Occupied(mut occupied_entry) => { let old_value = occupied_entry.insert(address); diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 41cb7091c..86d695d35 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -9,6 +9,7 @@ use std::{ convert::TryFrom, net::{Ipv4Addr, Ipv6Addr, SocketAddrV4, SocketAddrV6}, + num::NonZeroU8, }; use bytes::{Buf, BufMut}; @@ -120,7 +121,7 @@ macro_rules! make_struct { pub(crate) initial_max_path_id: Option, /// Nat traversal draft - pub nat_traversal: Option, + pub max_remote_nat_traversal_addresses: Option, } // We deliberately don't implement the `Default` trait, since that would be public, and @@ -146,7 +147,7 @@ macro_rules! make_struct { write_order: None, address_discovery_role: address_discovery::Role::Disabled, initial_max_path_id: None, - nat_traversal: None, + max_remote_nat_traversal_addresses: None, } } } @@ -196,7 +197,7 @@ impl TransportParameters { }), address_discovery_role: config.address_discovery_role, initial_max_path_id: config.get_initial_max_path_id(), - nat_traversal: config.get_nat_traversal_concurrency_limit(), + max_remote_nat_traversal_addresses: config.max_remote_nat_traversal_addresses, ..Self::default() } } @@ -214,7 +215,7 @@ impl TransportParameters { || cached.max_datagram_frame_size > self.max_datagram_frame_size || cached.grease_quic_bit && !self.grease_quic_bit || cached.address_discovery_role != self.address_discovery_role - || cached.nat_traversal != self.nat_traversal + || cached.max_remote_nat_traversal_addresses != self.max_remote_nat_traversal_addresses { return Err(TransportError::PROTOCOL_VIOLATION( "0-RTT accepted with incompatible transport parameters", @@ -415,10 +416,10 @@ impl TransportParameters { } } TransportParameterId::IrohNatTraversal => { - if let Some(val) = self.nat_traversal { + if let Some(val) = self.max_remote_nat_traversal_addresses { w.write_var(id as u64); - w.write_var(val.size() as u64); - w.write(val); + w.write(VarInt(1)); + w.write(val.get()); } } id => { @@ -547,20 +548,17 @@ impl TransportParameters { params.initial_max_path_id = Some(value); } TransportParameterId::IrohNatTraversal => { - if params.nat_traversal.is_some() { + if params.max_remote_nat_traversal_addresses.is_some() { + return Err(Error::Malformed); + } + if len != 1 { return Err(Error::Malformed); } - let value: VarInt = r.get()?; - if len != value.size() { - return Err(Error::Malformed); - } + let value: u8 = r.get()?; + let value = NonZeroU8::new(value).ok_or(Error::IllegalValue)?; - if value.into_inner() == 0 { - return Err(Error::IllegalValue); - } - - params.nat_traversal = Some(value); + params.max_remote_nat_traversal_addresses = Some(value); } _ => { macro_rules! parse { From 64ff19147d30da863dbd4323dc42fefaf2033195 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Thu, 20 Nov 2025 12:12:53 +0100 Subject: [PATCH 25/40] Hook up id tryfrom --- quinn-proto/src/transport_parameters.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/quinn-proto/src/transport_parameters.rs b/quinn-proto/src/transport_parameters.rs index 86d695d35..bae0b9cc0 100644 --- a/quinn-proto/src/transport_parameters.rs +++ b/quinn-proto/src/transport_parameters.rs @@ -117,7 +117,7 @@ macro_rules! make_struct { /// The role of this peer in address discovery, if any. pub(crate) address_discovery_role: address_discovery::Role, - // Multipath extension + /// Multipath extension pub(crate) initial_max_path_id: Option, /// Nat traversal draft @@ -802,6 +802,7 @@ impl TryFrom for TransportParameterId { id if Self::MinAckDelayDraft07 == id => Self::MinAckDelayDraft07, id if Self::ObservedAddr == id => Self::ObservedAddr, id if Self::InitialMaxPathId == id => Self::InitialMaxPathId, + id if Self::IrohNatTraversal == id => Self::IrohNatTraversal, _ => return Err(()), }; Ok(param) @@ -842,6 +843,7 @@ mod test { min_ack_delay: Some(2_000u32.into()), address_discovery_role: address_discovery::Role::SendOnly, initial_max_path_id: Some(PathId::MAX), + max_remote_nat_traversal_addresses: Some(5u8.try_into().unwrap()), ..TransportParameters::default() }; params.write(&mut buf); From f71832f0bef0b8365e85d4629f65e5a188496c78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 20 Nov 2025 06:14:27 -0500 Subject: [PATCH 26/40] Queue server challenges --- quinn-proto/src/connection/mod.rs | 49 +++++++++++++++++++++- quinn-proto/src/connection/spaces.rs | 24 +++++------ quinn-proto/src/iroh_hp.rs | 63 ++++++++++++++++++++++++++-- quinn-proto/src/lib.rs | 2 +- 4 files changed, 120 insertions(+), 18 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index c6367bccf..f90729bfb 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4488,8 +4488,52 @@ impl Connection { ))); } } - Frame::ReachOut(_frame) => { - // TODO(@divma): handle + Frame::ReachOut(reach_out) => { + let Some(hp_state) = self.iroh_hp.as_mut() else { + return Err(TransportError::PROTOCOL_VIOLATION( + "received REACH_OUT frame when iroh's nat traversal was not negotiated", + )); + }; + + match hp_state.handle_reach_out(reach_out) { + Ok(None) => { + // no action required here + } + Ok(Some((challenge_info, is_new_round))) => { + let iroh_hp::ChallengeNeeded { + token, + ip, + port, + round, + } = challenge_info; + if is_new_round { + // TODO(@divma): this depends on round starting on 1 right now, + // because the round should be greater to the default one, which is + // zero + self.spaces[SpaceId::Data].pending.challenges_round = round; + self.spaces[SpaceId::Data].pending.challenges.clear(); + } + + self.spaces[SpaceId::Data] + .pending + .challenges + .push((token, ip, port)); + } + Err(iroh_hp::Error::WrongConnectionSide) => { + return Err(TransportError::PROTOCOL_VIOLATION( + "server sent REACH_OUT frames for nat traversal", + )); + } + Err(iroh_hp::Error::TooManyAddresses) => { + return Err(TransportError::PROTOCOL_VIOLATION( + "client exceeded allowed REACH_OUT frames for this round", + )); + } + Err(error) => { + warn!(%error,"error handling REACH_OUT frame"); + // TODO(@divma): check if this is reachable + } + } } } } @@ -5391,6 +5435,7 @@ impl Connection { max_remote_addresses, max_local_addresses, self.side(), + self.rng.clone(), )); debug!( %max_remote_addresses, %max_local_addresses, diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index e0de609d8..6b298f23a 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -564,8 +564,12 @@ pub struct Retransmits { pub(super) remove_address: BTreeSet, /// Round and local addresses to advertise in `REACH_OUT` frames pub(super) reach_out: Option<(VarInt, Vec<(IpAddr, u16)>)>, - /// Round and remote addresses to which `PATH_CHALLENGE`s need to be sent - pub(super) challenges: Option<(VarInt, Vec<(IpAddr, u16)>)>, + /// Round of the nat traversal challenges that are pending + /// + /// This is only used for bitwise operations on the retransmission data. + pub(super) challenges_round: VarInt, + /// Tokens and remote addresses to which `PATH_CHALLENGE`s need to be sent + pub(super) challenges: Vec<(u64, IpAddr, u16)>, } impl Retransmits { @@ -592,7 +596,7 @@ impl Retransmits { && self.add_address.is_empty() && self.remove_address.is_empty() && self.reach_out.is_none() - && self.challenges.is_none() + && self.challenges.is_empty() } } @@ -633,15 +637,11 @@ impl ::std::ops::BitOrAssign for Retransmits { _ => {} } - // if there are two rounds, prefer the most recent pending challenges set - let lhs_round = self.challenges.as_ref().map(|(round, _)| *round); - let rhs_round = rhs.challenges.as_ref().map(|(round, _)| *round); - match (lhs_round, rhs_round) { - (None, Some(_)) => self.challenges = rhs.challenges.clone(), - (Some(lhs_round), Some(rhs_round)) if rhs_round > lhs_round => { - self.challenges = rhs.challenges.clone() - } - _ => {} + if self.challenges_round < rhs.challenges_round { + self.challenges_round = rhs.challenges_round; + self.challenges = rhs.challenges.clone(); + } else if self.challenges_round == rhs.challenges_round { + self.challenges.extend_from_slice(&rhs.challenges); } } } diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index 378da1a09..e3f23b208 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -3,11 +3,12 @@ use std::{ net::{IpAddr, SocketAddr}, }; +use rand::{Rng, rngs::StdRng}; use rustc_hash::FxHashMap; use crate::{ PathId, Side, VarInt, - frame::{AddAddress, RemoveAddress}, + frame::{AddAddress, ReachOut, RemoveAddress}, }; /// Maximum number of addresses to handle, applied both to local and remote addresses, regardless @@ -45,6 +46,17 @@ pub(crate) struct NatTraversalRound { pub(crate) prev_round_path_ids: Vec, } +pub(crate) struct ChallengeNeeded { + /// Token to send on the challenge + pub(crate) token: u64, + /// Destination address of the challenge + pub(crate) ip: IpAddr, + /// Destination port of the challenge + pub(crate) port: u16, + /// Round to which this challenge belongs to + pub(crate) round: VarInt, +} + // TODO(@divma): unclear to me what these events are useful for\ #[derive(Debug, Clone)] pub enum Event { @@ -63,6 +75,8 @@ pub(crate) struct State { /// /// This is set by the remote endpoint. max_local_addresses: usize, + /// Random generator + rng: StdRng, /// Candidate addresses the remote server reports as potentially reachable, to use for nat /// traversal attempts. remote_addresses: FxHashMap, @@ -83,7 +97,7 @@ pub(crate) struct State { round_path_ids: Vec, /// Challenges sent by servers to validate client addresses without attempting to open /// multipath paths - challenges: FxHashMap, + challenges: FxHashMap<(IpAddr, u16), u64>, } /// Nat traversal api exclusive to clients @@ -147,10 +161,16 @@ impl State { } } - pub(crate) fn new(max_remote_addresses: u8, max_local_addresses: u8, side: Side) -> Self { + pub(crate) fn new( + max_remote_addresses: u8, + max_local_addresses: u8, + side: Side, + rng: StdRng, + ) -> Self { Self { remote_addresses: Default::default(), local_addresses: Default::default(), + rng, next_local_addr_id: Default::default(), side, round: Default::default(), @@ -209,6 +229,43 @@ impl State { self.round_path_ids = path_ids; Ok(()) } + + /// Handles a received [`ReachOut`] + /// + /// It returns the token that should be sent in response to this frame as a challenge, and + /// whether this starts a new nat traversal round. + /// + /// If this frame was ignored, it returns `None`. + pub(crate) fn handle_reach_out( + &mut self, + reach_out: ReachOut, + ) -> Result, Error> { + let ReachOut { round, ip, port } = reach_out; + if self.side.is_client() { + return Err(Error::WrongConnectionSide); + } + + if round >= self.round { + let is_new_round = round > self.round; + if is_new_round { + self.challenges.clear(); + } + if self.challenges.len() >= self.max_remote_addresses { + return Err(Error::TooManyAddresses); + } + let token = self.rng.random(); + self.challenges.insert((ip, port), token); + let challenge_info = ChallengeNeeded { + token, + ip, + port, + round, + }; + return Ok(Some((challenge_info, is_new_round))); + } + + Ok(None) + } } impl<'a> ClientSide<'a> { diff --git a/quinn-proto/src/lib.rs b/quinn-proto/src/lib.rs index 86d10c058..9f3c3711e 100644 --- a/quinn-proto/src/lib.rs +++ b/quinn-proto/src/lib.rs @@ -329,7 +329,7 @@ pub struct Transmit { // /// The maximum number of CIDs we bother to issue per path -const LOC_CID_COUNT: u64 = 8; +const LOC_CID_COUNT: u64 = 12; const RESET_TOKEN_SIZE: usize = 16; const MAX_CID_SIZE: usize = 20; const MIN_INITIAL_SIZE: u16 = 1200; From 4817d9616f100e685cdfbd97097e4a2b386b8276 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 20 Nov 2025 10:17:23 -0500 Subject: [PATCH 27/40] send rand data instead of path challenges --- quinn-proto/src/connection/mod.rs | 31 ++++++++++++------ quinn-proto/src/connection/spaces.rs | 22 ++++++------- quinn-proto/src/iroh_hp.rs | 47 +++++++++++----------------- 3 files changed, 50 insertions(+), 50 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index f90729bfb..c11248df1 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -11,7 +11,7 @@ use std::{ use bytes::{BufMut, Bytes, BytesMut}; use frame::StreamMetaVec; -use rand::{Rng, SeedableRng, rngs::StdRng}; +use rand::{Rng, RngCore, SeedableRng, rngs::StdRng}; use rustc_hash::{FxHashMap, FxHashSet}; use thiserror::Error; use tracing::{debug, error, trace, trace_span, warn}; @@ -842,6 +842,18 @@ impl Connection { max_datagrams: usize, buf: &mut Vec, ) -> Option { + if let Some(address) = self.spaces[SpaceId::Data].pending.hole_punch_to.pop() { + buf.reserve_exact(8); // send 8 bytes of random data + self.rng.fill_bytes(buf); + return Some(Transmit { + destination: address.into(), + ecn: None, + size: 8, + segment_size: None, + src_ip: None, + }); + } + assert!(max_datagrams != 0); let max_datagrams = match self.config.enable_segmentation_offload { false => 1, @@ -4499,25 +4511,25 @@ impl Connection { Ok(None) => { // no action required here } - Ok(Some((challenge_info, is_new_round))) => { - let iroh_hp::ChallengeNeeded { - token, + Ok(Some(info)) => { + let iroh_hp::RandDataNeeded { ip, port, round, - } = challenge_info; + is_new_round, + } = info; if is_new_round { // TODO(@divma): this depends on round starting on 1 right now, // because the round should be greater to the default one, which is // zero - self.spaces[SpaceId::Data].pending.challenges_round = round; - self.spaces[SpaceId::Data].pending.challenges.clear(); + self.spaces[SpaceId::Data].pending.hole_punch_round = round; + self.spaces[SpaceId::Data].pending.hole_punch_to.clear(); } self.spaces[SpaceId::Data] .pending - .challenges - .push((token, ip, port)); + .hole_punch_to + .push((ip, port)); } Err(iroh_hp::Error::WrongConnectionSide) => { return Err(TransportError::PROTOCOL_VIOLATION( @@ -5435,7 +5447,6 @@ impl Connection { max_remote_addresses, max_local_addresses, self.side(), - self.rng.clone(), )); debug!( %max_remote_addresses, %max_local_addresses, diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs index 6b298f23a..b86dc8d9f 100644 --- a/quinn-proto/src/connection/spaces.rs +++ b/quinn-proto/src/connection/spaces.rs @@ -564,12 +564,12 @@ pub struct Retransmits { pub(super) remove_address: BTreeSet, /// Round and local addresses to advertise in `REACH_OUT` frames pub(super) reach_out: Option<(VarInt, Vec<(IpAddr, u16)>)>, - /// Round of the nat traversal challenges that are pending + /// Round of the nat traversal rand data that are pending /// - /// This is only used for bitwise operations on the retransmission data. - pub(super) challenges_round: VarInt, - /// Tokens and remote addresses to which `PATH_CHALLENGE`s need to be sent - pub(super) challenges: Vec<(u64, IpAddr, u16)>, + /// This is only used for bitwise operations on the pending data. + pub(super) hole_punch_round: VarInt, + /// Remote addresses to which random data needs to be sent + pub(super) hole_punch_to: Vec<(IpAddr, u16)>, } impl Retransmits { @@ -596,7 +596,7 @@ impl Retransmits { && self.add_address.is_empty() && self.remove_address.is_empty() && self.reach_out.is_none() - && self.challenges.is_empty() + && self.hole_punch_to.is_empty() } } @@ -637,11 +637,11 @@ impl ::std::ops::BitOrAssign for Retransmits { _ => {} } - if self.challenges_round < rhs.challenges_round { - self.challenges_round = rhs.challenges_round; - self.challenges = rhs.challenges.clone(); - } else if self.challenges_round == rhs.challenges_round { - self.challenges.extend_from_slice(&rhs.challenges); + if self.hole_punch_round < rhs.hole_punch_round { + self.hole_punch_round = rhs.hole_punch_round; + self.hole_punch_to = rhs.hole_punch_to.clone(); + } else if self.hole_punch_round == rhs.hole_punch_round { + self.hole_punch_to.extend_from_slice(&rhs.hole_punch_to); } } } diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index e3f23b208..de63ebee0 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -3,8 +3,7 @@ use std::{ net::{IpAddr, SocketAddr}, }; -use rand::{Rng, rngs::StdRng}; -use rustc_hash::FxHashMap; +use rustc_hash::{FxHashMap, FxHashSet}; use crate::{ PathId, Side, VarInt, @@ -46,15 +45,15 @@ pub(crate) struct NatTraversalRound { pub(crate) prev_round_path_ids: Vec, } -pub(crate) struct ChallengeNeeded { - /// Token to send on the challenge - pub(crate) token: u64, - /// Destination address of the challenge +pub(crate) struct RandDataNeeded { + /// Destination address of the hole punching random data pub(crate) ip: IpAddr, - /// Destination port of the challenge + /// Destination port of the hole punching random data pub(crate) port: u16, - /// Round to which this challenge belongs to + /// Round to which this hole punching random data belongs to pub(crate) round: VarInt, + /// Whether this starts a new round + pub(crate) is_new_round: bool, } // TODO(@divma): unclear to me what these events are useful for\ @@ -75,8 +74,6 @@ pub(crate) struct State { /// /// This is set by the remote endpoint. max_local_addresses: usize, - /// Random generator - rng: StdRng, /// Candidate addresses the remote server reports as potentially reachable, to use for nat /// traversal attempts. remote_addresses: FxHashMap, @@ -95,9 +92,8 @@ pub(crate) struct State { round: VarInt, /// [`PathId`]s used to probe remotes assigned to this round round_path_ids: Vec, - /// Challenges sent by servers to validate client addresses without attempting to open - /// multipath paths - challenges: FxHashMap<(IpAddr, u16), u64>, + /// Addresses to which random data sent by servers to attempt to hole punch to clients + server_sent_rand_data: FxHashSet<(IpAddr, u16)>, } /// Nat traversal api exclusive to clients @@ -161,21 +157,15 @@ impl State { } } - pub(crate) fn new( - max_remote_addresses: u8, - max_local_addresses: u8, - side: Side, - rng: StdRng, - ) -> Self { + pub(crate) fn new(max_remote_addresses: u8, max_local_addresses: u8, side: Side) -> Self { Self { remote_addresses: Default::default(), local_addresses: Default::default(), - rng, next_local_addr_id: Default::default(), side, round: Default::default(), round_path_ids: Default::default(), - challenges: Default::default(), + server_sent_rand_data: Default::default(), max_remote_addresses: max_remote_addresses.min(MAX_ADDRESSES).into(), max_local_addresses: max_local_addresses.min(MAX_ADDRESSES).into(), } @@ -239,7 +229,7 @@ impl State { pub(crate) fn handle_reach_out( &mut self, reach_out: ReachOut, - ) -> Result, Error> { + ) -> Result, Error> { let ReachOut { round, ip, port } = reach_out; if self.side.is_client() { return Err(Error::WrongConnectionSide); @@ -248,20 +238,19 @@ impl State { if round >= self.round { let is_new_round = round > self.round; if is_new_round { - self.challenges.clear(); + self.server_sent_rand_data.clear(); } - if self.challenges.len() >= self.max_remote_addresses { + if self.server_sent_rand_data.len() >= self.max_remote_addresses { return Err(Error::TooManyAddresses); } - let token = self.rng.random(); - self.challenges.insert((ip, port), token); - let challenge_info = ChallengeNeeded { - token, + self.server_sent_rand_data.insert((ip, port)); + let info = RandDataNeeded { ip, port, round, + is_new_round, }; - return Ok(Some((challenge_info, is_new_round))); + return Ok(Some(info)); } Ok(None) From 9188107ec1c77df7ec1e0258934b4807e789d87a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 20 Nov 2025 10:26:02 -0500 Subject: [PATCH 28/40] track sent reach outs in stats and sent frames --- quinn-proto/src/connection/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index c11248df1..3e2df2cf2 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4794,6 +4794,13 @@ impl Connection { let reach_out = frame::ReachOut::new(*round, local_addr); if buf.remaining_mut() > reach_out.size() { reach_out.write(buf); + let sent_reachouts = sent + .retransmits + .get_or_create() + .reach_out + .get_or_insert_with(|| (*round, Default::default())); + sent_reachouts.1.push(local_addr); + self.stats.frame_tx.reach_out = self.stats.frame_tx.reach_out.saturating_add(1); } else { addresses.push(local_addr); break; From 25b20638e57763bb1bf7c8b948c714bd1a99e50a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Thu, 20 Nov 2025 10:41:01 -0500 Subject: [PATCH 29/40] track sent add and remove address in stats and sent frames --- quinn-proto/src/connection/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 3e2df2cf2..4da0bcce0 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5300,6 +5300,11 @@ impl Connection { while space_id == SpaceId::Data && frame::AddAddress::SIZE_BOUND <= buf.remaining_mut() { if let Some(added_address) = space.pending.add_address.pop_last() { added_address.write(buf); + sent.retransmits + .get_or_create() + .add_address + .insert(added_address); + self.stats.frame_tx.add_address = self.stats.frame_tx.add_address.saturating_add(1); } else { break; } @@ -5308,6 +5313,12 @@ impl Connection { while space_id == SpaceId::Data && frame::RemoveAddress::SIZE_BOUND <= buf.remaining_mut() { if let Some(removed_address) = space.pending.remove_address.pop_last() { removed_address.write(buf); + sent.retransmits + .get_or_create() + .remove_address + .insert(removed_address); + self.stats.frame_tx.remove_address = + self.stats.frame_tx.remove_address.saturating_add(1); } else { break; } From ee888998585717e7b3f1e6e350c2776dc87483ef Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Fri, 21 Nov 2025 16:34:16 +0100 Subject: [PATCH 30/40] Change APIs to set addresses --- quinn-proto/src/connection/mod.rs | 40 ++++++++++++++++--------------- quinn-proto/src/iroh_hp.rs | 10 +++++++- quinn/src/connection.rs | 22 ++++++++--------- 3 files changed, 41 insertions(+), 31 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 4da0bcce0..a3f59c081 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5872,48 +5872,50 @@ impl Connection { /// If adding any address fails, an error is returned. Previous addresses might have been /// added. // TODO(@divma): this combined api has the issue that an error does not mean nothing was done - pub fn add_nat_traversal_addresses( - &mut self, - addresses: &[SocketAddr], - ) -> Result<(), iroh_hp::Error> { + pub fn add_nat_traversal_address(&mut self, address: SocketAddr) -> Result<(), iroh_hp::Error> { let hp_state = self .iroh_hp .as_mut() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; - for &address in addresses { - if let Some(added) = hp_state.add_local_address(address)? { - self.spaces[SpaceId::Data].pending.add_address.insert(added); - }; - } + if let Some(added) = hp_state.add_local_address(address)? { + self.spaces[SpaceId::Data].pending.add_address.insert(added); + }; Ok(()) } /// Removes an address the endpoing no longer considers reachable for nat traversal /// /// Addresses not present in the set will be silently ignored. - pub fn remove_nat_traversal_addresses( + pub fn remove_nat_traversal_address( &mut self, - addresses: &[SocketAddr], + address: SocketAddr, ) -> Result<(), iroh_hp::Error> { let is_server = self.side().is_server(); let hp_state = self .iroh_hp .as_mut() .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; - for address in addresses { - if let Some(removed) = hp_state.remove_local_address(*address) { - if is_server { - self.spaces[SpaceId::Data] - .pending - .remove_address - .insert(removed); - } + if let Some(removed) = hp_state.remove_local_address(address) { + if is_server { + self.spaces[SpaceId::Data] + .pending + .remove_address + .insert(removed); } } Ok(()) } + /// Get the current local nat traversal addresses + pub fn get_local_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { + let hp_state = self + .iroh_hp + .as_ref() + .ok_or(iroh_hp::Error::ExtensionNotNegotiated)?; + Ok(hp_state.get_local_nat_traversal_addresses()) + } + /// Get the currently advertised nat traversal addresses by the server pub fn get_remote_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { let hp_state = self diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index de63ebee0..709fbb903 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -171,6 +171,14 @@ impl State { } } + pub(crate) fn get_local_nat_traversal_addresses(&self) -> Vec { + self.local_addresses + .keys() + .copied() + .map(Into::into) + .collect() + } + pub(crate) fn get_remote_nat_traversal_addresses(&self) -> Result, Error> { if !self.side.is_client() { return Err(Error::WrongConnectionSide); @@ -195,7 +203,7 @@ impl State { return Err(Error::WrongConnectionSide); } - if self.local_addresses.is_empty() || self.remote_addresses.is_empty() { + if self.local_addresses.is_empty() { return Err(Error::NotEnoughAddresses); } diff --git a/quinn/src/connection.rs b/quinn/src/connection.rs index a99f9af41..a89665736 100644 --- a/quinn/src/connection.rs +++ b/quinn/src/connection.rs @@ -851,18 +851,15 @@ impl Connection { conn.inner.is_multipath_negotiated() } - /// Registers one or more addresses at which this endpoint is reachable + /// Registers one address at which this endpoint might be reachable /// /// When the NAT traversal extension is negotiated, servers send these addresses to clients in /// `ADD_ADDRESS` frames. This allows clients to obtain server address candidates to initiate /// NAT traversal attempts. Clients provide their own reachable addresses in `REACH_OUT` frames /// when [`Self::initiate_nat_traversal_round`] is called. - pub fn add_nat_traversal_addresses( - &self, - addresses: &[SocketAddr], - ) -> Result<(), iroh_hp::Error> { + pub fn add_nat_traversal_address(&self, address: SocketAddr) -> Result<(), iroh_hp::Error> { let mut conn = self.0.state.lock("add_nat_traversal_addresses"); - conn.inner.add_nat_traversal_addresses(addresses) + conn.inner.add_nat_traversal_address(address) } /// Removes one or more addresses from the set of addresses at which this endpoint is reachable @@ -874,12 +871,15 @@ impl Connection { /// For clients, removed addresses will no longer be advertised in `REACH_OUT` frames. /// /// Addresses not present in the set will be silently ignored. - pub fn remove_nat_traversal_addresses( - &self, - addresses: &[SocketAddr], - ) -> Result<(), iroh_hp::Error> { + pub fn remove_nat_traversal_address(&self, address: SocketAddr) -> Result<(), iroh_hp::Error> { let mut conn = self.0.state.lock("remove_nat_traversal_addresses"); - conn.inner.add_nat_traversal_addresses(addresses) + conn.inner.remove_nat_traversal_address(address) + } + + /// Get the current local nat traversal addresses + pub fn get_local_nat_traversal_addresses(&self) -> Result, iroh_hp::Error> { + let conn = self.0.state.lock("get_remote_nat_traversal_addresses"); + conn.inner.get_local_nat_traversal_addresses() } /// Get the currently advertised nat traversal addresses by the server From dcd734071d307379dcee6b558efa16d81c2b562a Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Fri, 21 Nov 2025 16:34:41 +0100 Subject: [PATCH 31/40] logging conventions --- quinn-proto/src/connection/mod.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index a3f59c081..087444d93 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4788,11 +4788,13 @@ impl Connection { self.stats.frame_tx.handshake_done.saturating_add(1); } + // REACH_OUT // TODO(@divma): path explusive considerations if let Some((round, addresses)) = space.pending.reach_out.as_mut() { while let Some(local_addr) = addresses.pop() { let reach_out = frame::ReachOut::new(*round, local_addr); if buf.remaining_mut() > reach_out.size() { + trace!(%round, ?local_addr, "REACH_OUT"); reach_out.write(buf); let sent_reachouts = sent .retransmits From 6a19cc71a46f6d996afe239f030a982427e9fc30 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Fri, 21 Nov 2025 17:24:19 +0100 Subject: [PATCH 32/40] write 8 bytes, not 0 --- quinn-proto/src/connection/mod.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 087444d93..f01e31d1d 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -11,7 +11,7 @@ use std::{ use bytes::{BufMut, Bytes, BytesMut}; use frame::StreamMetaVec; -use rand::{Rng, RngCore, SeedableRng, rngs::StdRng}; +use rand::{Rng, SeedableRng, rngs::StdRng}; use rustc_hash::{FxHashMap, FxHashSet}; use thiserror::Error; use tracing::{debug, error, trace, trace_span, warn}; @@ -844,7 +844,8 @@ impl Connection { ) -> Option { if let Some(address) = self.spaces[SpaceId::Data].pending.hole_punch_to.pop() { buf.reserve_exact(8); // send 8 bytes of random data - self.rng.fill_bytes(buf); + let tmp: [u8; 8] = self.rng.random(); + buf.put_slice(&tmp); return Some(Transmit { destination: address.into(), ecn: None, From 079d83b5cf368dceef08e6a5c14b9ab2613f5d17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Fri, 21 Nov 2025 13:09:33 +0100 Subject: [PATCH 33/40] Update `RttEstimator` from path challenge responses --- quinn-proto/src/connection/mod.rs | 42 ++++++++++++++++------------- quinn-proto/src/connection/paths.rs | 11 ++++---- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index f01e31d1d..47b07b26a 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -811,7 +811,6 @@ impl Connection { // for the path to be opened we need to send a packet on the path. Sending a challenge // guarantees this - data.challenge = Some(self.rng.random()); data.challenge_pending = true; let path = vacant_entry.insert(PathState { data, prev: None }); @@ -1593,11 +1592,9 @@ impl Connection { let (prev_cid, prev_path) = self.paths.get_mut(&path_id)?.prev.as_mut()?; if !prev_path.challenge_pending { return None; - } - prev_path.challenge_pending = false; - let token = prev_path - .challenge - .expect("previous path challenge pending without token"); + }; + let token = self.rng.random(); + prev_path.challenges_sent.insert(token, now); let destination = prev_path.remote; debug_assert_eq!( self.highest_space, @@ -1848,14 +1845,14 @@ impl Connection { if let Some((_, prev)) = path.prev.take() { path.data = prev; } - path.data.challenge = None; + path.data.challenges_sent.clear(); path.data.challenge_pending = false; } PathTimer::PathOpen => { let Some(path) = self.path_mut(path_id) else { continue; }; - path.challenge = None; + path.challenges_sent.clear(); path.challenge_pending = false; debug!("new path validation failed"); if let Err(err) = self.close_path( @@ -2401,7 +2398,7 @@ impl Connection { .remove_in_flight(&info); let app_limited = self.app_limited; let path = self.path_data_mut(path_id); - if info.ack_eliciting && path.challenge.is_none() { + if info.ack_eliciting && path.challenges_sent.is_empty() { // Only pass ACKs to the congestion controller if we are not validating the current // path, so as to ignore any ACKs from older paths still coming in. let rtt = path.rtt; @@ -4024,7 +4021,10 @@ impl Connection { .paths .get_mut(&path_id) .expect("payload is processed only after the path becomes known"); - if path.data.challenge == Some(token) && remote == path.data.remote { + + if remote != path.data.remote { + debug!(token, "ignoring invalid PATH_RESPONSE"); + } else if let Some(&challenge_sent) = path.data.challenges_sent.get(&token) { self.timers .stop(Timer::PerPath(path_id, PathTimer::PathValidation)); if !path.data.validated { @@ -4032,8 +4032,13 @@ impl Connection { } self.timers .stop(Timer::PerPath(path_id, PathTimer::PathOpen)); - path.data.challenge = None; + path.data.challenges_sent.clear(); + path.data.challenge_pending = false; path.data.validated = true; + path.data.rtt.update( + Duration::ZERO, + now.saturating_duration_since(challenge_sent), + ); self.events .push_back(Event::Path(PathEvent::Opened { id: path_id })); // mark the path as open from the application perspective now that Opened @@ -4047,7 +4052,7 @@ impl Connection { } } if let Some((_, ref mut prev)) = path.prev { - prev.challenge = None; + prev.challenges_sent.clear(); prev.challenge_pending = false; } } else { @@ -4644,13 +4649,11 @@ impl Connection { })); } } - new_path.challenge = Some(self.rng.random()); new_path.challenge_pending = true; let mut prev = mem::replace(path, new_path); // Don't clobber the original path if the previous one hasn't been validated yet - if prev.challenge.is_none() { - prev.challenge = Some(self.rng.random()); + if !prev.validated { prev.challenge_pending = true; // We haven't updated the remote CID yet, this captures the remote CID we were using on // the previous path. @@ -4912,16 +4915,19 @@ impl Connection { } // PATH_CHALLENGE - if buf.remaining_mut() > 9 && space_id == SpaceId::Data { + if buf.remaining_mut() > 9 && space_id == SpaceId::Data && !path.validated { // Transmit challenges with every outgoing packet on an unvalidated path - if let Some(token) = path.challenge { + if !path.validated { + // Generate a new challenge every time we send a new PC + let token = self.rng.random(); + path.challenges_sent.insert(token, now); sent.non_retransmits = true; sent.requires_padding = true; trace!("PATH_CHALLENGE {:08x}", token); buf.write(frame::FrameType::PATH_CHALLENGE); buf.write(token); - if is_multipath_negotiated && !path.validated && path.challenge_pending { + if is_multipath_negotiated && path.challenge_pending { // queue informing the path status along with the challenge space.pending.path_status.insert(path_id); } diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index 60ebc06c3..b2750d867 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -1,5 +1,6 @@ use std::{cmp, net::SocketAddr}; +use identity_hash::IntMap; use thiserror::Error; use tracing::{debug, trace}; @@ -128,7 +129,7 @@ pub(super) struct PathData { pub(super) congestion: Box, /// Pacing state pub(super) pacing: Pacer, - pub(super) challenge: Option, + pub(super) challenges_sent: IntMap, pub(super) challenge_pending: bool, /// Pending responses to PATH_CHALLENGE frames pub(super) path_responses: PathResponses, @@ -224,8 +225,8 @@ impl PathData { now, ), congestion, - challenge: None, - challenge_pending: false, + challenges_sent: Default::default(), + challenge_pending: Default::default(), path_responses: PathResponses::default(), validated: false, total_sent: 0, @@ -278,8 +279,8 @@ impl PathData { pacing: Pacer::new(smoothed_rtt, congestion.window(), prev.current_mtu(), now), sending_ecn: true, congestion, - challenge: None, - challenge_pending: false, + challenges_sent: Default::default(), + challenge_pending: Default::default(), path_responses: PathResponses::default(), validated: false, total_sent: 0, From bfb30e81bb3f486ae76f51b455f3cf18ecc73799 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Fri, 21 Nov 2025 18:32:52 +0100 Subject: [PATCH 34/40] stop LossDetection timer when giving up on a path might be in the wrong place --- quinn-proto/src/connection/mod.rs | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 47b07b26a..ab3f31c75 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -842,6 +842,7 @@ impl Connection { buf: &mut Vec, ) -> Option { if let Some(address) = self.spaces[SpaceId::Data].pending.hole_punch_to.pop() { + trace!(dst = ?address, "RAND_DATA packet"); buf.reserve_exact(8); // send 8 bytes of random data let tmp: [u8; 8] = self.rng.random(); buf.put_slice(&tmp); @@ -1854,6 +1855,11 @@ impl Connection { }; path.challenges_sent.clear(); path.challenge_pending = false; + + // TODO(flub): not sure yet + self.timers + .stop(Timer::PerPath(path_id, PathTimer::LossDetection)); + debug!("new path validation failed"); if let Err(err) = self.close_path( now, @@ -2461,7 +2467,7 @@ impl Connection { let (_, space) = match self.pto_time_and_space(now, path_id) { Some(x) => x, None => { - error!("PTO expired while unset"); + error!(?path_id, "PTO expired while unset"); return; } }; @@ -5305,9 +5311,16 @@ impl Connection { self.stats.frame_tx.stream += sent.stream_frames.len() as u64; } + // ADD_ADDRESS // TODO(@divma): check if we need to do path exclusive filters while space_id == SpaceId::Data && frame::AddAddress::SIZE_BOUND <= buf.remaining_mut() { if let Some(added_address) = space.pending.add_address.pop_last() { + trace!( + seq = %added_address.seq_no, + ip = ?added_address.ip, + port = added_address.port, + "ADD_ADDRESS", + ); added_address.write(buf); sent.retransmits .get_or_create() @@ -5319,8 +5332,10 @@ impl Connection { } } + // REMOVE_ADDRESS while space_id == SpaceId::Data && frame::RemoveAddress::SIZE_BOUND <= buf.remaining_mut() { if let Some(removed_address) = space.pending.remove_address.pop_last() { + trace!(seq = %removed_address.seq_no, "REMOVE_ADDRESS"); removed_address.write(buf); sent.retransmits .get_or_create() From 2e13da69ec8bec1ef61adf9c2a27e91704e26a4c Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sat, 22 Nov 2025 16:29:10 +0100 Subject: [PATCH 35/40] use canonical and ipv4-mapped ipv6 addresses when needed otherwise we're opening paths to/from the wrong remotes and packets remotes do not match the PathData remote. --- quinn-proto/src/connection/mod.rs | 15 +++++++++++++-- quinn-proto/src/iroh_hp.rs | 8 ++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index ab3f31c75..a5edaa0d6 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -5991,8 +5991,19 @@ impl Connection { let mut path_ids = Vec::with_capacity(addresses_to_probe.len()); let mut probed_addresses = Vec::with_capacity(addresses_to_probe.len()); - for address in addresses_to_probe { - let remote: SocketAddr = address.into(); + let ipv6 = self.paths.values().any(|p| p.data.remote.is_ipv6()); + + for (ip, port) in addresses_to_probe { + // If this endpoint is an IPv6 endpoint we use IPv6 addresses for all remotes. + let remote = match ip { + IpAddr::V4(addr) if ipv6 => SocketAddr::new(addr.to_ipv6_mapped().into(), port), + IpAddr::V4(addr) => SocketAddr::new(addr.into(), port), + IpAddr::V6(_) if ipv6 => SocketAddr::new(ip, port), + IpAddr::V6(_) => { + trace!("not using IPv6 nat candidate for IPv4 socket"); + continue; + } + }; match self.open_path_ensure(remote, PathStatus::Backup, now) { Ok((path_id, path_was_known)) if !path_was_known => { path_ids.push(path_id); diff --git a/quinn-proto/src/iroh_hp.rs b/quinn-proto/src/iroh_hp.rs index 709fbb903..c5d83b577 100644 --- a/quinn-proto/src/iroh_hp.rs +++ b/quinn-proto/src/iroh_hp.rs @@ -75,10 +75,10 @@ pub(crate) struct State { /// This is set by the remote endpoint. max_local_addresses: usize, /// Candidate addresses the remote server reports as potentially reachable, to use for nat - /// traversal attempts. + /// traversal attempts. Always canonical. remote_addresses: FxHashMap, /// Candidate addresses the local client reports as potentially reachable, to use for nat - /// traversal attempts. + /// traversal attempts. Always canonical. local_addresses: FxHashMap<(IpAddr, u16), VarInt>, /// The next id to use for local addresses sent to the client next_local_addr_id: VarInt, @@ -113,7 +113,7 @@ impl State { &mut self, address: SocketAddr, ) -> Result, Error> { - let address = (address.ip(), address.port()); + let address = (address.ip().to_canonical(), address.port()); let allow_new = self.local_addresses.len() < self.max_local_addresses; let is_server = self.side.is_server(); match self.local_addresses.entry(address) { @@ -275,7 +275,7 @@ impl<'a> ClientSide<'a> { add_addr: AddAddress, ) -> Result, Error> { let AddAddress { seq_no, ip, port } = add_addr; - let address = (ip, port); + let address = (ip.to_canonical(), port); let allow_new = self.state.remote_addresses.len() < self.state.max_remote_addresses; match self.state.remote_addresses.entry(seq_no) { Entry::Occupied(mut occupied_entry) => { From e3172987ef803dd45b24fb65df927be6d72d4e12 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Sat, 22 Nov 2025 16:30:31 +0100 Subject: [PATCH 36/40] stop all path timers when all path data is dropped --- quinn-proto/src/connection/mod.rs | 6 +----- quinn-proto/src/connection/timer.rs | 9 +++++++++ 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index a5edaa0d6..93399042e 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1855,11 +1855,6 @@ impl Connection { }; path.challenges_sent.clear(); path.challenge_pending = false; - - // TODO(flub): not sure yet - self.timers - .stop(Timer::PerPath(path_id, PathTimer::LossDetection)); - debug!("new path validation failed"); if let Err(err) = self.close_path( now, @@ -1886,6 +1881,7 @@ impl Connection { PathTimer::PathAbandoned => { // The path was abandoned and 3*PTO has expired since. Clean up all // remaining state and install stateless reset token. + self.timers.stop_per_path(path_id); if let Some(loc_cid_state) = self.local_cid_state.remove(&path_id) { let (min_seq, max_seq) = loc_cid_state.active_seq(); for seq in min_seq..=max_seq { diff --git a/quinn-proto/src/connection/timer.rs b/quinn-proto/src/connection/timer.rs index 8e1ff7f36..c7e68474c 100644 --- a/quinn-proto/src/connection/timer.rs +++ b/quinn-proto/src/connection/timer.rs @@ -278,6 +278,15 @@ impl TimerTable { } } + /// Stops all per-path timers + pub(super) fn stop_per_path(&mut self, path_id: PathId) { + for timer in PathTimer::VALUES { + if let Some(e) = self.path_timers.get_mut(&path_id) { + e.stop(timer); + } + } + } + /// Get the next queued timeout pub(super) fn peek(&mut self) -> Option { // TODO: this is currently linear in the number of paths From 61c87aba64021f314d6252fc9a687a253c70a04e Mon Sep 17 00:00:00 2001 From: dignifiedquire Date: Sat, 22 Nov 2025 17:07:44 +0100 Subject: [PATCH 37/40] refactor: use constants for error codes --- quinn-proto/src/connection/mod.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 93399042e..44e1295c1 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1899,7 +1899,11 @@ impl Connection { // frame. warn!(?path_id, "missing PATH_ABANDON from peer"); // TODO(flub): What should the error code be? - self.close(now, 0u8.into(), "peer ignored PATH_ABANDON frame".into()); + self.close( + now, + TransportErrorCode::NO_ERROR.into(), + "peer ignored PATH_ABANDON frame".into(), + ); } }, } @@ -4339,7 +4343,7 @@ impl Connection { // TODO(flub): which error code? self.close( now, - 0u8.into(), + TransportErrorCode::NO_ERROR.into(), Bytes::from_static(b"last path abandoned by peer"), ); } From 4da495fe672c7967da1c7c1a75c7a4396f9d5ae8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sat, 22 Nov 2025 13:04:18 +0100 Subject: [PATCH 38/40] Write `path_challenge_retransmit` test --- quinn-proto/src/connection/mod.rs | 1 + quinn-proto/src/connection/paths.rs | 10 ++++++ quinn-proto/src/tests/mod.rs | 47 +++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 44e1295c1..1fb6f88e8 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4932,6 +4932,7 @@ impl Connection { trace!("PATH_CHALLENGE {:08x}", token); buf.write(frame::FrameType::PATH_CHALLENGE); buf.write(token); + self.stats.frame_tx.path_challenge += 1; if is_multipath_negotiated && path.challenge_pending { // queue informing the path status along with the challenge diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index b2750d867..f9c2f87f1 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -349,11 +349,21 @@ impl PathData { /// Increment the total size of sent UDP datagrams pub(super) fn inc_total_sent(&mut self, inc: u64) { self.total_sent = self.total_sent.saturating_add(inc); + trace!( + remote = %self.remote, + anti_amplification_budget = %(self.total_recvd * 3).saturating_sub(self.total_sent), + "anti amplification budget decreased" + ); } /// Increment the total size of received UDP datagrams pub(super) fn inc_total_recvd(&mut self, inc: u64) { self.total_recvd = self.total_recvd.saturating_add(inc); + trace!( + remote = %self.remote, + anti_amplification_budget = %(self.total_recvd * 3).saturating_sub(self.total_sent), + "anti amplification budget increased" + ); } #[cfg(feature = "qlog")] diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index 4b3474260..e941476a7 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -1339,6 +1339,53 @@ fn migration() { ); } +#[test] +fn path_challenge_retransmit() { + let _guard = subscribe(); + let mut pair = Pair::default(); + let (client_ch, server_ch) = pair.connect(); + pair.drive(); + + let challenges_sent_before = pair + .server_conn_mut(server_ch) + .stats() + .frame_tx + .path_challenge; + + println!("-------- client migrates --------"); + pair.client.addr = SocketAddr::new( + Ipv4Addr::new(127, 0, 0, 1).into(), + CLIENT_PORTS.lock().unwrap().next().unwrap(), + ); + // Send more than a ping to make sure we have enough anti-amplification budget to resend + let stream_id = pair.client_streams(client_ch).open(Dir::Uni).unwrap(); + let to_write = [0u8; 1000]; + let mut written = 0; + while written < 1000 { + written += pair + .client_conn_mut(client_ch) + .send_stream(stream_id) + .write(&to_write[written..]) + .unwrap(); + } + + pair.drive_client(); // This will send the stream datagram + pair.drive_server(); // This will make the server receive the stream datagram, increase its anti-amp budget, and send the first path challenge + println!("-------- client loses messages --------"); + // Have the client lose the challenge + pair.client.inbound.clear(); + + pair.drive(); + + assert_eq!( + pair.server_conn_mut(server_ch) + .stats() + .frame_tx + .path_challenge, + challenges_sent_before + 2 + ); +} + fn test_flow_control(config: TransportConfig, window_size: usize) { let _guard = subscribe(); let mut pair = Pair::new( From fe22a2bf3744353b411df26abeca1c2dcd8a4c5f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sat, 22 Nov 2025 16:49:41 +0100 Subject: [PATCH 39/40] Introduce `PathData::is_validating_path` and fix send logic --- quinn-proto/src/connection/mod.rs | 35 +++++++++++++++-------------- quinn-proto/src/connection/paths.rs | 13 ++++++++--- 2 files changed, 28 insertions(+), 20 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 1fb6f88e8..0814d8cbb 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -811,7 +811,7 @@ impl Connection { // for the path to be opened we need to send a packet on the path. Sending a challenge // guarantees this - data.challenge_pending = true; + data.send_new_challenge = true; let path = vacant_entry.insert(PathState { data, prev: None }); @@ -1591,9 +1591,10 @@ impl Connection { path_id: PathId, ) -> Option { let (prev_cid, prev_path) = self.paths.get_mut(&path_id)?.prev.as_mut()?; - if !prev_path.challenge_pending { + if !prev_path.send_new_challenge { return None; }; + prev_path.send_new_challenge = false; let token = self.rng.random(); prev_path.challenges_sent.insert(token, now); let destination = prev_path.remote; @@ -1847,14 +1848,14 @@ impl Connection { path.data = prev; } path.data.challenges_sent.clear(); - path.data.challenge_pending = false; + path.data.send_new_challenge = false; } PathTimer::PathOpen => { let Some(path) = self.path_mut(path_id) else { continue; }; path.challenges_sent.clear(); - path.challenge_pending = false; + path.send_new_challenge = false; debug!("new path validation failed"); if let Err(err) = self.close_path( now, @@ -2404,7 +2405,7 @@ impl Connection { .remove_in_flight(&info); let app_limited = self.app_limited; let path = self.path_data_mut(path_id); - if info.ack_eliciting && path.challenges_sent.is_empty() { + if info.ack_eliciting && !path.challenges_sent.is_empty() { // Only pass ACKs to the congestion controller if we are not validating the current // path, so as to ignore any ACKs from older paths still coming in. let rtt = path.rtt; @@ -4039,7 +4040,7 @@ impl Connection { self.timers .stop(Timer::PerPath(path_id, PathTimer::PathOpen)); path.data.challenges_sent.clear(); - path.data.challenge_pending = false; + path.data.send_new_challenge = false; path.data.validated = true; path.data.rtt.update( Duration::ZERO, @@ -4059,7 +4060,7 @@ impl Connection { } if let Some((_, ref mut prev)) = path.prev { prev.challenges_sent.clear(); - prev.challenge_pending = false; + prev.send_new_challenge = false; } } else { debug!(token, "ignoring invalid PATH_RESPONSE"); @@ -4655,12 +4656,12 @@ impl Connection { })); } } - new_path.challenge_pending = true; + new_path.send_new_challenge = true; let mut prev = mem::replace(path, new_path); // Don't clobber the original path if the previous one hasn't been validated yet - if !prev.validated { - prev.challenge_pending = true; + if !prev.challenges_sent.is_empty() { + prev.send_new_challenge = true; // We haven't updated the remote CID yet, this captures the remote CID we were using on // the previous path. @@ -4921,10 +4922,10 @@ impl Connection { } // PATH_CHALLENGE - if buf.remaining_mut() > 9 && space_id == SpaceId::Data && !path.validated { + if buf.remaining_mut() > 9 && space_id == SpaceId::Data { // Transmit challenges with every outgoing packet on an unvalidated path - if !path.validated { - // Generate a new challenge every time we send a new PC + if path.is_validating_path() { + // Generate a new challenge every time we send a new PATH_CHALLENGE let token = self.rng.random(); path.challenges_sent.insert(token, now); sent.non_retransmits = true; @@ -4934,13 +4935,13 @@ impl Connection { buf.write(token); self.stats.frame_tx.path_challenge += 1; - if is_multipath_negotiated && path.challenge_pending { + if is_multipath_negotiated && !path.validated && path.send_new_challenge { // queue informing the path status along with the challenge space.pending.path_status.insert(path_id); } // But only send a packet solely for that purpose at most once - path.challenge_pending = false; + path.send_new_challenge = false; // Always include an OBSERVED_ADDR frame with a PATH_CHALLENGE, regardless // of whether one has already been sent on this path. @@ -5755,11 +5756,11 @@ impl Connection { /// may need to be sent. fn can_send_1rtt(&self, path_id: PathId, max_size: usize) -> SendableFrames { let path_exclusive = self.paths.get(&path_id).is_some_and(|path| { - path.data.challenge_pending + path.data.send_new_challenge || path .prev .as_ref() - .is_some_and(|(_, path)| path.challenge_pending) + .is_some_and(|(_, path)| path.send_new_challenge) || !path.data.path_responses.is_empty() }); let other = self.streams.can_send_stream_data() diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index f9c2f87f1..207919887 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -129,8 +129,10 @@ pub(super) struct PathData { pub(super) congestion: Box, /// Pacing state pub(super) pacing: Pacer, + /// Actually sent challenges (on the wire) pub(super) challenges_sent: IntMap, - pub(super) challenge_pending: bool, + /// Whether to *immediately* trigger another PATH_CHALLENGE (via Connection::can_send) + pub(super) send_new_challenge: bool, /// Pending responses to PATH_CHALLENGE frames pub(super) path_responses: PathResponses, /// Whether we're certain the peer can both send and receive on this address @@ -226,7 +228,7 @@ impl PathData { ), congestion, challenges_sent: Default::default(), - challenge_pending: Default::default(), + send_new_challenge: false, path_responses: PathResponses::default(), validated: false, total_sent: 0, @@ -280,7 +282,7 @@ impl PathData { sending_ecn: true, congestion, challenges_sent: Default::default(), - challenge_pending: Default::default(), + send_new_challenge: false, path_responses: PathResponses::default(), validated: false, total_sent: 0, @@ -302,6 +304,11 @@ impl PathData { } } + /// Whether we're in the process of validating this path with PATH_CHALLENGEs + pub(super) fn is_validating_path(&self) -> bool { + !self.challenges_sent.is_empty() || self.send_new_challenge + } + /// Resets RTT, congestion control and MTU states. /// /// This is useful when it is known the underlying path has changed. From 7a094bd12f5ee643be3fd873d6d6eef3fafbc0e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Diva=20Mart=C3=ADnez?= Date: Sun, 23 Nov 2025 04:56:38 -0500 Subject: [PATCH 40/40] fix panic --- quinn-proto/src/connection/mod.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 0814d8cbb..2ae8a921f 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -3590,9 +3590,12 @@ impl Connection { self.stats.frame_rx.record(&frame); - if let Frame::Close(_) = frame { + if let Frame::Close(error) = frame { trace!("draining"); self.state = State::Draining; + if self.error.is_none() { + self.error = Some(error.into()); + } break; } }