Subject: Re: [tsvwg] Review of draft-ietf-tsvwg-rfc4960-errata-04.txt

From: Michael Tuexen <[email-masked]>
Date: Mon, 5 Mar 2018 10:34:13 +0100

> On Dec 13, 2017, at 10:01 PM, Gorry Fairhurst <[email-masked]> wrote:
> 
> 
> This email contains a shepherd review for the above draft. There are several issues, that seem straight forward to correct and where alternate wording may be needed.
> 
> I have a comment about the use of lower case RFC2119 wording: While I know that documents continue to be published with a mix of upper case normative keywords and lower case words, I'd like the authors to explicitly check each of the lower case uses. In some places, the correct thing appears to be to use a normative version.
> 
> I suggest we should be extremely careful in this usage when considering progressing a document to full standard.  I'd advise replacing all other non-normative uses of /should/ and /may/ with /ought to/ and /could/ to remove this ambiguity. This would make it really clear what the normative requirements are.

Hi Gorry,

I've checked the usage and changed it to make it clear.
I looked for must, should, shall, may (not all caps) in the "New Texts".
Thanks for the comments, see my comments in-line.

Best regards
Michael
> Gorry
> (TSVWG Co-Chair).
> 
> ----
> 
> Abstract
> - English?
> /time based/
> - seems wrong, this could be hypenated, although maybe it would be better as /time ordered/?
Using time ordered.
> ---
> Introduction
> /the publishing of this/
> - may be better as /the publication of this/.
> ---
Taken.
> 3.1.2
> /the endpoint should mark the corresponding/
> - This seems like a protocol action, why is this not
> /the endpoint SHOULD mark the corresponding/
> ---
> /and may also/
> - similarly /and MAY also/
> - although I would understand why the latter was /and could also/ ... but I'd prefer to avoid the lower case MAY.
> ---
> 
> /After this, the endpoint should continue
>   HEARTBEAT on this destination address but should stop increasing the
>   counter./
> - are these /SHOULD/ if not, why not? and if a reason, I suggest you replace by /ought/.
> ---
Changed to SHOULD/MAY.
> 3.2.2
> /and should discard subsequent ULP shutdown requests./
> 
> - are these /SHOULD/ if not, why not? and if a reason, I suggest you replace by /ought/.
The text now says: it MUST ignore subsequent ULP shutdown requests
> ---
> 3.2.3
> /If it did the endpoints/
> - please insert comma aftre /did/.
Done.
> ---
> 3.3.2
> - RFC2434 needs to be replaced by RFC5226.
Changed. But it was already taken care of in 3.43.2.
> ---
> 3.6.2
> /An endpoint shall keep a counter on the total number of consecutive
>   retransmissions to its peer/
> - is this /SHOULD/ if not, I suggest you replace by /ought/.
Changed to SHOULD.
> ---
> /the path currently used for data transfer shall
>   not increment the association error counter, /
> - Could this lead to failure, is this /SHOULD NOT/ or actually /MUST NOT/?
I'm preferring SHOULD NOT, since you might implement other counter strategies.
> ---
> 3.9.2
> /to the current Path MTU/
> - Elsewhere this is called PMTU.
OK. Changed to PMTU.
> ---
> /for path MTU discovery./
> - seems wrong, should this be:
> /for Path MTU Discovery./ or /for PMTU Discovery./ ?
Took: PMTU Discovery
> ---
> /constraint of the path MTU/
> - Capital P, or PMTU?
PMTU chosen.
> ---
> 3.11.2
> /SCTP should perform slow/
> - If SCTP is to follow TCP CC, then I think this needs to be:
> /SCTP SHOULD perform slow/
Taken.
> ---
> 3.13.2
> /When a
>   HEARTBEAT ACK is received from the peer endpoint, the counter should
>   also be reset./
> - I think that is SHOULD be reset.
Taken.
> ---
> /The receiver of the HEARTBEAT ACK may choose not to
>   clear the counter if there is outstanding data on the association.
> - I think that is MAY or /could/
MAY
> ---
> /
> Upon the receipt of the HEARTBEAT ACK, the sender of the HEARTBEAT
>   should clear the error counter/
> - I think that is SHOULD be clear, since it sets what comes on the wire.
Taken.
> ---
> /The endpoint may optionally report/
> - /could optionally report/ or /is permitted to optionally report/?
We can use MAY.
> ---
> /The receiver of the HEARTBEAT ACK should also clear/
> - I'd have thought that was SHOULD.
Done.
> ---
> 3.15.2
> - PMTU or Path MTU.
PMTU.
> ---
> 3.16.2
> - The following text seems odd, but perhaps you could refer to
>  where this exact phrase is used?:
> /The initial value of ssthresh SHOULD be arbitrarily high (e.g.,
>      to the size of the largest possible advertised window)./
Not sure what is odd... We already provide a reference where the
same text is used in RFC 5681...
> ---
> 3.20.1
> /But receiving new packets from the peer does not guarantee peer's accessibility/
> - Maybe better as:
> /However, reception of new packets from the peer does not guarantee the peer's accessibility/
> - Is reachability better than accessibility?

