Subject: Re: [quicwg/base-drafts] Proposal for adding ECN support to QUIC. (#1372)

From: Kazuho Oku <[email-masked]>
Date: Fri, 01 Jun 2018 07:39:27 -0700

kazuho commented on this pull request.

> +
+The receiver side should implement three 64-bit counters that are copied to the
+ECN block when an ACK_ECN frame is generated:
+
+ECT_0:
+: Initial value = 0, incremented when a packet marked ECT(0) is received
+
+ECT_1:
+
+: Initial value = 0, incremented when a packet marked ECT(1) is received
+
+CE:
+
+: Initial value = 0, incremented when a packet marked CE is received
+
+Reception of duplicate packets SHOULD NOT increment the counters.

Can we count the packet (for all three counters) *before* detecting duplicates instead?

IMO, theoretically a receiver should send back all the information it has obtained. Ignoring the CE flag in duplicate packets goes against that principle. 

Counting packets after detecting duplicates makes sense only when we can assume that duplication happens near to the receiver than where ECN is applied. I do not think that we have such an assumption.

In practice, the recovery draft with the proposed changes only uses the CE counter to detect if _any_ congestion was reported by the ACK_ECN frame (it's essentially a binary flag). Considering that, I do not think that changing the moment of counting the packets causes any harm.

And lastly but importantly, by changing the rule as such, we can get rid of the requirement to detect duplicates.

See also: https://github.com/quicwg/base-drafts/issues/1405#issuecomment-393743048

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-125194246

---

From: Lars Eggert <[email-masked]>
Date: Sat, 02 Jun 2018 06:43:26 -0700

larseggert commented on this pull request.

Agree

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r192559987

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 06 Jun 2018 03:09:56 -0700

gloinul commented on this pull request.

Disagree, see my answer on issue #1405 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r193360841

---

From: Lars Eggert <[email-masked]>
Date: Wed, 06 Jun 2018 05:08:28 -0700

ah, yes

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r193386151

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 07 Jun 2018 09:29:31 +0000 (UTC)

@gloinul pushed 3 commits.

0f94847  Merge branch 'master' of https://github.com/quicwg/base-drafts into ecn
0e7d43b  Merge branch 'master' of https://github.com/quicwg/base-drafts into ecn
822aded  Changed so that additional ECN-CE marks are sent in immediate ACKs to avoid unclarity in regards to CE marks and receovery period.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/0bb5bad97e1d13094463938fcefaa8917f23ffb6..822aded3768e1a1a49a8e9632fe9cc96cbd8fe00

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 07 Jun 2018 03:13:41 -0700

@gloinul pushed 1 commit.

345f593  Fixed lint issues

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/822aded3768e1a1a49a8e9632fe9cc96cbd8fe00..345f593a92e4455989ada8ff7029e3935f5696d8

---

From: Magnus Westerlund <[email-masked]>
Date: Fri, 08 Jun 2018 07:39:37 -0700

e60cc8f  Adding security consideration around ECN into transport.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/345f593a92e4455989ada8ff7029e3935f5696d8..e60cc8fed8dc5b1d5d819b3ae3013715fcbbda5f

---

From: ianswett <[email-masked]>
Date: Mon, 11 Jun 2018 07:10:58 -0700

ianswett commented on this pull request.

Looks good, some editorial comments.

> @@ -417,6 +417,13 @@ to accelerate loss recovery.  The receiver SHOULD send an immediate ACK
 when it receives a new packet which is not one greater than the
 largest received packet number.
 
+Also, reception of an packet marked as ECN Congestion Experience
+(ECN-CE) SHOULD be acknowledged immediately to quicker react to
+congesiton events. Additional ECN-CE marks received during the same

congesiton -> congestion

> @@ -417,6 +417,13 @@ to accelerate loss recovery.  The receiver SHOULD send an immediate ACK
 when it receives a new packet which is not one greater than the
 largest received packet number.
 
+Also, reception of an packet marked as ECN Congestion Experience

I might say "Similarly," instead of "Also," since this is similar to the preceding paragraph.

> @@ -880,13 +892,31 @@ QUIC hosts MUST NOT send packets if they would increase bytes_in_flight
 the packet is a probe packet sent after the TLP or RTO alarm fires, as described
 in {{tlp}} and {{rto}}.
 
+## Explicit Congestion Notification {#congestion-ecn}
+
+If ECN {!RFC3168} has been verified to work for the current path QUIC
+will use the ECN Congestion Experienced (ECN-CE) IP packet marking as a
+signal of congestion as a complement to packet loss. This document
+specifies to use the classical ECN-CE response, i.e. the same response
+as for packet loss. However, there exist potential for future
+experimentation in using other response functions as discussed in
+{!RFC8311}.
+
+The ACK_ECN frame defined in {{QUIC-TRANSPORT}} does not provide
+information on which of the newly acknowledged packets that
+was marked with ECN-CE. Therefore, it will be assumed that

I would suggest rewriting as: "Because newly received marks cause an ACK_ECN be sent immediately, is is assumed the congestion event starts at the highest newly acknowledged packet number."

> @@ -977,6 +1009,12 @@ kLossReductionFactor (default 0.5):
 Variables required to implement the congestion control mechanisms
 are described in this section.
 
+ack_ce_cntr:
+: The ACK_ECN counter for ECN-CE marks previously processed. Used to
+  determine when one or more packet acknowledged by the ACK_ECN frame
+  was marked with ECN-CE.
+

extra linebreak?

> @@ -977,6 +1009,12 @@ kLossReductionFactor (default 0.5):
 Variables required to implement the congestion control mechanisms
 are described in this section.
 
+ack_ce_cntr:
+: The ACK_ECN counter for ECN-CE marks previously processed. Used to

I might suggest calling this previous_ecn_ce or previous_ecn_ce_ctr

> @@ -1042,6 +1081,26 @@ acked_packet from sent_packets.
          kDefaultMss * acked_packet.bytes / congestion_window
 ~~~
 
+### On Packets Marked
+
+Invoked by an increment in the number of CE marked packets, as
+indicated by a newly received ACK_ECN frame when compared to
+ack_ce_cntr.
+
+~~~
+   OnPacketsMarked(ce_counter):
+     if (ce_counter > ack_ce_cntr):
+       // update ack_ce_cntr
+       ack_ce_cntr = ce_counter
+     if (!InRecovery(largest_acked_packet)):

I think we want to use largest newly acked packet ideally.

Also, possibly create a single method like "MaybeStartCongestionEvent" to avoid the code duplication between these methods.

> @@ -1061,20 +1063,21 @@ negotiation, which is the same no matter which reserved version was sent.
 A server MAY therefore send different reserved version numbers in the Version
 Negotiation Packet and in its transport parameters.
 
-A client MAY send a packet using a reserved version number.  This can be used to
-solicit a list of supported versions from a server.
+A client MAY send a packet using a reserved version number. This can be

This change and many that follow are unrelated to this PR.  Can you revert them to make this PR smaller?

> @@ -1410,6 +1416,71 @@ a single packet.
 
 In TLS, the Retry packet type is used to carry the HelloRetryRequest message.
 
+## ECN capability check {#ecn-capability-check}
+
+Explicit Congestion Notification (ECN) {{!RFC3168}} is feature that allows for
+non-destructive congestion notification by a network node.  That is, packets are
+marked instead of being discarded by routers and other devices along a network
+path. QUIC endpoints perform capability verification on connection establishment
+as well as connection migration. The feature can be used unidirectional or

This sentence is awkward, maybe "ECN can be used unidirectionally, bidirectionally, or disabled depending on endpoint and path capabilities."?

> +endpoint individually performs an ECN capability check for its send path. The
+check is performed by attempting to set the ECN bits in the IP header to either
+ECT(0) or ECT(1) for packets generated by the endpoint, clients starting with
+the Initial packet ({{packet-initial}}) and servers with the Handshake packet
+(server) ({{packet-handshake}}). Note that Retry Packets ({{packet-retry}}) can
+be marked as ECT, but as they are not acknowledged by the peer, they provide no
+capability verification.
+
+The choice if ECT(0) and ECT(1) should be used is as per the guidelines in
+{{!RFC8311}}. ECT(0) is the default, if no ECN experiments are run. Upon
+reception of a packet with ECT(0), ECT(1) or ECN-CE marking by the peer
+endpoint, an ACK_ECN frame is transmitted back. This ACK_ECN frame indicates how
+many packets that are marked ECT(0), ECT(1) or ECN-CE. Thus the endpoint can
+determine if the endpoints and path is ECN capable.
+
+The ACK_ECN frame will, when received, confirm that the path direction supports

How about: "When ACK_ECN is received, the counter values can be used to confirm the expected number of marks were received."

> +packets that was marked as ECT in the sender, or if the comparision of the
+counters with the total of acknolwedged packets indicates that not all packets
+arrive as ECT(0), ECT(1) or ECN-CE, then the path is determined as not ECN
+Capable.  If the endpoint determine the path as currently not ECN capable it
+SHALL stop marking the packets as ECT, and instead mark them as Not-ECT. If the
+connection is migrated then the ECN capability check is rerun as specified in
+{{ecn-connection-migration}}.
+
+It is expected that QUIC discards duplicate packets early, however if that is
+not the case **\[ED note, have not seen any clear statement in the drafts]**,
+then it should be verified that the number of ECT marked packets are equal to or
+larger that the amount of ECT marked packets that have been transmitted.
+
+### Continous Verification of ECN {#ecn-continous-verification}
+
+If the ECN capabiity check was successful and the endpoint continus to send ECT

continues

> +connection is migrated then the ECN capability check is rerun as specified in
+{{ecn-connection-migration}}.
+
+It is expected that QUIC discards duplicate packets early, however if that is
+not the case **\[ED note, have not seen any clear statement in the drafts]**,
+then it should be verified that the number of ECT marked packets are equal to or
+larger that the amount of ECT marked packets that have been transmitted.
+
+### Continous Verification of ECN {#ecn-continous-verification}
+
+If the ECN capabiity check was successful and the endpoint continus to send ECT
+marked packets then continous verification is applied. This is to detect any
+cases when ECN field is bleached, that is, zeroed out by a network node, likely
+as the result of a routing changes since the ECN capability check.
+
+For each ACK_ECN frame that is received, the total number of ACKed packets are

Suggestion: "For each received ACK_ECN frame, the total number of newly acknowledged packets can be compared to the total increase in ECN counters.  If the increase in ECN counters is larger, then an ECN failure has occurred and ECN should be disabled."

> @@ -2816,6 +2960,10 @@ ACK Block (repeated):
 
 ### Sending ACK Frames
 
+ACK Frames MAY be sent when all packets to be acknowledge had an IP header with

acknowledged

> @@ -2816,6 +2960,10 @@ ACK Block (repeated):
 
 ### Sending ACK Frames
 
+ACK Frames MAY be sent when all packets to be acknowledge had an IP header with
+the ECN field marked as Not-ECT. If any packets where marked as ECT or ECN-CE

where -> were

> @@ -2816,6 +2960,10 @@ ACK Block (repeated):
 
 ### Sending ACK Frames
 
+ACK Frames MAY be sent when all packets to be acknowledge had an IP header with

Also, I think leading with this paragraph here is a bit confusing.  I would prefer not adding anything here about ACK_ECN and placing text in the ACK_ECN section that must be used when new marks are received.

> @@ -4196,6 +4416,23 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 the limit too low could affect performance when applications expect to open
 large number of streams.
 
+## Explicit Congestion Notification Attacks
+
+The ECN bits {{!RFC3168}} are an unauthenticated signal from the network. An
+on-path attacker may manipulate the value of the field. Thus, affecting the

I think this reads more clearly as one sentence.   ie: "An on-path attacker may manipulate the value of the field, affecting the congestion avoidance behavior of the sender."

> @@ -4196,6 +4416,23 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 the limit too low could affect performance when applications expect to open
 large number of streams.
 
+## Explicit Congestion Notification Attacks
+
+The ECN bits {{!RFC3168}} are an unauthenticated signal from the network. An
+on-path attacker may manipulate the value of the field. Thus, affecting the
+congestion avoidance behavior of the sender. By clearing any CE marks the
+connection can help drive a bottle neck queue into a loss regime. By setting
+the ECN field to CE marking it can drive down the senders congestion window
+thus resulting in reduced throughput. The later could equally be accomplished
+by dropping packets for the connection. Section 18 and 19 of {{!RFC3168}}
+discusses the effects of undesired manipulation of the ECN field in more
+details.
+
+If a receiver would not have packet duplication detection and not discard any

How about "If a receiver does not discard duplicate packets, an off-path attacker can retransmit packets with ECN bits set and manipulate the senders congestion avoidance state."

> @@ -4196,6 +4416,23 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 the limit too low could affect performance when applications expect to open
 large number of streams.
 
+## Explicit Congestion Notification Attacks
+
+The ECN bits {{!RFC3168}} are an unauthenticated signal from the network. An
+on-path attacker may manipulate the value of the field. Thus, affecting the
+congestion avoidance behavior of the sender. By clearing any CE marks the
+connection can help drive a bottle neck queue into a loss regime. By setting
+the ECN field to CE marking it can drive down the senders congestion window
+thus resulting in reduced throughput. The later could equally be accomplished
+by dropping packets for the connection. Section 18 and 19 of {{!RFC3168}}
+discusses the effects of undesired manipulation of the ECN field in more
+details.
+
+If a receiver would not have packet duplication detection and not discard any
+duplicates an off-path attacker that can receive copies of the connection's
+packets can manipulate the senders congestion avoidance state. If packet

packet duplicates -> duplicate packets

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-127544398

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 11 Jun 2018 18:34:06 -0700

#1439 proposes a different scheme that removes the requirement on the receiver-side to detect duplicates.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r194593812

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 01:58:29 -0700

fdf9df7  Reverting some rewrapping that are not necessary to reduce clutter.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/e60cc8fed8dc5b1d5d819b3ae3013715fcbbda5f..fdf9df7f91ea314d4e42e246150e2341001d84f8

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 01:59:11 -0700

I have reverted those that aren't necessary to make lint happy. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r194663034

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 02:26:33 -0700

+### On Packets Marked
+

Agreed on both, I have reshuffled the pseudocode and introduced a function CongestionEvent. I also noticed that InRecovery appears to be ending one packet to early: https://github.com/quicwg/base-drafts/issues/1440

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r194671437

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 09:33:52 +0000 (UTC)

Yes, except that larger needs to be less in this case. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r194673645

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 09:41:08 +0000 (UTC)

### Sending ACK Frames

Okay, I removed this paragraph, and moved the MAY statement to the ACK_ECN frame, the MUST statement was already present. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r194675822

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 09:48:28 +0000 (UTC)

ec97fd8  Addressing issues raised by Ian Sweet on 180611. Editorial improvements, new psuedo code function.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/fdf9df7f91ea314d4e42e246150e2341001d84f8..ec97fd81158d29261e3ea4bb6e80a64efc0aa7a2

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jun 2018 07:05:58 -0700

Some small comments, but I think this looks good

>  (ECN-CE) SHOULD be acknowledged immediately to quicker react to
-congesiton events. Additional ECN-CE marks received during the same
+congestion events. Additional ECN-CE marks received during the same

I don't think you need to say "during the same recovery period" since we're not proscribing any receiver side behavior corresponding to whether the marks are received within the same recovery period.

> @@ -904,9 +904,9 @@ experimentation in using other response functions as discussed in
 
 The ACK_ECN frame defined in {{QUIC-TRANSPORT}} does not provide
 information on which of the newly acknowledged packets that

nit: remove "that"

 ~~~
-   OnPacketsMarked(ce_counter):
-     if (ce_counter > ack_ce_cntr):
-       // update ack_ce_cntr
-       ack_ce_cntr = ce_counter
-     if (!InRecovery(largest_acked_packet)):
+   CongestionEvent(packet_number):

I'd prefer OnCongestionEvent to be consistent with the other names.

> @@ -1442,16 +1441,15 @@ endpoint, an ACK_ECN frame is transmitted back. This ACK_ECN frame indicates how
 many packets that are marked ECT(0), ECT(1) or ECN-CE. Thus the endpoint can
 determine if the endpoints and path is ECN capable.
 
-The ACK_ECN frame will, when received, confirm that the path direction supports
-ECN if the counters show a correct amount of packets received for a valid and
-expected counter combination. ECT marked packets can become remarked as CE along
-the path between the peers. The ACK_ECN counters are added together and compared
-to total number of ACKed packets, if the sum of the counters are equal too or
-larger than the number of ACKed packets then the path is ECN capable. This
-capability check thus has verified that one direction of the path between the
-peers is free from issues with ECN bleaching and that the application does not
-experience problems with access to the ECN field in the IP header. In this case
-the marking of the generated packets with ECT continues.
+When ACK_ECN is received, the counter values can be used to confirm the expected
+number of marks were received. ECT marked packets can become remarked as CE
+along the path between the peers. The ACK_ECN counters are added together and
+compared to total number of ACKed packets, if the sum of the counters are equal
+too or larger than the number of ACKed packets then the path is ECN capable.

too -> to, or just remove the too entirely

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-127977693

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 08:43:54 -0700

bf30190  Clarifying that both cases are capability checks. Correcting a number of editorial things.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/ec97fd81158d29261e3ea4bb6e80a64efc0aa7a2..bf30190128543f0349d8d0b8391b1213c7ebeb3f

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 12 Jun 2018 15:58:28 +0000 (UTC)

60468a7  Fixed a too too much

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/bf30190128543f0349d8d0b8391b1213c7ebeb3f..60468a79507a1d36a07825b80222da03773e571b

---

From: ianswett <[email-masked]>
Date: Tue, 12 Jun 2018 10:51:48 -0700

ianswett approved this pull request.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-128078553

---

From: Martin Thomson <[email-masked]>
Date: Tue, 12 Jun 2018 16:09:44 -0700

martinthomson commented on this pull request.

 Details of how TLS is integrated with QUIC is provided in more detail in
 {{QUIC-TLS}}.

 ## Transport Parameters
 
-During connection establishment, both endpoints make authenticated declarations
-of their transport parameters.  These declarations are made unilaterally by each
-endpoint.  Endpoints are required to comply with the restrictions implied by
-these parameters; the description of each parameter includes rules for its
-handling.
+During connection establishment, both endpoints make authenticated
+declarations of their transport parameters. These declarations are made
+unilaterally by each endpoint. Endpoints are required to comply with the
+restrictions implied by these parameters; the description of each
+parameter includes rules for its handling.
+

Please keep to one blank line between paragraphs.

> @@ -1410,6 +1416,68 @@ a single packet.

Title Case for Headings

> @@ -2882,6 +3020,83 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in
+the IP header. If the header is not readable from the application, the
+codepoint 00 (Not-ECT) MUST be assumed.
+
+ACK_ECN Frame MUST be used when when an endpoint is acknowledging a packet were
+the IP header ECN field was marked as ECT(0), ECT(1) or ECN-CE when received.

oxford comma

> @@ -4196,6 +4410,21 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 
+## Explicit Congestion Notification Attacks
+
+An on-path attacker may manipulate the value of the field, affecting the
+congestion avoidance behavior of the sender. By clearing any CE marks the
+connection can help drive a bottle neck queue into a loss regime. By setting

Removing any ECN-CE marking causes senders to maintain or increase their sending rate beyond that the path can sustain, which will eventually result in loss.  Adding an ECN-CE marking causes senders to reduce their sending rate.

> +|              Number of ECT(0) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECT(1) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECN-CE marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ECN-BLOCK-FIG title="ECN Block"}
+
+
+### ECN counters
+
+The receiver side should implement three 64-bit counters that are copied to the
+ECN block when an ACK_ECN frame is generated:
+
+ECT_0:

Please make these match the diagram.  "ECT(0) Count" might be more concise.

> + 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECT(0) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECT(1) marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|              Number of ECN-CE marked packets (i)            ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ECN-BLOCK-FIG title="ECN Block"}
+
+
+### ECN counters
+
+The receiver side should implement three 64-bit counters that are copied to the
+ECN block when an ACK_ECN frame is generated:

