Subject: [core] Tsvart last call review of draft-ietf-core-new-block-11

From: Colin Perkins via Datatracker <[email-masked]>
Date: Wed, 28 Apr 2021 14:54:34 -0700

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
[email-masked] 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.

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.

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.

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.

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).

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.

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, 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?

---

From: <[email-masked]>
Date: Thu, 29 Apr 2021 06:52:31 +0000

Hi Colin,

Thank you for the review. 

Please see inline.

Cheers,
Med

> -----Message d'origine-----
> De : Colin Perkins via Datatracker [mailto:[email-masked]]
> Envoyé : mercredi 28 avril 2021 23:55
> À : [email-masked]
> Cc : [email-masked]; [email-masked]; last-
> [email-masked]
> Objet : Tsvart last call review of draft-ietf-core-new-block-11
> 
> Reviewer: Colin Perkins
> 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 [email-masked] 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.

> - 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. 

> 
> 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).

> 
> 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

[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.

> 
> 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. 

> 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)

> 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.   

_________________________________________________________________________________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci.

This message and its attachments may contain confidential or privileged information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified.
Thank you.

---

From: Colin Perkins <[email-masked]>
Date: Fri, 30 Apr 2021 12:02:29 +0100

Hi,

[inline]

On 29 Apr 2021, at 7:52, [email-masked] wrote:
> [Med] Argh, I thought we added an example as we did for GET. Thanks 
> for catching this. We can cite POST or PATCH.

Thanks.

> [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.

>    using Q-Block1 and Q-Block2 will be Non-confirmable (Section 3.2).

Clearer, thanks.

> [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.

> [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”?

> [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.

> [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

---

From: <[email-masked]>
Date: Fri, 30 Apr 2021 13:13:01 +0000

Hi Colin, 

Please see inline. (removed items that are addressed)

Cheers,
Med

PS: The changes can be tracked here: https://tinyurl.com/new-block-latest 

> De : Colin Perkins [mailto:[email-masked]]
> Envoyé : vendredi 30 avril 2021 13:02
> À : BOUCADAIR Mohamed TGI/OLN <[email-masked]>
> Cc : [email-masked]; [email-masked]; draft-ietf-core-new-
> [email-masked]; [email-masked]
> Objet : Re: Tsvart last call review of draft-ietf-core-new-block-11
> 
> Hi,
> 
> [inline]
> 
> Envoyé :
> >> mercredi 28 avril 2021 23:55 À : [email-masked] Cc :
> [email-masked];
> >> [email-masked]; last- [email-masked]
> Objet :
> >> Tsvart last call review of draft-ietf-core-new-block-11
> >>
> >> Reviewer: Colin Perkins
> 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.
> 

[Med] Made this change: 

OLD:
      Note: The default value of 10 is chosen for reasons similar to
      those discussed in Section 5 of [RFC6928].

NEW:
      Note: The default value of 10 is chosen for reasons similar to
      those discussed in Section 5 of [RFC6928], especially given the
      target application discussed in Section 3.2.

> 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

[Med] Thank you, but I prefer the old wording.  

> Might be worth stating that in the draft?

[Med] After thinking about this further, we added this NEW text:

NEW:
   Following a period of 24 hours where no packet
   recovery was required, the value of MAX_PAYLOADS can be increased by
   1 (but without exceeding the default value) for a further 24 hour
   evaluation.

---

From: Colin Perkins <[email-masked]>
Date: Fri, 30 Apr 2021 14:18:58 +0100

That’s all fine – thank you!
Colin

> On 30 Apr 2021, at 14:13, <[email-masked]> <[email-masked]> wrote:
> 
> Hi Colin, 
> 
> PS: The changes can be tracked here: https://tinyurl.com/new-block-latest <https://tinyurl.com/new-block-latest> 
> 
>> De : Colin Perkins [mailto:[email-masked] <mailto:[email-masked]>]
>> À : BOUCADAIR Mohamed TGI/OLN <[email-masked] <mailto:[email-masked]>>
>> Cc : [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>; draft-ietf-core-new-
>> [email-masked] <mailto:[email-masked]>; [email-masked] <mailto:[email-masked]>

-- 
Colin Perkins
https://csperkins.org/