From c12f42580329a9fa6a724f7f0f56c7abb2a7dde6 Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Sun, 17 Sep 2023 23:00:05 +0400
Subject: [PATCH] ackhandler: don't fail ECN validation if less than 10 testing
 packets are lost (#4088)

* ackhandler: don't fail ECN validation less than 10 testing packets lost

* ackhandler: simplify checks for mangling and loss of all testing packets
---
 internal/ackhandler/ecn.go      | 22 +++++++++++++---------
 internal/ackhandler/ecn_test.go | 16 ++++++++++++++++
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/internal/ackhandler/ecn.go b/internal/ackhandler/ecn.go
index 43cc3dd6..3e851c8e 100644
--- a/internal/ackhandler/ecn.go
+++ b/internal/ackhandler/ecn.go
@@ -125,6 +125,10 @@ func (e *ecnTracker) LostPacket(pn protocol.PacketNumber) {
 		return
 	}
 	e.numLostTesting++
+	// Only proceed if we have sent all 10 testing packets.
+	if e.state != ecnStateUnknown {
+		return
+	}
 	if e.numLostTesting >= e.numSentTesting {
 		e.logger.Debugf("Disabling ECN. All testing packets were lost.")
 		if e.tracer != nil && e.tracer.ECNStateUpdated != nil {
@@ -223,16 +227,16 @@ func (e *ecnTracker) HandleNewlyAcked(packets []*packet, ect0, ect1, ecnce int64
 	e.numAckedECT1 = ect1
 	e.numAckedECNCE = ecnce
 
-	if e.state == ecnStateTesting || e.state == ecnStateUnknown {
-		// Detect mangling (a path remarking all ECN-marked testing packets as CE).
-		if e.numSentECT0+e.numSentECT1 == e.numAckedECNCE && e.numAckedECNCE >= numECNTestingPackets {
-			if e.tracer != nil && e.tracer.ECNStateUpdated != nil {
-				e.tracer.ECNStateUpdated(logging.ECNStateFailed, logging.ECNFailedManglingDetected)
-			}
-			e.state = ecnStateFailed
-			return false
+	// Detect mangling (a path remarking all ECN-marked testing packets as CE),
+	// once all 10 testing packets have been sent out.
+	if e.state == ecnStateUnknown && e.numSentECT0+e.numSentECT1 == e.numAckedECNCE {
+		if e.tracer != nil && e.tracer.ECNStateUpdated != nil {
+			e.tracer.ECNStateUpdated(logging.ECNStateFailed, logging.ECNFailedManglingDetected)
 		}
-
+		e.state = ecnStateFailed
+		return false
+	}
+	if e.state == ecnStateTesting || e.state == ecnStateUnknown {
 		var ackedTestingPacket bool
 		for _, p := range packets {
 			if e.isTestingPacket(p.PacketNumber) {
diff --git a/internal/ackhandler/ecn_test.go b/internal/ackhandler/ecn_test.go
index 26d54eed..78eef8c9 100644
--- a/internal/ackhandler/ecn_test.go
+++ b/internal/ackhandler/ecn_test.go
@@ -71,6 +71,22 @@ var _ = Describe("ECN tracker", func() {
 		ecnTracker.LostPacket(16)
 	})
 
+	It("only detects ECN mangling after sending all testing packets", func() {
+		tracer.EXPECT().ECNStateUpdated(logging.ECNStateTesting, logging.ECNTriggerNoTrigger)
+		for i := 0; i < 9; i++ {
+			Expect(ecnTracker.Mode()).To(Equal(protocol.ECT0))
+			ecnTracker.SentPacket(protocol.PacketNumber(i), protocol.ECT0)
+			ecnTracker.LostPacket(protocol.PacketNumber(i))
+		}
+		// Send the last testing packet, and receive a
+		tracer.EXPECT().ECNStateUpdated(logging.ECNStateUnknown, logging.ECNTriggerNoTrigger)
+		Expect(ecnTracker.Mode()).To(Equal(protocol.ECT0))
+		ecnTracker.SentPacket(9, protocol.ECT0)
+		// Now lose the last testing packet.
+		tracer.EXPECT().ECNStateUpdated(logging.ECNStateFailed, logging.ECNFailedLostAllTestingPackets)
+		ecnTracker.LostPacket(9)
+	})
+
 	It("passes ECN validation when a testing packet is acknowledged, while still in testing state", func() {
 		tracer.EXPECT().ECNStateUpdated(logging.ECNStateTesting, logging.ECNTriggerNoTrigger)
 		for i := 0; i < 5; i++ {