Merge pull request #1649 from lucas-clemente/handshake-retransmissions

simplify packing of handshake retransmissions

internal/protocol/params.go

@@ -11,10 +11,6 @@ const MaxPacketSizeIPv6 = 1232
 // MinStatelessResetSize is the minimum size of a stateless reset packet
 const MinStatelessResetSize = 1 + 20 + 16
 
-// NonForwardSecurePacketSizeReduction is the number of bytes a non forward-secure packet has to be smaller than a forward-secure packet
-// This makes sure that those packets can always be retransmitted without splitting the contained StreamFrames
-const NonForwardSecurePacketSizeReduction = 50
-
 const defaultMaxCongestionWindowPackets = 1000
 
 // DefaultMaxCongestionWindow is the default for the max congestion window

packet_packer.go

@@ -167,14 +167,12 @@ func (p *packetPacker) MaybePackAckPacket() (*packedPacket, error) {
 // For packets sent after completion of the handshake, it might happen that 2 packets have to be sent.
 // This can happen e.g. when a longer packet number is used in the header.
 func (p *packetPacker) PackRetransmission(packet *ackhandler.Packet) ([]*packedPacket, error) {
-	if packet.EncryptionLevel != protocol.Encryption1RTT {
-		p, err := p.packHandshakeRetransmission(packet)
-		return []*packedPacket{p}, err
-	}
-
 	var controlFrames []wire.Frame
 	var streamFrames []*wire.StreamFrame
 	for _, f := range packet.Frames {
+		// CRYPTO frames are treated as control frames here.
+		// Since we're making sure that the header can never be larger for a retransmission,
+		// we never have to split CRYPTO frames.
 		if sf, ok := f.(*wire.StreamFrame); ok {
 			sf.DataLenPresent = true
 			streamFrames = append(streamFrames, sf)
@@ -243,23 +241,6 @@ func (p *packetPacker) PackRetransmission(packet *ackhandler.Packet) ([]*packedP
 	return packets, nil
 }
 
-// packHandshakeRetransmission retransmits a handshake packet
-func (p *packetPacker) packHandshakeRetransmission(packet *ackhandler.Packet) (*packedPacket, error) {
-	sealer, err := p.cryptoSetup.GetSealerWithEncryptionLevel(packet.EncryptionLevel)
-	if err != nil {
-		return nil, err
-	}
-	header := p.getHeader(packet.EncryptionLevel)
-	header.Type = packet.PacketType
-	raw, err := p.writeAndSealPacket(header, packet.Frames, sealer)
-	return &packedPacket{
-		header:          header,
-		raw:             raw,
-		frames:          packet.Frames,
-		encryptionLevel: packet.EncryptionLevel,
-	}, err
-}
-
 // PackPacket packs a new packet
 // the other controlFrames are sent in the next packet, but might be queued and sent in the next packet if the packet would overflow MaxPacketSize otherwise
 func (p *packetPacker) PackPacket() (*packedPacket, error) {
@@ -391,6 +372,10 @@ func (p *packetPacker) getHeader(encLevel protocol.EncryptionLevel) *wire.Extend
 
 	if encLevel != protocol.Encryption1RTT {
 		header.IsLongHeader = true
+		// Always send Initial and Handshake packets with the maximum packet number length.
+		// This simplifies retransmissions: Since the header can't get any larger,
+		// we don't need to split CRYPTO frames.
+		header.PacketNumberLen = protocol.PacketNumberLen4
 		header.SrcConnectionID = p.srcConnID
 		// Set the length to the maximum packet size.
 		// Since it is encoded as a varint, this guarantees us that the header will end up at most as big as GetLength() returns.

packet_packer_test.go

@@ -115,7 +115,8 @@ var _ = Describe("Packet packer", func() {
 			h := packer.getHeader(protocol.EncryptionHandshake)
 			Expect(h.IsLongHeader).To(BeTrue())
 			Expect(h.PacketNumber).To(Equal(protocol.PacketNumber(0x42)))
-			Expect(h.PacketNumberLen).To(Equal(protocol.PacketNumberLen2))
+			// long headers always use 4 byte packet numbers, no matter what the packet number generator says
+			Expect(h.PacketNumberLen).To(Equal(protocol.PacketNumberLen4))
 			Expect(h.Version).To(Equal(packer.version))
 		})
 
@@ -748,40 +749,23 @@ var _ = Describe("Packet packer", func() {
 			sf := &wire.StreamFrame{Data: []byte("foobar")}
 
 			It("packs a retransmission with the right encryption level", func() {
+				f := &wire.CryptoFrame{Data: []byte("foo")}
 				pnManager.EXPECT().PeekPacketNumber().Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)
 				pnManager.EXPECT().PopPacketNumber().Return(protocol.PacketNumber(0x42))
 				sealingManager.EXPECT().GetSealerWithEncryptionLevel(protocol.EncryptionInitial).Return(sealer, nil)
 				packet := &ackhandler.Packet{
 					PacketType:      protocol.PacketTypeHandshake,
 					EncryptionLevel: protocol.EncryptionInitial,
-					Frames:          []wire.Frame{sf},
+					Frames:          []wire.Frame{f},
 				}
 				p, err := packer.PackRetransmission(packet)
 				Expect(err).ToNot(HaveOccurred())
 				Expect(p).To(HaveLen(1))
-				Expect(p[0].header.Type).To(Equal(protocol.PacketTypeHandshake))
-				Expect(p[0].frames).To(Equal([]wire.Frame{sf}))
+				Expect(p[0].header.Type).To(Equal(protocol.PacketTypeInitial))
+				Expect(p[0].frames).To(Equal([]wire.Frame{f}))
 				Expect(p[0].encryptionLevel).To(Equal(protocol.EncryptionInitial))
 			})
 
-			// this should never happen, since non forward-secure packets are limited to a size smaller than MaxPacketSize, such that it is always possible to retransmit them without splitting the StreamFrame
-			It("refuses to send a packet larger than MaxPacketSize", func() {
-				pnManager.EXPECT().PeekPacketNumber().Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)
-				sealingManager.EXPECT().GetSealerWithEncryptionLevel(gomock.Any()).Return(sealer, nil)
-				packet := &ackhandler.Packet{
-					EncryptionLevel: protocol.EncryptionHandshake,
-					Frames: []wire.Frame{
-						&wire.StreamFrame{
-							StreamID: 1,
-							Data:     bytes.Repeat([]byte{'f'}, int(maxPacketSize)),
-						},
-					},
-				}
-				_, err := packer.PackRetransmission(packet)
-				Expect(err).To(HaveOccurred())
-				Expect(err.Error()).To(ContainSubstring("PacketPacker BUG: packet too large"))
-			})
-
 			It("packs a retransmission for an Initial packet", func() {
 				pnManager.EXPECT().PeekPacketNumber().Return(protocol.PacketNumber(0x42), protocol.PacketNumberLen2)
 				pnManager.EXPECT().PopPacketNumber().Return(protocol.PacketNumber(0x42))

session.go

@@ -903,12 +903,7 @@ func (s *session) maybeSendRetransmission() (bool, error) {
 		break
 	}
 
-	if retransmitPacket.EncryptionLevel != protocol.Encryption1RTT {
-		s.logger.Debugf("Dequeueing handshake retransmission for packet 0x%x", retransmitPacket.PacketNumber)
-	} else {
-		s.logger.Debugf("Dequeueing retransmission for packet 0x%x", retransmitPacket.PacketNumber)
-	}
-
+	s.logger.Debugf("Dequeueing retransmission for packet 0x%x", retransmitPacket.PacketNumber)
 	packets, err := s.packer.PackRetransmission(retransmitPacket)
 	if err != nil {
 		return false, err