Re: [Tsv-art] Tsvart last call review of draft-ietf-core-new-block-11

[Colin Perkins <[e-mail]>]

Hi,

[inline]

On 29 Apr 2021, at 7:52, [e-mail] wrote:
> Hi Colin,
>
> Thank you for the review.
>
> Please see inline.
>
> Cheers,
> Med
>
>> -----Message d'origine-----
>> De : Colin Perkins via Datatracker [mailto:[e-mail]]
>> Envoyé : mercredi 28 avril 2021 23:55
>> À : [e-mail]
>> Cc : [e-mail]; [e-mail]; last-
>> [e-mail]
>> Objet : Tsvart last call review of draft-ietf-core-new-block-11
>>
>> Reviewer: Colin Perkins
>> Review result: Ready with Issues
>>
>> This document has been reviewed as part of the transport area review
>> team's ongoing effort to review key IETF documents. These comments
>> were written primarily for the transport area directors, but are
>> copied to the document's authors and WG to allow them to address any
>> issues raised and also to the IETF discussion list for information.
>>
>> When done at the time of IETF Last Call, the authors should consider
>> this review as part of the last-call comments they receive. Please
>> always CC [e-mail] if you reply to or forward this review.
>>
>> Thank you for preparing such a clearly written, precise,
>> specification. On the whole, this is very good. I just have some
>> minor issues to consider.
>
> [Med] Thanks.
>
>>
>> Section 4.1 says “To indicate support for Q-Block2 responses, the
>> CoAP client MUST include the Q-Block2 Option in a GET or similar
>> request (FETCH, for example), the Q-Block2 Option in a PUT or similar
>> request, or the Q-Block1 Option in a PUT or similar request so that
>> the server knows that the client supports this Q-Block 
>> functionality”
>> – It would be useful to enumerate what are “similar” requests.
>
> [Med] Argh, I thought we added an example as we did for GET. Thanks 
> for catching this. We can cite POST or PATCH.

Thanks.

>> Section 5: “Such messages must not be treated by the client as a
>> fatal error“
>> - I was surprised this is not a normative MUST NOT.
>
> [Med] We don't use the normative language here as we though this is 
> implicitly covered by the behavior in 4.3 where we indicate that the 
> client retransmits the missing blocks when such error is received.

Yes, it probably is implicitly covered.

My underlying concern here is that the draft seems a little inconsistent 
about when it uses the RFC 2119 normative language and when it 
doesn’t. There are a few places where I see “must not” and wonder 
why it’s not “MUST NOT”, and this was the most striking example.

>> Section 7.1: “For faster transmission rates, NSTART will need to be
>> increased from 1.  However, the other CON congestion control
>> parameters will need to be tuned to cover this change.  This tuning
>> is out of scope of this document as it is expected that all requests
>> and responses using Q-Block1 and Q-Block2 will be Non-confirmable
>> (Section 3.2).” - The way this is phrased is difficult to parse.
>> I can interpret it as saying that the transmission rate *does* need
>> to be faster, so implementations need to increase NSTART and tune the
>> other parameters.
>> Alternatively, I can interpret this as saying that *if* the
>> transmission needs to be faster, then NSTART and the other parameters
>> need to be tuned in some as-yet-unspecified way. The text would
>> benefit from being rephrased to clarify which meaning is intended.
>>
>> What happens when NSTART is increased beyond 1, and how the other
>> parameters are tuned, is unclear. The text would be better if it
>> either cross-referenced to the definition of how the parameters are
>> to be tuned, or explicitly stated that this is not yet supported and
>> will need to be defined in some future extension.
>
> [Med] Updated as follows:
>
> OLD:
>    Congestion control for CON requests and responses is specified in
>    Section 4.7 of [RFC7252].  For faster transmission rates, NSTART 
> will
>    need to be increased from 1.  However, the other CON congestion
>    control parameters will need to be tuned to cover this change.  
> This
>    tuning is out of scope of this document as it is expected that all
>    requests and responses using Q-Block1 and Q-Block2 will be Non-
>    confirmable (Section 3.2).
>
> NEW:
>    Congestion control for CON requests and responses is specified in
>    Section 4.7 of [RFC7252].  In order to benefit from faster
>    transmission rates, NSTART will need to be increased from 1.
>    However, the other CON congestion control parameters will need to 
> be
>    tuned to cover this change.  This tuning is not specified in this
>    document given that the applicability scope of the current
>    specification assumes that all requests and responses
>    using Q-Block1 and Q-Block2 will be Non-confirmable (Section 3.2).

Clearer, thanks.

>>
>> In Section 7.2, I’m not convinced by the argument to set 
>> MAX_PAYLOAD
>> to 10 for similar reasons to RFC 6928. The types of link layer that
>> CoAP runs over are very different to those measured to support the
>> increase in TCP’s initial window. An argument based on typical
>> properties of links and buffer space in networks used by CoAP would
>> be more convincing (I accept that using MAX_PAYLOAD of 10 is not
>> going to do any significant harm, even if it is higher than optimal).
>
> [Med] Actually we set it to 10 as the applicability scope of this spec 
> is DOTS which runs in environments similar to those of 6928. Please 
> see Section 3.2.

This would be a lot clearer if Section 3.2 were cross-referenced, and a 
reminder of the limited applicability of this specification was added to 
Section 7.2.

>> Section 7.2 also notes that “PROBING_RATE and other transmission
>> parameters are negotiated between peers”. It would be appropriate 
>> to
>> give some guidance on what are the bounds for safe values that can be
>> negotiated for these parameters.
>
> [Med] I'm afraid this is out of the scope of this spec. The intent of 
> this note is to provide an example of an application that negotiates 
> these parameters. Some of these details can be found in in 
> rfc8782#section-4.5.2 mentioned in the text you quoted.

Makes sense, although I wonder if the text in Section 7.2 might be more 
clearly written “The DOTS application uses customised defaults for 
PROBING_RATE and other transmission parameters, as discussed in Section 
4.5.2 of [RFC8782], that are negotiated between peers”?

>> Section 7.2 says:
>>
>>>   As the sending of many payloads of a single body may itself cause
>>>   congestion, it is RECOMMENDED that after transmission of every
>> set of
>>>   MAX_PAYLOADS payloads of a single body, a delay is introduced of
>>>   NON_TIMEOUT before sending the next set of payloads to manage
>>>   potential congestion issues.
>>
>> and the following paragraph has guidance for reducing MAX_PAYLOADS if
>> persistent congestion occurs “for at least a 24 hour period and it 
>> is
>> known that there are no other network issues over that period”. 
>> It’s
>> not clear how an implementation will know about other network issues,
>
> [Med] An example is a DDoS attack. Made this change: s/about other 
> network issues/about other network issues (e.g., DDoS attacks)

Makes sense, thanks.

>> and I would suggest that even if there are such issues, backoff would
>> be appropriate given persistent congestion.
>>
>> Finally, is there are mechanism for gradually recovering MAX_PAYLOADS
>> to its original value, if persistent loss ceases for some period?
>>
>
> [Med] This is covered by the configuration refresh/negotiation 
> mechanism. The peers will refresh the configuration parameters 
> following, for example, I-D.bosh-dots-quick-blocks.

Might be worth stating that in the draft?

Cheers,
Colin