[quicwg/base-drafts] Editorial-only NITS on QUIC-Transport-24 (#3214)

[Gorry Fairhurst <[e-mail]>]

/There are certain operations which an application MUST /
- ought this to be /which an /that an/

/However, implementations MAY choose to offer the ability to
   deliver data out of order to a receiving application./
- This doesn’t say the default is in-order delivery, which I would have thought was the case. It also doesn’t say whether this is per-connection, or per-stream (I imagined the latter).

/frames in multiple packets in order to make/
- Could be read as implying sequencing, I suggest removing /in order/ or saying /to ensure that

/A receiver maintains a cumulative sum of
   bytes received on all streams, which is used to check for flow
   control violations./
- It would have helped me, had this explained what was intended by “control violations” - I think it means to detect when the sender sends more than permitted by the flow control limit. I think the control part comes in a later sentence? (Is this measured against the largest advertised offset, or the current offset? … given what is written about reducing the advertised value?)


/However, multiplexing
   connections on the same local IP address and port while using zero-
   length connection IDs will cause failures in the presence of peer
   connection migration, NAT rebinding, and client port reuse; and
   therefore MUST NOT be done unless an endpoint is certain that those
   protocol features are not in use./
- Seems like dodgy requirements text to me. I likely agree with the conclusion, but what is the MUST actually determining when the sentence starts with a however and includes ‘certain’ … ’not in use”? I suspect it would be better to have a full stop after the examples of failure and then state what is required.

/An endpoint MAY limit the frequency or the total
   number of connection IDs issued for each connection to avoid the risk
   of running out of connection IDs; see Section 10.4.2./
- I don’t understand /frequency/ or /total number/ in this sentence. I had expected a limit on the outstanding number of unused connections or the total number of in-use (used or available) connections, but the words I saw didn’t make sense to me.

/An endpoint that migrates
   away from a local address SHOULD retire all connection IDs used on
   that address once it no longer plans to use that address./
- I expected to know what happens if the should is not respected, and wondered in this case whether the resources remain committed to the previous address and would become unable to be utilised?

/the peer MUST retire the corresponding connection IDs
   and send corresponding RETIRE_CONNECTION_ID frames.  Failing to
   retire /
- The text requires the sender to do something, ut clearly reordering and other things could follow - as it says in the next text - so why is this a MUST, not a SHOULD - with strong indication do otherwise will have implications?

In section 5.2 /Hosts try to/
- Is host the same endpoints, as elsewhere, or specifically the node hosting the stack? and why?

/Packets that don't match an existing connection are discarded
 / 
- I’d personally really prefer to avoid using “don’t” and “isn’t” (which currently occur a few times in this section).
-  also can the ID explicitly say which field did not match the ID.

/Servers that do not support a particular version
   are unlikely to be able to decrypt the payload of the packet.
   Servers SHOULD NOT attempt to decode or decrypt/
- This seems like tuning? Or is the intention to protect the endpoint from an attack, please explain the SHOULD.

/   Servers MUST drop incoming packets under all other circumstances./
- Normally in other transport protocols, this requirement would be accompanied by a “and SHOULD log the event” to enable people to understand what has happened.

/There are certain operations which an application MUST/
- I think /which/ needs to /that/.


/ The size of the first packet sent by a client will determine whether
   a server sends a Version Negotiation packet.  Clients that support
   multiple QUIC versions SHOULD pad the first packet they send to the
   largest of the minimum packet sizes across all versions they support.
   This ensures that the server responds if there is a mutually
   supported version.
/
- Are you saying a client that does not PAD does not negotiate? If so: What is the logic behind that design decision?
- I don’t read that as it assures what it say. To me, the padding assures the client can send this side of packet to the remote endpoint.
- Does this agree with what is written in section 8.1 on padding? and Section 14?

/An endpoint can verify support for Explicit Congestion Notification
   (ECN) in the first packets it sends, /
- Nearly - It can verify the endpoints support ECN and the current path could be used to transfer ECN-marked packets. That’s not quite enough to complete verification that the path and endpoints have full ECN support.

/MUST specify whether they MUST,/
- could be written is /is REQUIRED to specify whether they MUST/

/ and absent new values from the server, the default value./
- what does ‘absent’ mean in this clause, could you reword?

/An endpoint MUST ignore transport parameters that it does
   not support./
- What happens if the parameter definition requires that it is stored? How would it know?

/Address validation is used by QUIC to avoid being used for a traffic
   amplification attack.  /
- This doesn’t parse to me, /is used…being bused/, please could you rephrase?

/A token SHOULD be constructed in a way that allows the server to
   distinguish it from tokens that are sent in Retry packets as they are
   carried in the same field./
- I’m not sure whether this intended /as/ or /because/ - but either way I did not understand that sentence.

/claimed client address (IP
   and port)/
- What is IP and port? - is this both IP addresses or the remote address? (do you care about Address Family?) and pair of transport ports (or one of the ports)

/MUST respond
   immediately by echoing the data/
- Of course it may not be able to immediately send anything:-) , but I suspect this needs to not delay?