Text taken and using reachability.
> 
> ---
> 3.20.2
> /should not increment the error counter/
> - /ought not/ or /should not/.
SHOULD NOT
> ---
> /This is because the receiver MAY
>   keep its window closed for an indefinite time./
> - is this really /MAY/ - are we permitting something or saying it can be so?
> If the latter, please use /could/ in place of MAY.
could
> ---
> - This text seems odd:
> /Refer to Section 6.2 on the receiver behavior when it advertises a zero window./
> - would this be better as:
> 
> /Section 6.2 deescribes the receiver behavior when it advertises a zero window./
Taken.
> 
> ---
> 3.21.2 (multiple)
> /When
>      this flag is set, the stream id and Stream Sequence Number must
>      accompany this receive./
> - This seems like protocol action is it /MUST/?
Section 10 is informative, so no normative language can be used.
Therefore no change.

Now we are in 3.23.2
> ---
> /the endpoint shall
>   clear the error counter of the destination transport address to which
>   the DATA chunk was last sent (or HEARTBEAT was sent), and SHOULD/
> - why is the first /shall/ lower case, and not an /ought to/ or a SHALL?
SHOULD.
> ---
> /should clear the error counter/
> - why is the  /should/ lower case, and not an /ought to/ or a SHALL?
Using a SHOULD.
> --- (multiple)
> / An endpoint should limit the number of retransmissions of the
>   SHUTDOWN chunk to the protocol parameter 'Association.Max.Retrans'./
> - why is the  /should/ lower case, and not an /ought to/ or a SHALL?
Changed to SHOULD.
> ---
Now you are in 3.25.2:
> - A MUST states a requirement:
> /Furthermore, we require
>   that the receiver of an INIT chunk MUST enforce these rules by
>   silently discarding the INIT chunk and all further chunks if the INIT
>   chunk is bundled with other chunks or the packet has a non-zero
>   verification tag. /
> - needs to be rewritten as:
> /The receiver of an INIT chunk MUST silently discard the INIT chunk
>   and all further chunks if the INIT
>   chunk is bundled with other chunks or the packet has a non-zero
>   verification tag./
Changed as suggested.
> ---
> 3.26.2
> /
>   When cwnd is greater than ssthresh, cwnd should be incremented by
>   1*MTU per RTT if the sender has cwnd or more bytes of data
>   outstanding for the corresponding transport address./
> - /SHOULD/.
Done.
> ---
> 3.27.2
> /
> When the endpoint does not transmit data on a given transport
>      address, the cwnd of the transport address should be adjusted to
>      max(cwnd/2, 4*MTU) per RTO. At the first cwnd adjustment, the
>      ssthresh of the transport address should be adjusted to the cwnd./
> - /SHOULD be adjusted/ (twice)?
Done.
> ---
> 3.32.1
> - This update is based on RFC6298.
> I think the updated text should in some way refer
> to this update as the basis of the method.
Why? The intention of this document is provide the background for
all changes. So we don't need to change the style of the SCTP specification.
We don't provide in reference for the values given in the table.
> ---
> 3.33.2.
> /
> The endpoint may bundle the ERROR chunk/
> - /MAY/.
Done.
> ---
> 3.35.2
> - This text doesn't seem very clear to me:
> /
>   SCTP implementations MAY allow an application to configure the
>   Differentiated Services Code Point (DSCP) used for sending packets.
>   If a DSCP change might result in outgoing packets being queued in
>   different queues, the congestion control parameters for all affected
>   destination addresses MUST be reset to their initial values.
> 
> /
> - (1) Does this mean for each packet? or for the entire asscociation or what?
> (I think it reads like an association?)
I think it is pretty clear... The socket API allow the DSCP to be set per
remote address, but this is just an example. The crucial point is that
you reset the congestion controller to its initial state for all affected
destination addresses. This is what the text describes.
> - What does /being queued in different queues/ mean? ... any change to the
> DSCP could result in different results, but this is more likely if the change
The text says, when you are in doubt, reset the congestion controller.
> modifies an AF class. It is probably safer to point to RFC7657 for specifics.
> - Please update this.
Again: I think this is pretty clear. But I'm open to concrete text suggestions.
> ---
> 3.40.2
> - The update you note in the following two specific cases is not in the ID you cite, but in AccECN I think... where the change
I think RFC 3168 describes a way to set the ECE and the CWR bit.
This is what we are referring to. Not to something improved.
There was more specific ID, but that never progressed.
> from RTT-based reduction is mentioned. Please check and update.
> /
> [RFC3168] updated by [I-D.ietf-tsvwg-ecn-experimentation]
>   details a specific bit for a receiver to send back in its
>   TCP acknowledgements to notify the sender of the Congestion
>   Experienced (CE) bit having arrived from the network./
> -
> /
>   RFC3168] updated by [I-D.ietf-tsvwg-ecn-experimentation]
>   details a specific bit for a sender to send in the header
>   of its next outbound TCP segment to indicate to its peer that it has
>   reduced its congestion window./
> - Although maybe you could choose not to cite this?
So you want to remove(!) the reference to [RFC3168]? I don't think the text
then makes much sense. The best way would be to specify ECN completely and
not give some hints in an Appendix, but this would require the ECN for SCTP
ID to be progressed.

