[dtn-interest] IRSG review of draft-irtf-dtnrg-tcp-clayer-06.txt

[Dirk Kutscher <[e-mail]>]

Hello,

I did a review of draft-irtf-dtnrg-tcp-clayer-06 -- apologies to the authors that it took me that long.

In summary, I don't see major technical issues. I have provided some technical and editorial comments -- I hope it's useful.

Best regards,
Dirk

------------------------------------------------------------------------------------------------------------------
GENERAL ASSESSMENT
------------------

There are no major technical issues with this document. It can be
improved in a few places, mostly for clarity.


TECHNICAL COMMENTS
------------------


- section 3:
  <comment>
   The protocol overview needs to say that a TCPCL can be used to
   transmit several bundles, but that multiple bundles MUST be
   transmitted consecutively (i.e., no inter-leaving of bundle
   segments). In addition, I think, you also have to say that the
   blocks of a particular bundle MUST be sent sequentially (no change
   of block order).
  </comment>

- section 3:
  <oldtext>
   This protocol provides bundle conveyance over a TCP connection and
   specifies the encapsulation of bundles as well as procedures for TCP
   connection setup and teardown.
   </oldtext>

   <comment>
    "The service of this protocol is the transmission of DTN bundles
     over TCP. This document specifies the encapsulation of bundles,
     procedures for TCP setup and teardown, and a set of messages and
     node requirements."?
   </comment>

- section 3.1:

  <comment>
   The first paragraph is not very clear. You want to say that there
   are different specific messages for sending and receiving
   operations (in addition to connection setup/teardown).  TCPCL is
   symmetric, i.e., both sides can start sending data segments in a
   connection, and one side's bundle transfer does not have to
   complete before the other side can start sending data segments on
   its own. Hence, the protocol allows for a bi-directional mode of
   communication.
  </comment>


- section 3.2:

  <oldtext> 
   Note that the sending node may transmit multiple
   DATA_SEGMENT messages without necessarily waiting for the
   corresponding ACK_SEGMENT responses.  This enables pipelining of
   messages on a channel.
  </oldtext> 

  <comment>
   This is a too important property to just mention it on the side of
   an example description. You should mention this in the protocol
   overview. You should also mention that there is no explicit flow
   control on the TCPCL layer.
  </comment>
    
- section 3.2:

  <oldtext> 
    However, interleaving data segments from different bundles is not
    allowed.
  </oldtext> 

  <comment>
   Again, this is too important to not mention it earlier.
  </comment>


-section 4:

 <oldtext>
   The manner in which a bundle node makes the decision to establish
   such a connection is implementation-dependent.
 </oldtext>

 <comment> 
  I think you want to say that the implementation can decide, possibly
  case-by-case, at runtime (unless it is fixed). Perhaps better so
  say: "It is up to the implementation to decide how and when
  connection setup is triggered. For example..."
 </comment>


-section 4: 
 <oldtext>
   Therefore, the node MUST retry
   the connection setup only after some delay and it SHOULD use a
   (binary) exponential backoff mechanism to increase this delay in case
   of repeated failures.
 </oldtext>

 <comment>
  You should specify what is meant by "some delay". Is it completely
  up to the implementation? Do you want to RECOMMEND a minimum delay?
 </comment>



- section 4.1: 

 <oldtext>
   magic:  A four byte field that always contains the byte sequence 0x64
        0x74 0x6e 0x21, i.e., the text string "dtn!".
 </oldtext>


 <newtext>
   magic:  A four byte field that always contains the byte sequence 0x64
        0x74 0x6e 0x21, i.e., the text string "dtn!" in US-ASCII.
 </newtext>


- section 4.1: 

 <oldtext>
   version:  A one byte field value containing the current version of
        the protocol.
 </oldtext>


 <newtext>
   version:  A one byte field value containing the value 3 (current version of
        the protocol).
 </newtext>


- section 5.2

 <oldtext>
  However, a node MUST be able to receive a bundle that has been
   transmitted in any segment size.
 </oldtext>

 <comment>

  How feasible is this, for example for constrained devices? Even if a
  node does not intend to store and reassemble bundles, there may be
  certain limits for the maximum size of segments that it can
  receive. Has there been a discussion to make this
  negotiable/configurable? How should a node react if it cannot
  receive/process any more bytes for a transmitted segment?
 </comment>


- section 5.2

 <oldtext>
   Following the message header, the length field is an SDNV containing
   the number of bytes of bundle data that are transmitted in this
   segment.  Following this length is the actual data contents.
 </oldtext>


 <comment>
  This should be mentioned earlier in section 5.2
 </comment>