/as it might be a result of a forwarded
   packet (see Section 9.3.3) or misrouting. /
- I think “forwarding” in a transport spec means normal network behaviour, and routing is the thing controls that. nIf the spec needs to use these words can they be prefixed with something “proxy forwarding - or application forwarding”- or whatever to indicate that these are middleboxes, rather than routers/switches.


/9.2.  Initiating Connection Migration/
- I assert the new path may not have the same RTT, because the path could traverse a different set of network devices or be subject to different policies that influence forwarding behaviour. These are not currently listed here, but are the basis for some later text in 9.3.1. Can we mention here?

/This style of attack relies on the attacker using a path that is
   approximately as fast/
- In the sense of this phrase, I think “fast” is not referring to line rate or transfer speed, but to the same amount of time … I think the word needs to be replaced.

/   At any time, endpoints MAY change the Destination Connection ID they
   send to a value that has not been used on another path./
- I understand the idea of changing, but what is the “send to a value” text about?

/If path validation succeeds, the client SHOULD immediately begin
   sending all future packets to the new server address/
- /immediately begin sending/ troubles me here, and I would prefer this to also explicitly say that is subject to 9.4 (because we all know people read documents with certain emphasis, and I wouldn’t like this specific detail to be missed.

/Migrating a connection to a new server address mid-connection is left
   for future work./
- I’d prefer to say is not described in this spec or is not supported by this spec - rather than these words, I think the SHOULD indicates it is likely the former.

Section 11:
/   An endpoint that detects an error SHOULD signal the existence of that
   error to its peer. /
- Normally in other transport protocols, this requirement would be accompanied by a “and SHOULD log the event” to enable people to understand what has happened.

This statement is dangerous:
/However, an implementor should
   only deviate from these requirements after careful consideration of
   the performance implications of doing so./
- Let me argue this statement needs to be replaced by:
- “The requirements described are baed on consideration of
   the performance implications and the on other flows that could share network capacity”.
- I strongly argue that implementors how do not implement a PS do not conform to that PS. That happens, but I don’t see the point in a spec saying if you don’t comply you’re still compliant.

/Limiting ACK frames avoids an infinite feedback loop of acknowledgements/
- Unclear, is the text about limiting which sets of frames result in ACK frames? or something else?

In Section 13.4 
/An endpoint
   validates the use of ECN on the path, both during connection
   establishment and when migrating to a new path (Section 9)./
- I think the QUIC endpoint MAY also validate ECN is still support by the path during communication. This may be necessary when ECN is enabled, but a path change results in there no longer being support.
- I think this needs to also be permitted in Section  13.4.2, allowing QUIC to suspend ECN usage for a path or to an endpoint for which it currently has a connection.

/Note that this requires being able
   to read the ECN codepoints from the enclosing IP packet, which is not
   possible on all platforms./
- That sounds like it could be an issue, which it is not. Could we add “On those platforms, QUIC will not enable ECN for this endpoint”.

/To provide robust connectivity in
   the presence of such devices, each endpoint independently validates
   ECN counts and disables ECN if errors are detected./
- specifically, I think this could be: /To provide robust connectivity in
   the presence of such devices, each endpoint independently validates
   ECN counts and disables ECN if the path is does not show consistent support for ECN./

/   Even if validation fails, an endpoint MAY revalidate ECN on the same
   path at any later time in the connection./
- That’s possible, meaning try again to use ECN? I think the text needs to be clearer, that this can be used to enable ECN to be used within the connection.

/path supports a reasonable Maximum Transmission Unit (MTU)./
- I think this should read /path supports a reasonable Path Maximum Transmission Unit (PMTU)/

Also:
/   Prior to validating the client address, servers MUST NOT send more
   than three times as many bytes as the number of bytes they have
   received./
- So, let me understand - does that include retransmissions? or is this constrain within the PTO or something?


-- 
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/issues/3214