I leave it for now. However, you can make a concrete text propsoal for
changing Appendix A. This current goal was just to replace the reference
to RFC 3168 by a reference to RFC 3168 and RFC 8311. Just the references,
not the actual content...
> ---
> 3.44
> - Section 3.44 clearly DOES have IANA implications. This represents changes
> that were made to port assignment, and although these are captured in
> a published RFC they are re-stated here and I think this NEEDS to be
> called out in the IANA considerations. IANA would be expected to review
> these changes (again), and this needs to be explained in section 4.
OK. I changed it to:

<t><xref target="Integration_RFC_6335"/> of this document updates the port
number registry for SCTP to be consistent with <xref target="RFC6335"/>.
IANA is requested to view <xref target="Integration_RFC_6335"/>.</t>

> ---
The following seems to be from 3.41.2:
> /
>      If the name resolution is not successful, the endpoint MUST
>      immediately send an ABORT with "Unresolvable Address" error cause
>      to its peer.  The ABORT shall be sent to the source IP address
>      from which the last peer packet was received./
> - please replace/update the /shall/.
This is "old text", only cited and can't be changed.
> ---
> /Should be set to all '0's and ignored by the receiver./
> - SHOULD.
Fixed.
> ---
> 3.4.62
> - Can one of the authors give me assurance that the code quoted is still correct at this time?
I checked it when getting the source code in the ID.
> ---

---

From: Gorry Fairhurst <[email-masked]>
Date: Tue, 6 Mar 2018 13:40:28 +0000

The changes in the new revision look good to me. Thank you Michael for 
making these changes. We will now proceed with WG writeup for publication.

Gorry