- section 5.3 

  <oldtext>
   To transmit an acknowledgment, a node first transmits a message
   header with the ACK_SEGMENT type code and all flags set to zero, then
   transmits an SDNV containing the cumulative length of the received
   segment(s) of the current bundle.
  </oldtext>


  <newtext>
   To transmit an acknowledgment, a node first transmits a message
   header with the ACK_SEGMENT type code and all flags set to zero,
   then transmits an SDNV containing the cumulative length in bytes of
   the received segment(s) of the current bundle.
  </newtext>


- section 5.4

  <oldtext>
   Note: If a bundle transmission if aborted in this way, the receiver
   may not receive a segment with the 'E' flag set to '1' for the
   aborted bundle.  The beginning of the next bundle is identified by
   the 'S' bit set to '1', indicating the start of a new bundle.
  </oldtext>
  
  <comment>

   Should this be turned into a general rule? I.e.: "if a receiver
   receives a segment for a new bundle without having seen the final
   segment of a previous bundle, it SHOULD disregard all segments of
   the incompletely received bundle."

   (wording may need some work)

   It's probably good to use "SHOULD" here, because the node may
   already have forwarded the segments...
  </comment>


- section 6.1

  <oldtext>
   The format of the shutdown message is as follows:

                        1 1 1 1 1 1 1 1 1 1 2 2 2 2 2 2 2 2 2 2 3 3
    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
   |  0x5  |0|0|R|D| reason (opt)  | reconnection delay (opt)      |
   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

               Figure 8: Format of bundle shutdown messages

  </oldtext>

  <comment>
   Is it really efficient to have variable length messages here by
   making the reason code optional? How about always requiring it and
   then allow for a default value (0x00)?
  </comment>


- section 6.1

  <oldtext>
   If either node terminates a connection prematurely in this manner, it
   SHOULD send a SHUTDOWN message and MUST indicate a reason code unless
   the incoming connection did not include the magic string.
  </oldtext>

  <comment>
   Why not send a reason like "illegal syntax"?
  </comment>


- section 6.7

  <comment>
  
  Regarding the peer entity authentication issue, I think you have to
  use a stronger statement, like "a node SHALL NO" use the endpoint
  identifier conveyed in the TCPCL connection message to derive a peer
  node's entity unless it can ascertain it via other means."

  If it is possible to do that through the Bundle Security Protocol,
  you could be a bit more specific on how the Bundle Authentication
  Block would be used. I assume one TCPCL peer node would become a
  security source in an RFC 6257 sense?

  BTW, you are using the term "Bundle Authentication Header" -- I
  think it's "Bundle Authentication Block".

  In general, it could be worthwhile stating that TCPCL does not
  provide any security services and that RFC 6257's mechanisms should
  be used.

  BTW, is the use of TLS for TCPCL defined or excluded? You may want
  to say something about why it's not applicable
  </comment>


- section 10.2

  <oldtext>
   [RFC6256]  Eddy, W. and E. Davies, "Using Self-Delimiting Numeric
              Values in Protocols", RFC 6256, May 2011.
  </oldtext>

  <comment>
    Isn't that a normative reference for this spec?
  </comment>



EDITORIAL COMMENTS
------------------

General:

- make usage and capitalization of Bundle Protocol (sometimes referred
  to as "Bundling Protocol", "bundle protocol" etc.) consistent

- section 2.1
   <oldtext>
     Bundle payload --  A bundle payload (or simply "payload") is the
        application data whose conveyance to the bundle's destination is
        the purpose for the transmission of a given bundle.
   </oldtext>

   <comment>
     IMO this can be said in a clearer way: "... application
     data that is transmitted to a BP application at a Bundle
     Endpoint"?
   </comment>



- section 2.1:
   <oldtext>
   Bundle node --  A bundle node (or simply a "node") is any entity that
        can send and/or receive bundles.  The particular instantiation
        of this entity is deliberately unconstrained, allowing for
        implementations in software libraries, long-running processes,
        or even hardware.  One component of the bundle node is the
        implementation of a convergence layer adapter.
   </oldtext>

   <comment>
     I don't think the second sentence ("The particular
     instantiation...") is really needed.  This holds for most network
     elements these days.
   </comment>


- section 2.1:
   <oldtext>
   Convergence layer adapter --  A convergence layer adapter (CLA) sends
        and receives bundles utilizing the services of some 'native'
        link, network, or internet protocol.  This document describes
        the manner in which a CLA sends and receives bundles when using
        the TCP protocol for inter-node communication.
   </oldtext>

   <comment>
      Does it make make sense to define "Convergence Layer" first,
      before defining CLA? Is the CLA definition needed at all
      (assuming CL is defined)?
   </comment>

