Repository: quic-go/quic-go
Pull request #4890 (2025-01-20 04:45:43)

ackhandler: remove unneeded SetHandshakeConfirmed from SentPacketHandler
According to section 4.9.2 of RFC 9001, dropping the Handshake packet number spaces happens when the handshake is confirmed.

I noticed this when rewriting the test suite in #4881.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4890?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 83.25%. Comparing base [(`92dc197`)](https://app.codecov.io/gh/quic-go/quic-go/commit/92dc1970ec21c6b035e9d6763e258deae27b02f7?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`7b757e3`)](https://app.codecov.io/gh/quic-go/quic-go/commit/7b757e34443e3050e76d21ebd20318436b8a2ce9?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4890      +/-   ##
==========================================
- Coverage   83.38%   83.25%   -0.12%     
==========================================
  Files         151      148       -3     
  Lines       16230    15878     -352     
==========================================
- Hits        13532    13219     -313     
+ Misses       2160     2140      -20     
+ Partials      538      519      -19     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4890?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).


Patch:
diff --git a/connection.go b/connection.go
index 6bc9169b..f6ae8759 100644
--- a/connection.go
+++ b/connection.go
@@ -794,7 +794,6 @@ func (s *connection) handleHandshakeConfirmed(now time.Time) error {
 	}
 
 	s.handshakeConfirmed = true
-	s.sentPacketHandler.SetHandshakeConfirmed(now)
 	s.cryptoStreamHandler.SetHandshakeConfirmed()
 
 	if !s.config.DisablePathMTUDiscovery && s.conn.capabilities().DF {
diff --git a/internal/ackhandler/interfaces.go b/internal/ackhandler/interfaces.go
index 426c58b9..acf95426 100644
--- a/internal/ackhandler/interfaces.go
+++ b/internal/ackhandler/interfaces.go
@@ -17,7 +17,6 @@ type SentPacketHandler interface {
 	ReceivedBytes(_ protocol.ByteCount, rcvTime time.Time)
 	DropPackets(_ protocol.EncryptionLevel, rcvTime time.Time)
 	ResetForRetry(rcvTime time.Time)
-	SetHandshakeConfirmed(now time.Time)
 
 	// The SendMode determines if and what kind of packets can be sent.
 	SendMode(now time.Time) SendMode
diff --git a/internal/ackhandler/sent_packet_handler.go b/internal/ackhandler/sent_packet_handler.go
index 7f64cf8f..91a1a2cb 100644
--- a/internal/ackhandler/sent_packet_handler.go
+++ b/internal/ackhandler/sent_packet_handler.go
@@ -179,6 +179,9 @@ func (h *sentPacketHandler) DropPackets(encLevel protocol.EncryptionLevel, now t
 	case protocol.EncryptionInitial:
 		h.initialPackets = nil
 	case protocol.EncryptionHandshake:
+		// Dropping the handshake packet number space means that the handshake is confirmed,
+		// see section 4.9.2 of RFC 9001.
+		h.handshakeConfirmed = true
 		h.handshakePackets = nil
 	case protocol.Encryption0RTT:
 		// This function is only called when 0-RTT is rejected,
@@ -912,16 +915,3 @@ func (h *sentPacketHandler) ResetForRetry(now time.Time) {
 	}
 	h.ptoCount = 0
 }
-
-func (h *sentPacketHandler) SetHandshakeConfirmed(now time.Time) {
-	if h.initialPackets != nil {
-		panic("didn't drop initial correctly")
-	}
-	if h.handshakePackets != nil {
-		panic("didn't drop handshake correctly")
-	}
-	h.handshakeConfirmed = true
-	// We don't send PTOs for application data packets before the handshake completes.
-	// Make sure the timer is armed now, if necessary.
-	h.setLossDetectionTimer(now)
-}
diff --git a/internal/mocks/ackhandler/sent_packet_handler.go b/internal/mocks/ackhandler/sent_packet_handler.go
index b2134b16..bac7ec78 100644
--- a/internal/mocks/ackhandler/sent_packet_handler.go
+++ b/internal/mocks/ackhandler/sent_packet_handler.go
@@ -493,42 +493,6 @@ func (c *MockSentPacketHandlerSentPacketCall) DoAndReturn(f func(time.Time, prot
 	return c
 }
 
-// SetHandshakeConfirmed mocks base method.
-func (m *MockSentPacketHandler) SetHandshakeConfirmed(now time.Time) {
-	m.ctrl.T.Helper()
-	m.ctrl.Call(m, "SetHandshakeConfirmed", now)
-}
-
-// SetHandshakeConfirmed indicates an expected call of SetHandshakeConfirmed.
-func (mr *MockSentPacketHandlerMockRecorder) SetHandshakeConfirmed(now any) *MockSentPacketHandlerSetHandshakeConfirmedCall {
-	mr.mock.ctrl.T.Helper()
-	call := mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetHandshakeConfirmed", reflect.TypeOf((*MockSentPacketHandler)(nil).SetHandshakeConfirmed), now)
-	return &MockSentPacketHandlerSetHandshakeConfirmedCall{Call: call}
-}
-
-// MockSentPacketHandlerSetHandshakeConfirmedCall wrap *gomock.Call
-type MockSentPacketHandlerSetHandshakeConfirmedCall struct {
-	*gomock.Call
-}
-
-// Return rewrite *gomock.Call.Return
-func (c *MockSentPacketHandlerSetHandshakeConfirmedCall) Return() *MockSentPacketHandlerSetHandshakeConfirmedCall {
-	c.Call = c.Call.Return()
-	return c
-}
-
-// Do rewrite *gomock.Call.Do
-func (c *MockSentPacketHandlerSetHandshakeConfirmedCall) Do(f func(time.Time)) *MockSentPacketHandlerSetHandshakeConfirmedCall {
-	c.Call = c.Call.Do(f)
-	return c
-}
-
-// DoAndReturn rewrite *gomock.Call.DoAndReturn
-func (c *MockSentPacketHandlerSetHandshakeConfirmedCall) DoAndReturn(f func(time.Time)) *MockSentPacketHandlerSetHandshakeConfirmedCall {
-	c.Call = c.Call.DoAndReturn(f)
-	return c
-}
-
 // SetMaxDatagramSize mocks base method.
 func (m *MockSentPacketHandler) SetMaxDatagramSize(count protocol.ByteCount) {
 	m.ctrl.T.Helper()


Test patch:
diff --git a/internal/ackhandler/sent_packet_handler_test.go b/internal/ackhandler/sent_packet_handler_test.go
index ce08c994..90eca530 100644
--- a/internal/ackhandler/sent_packet_handler_test.go
+++ b/internal/ackhandler/sent_packet_handler_test.go
@@ -133,7 +133,6 @@ var _ = Describe("SentPacketHandler", func() {
 	setHandshakeConfirmed := func() {
 		handler.DropPackets(protocol.EncryptionInitial, time.Now())
 		handler.DropPackets(protocol.EncryptionHandshake, time.Now())
-		handler.SetHandshakeConfirmed(time.Now())
 	}
 
 	Context("registering sent packets", func() {