Rather than phrase this as "implement these counters" just define the fields in the frame.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-128169112

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 13 Jun 2018 01:27:36 -0700

f577c8e  Addressing Martin Thomson's comments. Several editorial changes. The biggest on the ECN block definition.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/60468a79507a1d36a07825b80222da03773e571b..f577c8e94d32848feb5cd1582b3ad4b2cc32999c

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 13 Jun 2018 06:51:47 -0700

2d76935  Added paragraph on dealing with persistent loss of acknowledgement of some sent packets. Removed editorial note that are covered by issue #1402.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/f577c8e94d32848feb5cd1582b3ad4b2cc32999c..2d76935cb8b2f96c4378e86852ae7376cbbb5f91

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 13 Jun 2018 13:57:08 +0000 (UTC)

Note, I have just added a paragraph for an issue related to persistent ACK loss for some packets. Such events causes an insensitiveness in the continuous verification equal to the number of packets that the receiver seen with ECN markings and never acknowledged in the sender. Please review this late addition. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#issuecomment-396946546

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 13 Jun 2018 07:02:53 -0700

4af108c  Fixing typo in new paragraph.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/2d76935cb8b2f96c4378e86852ae7376cbbb5f91..4af108c47fd2cbaea42b6b83c8d19f1d52513443

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 18 Jun 2018 06:34:02 -0700

