[tsvwg] RFC2119 Keywords in: draft-ietf-tsvwg-rfc4960-bis-06.txt

Gorry Fairhurst <gorry@erg.abdn.ac.uk> Tue, 07 April 2020 12:29 UTC

Return-Path: <gorry@erg.abdn.ac.uk>
X-Original-To: tsvwg@ietfa.amsl.com
Delivered-To: tsvwg@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 2CE4F3A07E2 for <tsvwg@ietfa.amsl.com>; Tue, 7 Apr 2020 05:29:51 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -1.897
X-Spam-Level:
X-Spam-Status: No, score=-1.897 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_NONE=0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id no1L7P-dGotV for <tsvwg@ietfa.amsl.com>; Tue, 7 Apr 2020 05:29:48 -0700 (PDT)
Received: from pegasus.erg.abdn.ac.uk (pegasus.erg.abdn.ac.uk [IPv6:2001:630:42:150::2]) by ietfa.amsl.com (Postfix) with ESMTP id 886093A07E1 for <tsvwg@ietf.org>; Tue, 7 Apr 2020 05:29:48 -0700 (PDT)
Received: from GF-MacBook-Pro.local (fgrpf.plus.com [212.159.18.54]) by pegasus.erg.abdn.ac.uk (Postfix) with ESMTPSA id 1D30A1B00227; Tue, 7 Apr 2020 13:29:44 +0100 (BST)
From: Gorry Fairhurst <gorry@erg.abdn.ac.uk>
To: tsvwg@ietf.org
Cc: Michael Tuexen <tuexen@fh-muenster.de>
References: <158625880871.8327.14435196984662146552@ietfa.amsl.com>
Message-ID: <9e46794c-9510-06f2-cc5b-93727e0ffc2b@erg.abdn.ac.uk>
Date: Tue, 7 Apr 2020 13:29:43 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:68.0) Gecko/20100101 Thunderbird/68.6.0
MIME-Version: 1.0
In-Reply-To: <158625880871.8327.14435196984662146552@ietfa.amsl.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-GB
Archived-At: <https://mailarchive.ietf.org/arch/msg/tsvwg/tppoph0FKDv2z4FpwjjO-chqLN0>
Subject: [tsvwg] RFC2119 Keywords in: draft-ietf-tsvwg-rfc4960-bis-06.txt
X-BeenThere: tsvwg@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Transport Area Working Group <tsvwg.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/tsvwg>, <mailto:tsvwg-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/tsvwg/>
List-Post: <mailto:tsvwg@ietf.org>
List-Help: <mailto:tsvwg-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/tsvwg>, <mailto:tsvwg-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 07 Apr 2020 12:29:51 -0000

Thanks for posting rev -06.

I have now read:
https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-bis-06

Overall, I found the document mature, but a spec of this size has lots 
of requirements and this review I went through these. I did not find 
many substantial issues, but I did find lots of places where the text 
was either unclear or the recommendation was not well-placed within the 
text.

Gorry

The following comments may help in preparation of the next revision:

(1) There are currently keywords used before the RFC2119 bolierplate. 
Perhaps the boilerplaet should be moved earlier?

(2)
"The port number 0 MUST NOT be used." - this appears twice, but would be 
simpler if one inserted "destination" and one "source".

(3)
"The 'Stream Sequence Number' field in a DATA chunk with U flag set to 1 
has no significance. The sender can fill it with arbitrary value, but 
the receiver MUST ignore the field."
- can we change "it" to the 'Stream Sequence Number' field or something 
more specific?

(4)
This sentence seems slightly mangled:
"The endpoint MUST also treat all the DATA chunks with TSNs not included 
in the Gap Ack Blocks reported by the SACK as "missing".
- Maybe this could say something like:
"All DATA chunks with TSNs not included in the Gap Ack Blocks reported 
by a SACK MUST be treated as "missing" by the sending endpoint."

(5)
"The number of "missing" reports for each outstanding DATA chunk MUST be 
recorded by the data sender in order to make retransmission decisions."
- is "in order" denoting a time-order, or can this be omitted to avoid 
potential confusion?

