Re: [tsvwg] Review of draft-ietf-tsvwg-rfc4960-bis-08

[Gorry Fairhurst <[e-mail]>]

Thanks Timo for the review...

On 31/12/2020 10:02, Timo Völker wrote:
> Hi,
>
> Here is my review for the RFC4960bis draft (draft-ietf-tsvwg-rfc4960-bis-08).
>
> My list starts with minor issues (typos, etc.) followed by comments about some terms.
>
>
> *Typos, Wording*
>
> Section 3.1
>
> Verification Tag:
> OLD: The receiver of this packet uses the Verification Tag to validate the sender of this SCTP packet. On transmit, the value of this Verification Tag MUST be set to...
> NEW: The receiver of an SCTP packet uses the Verification Tag to validate the sender of this packet. On transmit, the value of the Verification Tag MUST be set to...
>
> Checksum:
> OLD: This field contains the checksum of this SCTP packet.
> NEW: This field contains the checksum of the SCTP packet.
>
>
> Section 3.2
>
> Chunk Type encoding
> OLD: ...and report the unrecognized chunk in an 'Unrecognized Chunk Type'.
> NEW: ...and report the unrecognized chunk in an ERROR chunk using the 'Unrecognized Chunk Type' cause of error.
>
> Text about padding in a chunk repeats. First described in the description for Chunk Length field and then directly below the description of the chunk fields. I suggest to remove it in the description of the Chunk Length field, but to keep the relevant part for the field.
> OLD: The Chunk Length field does not count any chunk padding. Chunks (including Type, Length, and Value fields) are padded...
> NEW: The value in the Chunk Length field does not include any chunk padding. However, it does include padding of any variable-length parameter except the last parameter in the chunk.
>
>
> Section 3.2.1
>
> Start the enumeration with the word Parameter.
> OLD: The total length of a parameter (including Type, Parameter Length, and Value fields)...
> NEW: The total length of a parameter (including Parameter Type, Length, and Value fields)...
>
> The text about Parameters begins like any chunk may include parameters. If this generic form was used by intention, I suggest to rewrite the part below Table 3, which refers to specific chunks.
> OLD: Please note that in all four cases, an INIT ACK or COOKIE ECHO chunk is sent.
> NEW: Please note that, when an INIT or INIT ACK chunk is received, in all four cases, an INIT ACK or COOKIE ECHO chunk is sent in response, respectively.
>
> This continues in Section 3.2.2.
> OLD: If the receiver of an INIT ACK chunk detects unrecognized parameters and has to report them according to Section 3.2.1, it SHOULD bundle the ERROR chunk containing the 'Unrecognized Parameters' error cause with the COOKIE ECHO chunk sent in response to the INIT ACK chunk.
> NEW: If the receiver of any other chunk (e.g., INIT ACK) detects unrecognized parameters and has to report them according to Section 3.2.1, it SHOULD bundle the ERROR chunk containing the 'Unrecognized Parameters' error cause with a chunk sent in response (e.g., COOKIE ECHO).
>
>
> Section 3.3.2
>
> Table 5 and 6 lists all specified parameters for the INIT chunk, including the Host Name Address parameter. The text before the tables "The INIT chunk contains the following parameters" does not match to the text after the tables "An INIT chunk MUST NOT contain the Host Name Address parameter".
> OLD: The INIT chunk contains the following parameters.
> NEW: The following parameters are specified for the INIT chunk.
> OLD: Host Name Address Optional 11
> NEW: Host Name Address Deprecated 11
>
> I find the text about known parameters that are not optional parameters difficult to understand, since based on the tables, I assumed not optional means mandatory.
> OLD: If an INIT chunk is received with known parameters that are not optional parameters of the INIT chunk,
> NEW: If an INIT chunk is received with parameters that are specified for the INIT chunk,
>
>
> Section 3.3.3
>
> It seems like the position of tables 7 and 8 should be directly after this paragraph "The parameter part of INIT ACK is formatted similarly to the INIT chunk. It uses two extra variable parameters: The State Cookie and the Unrecognized Parameter."
>
> The description for the Advertised Receiver Window Credit in the INIT chunk contains an additional half sentence.
> OLD: ...SHOULD NOT be lessened (i.e., dedicated buffers taken away from this association).
> NEW: ...SHOULD NOT be lessened (i.e., dedicated buffers taken away from this association); however, an endpoint MAY change the value of a_rwnd it sends in SACK chunks.


