Updated ACK_FREQUENCY according to draft-ietf-quic-ack-frequency-10

src/core/ack_tracker.c

@@ -107,22 +107,25 @@
     )
 {
     if (ReorderingThreshold == 0) {
         return FALSE;
     }
 
     const uint64_t LargestUnacked = QuicRangeGetMax(&Tracker->PacketNumbersToAck);
+    const uint64_t SmallestTracked = QuicRangeGet(&Tracker->PacketNumbersToAck, 0)->Low;
     uint64_t LargestReported;  // The largest packet number that could be declared lost 
 
     //
-    // Largest Reported is equal to the largest packet number acknowledged minus the Reordering Threshold.
-    // If the difference between the largest packet number acknowledged and the Reordering Threshold is smaller than the
-    // smallest packet in the ack tracker, then the largest reported is the smallest packet in the ack tracker.
+    // Largest Reported is equal to the largest packet number acknowledged minus the 
+    // Reordering Threshold. If the difference between the largest packet number
+    // acknowledged and the Reordering Threshold is smaller than the smallest packet 
+    // in the ack tracker, then the largest reported is the smallest packet in the ack
+    //  tracker.
     //
 
-    if (Tracker->LargestPacketNumberAcknowledged >= QuicRangeGet(&Tracker->PacketNumbersToAck, 0)->Low + ReorderingThreshold) {
+    if (Tracker->LargestPacketNumberAcknowledged >= SmallestTracked + ReorderingThreshold) {
         LargestReported = Tracker->LargestPacketNumberAcknowledged - ReorderingThreshold + 1;
     } else {
-        LargestReported = QuicRangeGet(&Tracker->PacketNumbersToAck, 0)->Low;
+        LargestReported = SmallestTracked;
     }
 
     //
@@ -134,23 +137,29 @@
     //
 
     for (uint32_t Index = QuicRangeSize(&Tracker->PacketNumbersToAck) - 1; Index > 0; --Index) {
-        uint64_t SmallestMissing = QuicRangeGetHigh(QuicRangeGet(&Tracker->PacketNumbersToAck, Index - 1)) + 1; // Smallest missing in the previous gap
-        uint64_t RangeStart = QuicRangeGet(&Tracker->PacketNumbersToAck, Index)->Low; // Lowest Packet number in the subrange
+        uint64_t PreviousSmallestMissing = QuicRangeGetHigh(QuicRangeGet(&Tracker->PacketNumbersToAck, Index - 1)) + 1;
+        const uint64_t RangeStart = QuicRangeGet(&Tracker->PacketNumbersToAck, Index)->Low;
 
         // 
-        // Check if largest reported packet is missing. In that case, the smalles missing packet becomes the largest reported packet.
+        // Check if largest reported packet is missing. In that case, the smallest missing 
+        // packet becomes the largest reported packet.
         //
-        if (LargestReported > SmallestMissing) {
-            if (RangeStart > LargestReported) {
-                SmallestMissing = LargestReported;
-            } else {
-                return FALSE;
-            }
+
+        if (LargestReported >= RangeStart) {
+            //
+            // Since we are only looking for packets more than LargestReported, we return
+            // false here.
+            //
+            return FALSE;
+        }
+
+        if (LargestReported > PreviousSmallestMissing) {
+            PreviousSmallestMissing = LargestReported;
         }
-        if (LargestUnacked - SmallestMissing >= ReorderingThreshold) {
+        if (LargestUnacked - PreviousSmallestMissing >= ReorderingThreshold) {
             return TRUE;
         }
     }
     return FALSE;
 }
 
@@ -336,12 +345,11 @@
 
     //
     // Drop all packet numbers less than or equal to the largest acknowledged
-    // packet number - Reordering Threshold + 1. This is so that we dont lose 
-    // memory of packets that are missing and are yet to be reported. 
+    // packet number.
     //
     QuicRangeSetMin(
         &Tracker->PacketNumbersToAck,
-        LargestAckedPacketNumber - Connection->ReorderingThreshold + 2);
+        LargestAckedPacketNumber + 1);
 
     if (!QuicAckTrackerHasPacketsToAck(Tracker) &&
         Tracker->AckElicitingPacketsToAcknowledge) {

src/core/unittest/FrameTest.cpp

@@ -243,13 +243,20 @@ TEST(FrameTest, ReliableResetStreamFrameEncodeDecode)
     ASSERT_EQ(Frame.ReliableSize, DecodedFrame.ReliableSize);
 }
 
-BOOLEAN DidHitReorderingThreshold(
+// 
+// Tests if the reordering threshold has been hit. This function initializes a
+// QUIC_ACK_TRACKER and populates it with the provided packet numbers. It then 
+// calls QuicAckTrackerDidHitReorderingThreshold to determine if the reordering
+// threshold has been hit.
+// 
+// 
+bool TestReorderingThreshold(
     uint8_t ReorderingThreshold,
     uint64_t LargestPacketNumberAcknowledged, 
     const std::vector<std::vector<int>>& AckTrackerLists) 
 {
     QUIC_ACK_TRACKER Tracker;
-    QuicAckTrackerInitialize(&Tracker);;
+    QuicAckTrackerInitialize(&Tracker);
     for (const auto& List : AckTrackerLists) {
         for (int i : List) {
             QuicRangeAddValue(&Tracker.PacketNumbersToAck, i);
@@ -263,30 +270,31 @@ BOOLEAN DidHitReorderingThreshold(
 
 TEST(FrameTest, TestQuicAckTrackerDidHitReorderingThreshold)
 {
-
-    ASSERT_FALSE(DidHitReorderingThreshold(0, 0, {{100}}));
-
-    // Case 1 
-
-    ASSERT_FALSE(DidHitReorderingThreshold(3, 0, {{0}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(3, 0, {{0, 1}}));    
-    ASSERT_FALSE(DidHitReorderingThreshold(3, 0, {{0, 1}, {3}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(3, 0, {{0, 1}, {3, 4}}));
-    ASSERT_TRUE(DidHitReorderingThreshold(3, 0, {{0, 1}, {3, 4, 5}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(3, 5, {{0, 1}, {3, 4, 5}, {8}}));
-    ASSERT_TRUE(DidHitReorderingThreshold(3, 5, {{0, 1}, {3, 4, 5}, {8, 9}}));
-    ASSERT_TRUE(DidHitReorderingThreshold(3, 9, {{0, 1}, {3, 4, 5}, {8, 9, 10}}));
+    ASSERT_FALSE(TestReorderingThreshold(0, 0, {{100}}));
+
+    // Case 1
+    ASSERT_FALSE(TestReorderingThreshold(3, 0, {{0}}));
+    ASSERT_FALSE(TestReorderingThreshold(3, 0, {{0, 1}}));    
+    ASSERT_FALSE(TestReorderingThreshold(3, 0, {{0, 1}, {3}}));
+    ASSERT_FALSE(TestReorderingThreshold(3, 0, {{0, 1}, {3, 4}}));
+    ASSERT_TRUE(TestReorderingThreshold(3, 0, {{0, 1}, {3, 4, 5}}));
+    ASSERT_FALSE(TestReorderingThreshold(3, 5, {{0, 1}, {3, 4, 5}, {8}}));
+    ASSERT_TRUE(TestReorderingThreshold(3, 5, {{0, 1}, {3, 4, 5}, {8, 9}}));
+    ASSERT_TRUE(TestReorderingThreshold(3, 9, {{0, 1}, {3, 4, 5}, {8, 9, 10}}));
 
     // Case 2
-    ASSERT_FALSE(DidHitReorderingThreshold(5, 0, {{0}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(5, 0, {{0, 1}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(5, 0, {{0, 1}, {3}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(5, 0, {{0, 1}, {3}, {5}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(5, 0, {{0, 1}, {3}, {5, 6}}));
-    ASSERT_TRUE(DidHitReorderingThreshold(5, 0, {{0, 1}, {3}, {5, 6, 7}}));
-    ASSERT_FALSE(DidHitReorderingThreshold(5, 7, {{0, 1}, {3}, {5, 6, 7, 8}}));
-    ASSERT_TRUE(DidHitReorderingThreshold(5, 7, {{0, 1}, {3}, {5, 6, 7, 8, 9}}));
-
+    ASSERT_FALSE(TestReorderingThreshold(5, 0, {{0}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 0, {{0, 1}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 0, {{0, 1}, {3}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 0, {{0, 1}, {3}, {5}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 0, {{0, 1}, {3}, {5, 6}}));
+    ASSERT_TRUE(TestReorderingThreshold(5, 0, {{0, 1}, {3}, {5, 6, 7}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 7, {{0, 1}, {3}, {5, 6, 7, 8}}));
+    ASSERT_TRUE(TestReorderingThreshold(5, 7, {{0, 1}, {3}, {5, 6, 7, 8, 9}}));
+
+    ASSERT_TRUE(TestReorderingThreshold(5, 4, {{1, 2}, {4}, {10}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 0, {{1, 2}, {4}}));
+    ASSERT_FALSE(TestReorderingThreshold(5, 2, {{1, 2}, {4}}));
 }
 
 struct ResetStreamFrameParams {