Bundle Protocol Version 7
draft-ietf-dtn-bpbis-31

I was going to ballot as Discuss, but I lack some context on the history
of this document and its evolution.  Nevertheless, hopefully the "discuss"
points below leave a record of things about which I had some concern.


[[ discuss ]]

[ section 4.1.5.1.2 ]

* The encoding considerations mentions "dtn" scheme but this is the "ipn"
  scheme section so...should this be "ipn"?

[ section 5.2 ]

* Should step 2 be step 1 of 5.3 (not 5.4)?

[ section 5.4.2 ]

* It seems to me that the behaviour in step 1, which I think does make some
  sense, pretty easily sets up the potential for ping-ponging (AIUI).

  If true, should there be some text (perhaps in 5.4) acknowledging that
  forwarding a given bundle to a node for the 2nd time might warrant, at least,
  some reassessment of the routing/reachability state in the node?

  I fully understand that extensive routing discussion is out of scope
  for this document.

[ section 5.6 ]

* Does "cannot process" include "does not understand"?

  If so it might be good to use a different reason value from
  "Block unintelligible" so that some other node can understand that, for
  example, the CRC (if sent) was valid.

  Basically, consider differentiating between "understood but malformed" and
  "not understood".

[ section 5.9 ]

* The mention of non-overlapping fragments brings a few issues to mind that
  have been encountered in IPv6 when considering fragment handling:

    [1] How are overlapping fragments to be handled?  Are they ignored
    and/or cause any specific error to be generated? (vis. IPv6: RFC 5722)

    [2] What about "atomic fragments", i.e. a fragment that starts at
    offset zero and has a total payload length equal to the actual length
    of the payload (i.e. a fragmented payload consisting of a single fragment)?
    (vis. IPv6: RFC 6949)

    I'm guessing these should be silently accepted, but there might be some
    text saying they MUST/SHOULD NOT be generated.


[[ comments ]]

[ section 4.2.2 ]

* The Creation Timestamp element description seems to copy a bunch of text
  from 4.1.7.  Might be able to shorten the document a bit by referring to
  4.1.7 for extended discussion of creation timestamp operations.


[[ questions ]]

[ section 4.3.3 ]

* What's the value of using a {limit, incrementing_value} pair like this over
  just a single decrements_to_zero integer?  I'm not suggesting this be
  changed (it's probably too late anyway), but I was just curious as to the
  motivation.

Thank you for addressing my DISCUSS and COMMENTs items.

Thank you for the work put into this document. And it is a really interesting topic !

The document is not really easy to read with some long and convoluted sentence such as
  "An application data unit is the unit
   of data whose conveyance to the bundle's destination is the purpose
   for the transmission of some bundle that is not a fragment (as
   defined below)."

Another one:
  "The payload for a bundle
   forwarded in response to reception of a bundle is the payload of the
   received bundle."

The above sentences and others make the reading really difficult. Probably too late to change now... 

The section 3.2 go a little too deep IMHO in details (do we care whether the BPA is general-purpose computer?)

As a side note, I would be interested by the potential use of this bundle protocol to transport the future webpack ;-)

Please find below some non-blocking COMMENTs.
C1) is there any reason why the reserved bits in section 4.3.1 are not specified with the usual 'transmitted as 0 and ignored by receiver' ?
C2) in section 4.1.5, is there a registry or a mechanism to avoid / detect identifier collisions ?
C3) just curious, why is it version 7 ? 
C4) in section 4.3.2, if the bundle age is expressed in microseconds, then I would suggest to define exactly 'most recently forwarded': by which component of the bundle node ? is it measured at the completion of the action (e.g., layer-2 serialization is finished). Section 5.4 adds some more information but not enough to my taste (is it when the forwarding decision is made or when the convergence layer has accepted it).
C5) in section 4.3.3., why not following the usual method of hop-limit starting at a non-zero value and being decremented at each forwarding operation ? it is like using DTN time from 2000 rather than the usual timestamp definition. Nothing bad or blocking, but, I wonder why re-inventing the wheel if there is no real reason
C6) the possibility of having a single bundle to generate multiple reports along its path to the 'report-to' endpoint ID per section 5.6 could possibly be used for an amplification attacks + resource exhaustion attack on traversed nodes. Should a rate limiting be mentionned ?
C7) section 5.9, should the re-assembly process time out ? E.g., taking into account the max bundle age ?
C8) section 8, I have hard time to parse the following "the Bundle Protocol version 7 specification draft (version 6)" even if I think I got it right
C9) section 8, an interop status (if any) between the three existing implementations would be welcome

I hope that this helps to improve the document,

Regards,

-éric

I am clearing my DISCUSS with understanding that future work will be done in this area:

1) Retry strategies are listed as out-of-scope for this document. This is not sufficient for writing an interoperable implementation, unless there is a separate document that provides such details.
Below I describe 1 remaining relevant place in the text and suggest some possible ways of addressing my DISCUSS:

   Step 5: When all selected convergence layer adapters have informed
   the bundle protocol agent that they have concluded their data
   sending procedures with regard to this bundle, processing may depend
   on the results of those procedures.  If completion of the data
   sending procedures by all selected convergence layer adapters has
   not resulted in successful forwarding of the bundle (an
   implementation-specific determination that is beyond the scope of
   this specification), then the bundle protocol agent MAY choose (in
   an implementation-specific manner, again beyond the scope of this
   specification) to initiate another attempt to forward the bundle.

Similar to the above: retries affect interoperability and should be documented
as description of a convergence layer document.
I think you need to add this as a requirement to section 7 ("Services Required of the Convergence Layer").

   In that event, processing proceeds from Step 4 of Section 5.4.


