From 41808516c8a0d9f5d1b8bc6642748657ed36355f Mon Sep 17 00:00:00 2001
From: Marten Seemann <martenseemann@gmail.com>
Date: Thu, 27 Sep 2018 12:11:52 -0600
Subject: [PATCH] merge ACK and ACK_ECN into a single frame

---
 internal/wire/ack_frame.go         |  34 +++++-----
 internal/wire/ack_frame_test.go    | 102 ++++++++++++++---------------
 internal/wire/frame_parser.go      |   9 +--
 internal/wire/frame_parser_test.go |   2 +-
 4 files changed, 68 insertions(+), 79 deletions(-)

diff --git a/internal/wire/ack_frame.go b/internal/wire/ack_frame.go
index 00759db4..8eeb4001 100644
--- a/internal/wire/ack_frame.go
+++ b/internal/wire/ack_frame.go
@@ -19,23 +19,17 @@ type AckFrame struct {
 	DelayTime time.Duration
 }
 
-func parseAckFrame(r *bytes.Reader, version protocol.VersionNumber) (*AckFrame, error) {
-	return parseAckOrAckEcnFrame(r, false, version)
-}
-
-func parseAckEcnFrame(r *bytes.Reader, version protocol.VersionNumber) (*AckFrame, error) {
-	return parseAckOrAckEcnFrame(r, true, version)
-}
-
 // parseAckFrame reads an ACK frame