(6)
This seems very long:
"When a single SACK cannot cover all the Gap Ack Blocks needed to be 
reported due to the MTU limitation, the endpoint MUST send only one 
SACK, reporting the Gap Ack Blocks from the lowest to highest TSNs, 
within the size limit set by the MTU, and leave the remaining highest 
TSN numbers unacknowledged."
Is this a possible alternative:
"When a single SACK cannot cover all the Gap Ack Blocks needed to be 
reported due tothe MTU limitation, the endpoint MUST send only one SACK. 
This single SACK MUST report the Gap Ack Blocks from the lowest to 
highest TSNs, within the size limit set by the MTU, and leave the 
remaining highest TSN numbers unacknowledged."

(7) See this, and another comment later:
"However, in so doing, it MUST react just like an
    implementation that does NOT support fragmentation, i.e., it MUST
    reject sends that exceed the current Path MTU (PMTU)."
- Would be more complete as:
"An implementation that disables fragmentation MUST react just like an
    implementation that does NOT support fragmentation, i.e., it MUST
    reject sends that exceed the current Path MTU (PMTU)."

(8)
"Partial chunks MUST NOT be placed in an SCTP packet.  A partial chunk
    is a chunk that is not completely contained in the SCTP packet; i.e.,
    the SCTP packet is too short to contain all the bytes of the chunk as
    indicated by the chunk length."
- what does the specification say to do if the chunk size is too large
to be sent in a full-sized SCTP packet with the current PMTU?

(9)
"The basic
    guidelines for incrementing cwnd during congestion avoidance are as  
follows:

    o  SCTP MAY increment cwnd by 1*MTU.

    o  SCTP SHOULD increment cwnd by 1*MTU once per RTT when the sender
       has cwnd or more bytes of data outstanding for the corresponding
       transport address.

    o  SCTP MUST NOT increment cwnd by more than 1*MTU per RTT."

- these seem IETF "recommended" rather than guidelines?


(10)

“The parameters SHOULD
       decay if the address is not used for a long enough time period.”
- how long? There needs to be some guidance, even if this is in the form 
of a note.

(11)
"The counter MUST be reset each time a DATA chunk sent to that peer
    endpoint is acknowledged (by the reception of a SACK). "
- Could this be: "The counter used for endpoint failure detection MUST 
be..."
- Actually, this places a formal requiremen on a counter that is a 
"SHOULD" to implement!
- So, should this be the "The method used for endpoint failure detection 
MUST be..."?
- Please suggest something.

(12) The text:
"The receiver of an OOTB packet MUST do the following:"
  is followed by a list of shoulds and musts. I think this construct is 
not helpful.
Please replace each bullet with a SHOULD or MUST requirement and remove 
the "MUST"
from the top of the list!

(13)
"Res: 4 bits Should be set to all '0's and ignored by the receiver."
- Why is this not a MUST be set to zero and MUST be ignored by the receiver?
- Albeit you could not that future RFCs might update this requirement?

(14)

This didn't quite make sense to me:
"The Gap Ack Blocks SHOULD be
    isolated.  This means that the TSN just before each Gap Ack Block and
    the TSN just after each Gap Ack Block have not been received. By
    definition, all TSNs acknowledged by Gap Ack Blocks are greater than
    the value of the Cumulative TSN Ack."
- Please consider if you can make the SHOULD clearer and the following 
sentence.

(15)
"An endpoint SHOULD send this chunk to its peer endpoint to probe"
- ought to be something like:
"An endpoint SHOULD send a Heartbeat Request chunk to its peer endpoint 
to probe"

(16)
"If the sender does not wish to
       provide this information, it SHOULD set the Measure of Staleness
       field to the value of zero."
- ought to be something like:
"If the sender does not wish to
       provide the Measure of Staleness, it SHOULD set this
       field to the value of zero."

(17)

"  An endpoint SHOULD NOT send a DATA chunk with no user data part."
- Why not? What is the negative impact of not following this 
recommendation in 6.2?

(18)  This appears a requirement in an implementation note. Please either
remove the "implementation note", or replace by "ought" or something 
different:
"  IMPLEMENTATION NOTE: RTT measurements SHOULD only be made using
         a chunk with TSN r if no chunk with TSN less than or equal to r
         is retransmitted since r is first sent."

(19)

I wonder what the requirement really is here...
This comes after a normative requirement to use a CRC32c, but I am not clear
what action is needed in this recommendation:
"   Any hardware implementation SHOULD be done in a way that is
    verifiable by the software."
