[core] AD review of draft-ietf-core-new-block-09

[Francesca Palombini <[e-mail]>]

Thanks for the work on this document.  I have a number of comments about things that I think need clarification or rewording in my review below, so I’m setting the state on the document to “revised I-D needed”, and hope we can get an update solving those before we move to IETF last call.  Please keep me in the "to" field to make sure I see any reply.

Please note that the comments are reported while reading the document from top to bottom - some comments could be avoided by referencing the right sections or restructuring the document. If I notice that this is the case, I make a suggestion in the comment - if I do miss something that is explained later on, please consider the comment a suggestion to add a reference to improve readability and add clarifications.

General:

1. 
[FP]: Throughout the text, it seems that "payload" is used as a synonym to "CoAP request/response" or "block". This is incorrect and should be fixed. (See for example "received payload")

-----

2.
[FP]: Throughout the document, SHOULD is used without explanation of why the recommendation is just that - a recommendation - and not a requirement. There needs to be more clarification to implementers when it is ok and can be expected to deviate from the recommendations. I noted those that jumped to my attention, but I suspect a general re-read of the document with this comment in mind should be done, as there might be more I missed.

=====

Section 1:

3. 
   o  They can operate in environments where packet loss is highly
      asymmetrical.

[FP]: Please clarify: Block1 and Block2 can also be used in environments where packet loss is asymmetrical. What do these new options add in that case? 

-----

4.
   o  To reduce the transmission times for CON transmission of large
      bodies, NSTART needs to be increased from 1, but this affects

[FP]: NSTART appears here for the first time, please add a reference to where this is defined (fw reference to a section of this specification or to a different document).

-----

5.
   Block2 Options when the different transmission properties are
   required.  If the new options are not supported by a peer, then

[FP]: I hope these transmission properties are explained into detail later on. If so, please add a fw reference. If not, please clarify what these properties are.

-----

6.
   The No-Response Option was considered but was abandoned as it does

[FP]: No-Response Option needs a reference.

=====

Section 2:

7.
[FP]: Because of the way the document is structured, the subsections of section 1 use terminology that is given for granted, and mentioned in section 2. This create some confusion in the reader. I suggest to re-structure the sections so that Terminology is right after 1. Introduction and before 1.1

=====

Section 3:

8.
   The Content-Format Option applies to the body, not to the payload
   (i.e., it must be the same for all payloads of the same body).

[FP]: I think this is missing that the previous sentence is true "when present together with the Q-Block1 or Q-Block2 Option".

-----

9.
   include the Q-Block2 Option in a GET or similar request, 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

[FP]: I am confused by the meaning of "similar request": what is considered a similar request to a GET request? How is a PUT request different from a GET request? Additionally, how does the client decides if it needs to include a Q-Block1 or Q-Block2 Option in a PUT (or similar) request to indicate support for these two options?

-----

10.
   Note that if Q-Block1 or Q-Block2 Options are included in a packet as
   Inner options, Block1 or Block2 Options MUST NOT be included as Inner
   options.  Similarly there MUST NOT be a mix of Q-Block and Block for

[FP]: Maybe this is described later on, but - what is the node to do if it receives a message with both? Is the message to be rejected with an error? This should be clarified.

-----

11.
   being transferred.  It is also used to identify a particular payload
   of a body that needs to be retransmitted.  The Request-Tag is opaque,

[FP]: If it is the same for all the requests carrying one same body, how does it identify one particular payload? Also I am assuming that "a particular payload of a body" means a block of the body - if that is not correct, I think the "payload of a body" should be rephrased.

-----

12.
   Each individual payload of the body is treated as a new request
   (Section 5).

[FP]: Again, I suspect the meaning of this sentence will be clarified in section 5, but I think it is incorrect to say that each payload is treated as a new request - each payload is treated as a payload. I think this needs to be rephrased, but at this point in the text I can't say what the intent of this sentence was, so can't suggest a rephrasing.

-----

13.
      and that the resource was created.  The token used SHOULD be from
      the last received payload.  The client should then release all of

[FP]: I think it would be worth to highlight that the last received block is not necessarily the block with the highest block number. (note this appears in more than one occurrence)

-----

14.
      M bit set) have been successfully received.  The token used SHOULD
      be from the last received payload.

[FP]: "SHOULD" must be motivated: why is this not a MUST? What are the conditions where it is acceptable that the token is different from the one in the request with the last received payload? (note: several occurrences)

-----

15.
      A response using this Response Code SHOULD NOT be generated for
      every received Q-Block1 Option request.  It SHOULD only be
      generated when all the payload requests are Non-confirmable and a
      set of MAX_PAYLOADS (Section 6.2) payloads have been received by
      the server.

      It SHOULD NOT be generated for CON.

[FP]: Same remark as previous comment - please motivate why this is SHOULD and is not MUST. When using SHOULD, guidance should be given to implementers about in which case it is ok to deviate from the recommendations.

-----

16.
   SHOULD "forget" all tracked tokens associated with the body's

   token in the 4.08 (Request Entity Incomplete) response.  The server
   on receipt of the reset message SHOULD delete the partial body.

   it SHOULD ignore the payload of the packet, but MUST still respond as

   A server SHOULD only maintain a partial body (missing payloads) for

[FP]: Same remark for the use of SHOULD. (I stopped nothing this down at this point in the review, but noted this in the general comments above)