- section 2.2:
  <oldtext>
   TCPCL Connection --  [...] The lifetime of a TCPCL connection is one-to-one
        with the lifetime of an underlying TCP connection.
	[...]
   </oldtext>

   <comment>
      "The lifetime of a TCPCL connection is bound to the
      lifetime of the underlying TCP connection."?
   </comment>




-section 4:

 <oldtext>
   Once a TCP connection is established, each node MUST immediately
   transmit a contact header over the TCP connection.  The format of the
   contact header is described in Section 4.1).
 </oldtext>

 <newtext>
   Once a TCP connection is established, each node MUST immediately
   transmit a contact header over the TCP connection.  The format of the
   contact header is described in Section 4.1.
 </newtext>




- section 4.1:

 <oldtext>
   local EID:  An octet string containing the EID of some singleton
        endpoint in which the sending node is a member, in the canonical
        format of <scheme name>:<scheme-specific part>.  A eight byte
        EID is shown the clarity of the figure.
 </oldtext>


 <newtext>
   local EID:  An octet string containing the EID of some singleton
        endpoint in which the sending node is a member, in the canonical
        format of <scheme name>:<scheme-specific part>.  A eight byte
        EID is shown for clarity of the figure.
 </newtext>


- section 4.2:


 <oldtext>
        The reactive fragmentation enabled parameter is set to true iff
        the corresponding flag is set in both contact headers.

        The bundle refusal capability may only be used iff both peers
        indicate support for it in their contact header and segment
        acknowledgement has been enabled.
 </oldtext>


 <newtext>
        The reactive fragmentation enabled parameter is set to true iff
        the corresponding flag is set in both contact headers.

        The bundle refusal capability is set to true if the
        corresponding flag is set in both contact headers and if
        segment acknowledgment has been enabled.
 </newtext>

 <comment>
  I'd raher use the same or similar wording to not irritate the implementer.
 </comment>


- section 5.1

 <oldtext>
  Table 2: TCPCL Header Types
 </oldtext>


 <newtext>
  Table 2: TCPCL Message Types
 </newtext>


- section 5

  <comment>
   For the different subsections for the individual messages, please
   use the message type names from table 2 in the headings, for
   example: "Bundle Data Transmission (DATA_SEGMENT)". Also, I'd
   recommend to align the order of the subsections with the order of
   message types in table 2.
  </comment>

  <comment>
   I would also recommend to use a consistent structure for each of
   the subsections, i.e., perhaps
     - purpose
     - syntax
     - procedures / node requirements
  </comment>




- section 5.3

  <oldtext>
   If so, then
   the receiver MUST transmit a bundle acknowledgment header when it
   successfully receives each data segment.
  </oldtext>


  <oldtext>
   If so, then
   the receiver MUST transmit a bundle acknowledgment message when it
   successfully receives each data segment.
  </oldtext>



- section 5.6

  <oldtext>
   If no message (keepalive or other) has been received for at least
   twice the keepalive interval, then either party may terminate the
   session by transmitting a one byte message type code of SHUTDOWN (as
   described in Table 2) and closing the TCP connection.
  </oldtext>


  <newtext>
   If no message (keepalive or other) has been received for at least
   twice the keepalive interval, then either party MAY terminate the
   session by transmitting a one byte SHUTDOWN message  (as
   described in Table 2) and by closing the TCP connection.
  </newtext>



- section 6.1

  <oldtext>
   In case acknowledgments have been
   negotiated, it is advisable to acknowledge all received data segments
   first and then shut down the connection.
  </oldtext>


  <newtext>
   In case acknowledgments have been
   negotiated, a node SHOULD  acknowledge all received data segments
   first and then shut down the connection.
  </newtext>





- section 6.1

  <oldtext>
   This may, for example, be used to notify
   that the node is currently not capable of or willing to communicate.
  </oldtext>

  <newtext>
   This may, for example, be used to notify
   that the node is currently not able or willing to communicate.
  </newtext>




- section 10.2

  <oldtext>
   [refs.dtnsecurity]
              Symington, S., Farrell, S., and H. Weiss, "Bundle Security
              Protocol Specification", Internet Draft, work in progress
              draft-irtf-dtnrg-bundle-security-03.txt, April 2007.
  </oldtext>

  <comment>
     This is RFC 6257.
  </comment>