Based on the discussion in #1405 and the text that is in master branch towards next version requiring frame types to be idempotent I will add text so that the ACK_ECN frame will have this property even if packet duplication occurs. I will do so in what I consider a straight forward and simple way. I will require that only the first received ECN field for an packet number is added to the counters. This should be straightforward as the receiver's ACK data structure should allow one to determine if a packet is older than anything retained, new or duplicate. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#issuecomment-398055463

---

From: mirjak <[email-masked]>
Date: Mon, 18 Jun 2018 06:36:43 -0700

Actually if possible, CE should never be ignored because a network node setting CE does not know if a packet is a duplicate or not...

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#issuecomment-398056289

---

From: ianswett <[email-masked]>
Date: Mon, 18 Jun 2018 13:44:53 +0000 (UTC)

@mirjak  In general, QUIC doesn't process duplicate packets and QUIC doesn't ACK anything it can't process, so ignoring duplicate marked packets matches with this model.

I suspect a lot more marked packets will be dropped because they were undecryptable(ie: 0RTT) than because of duplication, since duplication is very rare in my experience, and when it does occur, it'd commonly be better to ignore the second mark anyway, since the marking would occur before the duplication.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#issuecomment-398058825

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 18 Jun 2018 07:14:56 -0700

@gloinul pushed 2 commits.

