Report ACK delay to the peer

This change enables reporting ack delay to the peer, which will give the peer
a better estimate about actual path latency. The actual delay will later on be be
be determined between the time we first receive a packet and an event
(eg another packet or a timer) unblocked the ACK. This part is not yet implemented,
so the delay is 0 for now.

To enable sending ACK delay information, the default TransportParameters
has been changed to use a non 0 max ack delay value (since otherwise that
value will be picked up).

This also fixes a tiny bug where ack_delay wasn't considered in the path for
a RTT of 0ns.

quinn-proto/src/connection/mod.rs

@@ -1451,7 +1451,7 @@ where
             }
         }
         let space = &mut self.spaces[space_id];
-        space.pending_acks.insert_one(packet);
+        space.pending_acks.insert_one(packet, now);
         if packet >= space.rx_packet {
             space.rx_packet = packet;
             // Update outgoing spin bit, inverting iff we're the client
@@ -2599,18 +2599,8 @@ where
         }
 
         // ACK
-        // 0-RTT packets must never carry acks (which would have to be of handshake packets)
         if !space.pending_acks.ranges().is_empty() {
-            debug_assert!(space.crypto.is_some(), "tried to send ACK in 0-RTT");
-            trace!("ACK");
-            let ecn = if self.receiving_ecn {
-                Some(&space.ecn_counters)
-            } else {
-                None
-            };
-            sent.acks = space.pending_acks.ranges().clone();
-            frame::Ack::encode(0, &sent.acks, ecn, buf);
-            self.stats.frame_tx.acks += 1;
+            Self::populate_acks(self.receiving_ecn, &mut sent, space, buf, &mut self.stats);
         }
 
         // PATH_CHALLENGE
@@ -2747,6 +2737,40 @@ where
         sent
     }
 
+    /// Write pending ACKs into a buffer
+    ///
+    /// This method assumes ACKs are pending, and should only be called if
+    /// `!PendingAcks::ranges().is_empty()` returns `true`.
+    fn populate_acks(
+        receiving_ecn: bool,
+        sent: &mut SentFrames,
+        space: &mut PacketSpace<S>,
+        buf: &mut Vec<u8>,
+        stats: &mut ConnectionStats,
+    ) {
+        debug_assert!(!space.pending_acks.ranges().is_empty());
+
+        // 0-RTT packets must never carry acks (which would have to be of handshake packets)
+        debug_assert!(space.crypto.is_some(), "tried to send ACK in 0-RTT");
+        let ecn = if receiving_ecn {
+            Some(&space.ecn_counters)
+        } else {
+            None
+        };
+        sent.acks = space.pending_acks.ranges().clone();
+
+        let delay_micros = space.pending_acks.ack_delay().as_micros() as u64;
+
+        // TODO: This should come frome `TransportConfig` if that gets configurable
+        let ack_delay_exp = TransportParameters::default().ack_delay_exponent;
+        let delay = delay_micros >> ack_delay_exp.into_inner();
+
+        trace!("ACK {:?}, Delay = {}us", sent.acks, delay);
+
+        frame::Ack::encode(delay as _, &sent.acks, ecn, buf);
+        stats.frame_tx.acks += 1;
+    }
+
     fn close_common(&mut self) {
         trace!("connection closed");
         for &timer in &Timer::VALUES {

quinn-proto/src/connection/paths.rs

@@ -113,7 +113,7 @@ impl RttEstimator {
         self.min = cmp::min(self.min, self.latest);
         // Based on RFC6298.
         if let Some(smoothed) = self.smoothed {
-            let adjusted_rtt = if self.min + ack_delay < self.latest {
+            let adjusted_rtt = if self.min + ack_delay <= self.latest {
                 self.latest - ack_delay
             } else {
                 self.latest

quinn-proto/src/connection/spaces.rs

@@ -3,7 +3,7 @@ use std::{
     collections::{BTreeMap, VecDeque},
     mem,
     ops::{Index, IndexMut},
-    time::Instant,
+    time::{Duration, Instant},
 };
 
 use fxhash::FxHashSet;
@@ -437,6 +437,12 @@ impl SendableFrames {
 pub(crate) struct PendingAcks {
     permit_ack_only: bool,
     ranges: ArrayRangeSet,
+    /// This value will be used for calculating ACK delay once it is implemented
+    ///
+    /// ACK delay will be the delay between when a packet arrived (`latest_incoming`)
+    /// and between it will be allowed to be acknowledged (`can_send() == true`).
+    latest_incoming: Option<Instant>,
+    ack_delay: Duration,
 }
 
 impl PendingAcks {
@@ -445,6 +451,11 @@ impl PendingAcks {
         self.permit_ack_only && !self.ranges.is_empty()
     }
 
+    /// Returns the duration the acknowledgement of the latest incoming packet has been delayed
+    pub fn ack_delay(&self) -> Duration {
+        self.ack_delay
+    }
+
     /// Should be called whenever an ACK eliciting frame was received
     ///
     /// This requires sending new outgoing ACKs
@@ -466,8 +477,10 @@ impl PendingAcks {
     }
 
     /// Insert one packet that needs to be acknowledged
-    pub fn insert_one(&mut self, packet: u64) {
+    pub fn insert_one(&mut self, packet: u64, now: Instant) {
         self.ranges.insert_one(packet);
+        self.latest_incoming = Some(now);
+
         if self.ranges.len() > MAX_ACK_BLOCKS {
             self.ranges.pop_min();
         }

quinn-proto/src/tests/mod.rs

@@ -869,7 +869,7 @@ fn instant_close_2() {
 #[test]
 fn idle_timeout() {
     let _guard = subscribe();
-    const IDLE_TIMEOUT: Duration = Duration::from_millis(10);
+    const IDLE_TIMEOUT: Duration = Duration::from_millis(100);
     let server = ServerConfig {
         transport: Arc::new(TransportConfig {
             max_idle_timeout: Some(IDLE_TIMEOUT),

quinn-proto/src/tests/util.rs

@@ -260,10 +260,10 @@ impl TestEndpoint {
         }
 
         while self.inbound.front().map_or(false, |x| x.0 <= now) {
-            let (_, ecn, packet) = self.inbound.pop_front().unwrap();
+            let (recv_time, ecn, packet) = self.inbound.pop_front().unwrap();
             if let Some((ch, event)) =
                 self.endpoint
-                    .handle(now, remote, None, ecn, packet.as_slice().into())
+                    .handle(recv_time, remote, None, ecn, packet.as_slice().into())
             {
                 match event {
                     DatagramEvent::NewConnection(conn) => {

quinn-proto/src/transport_parameters.rs

@@ -139,7 +139,6 @@ impl TransportParameters {
                     .try_into()
                     .expect("setter guarantees this is in-bounds")
             }),
-            max_ack_delay: 0u32.into(),
             disable_active_migration: server_config.map_or(false, |c| !c.migration),
             active_connection_id_limit: if cid_gen.cid_len() == 0 {
                 2 // i.e. default, i.e. unsent