ack-frequencies is allowed together with multipath (#55)

flub · web-flow · commit ff58f71b9ce3 · 2025-04-15T10:42:19.000+02:00
This moves the immediate-ack state to the path, I think this is
multipath-compatible as much as normal acks are.
diff --git a/quinn-proto/src/connection/mod.rs b/quinn-proto/src/connection/mod.rs
@@ -571,7 +571,9 @@ impl Connection {
             // Whether the next packet will contain ack-eliciting frames.
             let mut ack_eliciting = !self.spaces[space_id].pending.is_empty(&self.streams)
                 || self.spaces[space_id].for_path(path_id).ping_pending
-                || self.spaces[space_id].immediate_ack_pending;
+                || self.spaces[space_id]
+                    .for_path(path_id)
+                    .immediate_ack_pending;
             if space_id == SpaceId::Data {
                 let pn = self.spaces[SpaceId::Data]
                     .for_path(path_id)
@@ -821,7 +823,7 @@ impl Connection {
                     .buf
                     .segment_size()
                     .saturating_sub(self.predict_1rtt_overhead(pn, path_id));
-                let can_send = self.space_can_send(space_id, frame_space_1rtt);
+                let can_send = self.space_can_send(space_id, path_id, frame_space_1rtt);
                 debug_assert!(
                     !(sent_frames.is_ack_only(&self.streams)
                         && !can_send.acks
@@ -911,7 +913,7 @@ impl Connection {
                                 .segment_size()
                                 .saturating_sub(self.predict_1rtt_overhead(pn, path_id));
                             if self
-                                .space_can_send(next_space_id, frame_space_1rtt)
+                                .space_can_send(next_space_id, path_id, frame_space_1rtt)
                                 .is_empty()
                             {
                                 break;
@@ -1043,7 +1045,7 @@ impl Connection {
             .saturating_sub(self.predict_1rtt_overhead(pn, path_id));
         let mut space_id = current_space_id;
         loop {
-            let can_send = self.space_can_send(space_id, frame_space_1rtt);
+            let can_send = self.space_can_send(space_id, path_id, frame_space_1rtt);
             if !can_send.is_empty() || (close && self.spaces[space_id].crypto.is_some()) {
                 return Some(space_id);
             }
@@ -1113,7 +1115,12 @@ impl Connection {
     }
 
     /// Indicate what types of frames are ready to send for the given space
-    fn space_can_send(&self, space_id: SpaceId, frame_space_1rtt: usize) -> SendableFrames {
+    fn space_can_send(
+        &self,
+        space_id: SpaceId,
+        path_id: PathId,
+        frame_space_1rtt: usize,
+    ) -> SendableFrames {
         if self.spaces[space_id].crypto.is_none()
             && (space_id != SpaceId::Data
                 || self.zero_rtt_crypto.is_none()
@@ -1122,8 +1129,7 @@ impl Connection {
             // No keys available for this space
             return SendableFrames::empty();
         }
-        let space = &self.spaces[space_id];
-        let mut can_send = space.can_send(&self.streams, space.immediate_ack_pending);
+        let mut can_send = self.spaces[space_id].can_send(path_id, &self.streams);
         if space_id == SpaceId::Data {
             can_send.other |= self.can_send_1rtt(frame_space_1rtt);
         }
@@ -3021,7 +3027,7 @@ impl Connection {
                         // PATH_CHALLENGE on active path, possible off-path packet forwarding
                         // attack. Send a non-probing packet to recover the active path.
                         match self.peer_supports_ack_frequency() {
-                            true => self.immediate_ack(),
+                            true => self.immediate_ack(path_id),
                             false => self.ping(),
                         }
                     }
@@ -3523,7 +3529,7 @@ impl Connection {
         }
 
         // IMMEDIATE_ACK
-        if mem::replace(&mut space.immediate_ack_pending, false) {
+        if mem::replace(&mut space.for_path(path_id).immediate_ack_pending, false) {
             trace!("IMMEDIATE_ACK");
             buf.write(frame::FrameType::IMMEDIATE_ACK);
             sent.non_retransmits = true;
@@ -4029,8 +4035,10 @@ impl Connection {
     ///
     /// According to the spec, this will result in an error if the remote endpoint does not support
     /// the Acknowledgement Frequency extension
-    pub(crate) fn immediate_ack(&mut self) {
-        self.spaces[self.highest_space].immediate_ack_pending = true;
+    pub(crate) fn immediate_ack(&mut self, path_id: PathId) {
+        self.spaces[self.highest_space]
+            .for_path(path_id)
+            .immediate_ack_pending = true;
     }
 
     /// Decodes a packet, returning its decrypted payload, so it can be inspected in tests
diff --git a/quinn-proto/src/connection/spaces.rs b/quinn-proto/src/connection/spaces.rs
@@ -31,8 +31,6 @@ pub(super) struct PacketSpace {
     /// Current offset of outgoing cryptographic handshake stream
     pub(super) crypto_offset: u64,
 
-    pub(super) immediate_ack_pending: bool,
-
     /// Multipath packet number spaces
     ///
     /// Each [`PathId`] has it's own [`PacketNumberSpace`].  Only the [`SpaceId::Data`] can
@@ -52,7 +50,6 @@ impl PacketSpace {
             pending_acks: PendingAcks::new(),
             crypto_stream: Assembler::new(),
             crypto_offset: 0,
-            immediate_ack_pending: false,
             number_spaces: BTreeMap::from([(PathId(0), number_space_0)]),
         }
     }
@@ -68,7 +65,6 @@ impl PacketSpace {
             pending_acks: PendingAcks::new(),
             crypto_stream: Assembler::new(),
             crypto_offset: 0,
-            immediate_ack_pending: false,
             number_spaces: BTreeMap::from([(PathId(0), number_space_0)]),
         }
     }
@@ -125,7 +121,7 @@ impl PacketSpace {
         if request_immediate_ack {
             // The probe should be ACKed without delay (should only be used in the Data space and
             // when the peer supports the acknowledgement frequency extension)
-            self.immediate_ack_pending = true;
+            self.for_path(path_id).immediate_ack_pending = true;
         }
 
         // Retransmit the data of the oldest in-flight packet
@@ -154,19 +150,20 @@ impl PacketSpace {
         // TODO(flub): Sending a ping on all paths is wasteful, but we also need per-path
         //   pings so doing this is easier for now.  Maybe later introduce a
         //   connection-level ping again.
-        if !self.immediate_ack_pending {
+        if !self.for_path(path_id).immediate_ack_pending {
             self.number_spaces
                 .values_mut()
                 .for_each(|s| s.ping_pending = true);
         }
     }
 
     /// Whether there is anything to send.
-    pub(super) fn can_send(
-        &self,
-        streams: &StreamsState,
-        immediate_ack_pending: bool,
-    ) -> SendableFrames {
+    pub(super) fn can_send(&self, path_id: PathId, streams: &StreamsState) -> SendableFrames {
+        let immediate_ack_pending = self
+            .number_spaces
+            .get(&path_id)
+            .map(|pns| pns.immediate_ack_pending)
+            .unwrap_or_default();
         let acks = self.pending_acks.can_send();
         let other = !self.pending.is_empty(streams)
             || self
@@ -230,7 +227,10 @@ pub(super) struct PacketNumberSpace {
     pub(super) in_flight: u64,
     /// Number of packets sent in the current key phase
     pub(super) sent_with_keys: u64,
+    /// A PING frame needs to be sent on this path
     pub(super) ping_pending: bool,
+    /// An IMMEDIATE_ACK (draft-ietf-quic-ack-frequency) frame needs to be sent on this path
+    pub(super) immediate_ack_pending: bool,
 
     //
     // Loss Detection
@@ -272,6 +272,7 @@ impl PacketNumberSpace {
             in_flight: 0,
             sent_with_keys: 0,
             ping_pending: false,
+            immediate_ack_pending: false,
             pto_count: 0,
             time_of_last_ack_eliciting_packet: None,
             loss_time: None,
@@ -298,6 +299,7 @@ impl PacketNumberSpace {
             in_flight: 0,
             sent_with_keys: 0,
             ping_pending: false,
+            immediate_ack_pending: false,
             pto_count: 0,
             time_of_last_ack_eliciting_packet: None,
             loss_time: None,
@@ -325,6 +327,7 @@ impl PacketNumberSpace {
             in_flight: 0,
             sent_with_keys: 0,
             ping_pending: false,
+            immediate_ack_pending: false,
             pto_count: 0,
             time_of_last_ack_eliciting_packet: None,
             loss_time: None,
diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs
@@ -2601,7 +2601,7 @@ fn immediate_ack_triggers_ack() {
 
     let acks_after_connect = pair.client_conn_mut(client_ch).stats().frame_rx.acks;
 
-    pair.client_conn_mut(client_ch).immediate_ack();
+    pair.client_conn_mut(client_ch).immediate_ack(PathId(0));
     pair.drive_client(); // Send immediate ack
     pair.drive_server(); // Process immediate ack
     pair.drive_client(); // Give the client a chance to process the ack