55fa3d6  Merge branch 'master' of https://github.com/quicwg/base-drafts into ecn
e06a2a0  Added requirement on ECN marking suppression for packet duplicates.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/4af108c47fd2cbaea42b6b83c8d19f1d52513443..e06a2a0efb1e655ecd1fa4e3a8e182689a50ff54

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 18 Jun 2018 07:22:56 -0700

> @@ -2887,6 +3033,95 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording
+the number of packets that were received with the corresponding ECN
+codepoint in the IP header. If the header is not readable from the
+application, the codepoint 00 (Not-ECT) MUST be assumed. If any packet
+are duplicated by the network then only the value of the ECN field of
+the packet copy first received SHALL be included in the counters, the ECN
+field value for a duplicate SHALL be ignored. This to prevent the

Clarification question: considering the use of "SHALL," does this imply that an endpoint MUST detect all duplicates?

I understand that it is possible to implement such a scheme; an endpoint will keep track of the cut-off PN (the biggest PN that it would ignore) as well as all the PNs that it have seen above the cut-off PN. But I wonder if the intent here is to require all implementations to have such defense.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-129584511

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 18 Jun 2018 07:40:13 -0700

to decipher the packet.

Yes, that is the intention, to avoid getting ECN field values for duplicates added to the counters for packets that are included in any ACK. So if a receiver has included it in an ACK, it need to remember that this PN already have an ECN field valued added to the counters. But it doesn't need to detect duplicates for packet that are discarded directly after decryption, i.e. older than the cut-off PN.   

This is straightforward and fulfills the requirements that appears to currently exist. If #1405 concludes it would be easier to determine if there are other approaches including other ECN field value encodings as discussed in #1439.  

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196103436

---

From: Kazuho Oku <[email-masked]>
Date: Mon, 18 Jun 2018 14:59:41 +0000 (UTC)

to decipher the packet.

Thank you for the clarification. My two cents go to that this is an unnecessary burden, because IMO while idempotency is required for frame payloads, it is not required for ECN signal that is injected by an on-path device (even though we need some defense against injection attacks).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196110762

---

From: mirjak <[email-masked]>
Date: Mon, 18 Jun 2018 08:09:45 -0700

mirjak commented on this pull request.

 to decipher the packet.

This is incorrect. The requirements that currently exists in tsv is that ALL CEs must be counted because the network does not know if a packet is a duplicate or not!

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196114651

---

From: ianswett <[email-masked]>
Date: Mon, 18 Jun 2018 15:13:59 +0000 (UTC)

to decipher the packet.

I believe the reason TCP counts all CEs is because TCP retransmits packets, not because of an effort to count packets the network inadvertently duplicated.  QUIC does not duplicate packets and instead uses new packet numbes for retransmissions, so I believe this embodies the intent of the requirement.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196116256

---

From: mirjak <[email-masked]>
Date: Mon, 18 Jun 2018 08:21:27 -0700

to decipher the packet.

No, that is not true. The network marks a packet as congested because it is congested and want to signal this to the endpoint. The network does not distinguish between retransmission or duplicates or anything because all these packets need the same amount of network resources and therefore cause congestion. RFC8085 even says 
"A receiver that enables the use of ECN for a UDP port MUST check
      the ECN field at the receiver for each UDP datagram that it
      receives on this port." 
and 
"This MUST report the
      presence of datagrams received with a CE-mark by providing a
      mechanism to feed this congestion information back to the sending
      application."
You cannot just ignore an CE because it's on a retransmission or a duplicated packet!

The problem with duplicates it that a packet could have been duplicated before or after it was CE marked. If it gets duplicated after it was marked, this is not a real congestion indication and if counted overstates the congestion. However, if it gets marked after it was duplicated, it is an indication of congestion. As you don't know when it got marked, you need to be conservative and account for it.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196119153

---

From: mirjak <[email-masked]>
Date: Mon, 18 Jun 2018 08:24:56 -0700

to decipher the packet.

Also QUIC does process duplicate packets to the point where it can detect that it is duplicated which probably already gives you some confidence that it is a valid quic packet and therefore you can also check the ECN IP mark at this point of time.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196120477

---

From: ianswett <[email-masked]>
Date: Mon, 18 Jun 2018 15:28:20 +0000 (UTC)

to decipher the packet.

Fair enough, then I think that text wasn't written with QUIC in mind.

As I mentioned previously, I think this is an edge case that is not a practical problem in real networks.  Unnecessary packet duplication is the real problem, not it's implications on ECN.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196121655

---

From: mirjak <[email-masked]>
Date: Mon, 18 Jun 2018 08:35:16 -0700

to decipher the packet.

I agree that this is an edge case and should hopefully not be a problem but saying in the spec that duplicated shall be ignored is not the right thing to do.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196124349

