use PATH_CHALLENGE for hole punching

quinn-proto/src/cid_queue.rs

@@ -125,8 +125,6 @@ impl CidQueue {
     ///
     /// If there's no more CIDs in the ready set, this will return None.
     /// CIDs marked as reserved will be skipped when the active one advances.
-    // TODO(@divma): remove
-    #[cfg(test)]
     pub(crate) fn next_reserved(&mut self) -> Option<ConnectionId> {
         let (i, cid_data) = self.iter_from_reserved().nth(1)?;
 

quinn-proto/src/connection/mod.rs

@@ -892,26 +892,6 @@ impl Connection {
         max_datagrams: usize,
         buf: &mut Vec<u8>,
     ) -> Option<Transmit> {
-        if let Some(probing) = self
-            .iroh_hp
-            .server_side_mut()
-            .ok()
-            .and_then(iroh_hp::ServerState::next_probe)
-        {
-            let destination = probing.remote();
-            trace!(%destination, "RAND_DATA packet");
-            let token: u64 = self.rng.random();
-            buf.put_u64(token);
-            probing.finish(token);
-            return Some(Transmit {
-                destination,
-                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,
@@ -1118,6 +1098,42 @@ impl Connection {
                     break;
                 }
 
+                if transmit.is_empty() {
+                    // Nothing to send on this path, and nothing yet built; try to send hole punching
+                    // path challenges.
+                    if let Some(probing) = self
+                        .iroh_hp
+                        .server_side_mut()
+                        .ok()
+                        .and_then(iroh_hp::ServerState::next_probe)
+                    {
+                        if let Some(new_cid) = self
+                            .rem_cids
+                            .get_mut(&path_id)
+                            .and_then(CidQueue::next_reserved)
+                        {
+                            let destination = probing.remote();
+                            let token: u64 = self.rng.random();
+                            let challenge = frame::PathChallenge(token);
+                            trace!(%destination, cid=%new_cid, %challenge, "Nat traversal: PATH_CHALLENGE packet");
+                            buf.write(challenge);
+                            QlogSentPacket::default().frame(&Frame::PathChallenge(challenge));
+                            self.stats.frame_tx.path_challenge += 1;
+
+                            // TODO(@divma): padding
+                            // Remember the token sent to this remote
+                            probing.finish(token);
+                            return Some(Transmit {
+                                destination,
+                                ecn: None,
+                                size: 8,
+                                segment_size: None,
+                                src_ip: None,
+                            });
+                        }
+                    }
+                }
+
                 match self.paths.keys().find(|&&next| next > path_id) {
                     Some(next_path_id) => {
                         // See if this next path can send anything.
@@ -1319,12 +1335,29 @@ impl Connection {
 
             // Send an off-path PATH_RESPONSE. Prioritized over on-path data to ensure that
             // path validation can occur while the link is saturated.
-            if space_id == SpaceId::Data && builder.buf.num_datagrams() == 1 {
+            if space_id == SpaceId::Data && builder.buf.is_empty() {
                 let path = self.path_data_mut(path_id);
                 if let Some((token, remote)) = path.path_responses.pop_off_path(path.remote) {
-                    // TODO(flub): We need to use the right CID!  We shouldn't use the same
-                    //    CID as the current active one for the path.  Though see also
-                    //    https://github.com/quinn-rs/quinn/issues/2184
+                    let mut builder = if let Some(fresh_cid) = self
+                        .rem_cids
+                        .get_mut(&path_id)
+                        .and_then(CidQueue::next_reserved)
+                    {
+                        PacketBuilder::new(
+                            now,
+                            space_id,
+                            path_id,
+                            fresh_cid,
+                            &mut transmit,
+                            can_send.other,
+                            self,
+                            &mut qlog,
+                        )?
+                    } else {
+                        // TODO(@divma): keep the old logic. We would need to push the challenge
+                        // back, but we have lost the packet number.
+                        builder
+                    };
                     let response = frame::PathResponse(token);
                     trace!(%response, "(off-path)");
                     builder.frame_space_mut().write(response);
@@ -4262,6 +4295,9 @@ impl Connection {
                                 self.ping_path(path_id).ok();
                             }
                         }
+                    } else if self.iroh_hp.client_side().is_ok() {
+                        // TODO(@divma): temp log. This might be too much
+                        debug!("Potential Nat traversal PATH_CHALLENGE received");
                     }
                 }
                 Frame::PathResponse(response) => {
@@ -4270,44 +4306,19 @@ impl Connection {
                         .get_mut(&path_id)
                         .expect("payload is processed only after the path becomes known");
 
-                    match path.data.challenges_sent.get(&response.0) {
-                        // Response to an on-path PathChallenge
-                        Some(info) if info.remote == remote && path.data.remote == remote => {
-                            let sent_instant = info.sent_instant;
-                            // TODO(@divma): reset timers using the remaining off-path challenges
-                            self.timers.stop(
-                                Timer::PerPath(path_id, PathTimer::PathValidation),
-                                self.qlog.with_time(now),
-                            );
-                            self.timers.stop(
-                                Timer::PerPath(path_id, PathTimer::PathChallengeLost),
-                                self.qlog.with_time(now),
-                            );
-                            if !path.data.validated {
-                                trace!("new path validated");
-                            }
-                            self.timers.stop(
-                                Timer::PerPath(path_id, PathTimer::PathOpen),
-                                self.qlog.with_time(now),
-                            );
-                            // Clear any other on-path sent challenge.
-                            path.data
-                                .challenges_sent
-                                .retain(|_token, info| info.remote != remote);
-                            path.data.send_new_challenge = false;
-                            path.data.validated = true;
-
-                            // This RTT can only be used for the initial RTT, not as a normal
-                            // sample: https://www.rfc-editor.org/rfc/rfc9002#section-6.2.2-2.
-                            let rtt = now.saturating_duration_since(sent_instant);
-                            path.data.rtt.reset_initial_rtt(rtt);
-
-                            self.events
-                                .push_back(Event::Path(PathEvent::Opened { id: path_id }));
-                            // mark the path as open from the application perspective now that Opened
-                            // event has been queued
-                            if !std::mem::replace(&mut path.data.open, true) {
-                                trace!("path opened");
+                    use paths::OnPathResponseReceived::*;
+                    match path.data.on_path_response_received(now, response.0, remote) {
+                        OnPath { was_open } => {
+                            use PathTimer::*;
+                            let qlog = self.qlog.with_time(now);
+
+                            self.timers
+                                .stop(Timer::PerPath(path_id, PathValidation), qlog.clone());
+                            self.timers
+                                .stop(Timer::PerPath(path_id, PathOpen), qlog.clone());
+                            if !was_open {
+                                self.events
+                                    .push_back(Event::Path(PathEvent::Opened { id: path_id }));
                                 if let Some(observed) = path.data.last_observed_addr_report.as_ref()
                                 {
                                     self.events.push_back(Event::Path(PathEvent::ObservedAddr {
@@ -4320,20 +4331,44 @@ impl Connection {
                                 prev.challenges_sent.clear();
                                 prev.send_new_challenge = false;
                             }
+
+                            match path.data.earliest_expiring_challenge() {
+                                Some(new_expire_time) => {
+                                    let expires = new_expire_time
+                                        + self.ack_frequency.max_ack_delay_for_pto();
+                                    self.timers.set(
+                                        Timer::PerPath(path_id, PathChallengeLost),
+                                        expires,
+                                        qlog,
+                                    )
+                                }
+                                None => self
+                                    .timers
+                                    .stop(Timer::PerPath(path_id, PathChallengeLost), qlog),
+                            }
                         }
-                        // Response to an off-path PathChallenge
-                        Some(info) if info.remote == remote => {
+                        OffPath => {
                             debug!("Response to off-path PathChallenge!");
-                            path.data
-                                .challenges_sent
-                                .retain(|_token, info| info.remote != remote);
+                            match path.data.earliest_expiring_challenge() {
+                                Some(new_expire_time) => {
+                                    let expires = new_expire_time
+                                        + self.ack_frequency.max_ack_delay_for_pto();
+                                    self.timers.set(
+                                        Timer::PerPath(path_id, PathTimer::PathChallengeLost),
+                                        expires,
+                                        self.qlog.with_time(now),
+                                    )
+                                }
+                                None => self.timers.stop(
+                                    Timer::PerPath(path_id, PathTimer::PathChallengeLost),
+                                    self.qlog.with_time(now),
+                                ),
+                            }
                         }
-                        // Response to a PathChallenge we recognize, but from an invalid remote
-                        Some(info) => {
-                            debug!(%response, from=%remote, expected=%info.remote, "ignoring invalid PATH_RESPONSE")
+                        Unknown => debug!(%response, "ignoring invalid PATH_RESPONSE"),
+                        Invalid { expected } => {
+                            debug!(%response, from=%remote, %expected, "ignoring invalid PATH_RESPONSE")
                         }
-                        // Response to an unknown PathChallenge
-                        None => debug!(%response, "ignoring invalid PATH_RESPONSE"),
                     }
                 }
                 Frame::MaxData(bytes) => {

quinn-proto/src/connection/paths.rs

@@ -375,6 +375,64 @@ impl PathData {
         }
     }
 
+    /// The earliest time at which a sent challenge is considered lost.
+    pub(super) fn earliest_expiring_challenge(&self) -> Option<Instant> {
+        if self.challenges_sent.is_empty() {
+            return None;
+        }
+        let pto = self.rtt.pto_base();
+        self.challenges_sent
+            .values()
+            .map(|info| info.sent_instant)
+            .min()
+            .map(|sent_instant| sent_instant + pto)
+    }
+
+    /// Handle a receiving a PATH_RESPONSE.
+    pub(super) fn on_path_response_received(
+        &mut self,
+        now: Instant,
+        token: u64,
+        remote: SocketAddr,
+    ) -> OnPathResponseReceived {
+        match self.challenges_sent.get(&token) {
+            // Response to an on-path PathChallenge
+            Some(info) if info.remote == remote && self.remote == remote => {
+                let sent_instant = info.sent_instant;
+                if !std::mem::replace(&mut self.validated, true) {
+                    trace!("new path validated");
+                }
+                // Clear any other on-path sent challenge.
+                self.challenges_sent
+                    .retain(|_token, info| info.remote != remote);
+
+                self.send_new_challenge = false;
+
+                // This RTT can only be used for the initial RTT, not as a normal
+                // sample: https://www.rfc-editor.org/rfc/rfc9002#section-6.2.2-2.
+                let rtt = now.saturating_duration_since(sent_instant);
+                self.rtt.reset_initial_rtt(rtt);
+
+                // mark the path as open from the application perspective now that Opened
+                // event has been queued
+                let was_open = std::mem::replace(&mut self.open, true);
+                OnPathResponseReceived::OnPath { was_open }
+            }
+            // Response to an off-path PathChallenge
+            Some(info) if info.remote == remote => {
+                self.challenges_sent
+                    .retain(|_token, info| info.remote != remote);
+                OnPathResponseReceived::OffPath
+            }
+            // Response to a PathChallenge we recognize, but from an invalid remote
+            Some(info) => OnPathResponseReceived::Invalid {
+                expected: info.remote,
+            },
+            // Response to an unknown PathChallenge
+            None => OnPathResponseReceived::Unknown,
+        }
+    }
+
     #[cfg(feature = "qlog")]
     pub(super) fn qlog_recovery_metrics(
         &mut self,
@@ -459,6 +517,20 @@ impl PathData {
     }
 }
 
+pub(super) enum OnPathResponseReceived {
+    /// This response validates the path on it's current remote address.
+    OnPath { was_open: bool },
+    /// This response is valid, but it's for a remote other than the path's current remote address.
+    OffPath,
+    /// The received token is unknown.
+    Unknown,
+    /// The response is invalid.
+    Invalid {
+        /// The remote that was expected for this token
+        expected: SocketAddr,
+    },
+}
+
 /// Congestion metrics as described in [`recovery_metrics_updated`].
 ///
 /// [`recovery_metrics_updated`]: https://datatracker.ietf.org/doc/html/draft-ietf-quic-qlog-quic-events.html#name-recovery_metrics_updated

quinn-proto/src/connection/qlog.rs

@@ -369,6 +369,7 @@ impl QlogSink {
 }
 
 /// A [`QlogSink`] with a `now` timestamp.
+#[derive(Clone)]
 pub(super) struct QlogSinkWithTime<'a> {
     #[cfg(feature = "qlog")]
     sink: &'a QlogSink,