-func parseAckOrAckEcnFrame(r *bytes.Reader, ecn bool, version protocol.VersionNumber) (*AckFrame, error) {
+func parseAckFrame(r *bytes.Reader, version protocol.VersionNumber) (*AckFrame, error) {
 	if !version.UsesIETFFrameFormat() {
 		return parseAckFrameLegacy(r, version)
 	}
 
-	if _, err := r.ReadByte(); err != nil {
+	typeByte, err := r.ReadByte()
+	if err != nil {
 		return nil, err
 	}
+	ecn := typeByte&0x1 > 0
 
 	frame := &AckFrame{}
 
@@ -50,14 +44,6 @@ func parseAckOrAckEcnFrame(r *bytes.Reader, ecn bool, version protocol.VersionNu
 	}
 	frame.DelayTime = time.Duration(delay*1<<ackDelayExponent) * time.Microsecond
 
-	if ecn {
-		for i := 0; i < 3; i++ {
-			if _, err := utils.ReadVarInt(r); err != nil {
-				return nil, err
-			}
-		}
-	}
-
 	numBlocks, err := utils.ReadVarInt(r)
 	if err != nil {
 		return nil, err
@@ -103,6 +89,16 @@ func parseAckOrAckEcnFrame(r *bytes.Reader, ecn bool, version protocol.VersionNu
 	if !frame.validateAckRanges() {
 		return nil, errInvalidAckRanges
 	}
+
+	// parse (and skip) the ECN section
+	if ecn {
+		for i := 0; i < 3; i++ {
+			if _, err := utils.ReadVarInt(r); err != nil {
+				return nil, err
+			}
+		}
+	}
+
 	return frame, nil
 }
 
@@ -112,7 +108,7 @@ func (f *AckFrame) Write(b *bytes.Buffer, version protocol.VersionNumber) error
 		return f.writeLegacy(b, version)
 	}
 
-	b.WriteByte(0x0d)
+	b.WriteByte(0x1a)
 	utils.WriteVarInt(b, uint64(f.LargestAcked()))
 	utils.WriteVarInt(b, encodeAckDelay(f.DelayTime))
 
diff --git a/internal/wire/ack_frame_test.go b/internal/wire/ack_frame_test.go
index 65c9e4ab..bdcc5a95 100644
--- a/internal/wire/ack_frame_test.go
+++ b/internal/wire/ack_frame_test.go
@@ -13,7 +13,7 @@ import (
 var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 	Context("parsing", func() {
 		It("parses an ACK frame without any ranges", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(100)...) // largest acked
 			data = append(data, encodeVarInt(0)...)   // delay
 			data = append(data, encodeVarInt(0)...)   // num blocks
@@ -28,7 +28,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 
 		It("parses an ACK frame that only acks a single packet", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(55)...) // largest acked
 			data = append(data, encodeVarInt(0)...)  // delay
 			data = append(data, encodeVarInt(0)...)  // num blocks
@@ -43,7 +43,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 
 		It("accepts an ACK frame that acks all packets from 0 to largest", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(20)...) // largest acked
 			data = append(data, encodeVarInt(0)...)  // delay
 			data = append(data, encodeVarInt(0)...)  // num blocks
@@ -58,7 +58,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 
 		It("rejects an ACK frame that has a first ACK block which is larger than LargestAcked", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(20)...) // largest acked
 			data = append(data, encodeVarInt(0)...)  // delay
 			data = append(data, encodeVarInt(0)...)  // num blocks
@@ -69,7 +69,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 
 		It("parses an ACK frame that has a single block", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(1000)...) // largest acked
 			data = append(data, encodeVarInt(0)...)    // delay
 			data = append(data, encodeVarInt(1)...)    // num blocks
@@ -90,7 +90,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 
 		It("parses an ACK frame that has a multiple blocks", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(100)...) // largest acked
 			data = append(data, encodeVarInt(0)...)   // delay
 			data = append(data, encodeVarInt(2)...)   // num blocks
@@ -114,7 +114,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 
 		It("errors on EOF", func() {
-			data := []byte{0xd}
+			data := []byte{0x1a}
 			data = append(data, encodeVarInt(1000)...) // largest acked
 			data = append(data, encodeVarInt(0)...)    // delay
 			data = append(data, encodeVarInt(1)...)    // num blocks
@@ -128,6 +128,46 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 				Expect(err).To(MatchError(io.EOF))
 			}
 		})
+
+		Context("ACK_ECN", func() {
+			It("parses", func() {
+				data := []byte{0x1b}
+				data = append(data, encodeVarInt(100)...)        // largest acked
+				data = append(data, encodeVarInt(0)...)          // delay
+				data = append(data, encodeVarInt(0)...)          // num blocks
+				data = append(data, encodeVarInt(10)...)         // first ack block
+				data = append(data, encodeVarInt(0x42)...)       // ECT(0)
+				data = append(data, encodeVarInt(0x12345)...)    // ECT(1)
+				data = append(data, encodeVarInt(0x12345678)...) // ECN-CE
+				b := bytes.NewReader(data)
+				frame, err := parseAckFrame(b, versionIETFFrames)
+				Expect(err).ToNot(HaveOccurred())
+				Expect(frame.LargestAcked()).To(Equal(protocol.PacketNumber(100)))
+				Expect(frame.LowestAcked()).To(Equal(protocol.PacketNumber(90)))
+				Expect(frame.HasMissingRanges()).To(BeFalse())
+				Expect(b.Len()).To(BeZero())
+			})
+
+			It("errors on EOF", func() {
+				data := []byte{0x1b}
+				data = append(data, encodeVarInt(1000)...)       // largest acked
+				data = append(data, encodeVarInt(0)...)          // delay
+				data = append(data, encodeVarInt(1)...)          // num blocks
+				data = append(data, encodeVarInt(100)...)        // first ack block
+				data = append(data, encodeVarInt(98)...)         // gap
+				data = append(data, encodeVarInt(50)...)         // ack block
+				data = append(data, encodeVarInt(0x42)...)       // ECT(0)
+				data = append(data, encodeVarInt(0x12345)...)    // ECT(1)
+				data = append(data, encodeVarInt(0x12345678)...) // ECN-CE
+				_, err := parseAckFrame(bytes.NewReader(data), versionIETFFrames)
+				Expect(err).NotTo(HaveOccurred())
+				for i := range data {
+					_, err := parseAckFrame(bytes.NewReader(data[0:i]), versionIETFFrames)
+					Expect(err).To(MatchError(io.EOF))
+				}
+			})
+
+		})
 	})
 
 	Context("when writing", func() {
@@ -138,7 +178,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 			}
 			err := f.Write(buf, versionIETFFrames)
 			Expect(err).ToNot(HaveOccurred())
-			expected := []byte{0xd}
+			expected := []byte{0x1a}
 			expected = append(expected, encodeVarInt(1337)...) // largest acked
 			expected = append(expected, 0)                     // delay
 			expected = append(expected, encodeVarInt(0)...)    // num ranges
@@ -149,7 +189,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		It("writes a frame that acks a single packet", func() {
 			buf := &bytes.Buffer{}
 			f := &AckFrame{
-				AckRanges: []AckRange{{Smallest: 0xdeadbeef, Largest: 0xdeadbeef}},
+				AckRanges: []AckRange{{Smallest: 0x1aeadbeef, Largest: 0x1aeadbeef}},
 				DelayTime: 18 * time.Millisecond,
 			}
 			err := f.Write(buf, versionIETFFrames)
@@ -167,7 +207,7 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		It("writes a frame that acks many packets", func() {
 			buf := &bytes.Buffer{}
 			f := &AckFrame{
-				AckRanges: []AckRange{{Smallest: 0x1337, Largest: 0xdeadbeef}},
+				AckRanges: []AckRange{{Smallest: 0x1337, Largest: 0x1aeadbeef}},
 			}
 			err := f.Write(buf, versionIETFFrames)
 			Expect(err).ToNot(HaveOccurred())
@@ -366,45 +406,3 @@ var _ = Describe("ACK Frame (for IETF QUIC)", func() {
 		})
 	})
 })
-
-var _ = Describe("ACK_ECN frame", func() {
-	Context("parsing", func() {
-		It("parses an ACK_ECN frame", func() {
-			data := []byte{0xd}
-			data = append(data, encodeVarInt(100)...)        // largest acked
-			data = append(data, encodeVarInt(0)...)          // delay
-			data = append(data, encodeVarInt(0x42)...)       // ECT(0)
-			data = append(data, encodeVarInt(0x12345)...)    // ECT(1)
-			data = append(data, encodeVarInt(0x12345678)...) // ECN-CE
-			data = append(data, encodeVarInt(0)...)          // num blocks
-			data = append(data, encodeVarInt(10)...)         // first ack block
-			b := bytes.NewReader(data)
-			frame, err := parseAckEcnFrame(b, versionIETFFrames)
-			Expect(err).ToNot(HaveOccurred())
-			Expect(frame.LargestAcked()).To(Equal(protocol.PacketNumber(100)))
-			Expect(frame.LowestAcked()).To(Equal(protocol.PacketNumber(90)))
-			Expect(frame.HasMissingRanges()).To(BeFalse())
-			Expect(b.Len()).To(BeZero())
-		})
-
-		It("errors on EOF", func() {
-			data := []byte{0x1a}
-			data = append(data, encodeVarInt(1000)...)       // largest acked
-			data = append(data, encodeVarInt(0)...)          // delay
-			data = append(data, encodeVarInt(0x42)...)       // ECT(0)
-			data = append(data, encodeVarInt(0x12345)...)    // ECT(1)
-			data = append(data, encodeVarInt(0x12345678)...) // ECN-CE
-			data = append(data, encodeVarInt(1)...)          // num blocks
-			data = append(data, encodeVarInt(100)...)        // first ack block
-			data = append(data, encodeVarInt(98)...)         // gap
-			data = append(data, encodeVarInt(50)...)         // ack block
-			_, err := parseAckEcnFrame(bytes.NewReader(data), versionIETFFrames)
-			Expect(err).NotTo(HaveOccurred())
-			for i := range data {
-				_, err := parseAckEcnFrame(bytes.NewReader(data[0:i]), versionIETFFrames)
-				Expect(err).To(MatchError(io.EOF))
-			}
-		})
-
-	})
-})
diff --git a/internal/wire/frame_parser.go b/internal/wire/frame_parser.go
index ce558e36..1990aca0 100644
--- a/internal/wire/frame_parser.go
+++ b/internal/wire/frame_parser.go
@@ -86,11 +86,6 @@ func parseIETFFrame(r *bytes.Reader, typeByte byte, v protocol.VersionNumber) (F
 		if err != nil {
 			err = qerr.Error(qerr.InvalidFrameData, err.Error())
 		}
-	case 0xd:
-		frame, err = parseAckFrame(r, v)
-		if err != nil {
-			err = qerr.Error(qerr.InvalidAckData, err.Error())
-		}
 	case 0xe:
 		frame, err = parsePathChallengeFrame(r, v)
 		if err != nil {
@@ -101,8 +96,8 @@ func parseIETFFrame(r *bytes.Reader, typeByte byte, v protocol.VersionNumber) (F
 		if err != nil {
 			err = qerr.Error(qerr.InvalidFrameData, err.Error())
 		}
-	case 0x1a:
-		frame, err = parseAckEcnFrame(r, v)
+	case 0x1a, 0x1b:
+		frame, err = parseAckFrame(r, v)
 		if err != nil {
 			err = qerr.Error(qerr.InvalidAckData, err.Error())
 		}
diff --git a/internal/wire/frame_parser_test.go b/internal/wire/frame_parser_test.go
index ca89d575..5543b04c 100644
--- a/internal/wire/frame_parser_test.go
+++ b/internal/wire/frame_parser_test.go
@@ -344,11 +344,11 @@ var _ = Describe("Frame parsing", func() {
 				0x09: qerr.InvalidBlockedData,
 				0x0a: qerr.InvalidFrameData,
 				0x0c: qerr.InvalidFrameData,
-				0x0d: qerr.InvalidAckData,
 				0x0e: qerr.InvalidFrameData,
 				0x0f: qerr.InvalidFrameData,
 				0x10: qerr.InvalidStreamData,
 				0x1a: qerr.InvalidAckData,
+				0x1b: qerr.InvalidAckData,
 			} {
 				_, err := ParseNextFrame(bytes.NewReader([]byte{b}), nil, versionIETFFrames)
 				Expect(err).To(HaveOccurred())