dcsctp: Report duplicate TSNs

Reporting the duplicate TSNs is a SHOULD in the RFC, and using the
duplicate TNSs is a MAY, and in reality I haven't seen an implementation
use it yet. However, it's good for debugging and for stats generation.

Bug: webrtc:12614
Change-Id: I1cc3f86961a8d289708cbf50d98dedfd25077955
Reviewed-on: https://webrtc-review.googlesource.com/c/src/+/219462
Reviewed-by: Florent Castelli <orphis@webrtc.org>
Commit-Queue: Victor Boivie <boivie@webrtc.org>
Cr-Commit-Position: refs/heads/master@{#34053}

net/dcsctp/packet/chunk/sack_chunk.cc

@@ -88,13 +88,12 @@ absl::optional<SackChunk> SackChunk::Parse(rtc::ArrayView<const uint8_t> data) {
     offset += kGapAckBlockSize;
   }
 
-  std::vector<TSN> duplicate_tsns;
+  std::set<TSN> duplicate_tsns;
-  duplicate_tsns.reserve(nbr_of_gap_blocks);
   for (int i = 0; i < nbr_of_dup_tsns; ++i) {
     BoundedByteReader<kDupTsnBlockSize> sub_reader =
         reader->sub_reader<kDupTsnBlockSize>(offset);
 
-    duplicate_tsns.push_back(TSN(sub_reader.Load32<0>()));
+    duplicate_tsns.insert(TSN(sub_reader.Load32<0>()));
     offset += kDupTsnBlockSize;
   }
   RTC_DCHECK(offset == reader->variable_data_size());
@@ -124,11 +123,11 @@ void SackChunk::SerializeTo(std::vector<uint8_t>& out) const {
     offset += kGapAckBlockSize;
   }
 
-  for (int i = 0; i < nbr_of_dup_tsns; ++i) {
+  for (TSN tsn : duplicate_tsns_) {
     BoundedByteWriter<kDupTsnBlockSize> sub_writer =
         writer.sub_writer<kDupTsnBlockSize>(offset);
 
-    sub_writer.Store32<0>(*duplicate_tsns_[i]);
+    sub_writer.Store32<0>(*tsn);
     offset += kDupTsnBlockSize;
   }
 

net/dcsctp/packet/chunk/sack_chunk.h

@@ -12,6 +12,7 @@
 #include <stddef.h>
 
 #include <cstdint>
+#include <set>
 #include <string>
 #include <utility>
 #include <vector>
@@ -48,7 +49,7 @@ class SackChunk : public Chunk, public TLVTrait<SackChunkConfig> {
   SackChunk(TSN cumulative_tsn_ack,
             uint32_t a_rwnd,
             std::vector<GapAckBlock> gap_ack_blocks,
-            std::vector<TSN> duplicate_tsns)
+            std::set<TSN> duplicate_tsns)
       : cumulative_tsn_ack_(cumulative_tsn_ack),
         a_rwnd_(a_rwnd),
         gap_ack_blocks_(std::move(gap_ack_blocks)),
@@ -63,7 +64,7 @@ class SackChunk : public Chunk, public TLVTrait<SackChunkConfig> {
   rtc::ArrayView<const GapAckBlock> gap_ack_blocks() const {
     return gap_ack_blocks_;
   }
-  rtc::ArrayView<const TSN> duplicate_tsns() const { return duplicate_tsns_; }
+  const std::set<TSN>& duplicate_tsns() const { return duplicate_tsns_; }
 
  private:
   static constexpr size_t kGapAckBlockSize = 4;
@@ -72,7 +73,7 @@ class SackChunk : public Chunk, public TLVTrait<SackChunkConfig> {
   const TSN cumulative_tsn_ack_;
   const uint32_t a_rwnd_;
   std::vector<GapAckBlock> gap_ack_blocks_;
-  std::vector<TSN> duplicate_tsns_;
+  std::set<TSN> duplicate_tsns_;
 };
 }  // namespace dcsctp
 

net/dcsctp/packet/chunk_validators.cc

@@ -38,9 +38,7 @@ SackChunk ChunkValidators::Clean(SackChunk&& sack) {
   // Not more than at most one remaining? Exit early.
   if (gap_ack_blocks.size() <= 1) {
     return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(),
-                     std::move(gap_ack_blocks),
+                     std::move(gap_ack_blocks), sack.duplicate_tsns());
-                     std::vector<TSN>(sack.duplicate_tsns().begin(),
-                                      sack.duplicate_tsns().end()));
   }
 
   // Sort the intervals by their start value, to aid in the merging below.
@@ -63,8 +61,7 @@ SackChunk ChunkValidators::Clean(SackChunk&& sack) {
   }
 
   return SackChunk(sack.cumulative_tsn_ack(), sack.a_rwnd(), std::move(merged),
-                   std::vector<TSN>(sack.duplicate_tsns().begin(),
+                   sack.duplicate_tsns());
-                                    sack.duplicate_tsns().end()));
 }
 
 bool ChunkValidators::Validate(const SackChunk& sack) {

net/dcsctp/rx/data_tracker.cc

@@ -52,7 +52,7 @@ void DataTracker::Observe(TSN tsn,
 
   // Old chunk already seen before?
   if (unwrapped_tsn <= last_cumulative_acked_tsn_) {
-    // TODO(boivie) Set duplicate TSN, even if it's not used in SCTP yet.
+    duplicate_tsns_.insert(unwrapped_tsn.Wrap());
     return;
   }
 
@@ -66,7 +66,11 @@ void DataTracker::Observe(TSN tsn,
       additional_tsns_.erase(additional_tsns_.begin());
     }
   } else {
-    additional_tsns_.insert(unwrapped_tsn);
+    bool inserted = additional_tsns_.insert(unwrapped_tsn).second;
+    if (!inserted) {
+      // Already seen before.
+      duplicate_tsns_.insert(unwrapped_tsn.Wrap());
+    }
   }
 
   // https://tools.ietf.org/html/rfc4960#section-6.7
@@ -178,15 +182,11 @@ SackChunk DataTracker::CreateSelectiveAck(size_t a_rwnd) {
   // that. So this SACK produced is more like a NR-SACK as explained in
   // https://ieeexplore.ieee.org/document/4697037 and which there is an RFC
   // draft at https://tools.ietf.org/html/draft-tuexen-tsvwg-sctp-multipath-17.
-  std::vector<TSN> duplicate_tsns;
+  std::set<TSN> duplicate_tsns;
-  duplicate_tsns.reserve(duplicates_.size());
+  duplicate_tsns_.swap(duplicate_tsns);
-  for (UnwrappedTSN tsn : duplicates_) {
-    duplicate_tsns.push_back(tsn.Wrap());
-  }
-  duplicates_.clear();
 
   return SackChunk(last_cumulative_acked_tsn_.Wrap(), a_rwnd,
-                   CreateGapAckBlocks(), duplicate_tsns);
+                   CreateGapAckBlocks(), std::move(duplicate_tsns));
 }
 
 std::vector<SackChunk::GapAckBlock> DataTracker::CreateGapAckBlocks() const {

net/dcsctp/rx/data_tracker.h

@@ -126,7 +126,7 @@ class DataTracker {
   UnwrappedTSN last_cumulative_acked_tsn_;
   // Received TSNs that are not directly following `last_cumulative_acked_tsn_`.
   std::set<UnwrappedTSN> additional_tsns_;
-  std::set<UnwrappedTSN> duplicates_;
+  std::set<TSN> duplicate_tsns_;
 };
 }  // namespace dcsctp
 