---

From: janaiyengar <[email-masked]>
Date: Mon, 18 Jun 2018 18:14:23 -0700

janaiyengar commented on this pull request.

 to decipher the packet.

Let's back up just a bit.

First, QUIC receivers are far less likely to receive duplicates because as @ianswett pointed out, retransmissions are not duplicates. As @mirjak notes, this doesn't mean that the network won't do stupid things, but that really is the exception, not the rule. Duplicates in QUIC are necessarily network duplication. This is a significant difference in terms of *how much* this problem matters. I think suppression is fine, since the tradeoff is between enabling an on-the-side performance attack vs. an infinitesimally more aggressive response to a CE marked packet.

Second, I agree with @kazuho that requiring duplicate detection should not be required, if only because I can cook up conditions under which an endpoint won't be able to do this without infinite state.  Consider an endpoint that nacks a packet (sends an ack with this packet in a gap), receives an ack for this nack, and moves its "left edge" past that PN. Now, it receives a packet with that PN. It cannot verify that this packet is a duplicate, since it may never have received the original.

I'd like to point out that this is ultimately a performance attack, and as with any other such attacks that use unauthenticated bits (ICMP?), we can provide some guidance to the receiver of a duplicate. I think the text here can changed a bit, with little to no real difference in impact.

I'll propose the following text: "The ECN counters that an endpoint reports include values of the ECN field only from packets that are subsequently newly acknowledged.  Note that an endpoint may newly acknowledge a duplicate packet if it no longer has state about the first receipt of the same packet. An endpoint may decrypt and drop a duplicate packet if it has state about the first receipt of the same packet, and in this case, the endpoint will not increase the ECN counters based on the duplicate packet."

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196270229

---

From: Martin Thomson <[email-masked]>
Date: Mon, 18 Jun 2018 23:39:24 -0700

to decipher the packet.

The problem with that text is that it assumes something quite dangerous: that a receiver might accept a packet when it isn't sure that it has received it before.  And content might be different.

What @ianswett suggested before is better: if you move the trailing edge forward, then packets that you believe to be behind that edge are dropped unconditionally.  Or, more concisely stated: you only accept packets that you know you haven't accepted before.  That errs on the side of dropping rather than accepting, which I believe to be safe.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196308696

---

From: Kazuho Oku <[email-masked]>
Date: Tue, 19 Jun 2018 01:09:48 -0700

to decipher the packet.

> The problem with that text is that it assumes something quite dangerous: that a receiver might accept a packet when it isn't sure that it has received it before. And content might be different.

I am not sure if the content could be different. I'd assume that the only case in which it could be different is when the sender reuses a packet number. I do not think that we need to detect that (and if we do, it should be a PROTOCOL_VIOLATION rather than a drop).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196330195

---

From: Martin Thomson <[email-masked]>
Date: Tue, 19 Jun 2018 02:08:48 -0700

There are a few sections here that seem to be quite verbose.  Nothing technically wrong, but I think that fewer words would be OK.

> @@ -1042,6 +1080,31 @@ acked_packet from sent_packets.
 
+### On Packets Marked
+
+previous_ecn_ce_ctr.

This looks like a generic function (it's invoked from OnPacketsLost).  It should probably be moved to a common section..

> @@ -1125,21 +1129,22 @@ multiple packets.  Reassembling messages requires that a server maintain state
 prior to establishing a connection, exposing the server to a denial of service
 risk.
 
-The first client packet of the cryptographic handshake protocol MUST fit within
-a 1232 octet QUIC packet payload.  This includes overheads that reduce the space
-available to the cryptographic handshake protocol.
+
+The first client packet of the cryptographic handshake protocol MUST fit
+within a 1232 octet QUIC packet payload. This includes overheads that
+reduce the space available to the cryptographic handshake protocol.

Not sure what is going on here.  Looks like gratuitous reflow.

> @@ -1427,6 +1432,77 @@ a single packet.

+## ECN Capability Check {#ecn-capability-check}
+

non-sequitur: this moves from saying what ECN is to capability verification.  Suggest: "QUIC endpoints determine whether a path correctly supports ECN marking by verifying paths when connections are established and when migrating the connection to a new path."

> @@ -1427,6 +1432,77 @@ a single packet.

+## ECN Capability Check {#ecn-capability-check}
+
+Explicit Congestion Notification (ECN) {{!RFC3168}} is feature that allows for
+non-destructive congestion notification by a network node.  That is, packets are
+marked instead of being discarded by routers and other devices along a network
+path. QUIC endpoints perform capability verification on connection establishment
+as well as connection migration. ECN can be used unidirectionally,
+bidirectionally, or disabled depending on endpoint and path capabilities.

This is odd, but you might instead say "Each peer independently validates network paths, which leads to ECN being enabled separately for each direction on a path."

> @@ -1427,6 +1432,77 @@ a single packet.

+## ECN Capability Check {#ecn-capability-check}
+
+Explicit Congestion Notification (ECN) {{!RFC3168}} is feature that allows for
+non-destructive congestion notification by a network node.  That is, packets are
+marked instead of being discarded by routers and other devices along a network
+path. QUIC endpoints perform capability verification on connection establishment
+as well as connection migration. ECN can be used unidirectionally,
+bidirectionally, or disabled depending on endpoint and path capabilities.
+
+The capability check makes use of the ACK_ECN frame in {{frame-ack-ecn}}. Each
+endpoint individually performs an ECN capability check for its send path. The
+check is performed by attempting to set the ECN bits in the IP header to either
+ECT(0) or ECT(1) for packets generated by the endpoint, clients starting with

I'm struggling a little with the length of this sentence, and I think that we could be a little more concise with the remainder of this section:

"""
ECN is validated by setting the ECT(0) or ECT(1) bit in the IP header, following the guidelines of {{!RFC8311}}.  Clients and servers both mark the IP packet containing an Initial packet ({{packet-initial}}).  If ECN is supported, the recipient of a marked packet will increase the corresponding counter and send an ACK_ECN frame.  

An endpoint uses the ACK_ECN frame to verify that the ECT markings were received by its peer.  The endpoint counts the number of acknowledged QUIC packets that were sent in marked IP packets.  If the counters in the ACK_ECN frame match that number, the path and peer do not remove ECN markings.  The endpoint records this path as being ECN capable and continues to mark IP packets that it sends.

If an endpoint receives an ACK frame, indicating that no markings were received, or the counter in the ACK_ECN frame do not match the number of QUIC packets that were sent in marked IP packets, then the path or peer remove ECN markings.  The endpoint records this path as not being ECN capable and it ceases marking of packets.

IP packets sent on a new path SHOULD be marked with ECT(0) or ECT(1) to verify that the new path supports ECN, see {{ecn-connection-migration}}.
"""

Note that I've chosen to assume the stream 0 changes here.  It simplifies things.

I've also made clear the distinction between QUIC packets and IP packets.  With coalescing, that distinction is critical.  I've opted for counting of QUIC packets in marked IP packets rather than marked IP packets.  More on this below...

> +This capability check thus has verified that one direction of the path between
+the peers is free from issues with ECN bleaching and that the application does
+not experience problems with access to the ECN field in the IP header. In this
+case the marking of the generated packets with ECT continues.
+
+If an ACK frame (not the ACK_ECN frame) is used to acknowledge reception of
+packets that was marked as ECT in the sender, or if the comparision of the
+counters with the total of acknolwedged packets indicates that not all packets
+arrive as ECT(0), ECT(1) or ECN-CE, then the path is determined as not ECN
+Capable.  If the endpoint determine the path as currently not ECN capable it
+SHALL stop marking the packets as ECT, and instead mark them as Not-ECT. If the
+connection is migrated then the ECN capability check is rerun as specified in
+{{ecn-connection-migration}}.
+
+
+### Continuous Verification of ECN {#ecn-continuous-verification}

If an endpoint successfully marks packets, then every IP packet it sends will be marked.  That leads to continuously increasing counters, which is a little inefficient.

A different design for ACK_ECN might be more efficient: record the number of exceptions.  An ACK frame is used if there is no ECN support, or bleaching.  ACK_ECN then records two counters: the number of packets with ECT(1) markings, and the number of packets with CE markings.

Yes, this optimizes for ECT(0) markings, but that's consistent with advice in 8311.  The counters should stay low, or at least increase only as congestion is avoided.

This requires that you count QUIC packets carried in marked IP packets, and not count IP packets, as above.

The only problem with this is that it breaks down if you have multiple paths and one of those paths bleaches.  You could use ACK for the bleaching path and ACK_ECN for the other.  Given that we only have to manage migration occasionally, that might be OK.

The ACK loss problem described below remains, unfortunately.  But it's much less likely to happen.  I'm not sure if that's a good thing or not.

> @@ -1783,6 +1860,75 @@ Note that receipt of packets with higher packet numbers from the legitimate peer
 address will trigger another connection migration.  This will cause the
 validation of the address of the spurious migration to be abandoned.
 
+### ECN Capability Check for Migrated Connection {#ecn-connection-migration}
+
+Connection migration requires that the path's ECN capability is verified
+again. The ECN capability as indicated in section {{ecn-capability-check}}
+should be repeated when a connection is migrated. This verifies that the
+endpoints are ECN-capable and that the ECN field is not bleached along the new
+path.

This section could probably be more concise.

"""
Each new path is probed to determine whether it supports ECN.  Packets sent on the new path are sent in IP packets with an ECT marking as described in {{previous-section-on-ecn}}.

Markings, or absence of markings, on packets sent on multiple paths can make it difficult to correctly attribute counters with markings on specific packets.  Recording the packet number when connection migration occurred might help in correlating increases in counters with packets sent on the new path.

If an acknowledgment indicates that the markings were retained, the path is marked as ECN capable and subsequent IP packets sent on that path continue to be ECT marked.  If an acknowledgment indicates that ECN markings are removed, subsequent packets are sent with Non-ECT.
"""

> +|                       ACK Block Count (i)                   ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                          ACK Blocks (*)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                           ECN Block                         ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ACN_ECN_FRAME_FORMAT title="ACK_ECN Frame Format"}
+
+
+### ECN Block {#ECN-Block}
+
+The ECN block is described below. The size (i) indicates variable-length
+encoding, explained in {{integer-encoding}}. The encoding length for each
+counter (1, 2, 4 or 8 bytes) should be selected such that all the
+significant bits are represented.

