save dummy packets in the packet history when skipping packet numbers

internal/ackhandler/interfaces.go

@@ -19,6 +19,7 @@ type Packet struct {
 
 	includedInBytesInFlight bool
 	declaredLost            bool
+	skippedPacket           bool
 }
 
 // SentPacketHandler handles ACKs received for outgoing packets

internal/ackhandler/packet_number_generator.go

@@ -5,19 +5,16 @@ import (
 	"math"
 
 	"github.com/lucas-clemente/quic-go/internal/protocol"
-	"github.com/lucas-clemente/quic-go/internal/wire"
 )
 
 // The packetNumberGenerator generates the packet number for the next packet
-// it randomly skips a packet number every averagePeriod packets (on average)
-// it is guarantued to never skip two consecutive packet numbers
+// it randomly skips a packet number every averagePeriod packets (on average).
+// It is guaranteed to never skip two consecutive packet numbers.
 type packetNumberGenerator struct {
 	averagePeriod protocol.PacketNumber
 
 	next       protocol.PacketNumber
 	nextToSkip protocol.PacketNumber
-
-	history []protocol.PacketNumber
 }
 
 func newPacketNumberGenerator(initial, averagePeriod protocol.PacketNumber) *packetNumberGenerator {
@@ -40,14 +37,9 @@ func (p *packetNumberGenerator) Pop() protocol.PacketNumber {
 	p.next++
 
 	if p.next == p.nextToSkip {
-		if len(p.history)+1 > protocol.MaxTrackedSkippedPackets {
-			p.history = p.history[1:]
-		}
-		p.history = append(p.history, p.next)
 		p.next++
 		p.generateNewSkip()
 	}
-
 	return next
 }
 
@@ -67,12 +59,3 @@ func (p *packetNumberGenerator) getRandomNumber() uint16 {
 	num := uint16(b[0])<<8 + uint16(b[1])
 	return num
 }
-
-func (p *packetNumberGenerator) Validate(ack *wire.AckFrame) bool {
-	for _, pn := range p.history {
-		if ack.AcksPacket(pn) {
-			return false
-		}
-	}
-	return true
-}

internal/ackhandler/packet_number_generator_test.go

@@ -4,7 +4,6 @@ import (
 	"math"
 
 	"github.com/lucas-clemente/quic-go/internal/protocol"
-	"github.com/lucas-clemente/quic-go/internal/wire"
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
 )
@@ -98,45 +97,4 @@ var _ = Describe("Packet Number Generator", func() {
 		Expect(largest).To(BeNumerically(">", math.MaxUint16-300))
 		Expect(sum / uint64(rep)).To(BeNumerically("==", uint64(math.MaxUint16/2), 1000))
 	})
-
-	It("validates ACK frames", func() {
-		var skipped []protocol.PacketNumber
-		var lastPN protocol.PacketNumber
-		for len(skipped) < 3 {
-			if png.Peek() > lastPN+1 {
-				skipped = append(skipped, lastPN+1)
-			}
-			lastPN = png.Pop()
-		}
-		invalidACK := &wire.AckFrame{
-			AckRanges: []wire.AckRange{{Smallest: 1, Largest: lastPN}},
-		}
-		Expect(png.Validate(invalidACK)).To(BeFalse())
-		validACK1 := &wire.AckFrame{
-			AckRanges: []wire.AckRange{{Smallest: 1, Largest: skipped[0] - 1}},
-		}
-		Expect(png.Validate(validACK1)).To(BeTrue())
-		validACK2 := &wire.AckFrame{
-			AckRanges: []wire.AckRange{
-				{Smallest: 1, Largest: skipped[0] - 1},
-				{Smallest: skipped[0] + 1, Largest: skipped[1] - 1},
-				{Smallest: skipped[1] + 1, Largest: skipped[2] - 1},
-				{Smallest: skipped[2] + 1, Largest: skipped[2] + 100},
-			},
-		}
-		Expect(png.Validate(validACK2)).To(BeTrue())
-	})
-
-	It("tracks a maximum number of protocol.MaxTrackedSkippedPackets packets", func() {
-		var skipped []protocol.PacketNumber
-		var lastPN protocol.PacketNumber
-		for len(skipped) < protocol.MaxTrackedSkippedPackets+3 {
-			if png.Peek() > lastPN+1 {
-				skipped = append(skipped, lastPN+1)
-			}
-			lastPN = png.Pop()
-			Expect(len(png.history)).To(BeNumerically("<=", protocol.MaxTrackedSkippedPackets))
-		}
-		Expect(len(png.history)).To(Equal(protocol.MaxTrackedSkippedPackets))
-	})
 })

internal/ackhandler/sent_packet_handler.go