**********************************************************************
* Note, that I am conducting an experiment when people aspiring to be*
* Area Directors get exposed to AD work ("AD shadowing experiment"). *
* As a part of this experiment they get to review documents on IESG  *
* telechats according to IESG Discuss criteria document and their    *
* comments get relayed pretty much verbatim to relevant editors/WGs. *
* As an AD I retain responsibility in defending their position when  *
* I agree with it.                                                   *
* Recipients of these reviews are encouraged to reply to me directly * 
* about perceived successes or failures of this experiment.          *
**********************************************************************

I also have some comments on Benjamin's comments below marked with "[[Alexey]]:"


The following comments were provided by Benjamin Schwartz <bemasc@google.com>:

Benjamin would have balloted *DISCUSS* on this document. He wrote:
This draft’s content seems sound but the text is difficult to understand and needs some editorial improvements.


## Abstract

Nit: Abstract should be before status.
Nit: “specification for Bundle Protocol” -> “for the Bundle Protocol”.

## Section 1

Title: If this document doesn’t describe the operation, routing, or management of bundles, maybe it doesn’t describe a complete protocol.  Consider retitling to “Bundle Protocol Version 7: Format and Architecture”

[[Alexey]]: This relates to my DISCUSS points, but my DISCUSS position is stronger than this statement.

## Section 4.1.3

Nit: It would be clearer to move the reserved and unassigned flags to an IANA considerations section, here and throughout.

## Section 4

Phrasing: “The last item of the array ... SHALL be a CBOR "break" stop code.”  This seems like a misuse of the word “item”.  I would suggest “The array SHALL be terminated by a CBOR “break” stop code.”  Similarly in Section 4.2.1.

## Section 4.1.4

Clarity: The “(Boolean)” specifiers seem redundant.  What is a non-boolean flag?

Clarity: The two lists here seem redundant.  Can they be collapsed?

Clarity: The phrasing “...the value of the "Transmit status report if block can't be processed" flag in every canonical block…” suggests that these flags should be named or numbered before attempting to describe them.

## Section 4.1.5.1

These two sentences seem contradictory:

* “Any scheme conforming to [URIREG] may be used in a bundle protocol endpoint ID.”
* “The first item of the array SHALL be the code number identifying the endpoint's URI scheme [URI], as defined in the registry of URI scheme code numbers”

Please rephrase.

[[Alexey]]: This is similar to Roman's DISCUSS.

## Section 5.4.2

Consistency: This section relies on the presence of a Previous Node block, but nothing in the forwarding procedure instructs any agent to add a Previous Node block.

Correctness: If two nodes both opt to return failed bundles, how are they to avoid a ping-pong loop?

[[Alexey]]: I included these in the DISCUSS portion of my ballot.

##Section 5.6

Error handling: What about CBOR parsing failures?

[[Alexey]]: I included these in the DISCUSS portion of my ballot.

## Section 5.8

Clarity: The insistence, here and earlier, that a bundle is only fragmented into two packets, and that this can then be performed recursively, seems unhelpful.  Bundles can be fragmented into arbitrarily many pieces, and this binary subdivision concept is not actually present in the protocol.  I suggest removing it from the text as well.

## Section 6.1

Formatting: This table is excessive for a single value.

Clarity: The text specifies the contents twice, which is unhelpful for comprehension.

## Section 10

Formatting: The extra line breaks in the vertical dividers are distracting.  Please fill them in or remove the breaks if possible.

I support Benjamin's first DISCUSS point.

The registration templates in 10.7 and 10.8 should include iesg@ietf.org as the contact information for the change controller.

I support the Sec ADs’ DISCUSS and comment points.  I have only a small thing to add to what’s already been said:

— Section 1 —
You mention RFC5050 several times here, but it isn’t actually cited as a reference until Section 10.1.  You would normally put it in as a citation the first time, at the beginning of Section 1, and I think you should.

[updating my position for the -29, but no further actions are expected
after my comments on the -27]

Section 10.7

I assume that as part of the IANA processing, we will have this document 
added as a reference for the updated "dtn" registration.  (Whether or
not that change is specifically mentioned in the final RFC is probably
not of great import.)

Sec 4.1.5.1.2 I though this section was all about endpoint IDs. So what are the "all other purposes" that involve ASCII representations?

Sec 7. Please add "congestion control" to the list of services the CL provides.

Sec 10. There are many instances in these registries of a codepoint only applying to Version 6, but including 'RFC-to-be' as a reference. Is this a mistake, or am I missing something?

Hi,

Thank you for this document.

Just a few minor observations/comments.  Quite possibly only the last comment is actionable, and the rest are just observations.

1) Figure 2: Components of a Bundle Node

It would be nice if this diagram is not split across page boundaries, hopefully the RFC editor can sort this out.

2) 4. Bundle Format

   Each bundle SHALL be a concatenated sequence of at least two blocks,
   represented as a CBOR indefinite-length array
   
It wasn't immediate obvious why this restriction was here, I presume that this is to ensure that there is a canonical representation?

4.1.1. CRC Type
I was surprised to see that CRC-16 is also supported.  I presume that reasoning for this is because blocks could be small, and hence the overhead from always using CRC32 was regarded as being too much?
 
4.1.2. CRC
I was also slightly surprised that these are encoded as fixed length byte strings rather than as unsigned integers.  I presume that this is done to make them fixed length and also easier to zero out when performing the CRC calculation?

It looks like RFC 2119 language used to define the bundle format both in section 4 and 4.2.1.  Possibly it would be better to not use RFC 2119 language in the intro of section 4, and instead refer to 4.2.1 for the normative definition?

Regards,
Rob