- Is this intended to somehow utilise appendix C?

(20)  Section 6.9:
- Section 6.9 has multiple “notes” that contain RFC2119 requirements. 
These need reworded to remove the requirement, or I think in this case 
to place the requirement outside of the note.
- Section 6.9 redefines PMTU - already defined earlier.
- “Once a message is fragmented, it cannot be re-fragmented.” and then 
proceeds to talk about IP fragmentation and later SCTP fragmentation. 
The type of fragmentation needs to be more clearly explained, this is 
confusing.
“If an implementation does not
    support fragmentation of outbound user messages, the endpoint MUST
    return an error to its upper layer and not attempt to send the user
    message.”
- This sentence needs the full context to provide a clear requirement, 
i.e. “An endpoint that does not support fragmentation and is requested 
to send a … MUST …”
“The association Path MTU is
    the smallest Path MTU of all destination addresses.”
- could be:
““The association Path MTU is defined as the smallest Path MTU for all 
destination addresses.”

(21)
“When the T3-rtx timer expires on an address, SCTP SHOULD perform slow
    start by:

       ssthresh = max(cwnd/2, 4*MTU)
       cwnd = 1*MTU
       partial_bytes_acked = 0“
- I have a question: Why is the reaction to the RTO not a requirement? 
I’d like to question whether this needs to be a “MUST”?

(22)

I suggest section 7.3 is normatively updated to replace RFC4821 with a 
reference to dplpmtud. - done in latest rev.

(23)

"An endpoint MUST  maintain separate MTU estimates for each destination 
address of
        its peer."
- I think this is about Path MTU, not MTU?

(24)
“Note: When configuring the SCTP endpoint, the user SHOULD. “
- RFC2119 text in a note. This note specifically is about user 
behaviour? Is this really just guidance - could it be “ought”?

(25)
“   The upper layer, i.e., the SCTP user, SHOULD standardize any specific
    protocol identifier with IANA if it is so desired.  The use of any
    specific payload protocol identifier is out of the scope of SCTP.”
- I am not sure whether this is a SHOULD, and if it is, what registry 
and procedure is to be used.
- What happens if they do not?

(26)

Appendix A contains normative text. I appreciate that ECN could be 
optional, but I am less than happy to see normative text in an appendix. 
Could this be moved to the body of the specification?

(27)

Appendix C contains normative text.  Could this be moved to the body of 
the specification?
- Appendix C contains a final note with a normative requirement. Could 
in this case it be replace /MUST abort associations/this specification 
requires associations to aborted/
- “do NOT support SCTP” should be “do not support SCTP”.

(28)
“Note that a parameter type MUST be unique across all chunks.  “
- RFC2119 text in a note. remove “Note”?

(29)
“Note that if the receiver of the INIT
    chunk is NOT going to establish an association (e.g., due to lack of
    resources), an 'Unrecognized Parameter' would NOT be included with
    any ABORT being sent to the sender of the INIT.”
/NOT/not/.

(30)
“   Note: Any time a COOKIE ECHO is sent in a packet, it MUST be the
    first chunk.”
- RFC2119 text in a note. remove “Note:”?

(31)
”Note that this field is NOT touched
       by an SCTP implementation; therefore, its byte order is NOT
       necessarily big endian.  “
/NOT/not/.

(32).
Is this restating requirements elsewhere?
“   Note 3: An INIT chunk MUST NOT contain the Host Name Address
    parameter.  The receiver of an INIT chunk containing a Host Name
    Address parameter MUST send an ABORT and MAY include an "Unresolvable
    Address" error cause.”
- If it is please make the keywords lowercase. If it is not please move 
this to the main body and remove the “note” tag.

(33)
“IMPLEMENTATION NOTE: If an INIT ACK chunk is received with known
    parameters that are not optional parameters of the INIT ACK chunk,
    then the receiver SHOULD process the INIT ACK chunk and send back a
    COOKIE ECHO.  The receiver of the INIT ACK chunk MAY bundle an ERROR
    chunk with the COOKIE ECHO chunk.  However, restrictive
    implementations MAY send back an ABORT chunk in response to the INIT
    ACK chunk.
“
- RFC2119 text in a note. remove “Note:”, or separate the NOTE into one 
para and the normative language into another.

