From 874be5e1216abee39be05c690ae6db30d058a011 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 1/8] 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 65ba04c21..54ed83c2a 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -805,7 +805,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 }); @@ -1574,11 +1573,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, @@ -1829,14 +1826,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( @@ -2382,7 +2379,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; @@ -4005,7 +4002,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 { @@ -4013,8 +4013,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 @@ -4028,7 +4033,7 @@ impl Connection { } } if let Some((_, ref mut prev)) = path.prev { - prev.challenge = None; + prev.challenges_sent.clear(); prev.challenge_pending = false; } } else { @@ -4536,13 +4541,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. @@ -4779,16 +4782,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 dc103f707..145051ccd 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 dc4025780b2350a5846f308e24789e759db2505a 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 2/8] 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 54ed83c2a..4be16762e 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4793,6 +4793,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 145051ccd..386b6e851 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 334ab73f58843908b63a78eb6b55d47993a229a7 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 3/8] 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 4be16762e..9e78a89b6 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -805,7 +805,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 }); @@ -1571,9 +1571,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; @@ -1827,14 +1828,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, @@ -2379,7 +2380,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; @@ -4014,7 +4015,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, @@ -4034,7 +4035,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"); @@ -4541,12 +4542,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. @@ -4782,10 +4783,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; @@ -4795,13 +4796,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. @@ -5530,11 +5531,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 386b6e851..0694caff7 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 07257da266cb4de6e60fd660070ec09608d5c171 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sat, 22 Nov 2025 18:13:32 +0100 Subject: [PATCH 4/8] Make sure to send a path challenge on the previous path, actually --- quinn-proto/src/connection/mod.rs | 2 +- quinn-proto/src/tests/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 9e78a89b6..7e0438b55 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4546,7 +4546,7 @@ impl Connection { 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.challenges_sent.is_empty() { + 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. diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index e941476a7..5421d0d91 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -1382,7 +1382,7 @@ fn path_challenge_retransmit() { .stats() .frame_tx .path_challenge, - challenges_sent_before + 2 + challenges_sent_before + 3 ); } From b13b0110075883596283b5974d5523819eb774b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sun, 23 Nov 2025 11:23:19 +0100 Subject: [PATCH 5/8] Write failing tests --- quinn-proto/src/connection/mod.rs | 10 +++++ quinn-proto/src/tests/mod.rs | 62 ++++++++++++++++++------------- 2 files changed, 47 insertions(+), 25 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 7e0438b55..3a9264b17 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1571,6 +1571,8 @@ impl Connection { path_id: PathId, ) -> Option { let (prev_cid, prev_path) = self.paths.get_mut(&path_id)?.prev.as_mut()?; + // TODO (matheus23): We could use !prev_path.is_validating() here instead to + // (possibly) also re-send challenges when they get lost. if !prev_path.send_new_challenge { return None; }; @@ -5519,6 +5521,14 @@ impl Connection { self.path_data(PathId::ZERO).current_mtu() } + /// Triggers path validation on all paths + #[cfg(test)] + pub(crate) fn trigger_path_validation(&mut self) { + for path in self.paths.values_mut() { + path.data.send_new_challenge = true; + } + } + /// Whether we have 1-RTT data to send /// /// This checks for frames that can only be sent in the data space (1-RTT): diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index 5421d0d91..c5f8f0117 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -1346,31 +1346,12 @@ fn path_challenge_retransmit() { let (client_ch, server_ch) = pair.connect(); pair.drive(); - let challenges_sent_before = pair - .server_conn_mut(server_ch) - .stats() - .frame_tx - .path_challenge; + pair.client_conn_mut(client_ch).ping(); + pair.drive(); - 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!("-------- server wants path validation --------"); + pair.server_conn_mut(server_ch).trigger_path_validation(); + pair.drive_server(); // Send the path challenge println!("-------- client loses messages --------"); // Have the client lose the challenge pair.client.inbound.clear(); @@ -1382,7 +1363,38 @@ fn path_challenge_retransmit() { .stats() .frame_tx .path_challenge, - challenges_sent_before + 3 + 2, + "expected server to send two path challenges" + ); +} + +#[test] +fn path_response_retransmit() { + let _guard = subscribe(); + let mut pair = Pair::default(); + let (client_ch, server_ch) = pair.connect(); + pair.drive(); + + pair.client_conn_mut(client_ch).ping(); + pair.drive(); + + println!("-------- server wants path validation --------"); + pair.server_conn_mut(server_ch).trigger_path_validation(); + pair.drive_server(); // Send the path challenge + pair.drive_client(); // Send the path response + println!("-------- server loses messages --------"); + // Have the server lose the path response + pair.server.inbound.clear(); + + pair.drive(); + + assert_eq!( + pair.client_conn_mut(server_ch) + .stats() + .frame_tx + .path_response, + 2, + "expected client to send two path challenges" ); } From f8da3052fab12e310c55ad0dc2512c6594001ec4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sun, 23 Nov 2025 11:58:10 +0100 Subject: [PATCH 6/8] Add a `PathChallengeLost` path timer for resending path challenges --- quinn-proto/src/connection/mod.rs | 16 ++++++++++++++++ quinn-proto/src/connection/timer.rs | 17 ++++++++++------- quinn-proto/src/tests/mod.rs | 29 ++++++++++++++++++----------- 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 3a9264b17..b6727915e 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -1825,6 +1825,8 @@ impl Connection { let Some(path) = self.paths.get_mut(&path_id) else { continue; }; + self.timers + .stop(Timer::PerPath(path_id, PathTimer::PathChallengeLost)); debug!("path validation failed"); if let Some((_, prev)) = path.prev.take() { path.data = prev; @@ -1832,6 +1834,13 @@ impl Connection { path.data.challenges_sent.clear(); path.data.send_new_challenge = false; } + PathTimer::PathChallengeLost => { + let Some(path) = self.paths.get_mut(&path_id) else { + continue; + }; + trace!("path challenge deemed lost"); + path.data.send_new_challenge = true; + } PathTimer::PathOpen => { let Some(path) = self.path_mut(path_id) else { continue; @@ -4011,6 +4020,8 @@ impl Connection { } else if let Some(&challenge_sent) = path.data.challenges_sent.get(&token) { self.timers .stop(Timer::PerPath(path_id, PathTimer::PathValidation)); + self.timers + .stop(Timer::PerPath(path_id, PathTimer::PathChallengeLost)); if !path.data.validated { trace!("new path validated"); } @@ -4797,6 +4808,11 @@ impl Connection { buf.write(frame::FrameType::PATH_CHALLENGE); buf.write(token); self.stats.frame_tx.path_challenge += 1; + let pto = self.ack_frequency.max_ack_delay_for_pto() + path.rtt.pto_base(); + self.timers.set( + Timer::PerPath(path_id, PathTimer::PathChallengeLost), + now + pto, + ); if is_multipath_negotiated && !path.validated && path.send_new_challenge { // queue informing the path status along with the challenge diff --git a/quinn-proto/src/connection/timer.rs b/quinn-proto/src/connection/timer.rs index 8e1ff7f36..fba714d81 100644 --- a/quinn-proto/src/connection/timer.rs +++ b/quinn-proto/src/connection/timer.rs @@ -44,25 +44,28 @@ pub(crate) enum PathTimer { PathIdle = 1, /// When to give up on validating a new path from RFC9000 migration PathValidation = 2, + /// When to resend a path challenge deemed lost + PathChallengeLost = 3, /// When to give up on validating a new (multi)path - PathOpen = 3, + PathOpen = 4, /// When to send a `PING` frame to keep the path alive - PathKeepAlive = 4, + PathKeepAlive = 5, /// When pacing will allow us to send a packet - Pacing = 5, + Pacing = 6, /// When to send an immediate ACK if there are unacked ack-eliciting packets of the peer - MaxAckDelay = 6, + MaxAckDelay = 7, /// When to clean up state for an abandoned path - PathAbandoned = 7, + PathAbandoned = 8, /// When the peer fails to confirm abandoning the path - PathNotAbandoned = 8, + PathNotAbandoned = 9, } impl PathTimer { - const VALUES: [Self; 9] = [ + const VALUES: [Self; 10] = [ Self::LossDetection, Self::PathIdle, Self::PathValidation, + Self::PathChallengeLost, Self::PathOpen, Self::PathKeepAlive, Self::Pacing, diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs index c5f8f0117..c9fe23c9a 100644 --- a/quinn-proto/src/tests/mod.rs +++ b/quinn-proto/src/tests/mod.rs @@ -1358,14 +1358,17 @@ fn path_challenge_retransmit() { pair.drive(); + let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx; + let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx; + assert_eq!( - pair.server_conn_mut(server_ch) - .stats() - .frame_tx - .path_challenge, - 2, + server_tx.path_challenge, 2, "expected server to send two path challenges" ); + assert_eq!( + client_tx.path_response, 1, + "expected client to send one path response" + ); } #[test] @@ -1386,15 +1389,19 @@ fn path_response_retransmit() { // Have the server lose the path response pair.server.inbound.clear(); + // The server should decide to re-send the path challenge pair.drive(); + let client_tx = pair.client_conn_mut(client_ch).stats().frame_tx; + let server_tx = pair.server_conn_mut(server_ch).stats().frame_tx; + assert_eq!( - pair.client_conn_mut(server_ch) - .stats() - .frame_tx - .path_response, - 2, - "expected client to send two path challenges" + server_tx.path_challenge, 2, + "expected server to send two path challenges" + ); + assert_eq!( + client_tx.path_response, 2, + "expected client to send two path responses" ); } From 8fc082b8d8446939d9cb731735196814f2be2b37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sun, 23 Nov 2025 12:02:30 +0100 Subject: [PATCH 7/8] Less brittle logic to decide whether to send PATH_CHALLENGE --- quinn-proto/src/connection/mod.rs | 79 ++++++++++++++--------------- quinn-proto/src/connection/paths.rs | 5 -- 2 files changed, 37 insertions(+), 47 deletions(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index b6727915e..cdb3a2bf4 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4796,53 +4796,48 @@ impl Connection { } // PATH_CHALLENGE - if buf.remaining_mut() > 9 && space_id == SpaceId::Data { - // Transmit challenges with every outgoing packet on an unvalidated path - 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; - sent.requires_padding = true; - trace!("PATH_CHALLENGE {:08x}", token); - buf.write(frame::FrameType::PATH_CHALLENGE); - buf.write(token); - self.stats.frame_tx.path_challenge += 1; - let pto = self.ack_frequency.max_ack_delay_for_pto() + path.rtt.pto_base(); - self.timers.set( - Timer::PerPath(path_id, PathTimer::PathChallengeLost), - now + pto, - ); + if buf.remaining_mut() > 9 && space_id == SpaceId::Data && path.send_new_challenge { + path.send_new_challenge = false; - 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); - } + // 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; + sent.requires_padding = true; + trace!("PATH_CHALLENGE {:08x}", token); + buf.write(frame::FrameType::PATH_CHALLENGE); + buf.write(token); + self.stats.frame_tx.path_challenge += 1; + let pto = self.ack_frequency.max_ack_delay_for_pto() + path.rtt.pto_base(); + self.timers.set( + Timer::PerPath(path_id, PathTimer::PathChallengeLost), + now + pto, + ); - // But only send a packet solely for that purpose at most once - path.send_new_challenge = false; + 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); + } - // Always include an OBSERVED_ADDR frame with a PATH_CHALLENGE, regardless - // of whether one has already been sent on this path. - if space_id == SpaceId::Data - && self - .config - .address_discovery_role - .should_report(&self.peer_params.address_discovery_role) - { - let frame = - frame::ObservedAddr::new(path.remote, self.next_observed_addr_seq_no); - if buf.remaining_mut() > frame.size() { - frame.write(buf); + // Always include an OBSERVED_ADDR frame with a PATH_CHALLENGE, regardless + // of whether one has already been sent on this path. + if space_id == SpaceId::Data + && self + .config + .address_discovery_role + .should_report(&self.peer_params.address_discovery_role) + { + let frame = frame::ObservedAddr::new(path.remote, self.next_observed_addr_seq_no); + if buf.remaining_mut() > frame.size() { + frame.write(buf); - self.next_observed_addr_seq_no = - self.next_observed_addr_seq_no.saturating_add(1u8); - path.observed_addr_sent = true; + self.next_observed_addr_seq_no = + self.next_observed_addr_seq_no.saturating_add(1u8); + path.observed_addr_sent = true; - self.stats.frame_tx.observed_addr += 1; - sent.retransmits.get_or_create().observed_addr = true; - space.pending.observed_addr = false; - } + self.stats.frame_tx.observed_addr += 1; + sent.retransmits.get_or_create().observed_addr = true; + space.pending.observed_addr = false; } } } diff --git a/quinn-proto/src/connection/paths.rs b/quinn-proto/src/connection/paths.rs index 0694caff7..106d002cb 100644 --- a/quinn-proto/src/connection/paths.rs +++ b/quinn-proto/src/connection/paths.rs @@ -304,11 +304,6 @@ 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 9f60bb7babeaca22561dfb4d0b0f16758008ece7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philipp=20Kr=C3=BCger?= Date: Sun, 23 Nov 2025 12:21:18 +0100 Subject: [PATCH 8/8] Fix merge --- quinn-proto/src/connection/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs index 61e70cf80..3fffa33c2 100644 --- a/quinn-proto/src/connection/mod.rs +++ b/quinn-proto/src/connection/mod.rs @@ -4676,7 +4676,7 @@ impl Connection { 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.is_validating() { + if !prev.is_validating_path() { 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.