This second sentence is unnecessary and potentially misleading.

> @@ -4200,6 +4434,20 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 
+## Explicit Congestion Notification Attacks {#security-ecn}
+
+An on-path attacker may manipulate the value of the field, affecting the
+congestion avoidance behavior of the sender. Removing any ECN-CE marking causes
+senders to maintain or increase their sending rate beyond that the path can
+sustain, which will eventually result in loss. Adding an ECN-CE marking causes
+senders to reduce their sending rate. The later could equally be accomplished by

latter

> @@ -4200,6 +4434,20 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 
+## Explicit Congestion Notification Attacks {#security-ecn}
+
+An on-path attacker may manipulate the value of the field, affecting the
+congestion avoidance behavior of the sender. Removing any ECN-CE marking causes
+senders to maintain or increase their sending rate beyond that the path can
+sustain, which will eventually result in loss. Adding an ECN-CE marking causes
+senders to reduce their sending rate. The later could equally be accomplished by
+dropping packets for the connection. Section 18 and 19 of {{!RFC3168}} discusses
+the effects of undesired manipulation of the ECN field in more details.

detail

> @@ -4200,6 +4434,20 @@ limit mitigates the effect of the stream commitment attack.  However, setting
 
+## Explicit Congestion Notification Attacks {#security-ecn}
+
+An on-path attacker may manipulate the value of the field, affecting the
+congestion avoidance behavior of the sender. Removing any ECN-CE marking causes
+senders to maintain or increase their sending rate beyond that the path can
+sustain, which will eventually result in loss. Adding an ECN-CE marking causes
+senders to reduce their sending rate. The later could equally be accomplished by
+dropping packets for the connection. Section 18 and 19 of {{!RFC3168}} discusses
+the effects of undesired manipulation of the ECN field in more details.
+
+If a receiver does not discard duplicate packets, an off-path attacker can
+retransmit packets with ECN bits set and manipulate the senders congestion
+avoidance state. If duplicate packets are dropped, the off-path attacker will

s/dropped/discarded

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-129837319

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 19 Jun 2018 02:45:25 -0700

to decipher the packet.

@martinthomson it was not my intention to require infinite state. I will see if I can reformulate to say that for any packet within the window of PNs that you will accept to process at all you need to maintain duplication detection before adding the ECN field values to the counters. 

@mirjak TSVWG has asked if there was anything from the QUIC ECN work that was of general nature and should be discussed in TSVWG and I think this is one of those things. However, my opinion is that due to ECN's unauthenticated nature, it is reasonable to suppress network duplicates. Otherwise use of ECN makes one more open to attacks to drive down the congestion window for a connection than when not using it. Yes, the connection will be slightly less responsive to the congestion signal, how much is depending on the amount of network duplication that occur. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196358953

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 19 Jun 2018 04:55:27 -0700

1636ebc  Clarifying that idefinite state are not requeired in duplication detection.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/e06a2a0efb1e655ecd1fa4e3a8e182689a50ff54..1636ebc60296c4af708cfd064cf38541eb41f352

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 19 Jun 2018 05:12:36 -0700

I think this proposal is very good, and agree about the IP packet thing and Stream 0 change. The only thing I 100% happy is the explicit mention of ECT(0) being the default. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196397525

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 19 Jun 2018 12:23:51 +0000 (UTC)

It also requires one to send one ACK_ECN frame for any marked, and one ACK for any Not-ECT marked IP packets. That was not my original intention, but I think works fine. So, yes I think changing the ACK_ECN frame to use this proposal would improve efficiency when it works. 

