feat(quinn-proto): Use RTT from path challenges, implement path challenge resending (#178)

- Adds a timer to resend another path challenge if the old one was lost
and the path isn't validated.
- The resent challenges now have different tokens. We keep track of all
of them to verify incoming path responses.
- The logic for when to send a path challenge is simplified to only send
challenges when `send_new_challenge` is set to `true` (instead of
triggering *always* when we're in path validation mode)

Author: matheus23

quinn-proto/src/connection/mod.rs

@@ -1591,6 +1591,8 @@ impl Connection {
         path_id: PathId,
     ) -> Option<Transmit> {
         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;
         };
@@ -1843,13 +1845,22 @@ 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;
                         }
                         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;
@@ -4037,6 +4048,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");
                         }
@@ -4663,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.challenges_sent.is_empty() {
+        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.
@@ -4925,48 +4938,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;
+        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;
                 }
             }
         }
@@ -5747,6 +5760,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):

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,

quinn-proto/src/tests/mod.rs

@@ -1346,43 +1346,62 @@ 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;
-
-    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.client_conn_mut(client_ch).ping();
+    pair.drive();
 
-    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();
 
     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,
-        challenges_sent_before + 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]
+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();
+
+    // 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!(
+        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"
     );
 }