net/dcsctp/rx/data_tracker_test.cc

@@ -25,6 +25,7 @@ namespace dcsctp {
 namespace {
 using ::testing::ElementsAre;
 using ::testing::IsEmpty;
+using ::testing::UnorderedElementsAre;
 
 constexpr size_t kArwnd = 10000;
 constexpr TSN kInitialTSN(11);
@@ -230,5 +231,43 @@ TEST_F(DataTrackerTest, WillNotAcceptInvalidTSNs) {
   EXPECT_FALSE(buf_.IsTSNValid(TSN(*last_tsn - 0x8000000)));
 }
 
+TEST_F(DataTrackerTest, ReportSingleDuplicateTsns) {
+  Observer({11, 12, 11});
+  SackChunk sack = buf_.CreateSelectiveAck(kArwnd);
+  EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(12));
+  EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty());
+  EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(11)));
+}
+
+TEST_F(DataTrackerTest, ReportMultipleDuplicateTsns) {
+  Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16});
+  SackChunk sack = buf_.CreateSelectiveAck(kArwnd);
+  EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(16));
+  EXPECT_THAT(sack.gap_ack_blocks(), IsEmpty());
+  EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(12), TSN(13)));
+}
+
+TEST_F(DataTrackerTest, ReportDuplicateTsnsInGapAckBlocks) {
+  Observer({11, /*12,*/ 13, 14, 13, 14, 15, 16});
+  SackChunk sack = buf_.CreateSelectiveAck(kArwnd);
+  EXPECT_EQ(sack.cumulative_tsn_ack(), TSN(11));
+  EXPECT_THAT(sack.gap_ack_blocks(), ElementsAre(SackChunk::GapAckBlock(2, 5)));
+  EXPECT_THAT(sack.duplicate_tsns(), UnorderedElementsAre(TSN(13), TSN(14)));
+}
+
+TEST_F(DataTrackerTest, ClearsDuplicateTsnsAfterCreatingSack) {
+  Observer({11, 12, 13, 14, 12, 13, 12, 13, 15, 16});
+  SackChunk sack1 = buf_.CreateSelectiveAck(kArwnd);
+  EXPECT_EQ(sack1.cumulative_tsn_ack(), TSN(16));
+  EXPECT_THAT(sack1.gap_ack_blocks(), IsEmpty());
+  EXPECT_THAT(sack1.duplicate_tsns(), UnorderedElementsAre(TSN(12), TSN(13)));
+
+  Observer({17});
+  SackChunk sack2 = buf_.CreateSelectiveAck(kArwnd);
+  EXPECT_EQ(sack2.cumulative_tsn_ack(), TSN(17));
+  EXPECT_THAT(sack2.gap_ack_blocks(), IsEmpty());
+  EXPECT_THAT(sack2.duplicate_tsns(), IsEmpty());
+}
+
 }  // namespace
 }  // namespace dcsctp