When it comes to ACK loss there will be no effect if the lost ACKs only acknowledged ECT(0) pakets. If there was ACKs lost that incremented the CE or ECT(1) counters then it would be in the same situation. The sender can detect the event and record a new comparison point so that it doesn't effect the future bleaching detections. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196400399

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 19 Jun 2018 05:35:35 -0700

139f1ee  Martin Thomson's editorial suggestions. Restructured pseudo code sections. Changed capability check and connection migration text significantly.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/1636ebc60296c4af708cfd064cf38541eb41f352..139f1ee2f508877ad0f8d4cb92170382d4707247

---

From: Magnus Westerlund <[email-masked]>
Date: Tue, 19 Jun 2018 06:46:21 -0700

If one is interested in seeing how this two counter proposal looks, please look at #1460. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196430233

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 20 Jun 2018 00:36:03 -0700

dbfe4d8  partial editorial fixes
73bf8cb  Merge pull request #2 from janaiyengar/ecn

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/139f1ee2f508877ad0f8d4cb92170382d4707247..73bf8cb46b529bd799a4cf7df370b54aab5b29b8

---

From: ianswett <[email-masked]>
Date: Wed, 20 Jun 2018 05:46:56 -0700

> +appended at the end.
+
+
+~~~
+ 0                   1                   2                   3
+ 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                     Largest Acknowledged (i)                ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                          ACK Delay (i)                      ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                       ACK Block Count (i)                   ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                          ACK Blocks (*)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                           ECN Block                         ...

As discussed on the list, I think it makes sense to but the ACK Block Count and ACK blocks at the end of this frame, to make it simpler to truncate the ack blocks if necessary.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-130375599

---

From: Magnus Westerlund <[email-masked]>
Date: Wed, 20 Jun 2018 07:08:13 -0700

+
+
+~~~
+ 0                   1                   2                   3

Agree, if no ones protests, I will implement that change tomorrow. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r196793189

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 21 Jun 2018 00:41:37 -0700

be91e8f  Moved the ECN block in the ACK prior to the ACK blocks themselves. Removed unneeded sentence.
f8d3d5c  Merge branch 'ecn' of https://github.com/gloinul/base-drafts into ecn

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/73bf8cb46b529bd799a4cf7df370b54aab5b29b8..f8d3d5c6040ad0da6595cf07019d5c4f755aa0f3

---

From: Magnus Westerlund <[email-masked]>
Date: Thu, 21 Jun 2018 00:41:46 -0700

+
+
+~~~
+ 0                   1                   2                   3

Now implemented

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r197037592

---

From: ianswett <[email-masked]>
Date: Fri, 22 Jun 2018 19:09:52 +0000 (UTC)

Two comments on the frame diagram.

> @@ -2999,10 +2999,10 @@ appended at the end.
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                       ACK Block Count (i)                   ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Can you move the ACK Block Count down as well.  It's odd to have that up here.

> @@ -2999,10 +2999,10 @@ appended at the end.
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                       ACK Block Count (i)                   ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Also, how about putting the ECN info in this diagram, rather than having it in a separate diagram?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-131313634

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 25 Jun 2018 01:27:46 -0700

cf312f9  Merged ECN counters into single ACK frame section and move them before the ACK Block Count.

-- 
You are receiving this because you are subscribed to this thread.
View it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372/files/f8d3d5c6040ad0da6595cf07019d5c4f755aa0f3..cf312f90a6d844953e35a095d411e20965724124

---

From: Magnus Westerlund <[email-masked]>
Date: Mon, 25 Jun 2018 01:30:45 -0700

Yes, I merged the ECN counters and text into a single section for the ACK_ECN frame. I think that is very reasonable. So please check this latest change. 

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#discussion_r197715962

---

From: ianswett <[email-masked]>
Date: Mon, 25 Jun 2018 07:19:17 -0700

Thanks for the update, looks good.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-131629374

---

From: Lars Eggert <[email-masked]>
Date: Tue, 26 Jun 2018 01:36:12 -0700

> @@ -2887,6 +2964,73 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in the
+IP header. If the header is not readable from the application, the codepoint 00
+(Not-ECT) MUST be assumed. If any packet are duplicated by the network then only

s/packet/packets/

> @@ -2887,6 +2964,73 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in the
+IP header. If the header is not readable from the application, the codepoint 00
+(Not-ECT) MUST be assumed. If any packet are duplicated by the network then only

s/by the network/by the network,/

> @@ -2887,6 +2964,73 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in the
+IP header. If the header is not readable from the application, the codepoint 00
+(Not-ECT) MUST be assumed. If any packet are duplicated by the network then only
+the value of the ECN field of the packet copy first received SHALL be included
+in the counters. This to prevent the on-the-side attack ({{security-ecn}}) and
+ensure that ACK_ECN frames becomes idempotent in the event of packet

s/becomes/are/

> @@ -2887,6 +2964,73 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in the
+IP header. If the header is not readable from the application, the codepoint 00
+(Not-ECT) MUST be assumed. If any packet are duplicated by the network then only
+the value of the ECN field of the packet copy first received SHALL be included
+in the counters. This to prevent the on-the-side attack ({{security-ecn}}) and
+ensure that ACK_ECN frames becomes idempotent in the event of packet
+duplication. Note, a receiver is not required to maintain indefinite state for
+which packet numbers have been received far into the history. Packets discarded
+for this reason, their ECN values are also not counted.

These sentences read awkwardly. Maybe "when information about old packets is discarded at a receiver, any ECN information associated with them is also discarded" or something like that?

> @@ -2887,6 +2964,73 @@ by a client in protected packets, because it is certain that the server is able
 to decipher the packet.

+## ACK_ECN Frame {#frame-ack-ecn}
+
+A QUIC connection MUST keep counters for each ECN codepoint, recording the
+number of packets that were received with the corresponding ECN codepoint in the
+IP header. If the header is not readable from the application, the codepoint 00
+(Not-ECT) MUST be assumed. If any packet are duplicated by the network then only
+the value of the ECN field of the packet copy first received SHALL be included
+in the counters. This to prevent the on-the-side attack ({{security-ecn}}) and
+ensure that ACK_ECN frames becomes idempotent in the event of packet
+duplication. Note, a receiver is not required to maintain indefinite state for
+which packet numbers have been received far into the history. Packets discarded
+for this reason, their ECN values are also not counted.
+
+ACK_ECN Frame MUST be used when when an endpoint is acknowledging a packet were

s/ACK_ECN Frame/An ACK_ECN frame/

There are other instances where "Frame" is capitalized; they should also be lowercased IMO for consistency with the rest of the docs.

> @@ -1427,6 +1431,63 @@ a single packet.

s/allows for/enables/