The word "lessened" seems a little odd in this context, since the 
sentenced is already being changed, maybe we could replace this with "be 
reduced" or similar?

and .. this: "(i.e., dedicated buffers taken away from this 
association):" would seem much clearer as:

"(i.e., dedicated buffers ought not to be taken away from this 
association);"

>
> Similar to my suggestion for the Section 3.3.2
> OLD: If an INIT ACK chunk is received with known parameters that are not optional parameters of the INIT ACK chunk,
> NEW: If an INIT ACK chunk is received with parameters that are not specified for the INIT ACK chunk,
>
>
> Section 3.3.3.1.2
>
> Type seems to me the better word here.
> OLD: ...an unrecognized parameter that has a value that indicates it SHOULD be reported to the sender.
> NEW: ...an unrecognized parameter that has a type that indicates it SHOULD be reported to the sender.
>
>
> Section 3.3.7
>
> OLD: ...but they MUST be placed before the ABORT in the SCTP packet or they will be ignored by the receiver.
> NEW: ...but they MUST be placed before the ABORT in the SCTP packet, otherwise they will be ignored by the receiver.
>
>
> Section 4
>
> State Diagram
> * Use send and start, instead of snd and strt.
> * Add a space after "(B)", "(C)" and "(D)"
> * There is an arrowhead missing on the line between SHUTDOWN-RECEIVED and SHUTDOWN-ACK-SENT.
> * The position of "(B)rcv SHUTDOWN" looks like it belongs to the arrow that is pointing to CLOSED.
>
>
> Section 5.1.6
>
> In Figure 4
> OLD: DATA [TSN=initial TSN_A
> NEW: DATA [TSN=init TSN_A
> OLD: DATA [TSN=init TSN_Z
> NEW: DATA [TSN=init TSN_Z,
>
>
> Section 6.4
>
> Remove space.
> OLD: When a receiver of a duplicate DATA chunk sends a SACK to a multi- homed endpoint...
> NEW: When a receiver of a duplicate DATA chunk sends a SACK to a multi-homed endpoint...
>
>
> Section 6.7
>
> The text reads like the sender should treat all data chunks not acknowledged in an SACK as missing. I assume the highest acknowledged TSN should take into account (as Section 7.2.4 describes).
> OLD: 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.
> NEW: All DATA chunks with TSNs not included in the Gap Ack Blocks which are smaller than the highest acknowledged TSN reported by a SACK MUST be treated as "missing" by the sending endpoint.
/which/that/ ?
>
>
> Section 8.4
>
> It seems an "Otherwise" is missing and a "process" should be removed.
> OLD: 3) ..., indicating that the Verification Tag is not reflected.
> NEW: 3) ..., indicating that the Verification Tag is not reflected. Otherwise,
> OLD: 4) If the packet contains a COOKIE ECHO in the first chunk, process it MUST be processed as described in Section 5.1.
> NEW: 4) If the packet contains a COOKIE ECHO in the first chunk, it MUST be processed as described in Section 5.1.
>
>
> Section 11
>
> Typo
> OLD: However, all SCTPs aer expected to provide a...
> NEW: However, all SCTPs are expected to provide a...
AGain, in thr orriginal: /all SCTPs/ ... what does this refer to /SCTP 
endpoints/ ? something else?
>
> Section 11.1.10
>
> OLD: 11.1.10. Request HeartBeat
> NEW: 11.1.10. Request HeartBeat
> OLD: Instructs the local endpoint to perform a HeartBeat on the...
> NEW: Instructs the local endpoint to perform a heartbeat on the...
>
>
> Section 11.1.14
>
> Typo
> OLD: payload protocol-id: The 32 bit unsigned integer that was sent to be sent to the peer...
> NEW: payload protocol-id: The 32 bit unsigned integer that was set to be sent to the peer...
>
>
> Section 14.2
>
> Remove space.
> OLD: Mapping Array: An array of bits or bytes indicating which out-of- order TSNs have been received (relative to the Last Rcvd TSN). If no gaps exist, i.e., no out-of- order packets...
> NEW: Mapping Array: An array of bits or bytes indicating which out-of-order TSNs have been received (relative to the Last Rcvd TSN). If no gaps exist, i.e., no out-of-order packets...
>
>
> *Terms, Comprehension*
>
> Far End
> Only Section 2.5.7 and 9.2 use the term "far end". I assume it is a synonym to, for example, the term "peer endpoint". If so, I suggest to use "peer end" or "peer endpoint" in these sections as well.
>
> RECEIVE_UNSENT, RECEIVE_UNACKED
> I do not understand what a RECEIVE_UNSENT and RECEIVE_UNACKED (Section 11.1.14 and 11.1.15) is used for. A sentence before listening the attributes would be helpful.
>
> Protection of Non-SCTP-Capable Hosts
> I might misunderstand Section 12.4, but for me it seems all paragraphs except the first one are not about protecting Non-SCTP-Capable Hosts. Does the last three paragraph belong to a Section 12.5, which title is missing?
>
> Chunk Bundling Option
> If I disable an option called chunk bundling, I would assume that SCTP does not bundle chunks at all. Section 2.5.5 describes that chunks are still bundled. Also, I guess this only applies to DATA chunks. A name like "Delay for DATA Chunk Bundling" would make it clearer. (Section 11.1.5 describes the no-bundle flag, which refers to that option).
>
> MTU
> For me, MTU describes the maximum size of an IP packet. Often, this document uses MTU as the maximum size of an SCTP packet. As the maximum size of an SCTP packet is not always the MTU minus IP header, why not using an own term? For example, Maximum SCTP Packet Size (MSPS). I think, this would help to make the text clearer.
> For example, Section 6.1 contains
> "the sender SHOULD create a SACK and bundle it with the outbound DATA chunk, as long as the size of the final SCTP packet does not exceed the current MTU."
> which seems wrong to me. I think replacing MTU with MSPS is the simplest solution.