@@ -161,6 +161,9 @@ func (h *sentPacketHandler) dropPackets(encLevel protocol.EncryptionLevel) {
 	case protocol.Encryption0RTT:
 		// TODO(#2067): invalidate sent data
 		h.appDataPackets.history.Iterate(func(p *Packet) (bool, error) {
+			if p.skippedPacket {
+				return true, nil
+			}
 			if p.EncryptionLevel != protocol.Encryption0RTT {
 				return false, nil
 			}
@@ -209,9 +212,7 @@ func (h *sentPacketHandler) SentPacket(packet *Packet) {
 		h.dropPackets(protocol.EncryptionInitial)
 	}
 	isAckEliciting := h.sentPacketImpl(packet)
-	if isAckEliciting {
-		h.getPacketNumberSpace(packet.EncryptionLevel).history.SentPacket(packet)
-	}
+	h.getPacketNumberSpace(packet.EncryptionLevel).history.SentPacket(packet, isAckEliciting)
 	if h.tracer != nil && isAckEliciting {
 		h.tracer.UpdatedMetrics(h.rttStats, h.congestion.GetCongestionWindow(), h.bytesInFlight, h.packetsInFlight())
 	}
@@ -268,10 +269,6 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En
 
 	pnSpace.largestAcked = utils.MaxPacketNumber(pnSpace.largestAcked, largestAcked)
 
-	if !pnSpace.pns.Validate(ack) {
-		return qerr.NewError(qerr.ProtocolViolation, "Received an ACK for a skipped packet number")
-	}
-
 	// Servers complete address validation when a protected packet is received.
 	if h.perspective == protocol.PerspectiveClient && !h.peerCompletedAddressValidation &&
 		(encLevel == protocol.EncryptionHandshake || encLevel == protocol.Encryption1RTT) {
@@ -312,6 +309,9 @@ func (h *sentPacketHandler) ReceivedAck(ack *wire.AckFrame, encLevel protocol.En
 		h.congestion.OnPacketLost(p.PacketNumber, p.Length, priorInFlight)
 	}
 	for _, p := range ackedPackets {
+		if p.skippedPacket {
+			return fmt.Errorf("received an ACK for skipped packet number: %d (%s)", p.PacketNumber, encLevel)
+		}
 		if p.includedInBytesInFlight && !p.declaredLost {
 			h.congestion.OnPacketAcked(p.PacketNumber, p.Length, priorInFlight, rcvTime)
 		}
@@ -520,7 +520,7 @@ func (h *sentPacketHandler) detectLostPackets(now time.Time, encLevel protocol.E
 		if packet.PacketNumber > pnSpace.largestAcked {
 			return false, nil
 		}
-		if packet.declaredLost {
+		if packet.declaredLost || packet.skippedPacket {
 			return true, nil
 		}
 
@@ -765,7 +765,7 @@ func (h *sentPacketHandler) ResetForRetry() error {
 		if firstPacketSendTime.IsZero() {
 			firstPacketSendTime = p.SendTime
 		}
-		if p.declaredLost {
+		if p.declaredLost || p.skippedPacket {
 			return true, nil
 		}
 		h.queueFramesForRetransmission(p)
@@ -774,7 +774,7 @@ func (h *sentPacketHandler) ResetForRetry() error {
 	// All application data packets sent at this point are 0-RTT packets.
 	// In the case of a Retry, we can assume that the server dropped all of them.
 	h.appDataPackets.history.Iterate(func(p *Packet) (bool, error) {
-		if !p.declaredLost {
+		if !p.declaredLost && !p.skippedPacket {
 			h.queueFramesForRetransmission(p)
 		}
 		return true, nil

internal/ackhandler/sent_packet_handler_test.go

@@ -88,7 +88,7 @@ var _ = Describe("SentPacketHandler", func() {
 		pnSpace := handler.getPacketNumberSpace(encLevel)
 		var length int
 		pnSpace.history.Iterate(func(p *Packet) (bool, error) {
-			if !p.declaredLost {
+			if !p.declaredLost && !p.skippedPacket {
 				length++
 			}
 			return true, nil
@@ -141,13 +141,6 @@ var _ = Describe("SentPacketHandler", func() {
 			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: 2, SendTime: sendTime.Add(time.Hour), EncryptionLevel: protocol.Encryption1RTT}))
 			Expect(handler.initialPackets.lastAckElicitingPacketTime).To(Equal(sendTime))
 		})
-
-		It("does not store non-ack-eliciting packets", func() {
-			handler.SentPacket(nonAckElicitingPacket(&Packet{PacketNumber: 1}))
-			Expect(handler.appDataPackets.history.Len()).To(BeZero())
-			Expect(handler.appDataPackets.lastAckElicitingPacketTime).To(BeZero())
-			Expect(handler.bytesInFlight).To(BeZero())
-		})
 	})
 
 	Context("ACK processing", func() {
@@ -557,11 +550,10 @@ var _ = Describe("SentPacketHandler", func() {
 			handler.ReceivedPacket(protocol.EncryptionHandshake)
 			cong.EXPECT().CanSend(gomock.Any()).Return(true).AnyTimes()
 			cong.EXPECT().OnPacketSent(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes()
-			for i := protocol.PacketNumber(1); i < protocol.MaxOutstandingSentPackets; i++ {
-				handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: i}))
+			for i := protocol.PacketNumber(0); i < protocol.MaxOutstandingSentPackets; i++ {
 				Expect(handler.SendMode()).To(Equal(SendAny))
+				handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: i}))
 			}
-			handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: protocol.MaxOutstandingSentPackets}))
 			Expect(handler.SendMode()).To(Equal(SendAck))
 		})
 
@@ -754,7 +746,7 @@ var _ = Describe("SentPacketHandler", func() {
 			Expect(handler.SendMode()).To(Equal(SendPTOInitial))
 			handler.SentPacket(initialPacket(&Packet{PacketNumber: 3}))
 			Expect(handler.SendMode()).To(Equal(SendPTOInitial))
-			handler.SentPacket(initialPacket(&Packet{PacketNumber: 3}))
+			handler.SentPacket(initialPacket(&Packet{PacketNumber: 4}))
 
 			Expect(handler.SendMode()).To(Equal(SendAny))
 		})
@@ -1045,6 +1037,9 @@ var _ = Describe("SentPacketHandler", func() {
 		// TODO(#2067): invalidate 0-RTT data when 0-RTT is rejected
 		It("retransmits 0-RTT packets when 0-RTT keys are dropped", func() {
 			for i := protocol.PacketNumber(0); i < 6; i++ {
+				if i == 3 {
+					continue
+				}
 				handler.SentPacket(ackElicitingPacket(&Packet{
 					PacketNumber:    i,
 					EncryptionLevel: protocol.Encryption0RTT,
@@ -1053,9 +1048,9 @@ var _ = Describe("SentPacketHandler", func() {
 			for i := protocol.PacketNumber(6); i < 12; i++ {
 				handler.SentPacket(ackElicitingPacket(&Packet{PacketNumber: i}))
 			}
-			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(12)))
+			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(11)))
 			handler.DropPackets(protocol.Encryption0RTT)
-			Expect(lostPackets).To(Equal([]protocol.PacketNumber{0, 1, 2, 3, 4, 5}))
+			Expect(lostPackets).To(Equal([]protocol.PacketNumber{0, 1, 2, 4, 5}))
 			Expect(handler.bytesInFlight).To(Equal(protocol.ByteCount(6)))
 		})
 

internal/ackhandler/sent_packet_history.go

@@ -9,22 +9,41 @@ import (
 )
 
 type sentPacketHistory struct {
-	rttStats   *utils.RTTStats
-	packetList *PacketList
-	packetMap  map[protocol.PacketNumber]*PacketElement
+	rttStats    *utils.RTTStats
+	packetList  *PacketList
+	packetMap   map[protocol.PacketNumber]*PacketElement
+	highestSent protocol.PacketNumber
 }
 
 func newSentPacketHistory(rttStats *utils.RTTStats) *sentPacketHistory {
 	return &sentPacketHistory{
-		rttStats:   rttStats,
-		packetList: NewPacketList(),
-		packetMap:  make(map[protocol.PacketNumber]*PacketElement),
+		rttStats:    rttStats,
+		packetList:  NewPacketList(),
+		packetMap:   make(map[protocol.PacketNumber]*PacketElement),
+		highestSent: protocol.InvalidPacketNumber,
 	}
 }
 
-func (h *sentPacketHistory) SentPacket(p *Packet) {
-	el := h.packetList.PushBack(*p)
-	h.packetMap[p.PacketNumber] = el
+func (h *sentPacketHistory) SentPacket(p *Packet, isAckEliciting bool) {
+	if p.PacketNumber <= h.highestSent {
+		panic("non-sequential packet number use")
+	}
+	// Skipped packet numbers.
+	for pn := h.highestSent + 1; pn < p.PacketNumber; pn++ {
+		el := h.packetList.PushBack(Packet{
+			PacketNumber:    pn,
+			EncryptionLevel: p.EncryptionLevel,
+			SendTime:        p.SendTime,
+			skippedPacket:   true,
+		})
+		h.packetMap[pn] = el
+	}
+	h.highestSent = p.PacketNumber
+
+	if isAckEliciting {
+		el := h.packetList.PushBack(*p)
+		h.packetMap[p.PacketNumber] = el
+	}
 }
 
 // Iterate iterates through all packets.
@@ -45,7 +64,7 @@ func (h *sentPacketHistory) Iterate(cb func(*Packet) (cont bool, err error)) err
 // FirstOutStanding returns the first outstanding packet.
 func (h *sentPacketHistory) FirstOutstanding() *Packet {
 	for el := h.packetList.Front(); el != nil; el = el.Next() {
-		if !el.Value.declaredLost {
+		if !el.Value.declaredLost && !el.Value.skippedPacket {
 			return &el.Value
 		}
 	}
@@ -79,7 +98,7 @@ func (h *sentPacketHistory) DeleteOldPackets(now time.Time) {
 		if p.SendTime.After(now.Add(-maxAge)) {
 			break
 		}
-		if !p.declaredLost { // should only happen in the case of drastic RTT changes
+		if !p.skippedPacket && !p.declaredLost { // should only happen in the case of drastic RTT changes
 			continue
 		}
 		delete(h.packetMap, p.PacketNumber)