> +number, the path and peer do not remove ECN markings. The endpoint records this
+path as being ECN capable and continues to mark IP packets that it sends.
+
+If an endpoint receives an ACK frame, indicating that no markings were received,
+or the counter in the ACK_ECN frame do not match the number of QUIC packets that
+were sent in marked IP packets, then the path or peer remove ECN markings. The
+endpoint records this path as not being ECN capable and it ceases marking of
+packets.
+
+IP packets sent on a new path SHOULD be marked with ECT(0) or ECT(1) to verify
+that the new path supports ECN, see {{ecn-connection-migration}}.
+
+### Continuous Verification of ECN {#ecn-continuous-verification}
+
+If the ECN capability check was successful and the endpoint continues to send
+ECT marked packets then continuous verification is applied. This is to detect

s/packets then/packets, then/

> +path as being ECN capable and continues to mark IP packets that it sends.
+
+If an endpoint receives an ACK frame, indicating that no markings were received,
+or the counter in the ACK_ECN frame do not match the number of QUIC packets that
+were sent in marked IP packets, then the path or peer remove ECN markings. The
+endpoint records this path as not being ECN capable and it ceases marking of
+packets.
+
+IP packets sent on a new path SHOULD be marked with ECT(0) or ECT(1) to verify
+that the new path supports ECN, see {{ecn-connection-migration}}.
+
+### Continuous Verification of ECN {#ecn-continuous-verification}
+
+If the ECN capability check was successful and the endpoint continues to send
+ECT marked packets then continuous verification is applied. This is to detect
+any cases when ECN field is bleached, that is, zeroed out by a network node,

Above, the text used the term "remove markings". We should pick one term - zeroed, bleached, removed - consistently.

> +endpoint records this path as not being ECN capable and it ceases marking of
+packets.
+
+IP packets sent on a new path SHOULD be marked with ECT(0) or ECT(1) to verify
+that the new path supports ECN, see {{ecn-connection-migration}}.
+
+### Continuous Verification of ECN {#ecn-continuous-verification}
+
+If the ECN capability check was successful and the endpoint continues to send
+ECT marked packets then continuous verification is applied. This is to detect
+any cases when ECN field is bleached, that is, zeroed out by a network node,
+likely as the result of a routing changes since the ECN capability check.
+
+For each received ACK_ECN frame, the total number of newly acknowledged packets
+can be compared to the total increase in ECN counters. If the increase in ECN
+counters is less, then an ECN failure has occurred and ECN should be disabled.

less than what?

> +that the new path supports ECN, see {{ecn-connection-migration}}.
+
+### Continuous Verification of ECN {#ecn-continuous-verification}
+
+If the ECN capability check was successful and the endpoint continues to send
+ECT marked packets then continuous verification is applied. This is to detect
+any cases when ECN field is bleached, that is, zeroed out by a network node,
+likely as the result of a routing changes since the ECN capability check.
+
+For each received ACK_ECN frame, the total number of newly acknowledged packets
+can be compared to the total increase in ECN counters. If the increase in ECN
+counters is less, then an ECN failure has occurred and ECN should be disabled.
+ECN is also disabled in case an ACK frame is received acknowledging any ECT sent
+packet.
+
+If the acknowledgements from the receiver are lost such that one or more packet

s/packet/packets/

+If the ECN capability check was successful and the endpoint continues to send
+ECT marked packets then continuous verification is applied. This is to detect
+any cases when ECN field is bleached, that is, zeroed out by a network node,
+likely as the result of a routing changes since the ECN capability check.
+
+For each received ACK_ECN frame, the total number of newly acknowledged packets
+can be compared to the total increase in ECN counters. If the increase in ECN
+counters is less, then an ECN failure has occurred and ECN should be disabled.
+ECN is also disabled in case an ACK frame is received acknowledging any ECT sent
+packet.
+
+If the acknowledgements from the receiver are lost such that one or more packet
+are received by the receiver, but never acknowledged to the sender an
+insensitivity to bleaching will be created. In this situation the ECN counters
+reported will have increase, but the sender side total for acknowledged packets

s/increase/increased/

> +ECN is also disabled in case an ACK frame is received acknowledging any ECT sent
+packet.
+
+If the acknowledgements from the receiver are lost such that one or more packet
+are received by the receiver, but never acknowledged to the sender an
+insensitivity to bleaching will be created. In this situation the ECN counters
+reported will have increase, but the sender side total for acknowledged packets
+will not have increased. Thus, a number of bleached packets equal to the number
+of packets that failed to be acknowledged can be received before triggering the
+continuous verification. To address this issue the sender detect the case when
+the ECN counters grows more than number of acknowledged packets when a ACK_ECN
+frame is received. In such cases a new comparison point is created by storing
+the current number of totally acknowledged packets and latest ECN counters. Then
+comparison are done by subtracting these stored values from the respective
+counters prior to the comparison. Note that any out-of-order ACK_ECN frames
+can't be used for determining any loss of acknowledgements.

This whole paragraph is really difficult to understand. "Comparison points" are not mentioned anywhere else, for starters.

> @@ -1783,6 +1845,21 @@ Note that receipt of packets with higher packet numbers from the legitimate peer
 
+### ECN Capability Check for Migrated Connection {#ecn-connection-migration}
+
+Each new path is probed to determine whether it supports ECN. Packets sent on
+the new path are sent in IP packets with an ECT marking as described in
+{{ecn-capability-check}}.
+
+Markings, or absence of markings, on packets sent on multiple paths can make it
+difficult to correctly attribute counters with markings on specific packets.
+Recording the packet number when connection migration occurred might help in
+correlating increases in counters with packets sent on the new path.

Might help how?

++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        ECT(0) Count (i)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        ECT(1) Count (i)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        ECN-CE Count (i)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                       ACK Block Count (i)                   ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                          ACK Blocks (*)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ACN_ECN_FRAME_FORMAT title="ACK_ECN Frame Format"}
+
+The receiver side report three ECN counters in the ACK_ECN frame. These counters

s/side //

+|                        ECT(0) Count (i)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        ECT(1) Count (i)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                        ECN-CE Count (i)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                       ACK Block Count (i)                   ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+|                          ACK Blocks (*)                     ...
++-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
+~~~
+{: #ACN_ECN_FRAME_FORMAT title="ACK_ECN Frame Format"}
+
+The receiver side report three ECN counters in the ACK_ECN frame. These counters
+counts the number of packets marked with this codepoint since the start of the

s/counts/count/

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#pullrequestreview-131908133

---

From: Martin Thomson <[email-masked]>
Date: Wed, 27 Jun 2018 00:07:37 +0000 (UTC)

Merged #1372.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#event-1702723512

---

From: janaiyengar <[email-masked]>
Date: Tue, 26 Jun 2018 18:00:56 -0700

@martinthomson : I assume you're planning to merge my PR on top of this?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#issuecomment-400509679

---

From: Martin Thomson <[email-masked]>
Date: Tue, 26 Jun 2018 18:28:42 -0700

Your PR went into master at the same time.  This closed when I merged your changes in.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/quicwg/base-drafts/pull/1372#issuecomment-400513796