Last Call Review of draft-ietf-core-new-block-10
review-ietf-core-new-block-10-genart-lc-resnick-2021-04-24-00

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-core-new-block-10
Reviewer: Pete Resnick
Review Date: 2021-04-24
IETF LC End Date: 2021-04-28
IESG Telechat date: Not scheduled for a telechat

Summary:

The document looks pretty solid to me. There is one item I marked as a minor
"issue", but it may simply be an editorial item that confused me; I figured I'd
call it an issue just in case so it doesn't get left to the last minute to look
at.

Do note that I have not reviewed the examples for correctness; I simply don't
have the expertise to be convinced I'd do it right.

Major issues:

None.

Minor issues:

In section 4.4:

I find this paragraph confusing:

   The requested missing block numbers MUST have an increasing block
   number in each additional Q-Block2 Option with no duplicates.  The
   server SHOULD respond with a 4.00 (Bad Request) to requests not
   adhering to this behavior.

So, given the SHOULD in the second sentence, it appears that the MUST in the
first sentence doesn't apply to the server (i.e., to enforce this), but rather
to the client doing the request. You should probably say it that way. Also, the
SHOULD in the second sentence is not entirely clear to me: Are you saying that
the server can choose to use some other response code, or are you saying that
the server can accept the request and do something interesting with it? Below
is an attempt to fix it, but might not be correct depending on what you mean:

   The client MUST use an increasing block number in each additional
   Q-Block2 Option when requesting missing block numbers, and MUST
   request no duplicates.  The server MUST reject requests  not adhering
   to this behavior and SHOULD respond with a 4.00 (Bad Request) to such
   requests.

There are other places in the document that use MUST with regard to what needs
to be in a piece of data (see for example sections 4.5 and 4.6), but don't make
it clear who is responsible for enforcing that MUST (the client or the server).
You should read through the entire document for MUSTs (or SHOULDs) like that
and make sure it's clear from the context.

Nits/editorial comments:

In section 4.3:

In several response code definitions:

   The token used MUST be any token that was received in a request using
   the same Request-Tag.

That doesn't really parse well. I think you either mean "The token used MUST be
a token" or you mean "The token used can be any token".

Specific response codes:

   4.00 (Bad Request)

      This Response Code MUST be returned if the request does not
      include neither a Request-Tag Option nor a Size1 Option but does
      include a Q-Block1 option.

Either change "neither...nor" to "either or", or change "does not include" to
"includes".

   4.02 (Bad Option)

      Either this Response Code (in case of Confirmable request) or a
      reset message (in case of Non-confirmable request) MUST be
      returned if the server does not support the Q-Block Options.

That sort of buries a MUST requirement for the Non-confirmable case inside this
requirement for a Response Code. I suggest instead:

      This Response Code MUST be returned for a Confirmable request if
      the server does not support the Q-Block Options. (A reset message
      is sent in case of Non-confirmable request.)

In section 4.4:

The passive here is not great form, particularly because it doesn't name the
actor:

   It is permissible to set the M bit to request this...

How about instead:

   The client MAY set the M bit to request this...

Maybe that's obvious, since the client does the requesting, but I think the
non-passive form is easier to read.

In the second to last paragraph:

   If the server transmits a new body of data (e.g., a triggered Observe
   notification) with a new ETag to the same client as an additional
   response, the client should remove any partially received body held
   for a previous ETag for that resource as it is unlikely the missing
   blocks can be retrieved.

I'm ambivalent about whether that "should" ought to be uppercased, but I just
wanted to make sure you intended the lowercase.

In section 7.2:

   For the server receiving NON Q-Block1 requests, it SHOULD send back a
   2.31 (Continue) Response Code on receipt of all of the MAX_PAYLOADS
   payloads to prevent the client unnecessarily delaying.  Otherwise...

When you say "Otherwise", Do you mean, "For other payloads"? Either way, you
should probably change that to make it clear.