-----

17.
   unset.  It is permissible to set the M bit to request this and
   missing blocks from this MAX_PAYLOADS set.  Further considerations

[FP]: "this and missing blocks" is unclear to me. Could you clarify?

-----

18.
   The requested missing block numbers MUST have an increasing block
   number in each additional Q-Block2 Option with no duplicates.  The

[FP]: What is the reason for mandating that the blocks requested must be in order (increasing block number)?

-----

19.
   packet.  The server acknowledges the initial request using an ACK
   with the payload, and then sends the subsequent payloads as CON

[FP]: I suspect that this means that the first response (containing the first block) is piggybacked on an ACK, see 5.2.1 of RFC 7252. That is though not always the case, see 5.2.2. I think this option of piggybacked should be kept, but from the text it sounds like it is the only option, so I believe some rephrasing to clarify that might be necessary.

-----

20.
   The ETag Option should not be used in the request for missing blocks

   client should treat the partial body with the old ETag as no longer

   response, the client should remove any partially received body held

[FP]: Why not replace the should (resp should not) with MUST (resp MUST NOT)?

=====

21.
Section 4:

   encoded as a CBOR Sequence [RFC8742].  It comprises of one or more
   CBOR encoded [RFC8949] missing block numbers.  The missing block

[FP]: I suggest to change to " missing block numbers encoded as CBOR unsigned integers [RFC8949]."

-----

22.
   response when created by the server; the client SHOULD drop any
   duplicates in the same 4.08 (Request Entity Incomplete) response.

[FP]: I think that "drop" here might be confusing, since the client is not dropping a packet - suggestion to change to "ignore".

-----

23.
   The Content-Format Option (Section 5.10.3 of [RFC7252]) MUST be used
   in the 4.08 (Request Entity Incomplete) response.  It MUST be set to
   "application/missing-blocks+cbor-seq" (Section 11.3).

[FP]: This is in direct contradiction with the following paragraph from a previous section:

      This Response Code returned without Content-Type "application/
      missing-blocks+cbor-seq" (Section 11.3) is handled as in
      Section 2.9.2 [RFC7959].

-----

24.
   Request-Tag would work, but providing the one used in the last

[FP]: Suggestion to rephrase "would work" to "would allow to identify the correct body" (or something similar).

-----

25.
   single packet.  If this is the case, then the server can send
   subsequent 4.08 (Request Entity Incomplete) responses containing the
   missing other blocks on receipt of a new request providing a missing
   payload with the same Request-Tag.

[FP]: Just another check: is it specified anywhere that reception of an error response does not mean the client should stop sending requests when Q-Block options are used? If not, it would be good to explicitly state that.

-----

26.
   The missing blocks MUST be reported in ascending order without any
   duplicates.  The client SHOULD silently drop 4.08 (Request Entity

[FP]: Same question: why does order matter? 

=====

Section 5:

27.
   Implementation Note:  To minimize on the number of tokens that have
      to be tracked by clients, it is suggested that the bottom 32 bits
      is kept the same for the same body and the upper 32 bits contains
      the current body's request number (incrementing every request,
      including every re-transmit).  This allows the client to be

[FP]: This seems to imply that tokens are always 8 bytes, which is not necessarily the case. I suggest to add "in the case the token is 8 bytes".

=====

Section 6:

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

[FP]: "it is expected ... Non-confirmable" Where does this come from? Maybe I misunderstand this sentence, could you please expand?

=====

Section 9:

29.
            +--------->| [[Payloads 3 - 9 not detailed]]

[FP]: Minor: I think it would make more sense to change "3 - 9" to "2 - 8" (and same for examples below)

-----

Thank you for the examples! Very clear and easy to follow.

-----

30.
           +--------->| NON PUT /path M:0x1f T:0xef RT=11 QB1:12/0/1024
           |   ...    |
        [[NON_RECEIVE_TIMEOUT (server) delay expires]]
           |     [[The server realizes a block is still missing and asks
           |        for the missing one]]
           |<---------+ NON 4.08 M:0x92 T:0xef [Missing 10]

[FP]: I am not sure this is already stated somewhere, and if so please ignore this comment, but there should be some considerations about NON_RECEIVE_TIMEOUT in comparison to the client's CoAP timer to deal with the response: what I am thinking about is the scenario from the following example, where the client has for some reason discarded the token 0xef (for example to free up space) and is unable to process the 4.08 response from the server correctly, since it doesn't know with which request it associates with. What happens then? I assume the server should try again and then eventually stop and release the partial body. Is this described?

-----

31.
          |<---------+ [[Payloads 3 - 8 not detailed]]

[FP]: This is inconsistent and should be either 3 - 9 or 2 - 8 as noted above.

-----

32.
[FP]: Figure 16: I am not sure how the server knows which block the client is requesting when it sends the following message:

          +--------->| NON FETCH /path M:0x58 T:0xc8 RT=31 QB2:1/0/1024

[FP]: In fact, if I understand correctly this could either be a request for the missing block from the second set of blocks, or the client could have not received block 1 from the previous set of blocks and could be requesting the same as in:

          +--------->| NON FETCH /path M:0x57 T:0xc7 RT=31 QB2:1/0/1024

[FP]: If all the following responses from the server would have been lost. The two requests contain the same RT. What am I missing?

=====

Francesca