(34)
“Note: Since the SHUTDOWN message does not contain Gap Ack Blocks,
       it cannot be used to acknowledge TSNs received out of order.  In a
       SACK, lack of Gap Ack Blocks that were previously included
       indicates that the data receiver reneged on the associated DATA
       chunks.  Since SHUTDOWN does not contain Gap Ack Blocks, the
       receiver of the SHUTDOWN MUST NOT interpret the lack of a Gap Ack
       Block as a renege.  (See Section 6.2 for information on reneging.)”
- RFC2119 appear to follow in a note. Please separate the NOTE into one 
para and the normative language into another.

(35)
Notes after figure 3 contain normative requirements. Suggest replace 
“NOTES”.

(36)
“Note: The COOKIE ECHO chunk MAY be bundled with any pending
        outbound DATA chunks, but it MUST be the first chunk in the
        packet and until the COOKIE ACK is returned the sender MUST NOT
        send any other packets to the peer.
“
- RFC2119 appear to follow in a note. Please separate the NOTE into one 
para and the normative language into another.

(37)
“   Note: T1-init timer and T1-cookie timer SHOULD follow the same rules
    given in Section 6.3.  If the application provided multiple IP
   addresses of the peer, there SHOULD be a T1-init and T1-cookie timer
    for each address of the peer.  Retransmissions of INIT chunks and
    COOKIE ECHO chunks SHOULD use all addresses of the peer similar to
    retransmissions of DATA chunks.”
- RFC2119 text in a note. remove “Note:”, or separate the NOTE into one 
para and the normative language into another.

(38)
“   Note that a COOKIE ECHO chunk that does NOT pass the integrity check
    is NOT considered an 'invalid parameter' and requires special
    handling; see Section 5.1.5.”
/NOT/not/.

(39)
“IMPLEMENTATION NOTE: If an SCTP endpoint that only supports either
    IPv4 or IPv6 receives IPv4 and IPv6 addresses in an INIT or INIT ACK
    chunk from its peer, it MUST use all the addresses belonging to the
    supported address family.  The other addresses MAY be ignored. The
    endpoint SHOULD NOT respond with any kind of error indication.

    IMPLEMENTATION NOTE: If an SCTP endpoint lists in the 'Supported
    Address Types' parameter either IPv4 or IPv6, but uses the other
    family for sending the packet containing the INIT chunk, or if it
    also lists addresses of the other family in the INIT chunk, then the
    address family that is not listed in the 'Supported Address Types'
    parameter SHOULD also be considered as supported by the receiver of
    the INIT chunk.  The receiver of the INIT chunk SHOULD NOT respond
    with any kind of error indication.”
- RFC2119 appear to follow in a note.

(40)
“   Note: For any case not shown in Table 2, the cookie SHOULD be
    silently discarded.”
- RFC2119 text in a note. remove “Note:”,
-
(41)
Notes after figure 6 contain normative requirements. Suggest replace 
“NOTES”.

(42)
“Note that a zero window probe SHOULD only be
        sent when all outstanding DATA chunks have been cumulatively
        acknowledged and no DATA chunks are in flight.  “
- RFC2119 text in a note. remove “Note that”,

(43)
“IMPLEMENTATION NOTE: When the window is full (i.e., transmission is
    disallowed by rule A and/or rule B), the sender MAY still accept send
    requests from its upper layer, but MUST “
- RFC2119 text in a note. remove “Note that”,  or “Implementation 
requirement”

(44)
“   Note: The data sender SHOULD NOT use a TSN that is more than 2**31 -
    1 above the beginning TSN of the current send window.

    Note: For each stream, the data sender SHOULD NOT have more than
    2**16 - 1 ordered user messages in the current send window.”
-RFC2119 text in a note. remove “Note that”,

—

I have three additional requests:

(a) Check the remaining text for RFC2119 text in notes. There are a few 
other cases and it would be silly to call all these out. The word “note” 
is always problematic when combined with a requirement.
(b) Please check for RFC2119 text with “Implementation note”
- could this be “Implementation requirements” or something similar 
calling out to implementors but avoiding the word note. The word “note” 
is always problematic when combined with a requirement.
(c) Please check for upper case “NOT” this appears to be using in 
several places where lower case “not” is actually correct/