Seems like the correct some rewording is needed... could it be PMTU?

That makes me also to suggest the editors take a look at the terms 
/current MTU/, MTU, /path MTU/ and PMTU and ensure these are consistent.

e.g.:, since you raise this (D) in 6.1 also mentions MTU for controlling 
maxburst, is this MTU or PMTU in this case?

       if ((flightsize + Max.Burst * MTU) < cwnd)
           cwnd = flightsize + Max.Burst * MTU;

And:
/the current MTU/ - is that also /PMTU/ or maybe /current PMTU/?

In 6.3.3, is this MTU or PMTU (if it's MTU, please say why):

/
    E1)  For the destination address for which the timer expires, adjust
         its ssthresh with rules defined inSection 7.2.3  <https://tools.ietf.org/html/draft-ietf-tsvwg-rfc4960-bis-08#section-7.2.3>  and set the
         cwnd <- MTU./

- E.3 might also refer to /PMTU/ for consistency:

/subject to the MTU
constraint for the path corresponding/
subject to the PMTU corresponding/

Another example...
OLD:
/ timer expired but did not fit in one MTU (rule E3 above) SHOULD be/
NEW:
/ timer expired, but did not fit in an SCTP packet smaller than the PMTU (rule E3 above) SHOULD be/

Note: the word /Path MTU/ and /PMTU/ are not used consistently, but I think refer to the same.
  

> Timo

Gorry