The Babel Routing Protocol
draft-ietf-babel-rfc6126bis-20

[ general ]

* I couldn't find where any of the multi-byte numeric fields have their
  endianness specified.  Consider a blanket "everything's network byte
  order" somewhere appropriate, or maybe spell it out explicitly in:

    * section 4.2:    Body length
    * section 4.6.3:  Interval
    * section 4.6.5:  Seqno
    * section 4.6.5:  Interval
    * section 4.6.6:  Rxcost
    * section 4.6.6:  Interval
    * section 4.6.9:  Interval
    * section 4.6.9:  Seqno
    * section 4.6.9:  Metric
    * section 4.6.11: Seqno

[ section 3.6 ]

* s/this kind of instabilities/these kind of instabilities/, I think

[ section 3.8.1.2 ]

* Perhaps s/it has finite metric/it has a finite metric/

[ section 4.4 ]

* Can a sub-TLV have sub-TLVs of its own?

[ section 4.5 ]

* s/derived derived/derived/

[ Appendix A.3 ]

* s/results ;/results;/ maybe

My review is focusing mainly on the diff with RFC6126 for expediency.  The technical material seems fine to me, so I just have some editorial nits to offer:

Section 1.2:

* "The ability to build such heterogeneous networks makes Babel particularly adapted to the unmanaged and wireless environment." -- The ending feels awkward.  Maybe "to unmanaged, wireless environments"?  Or maybe "or" in between?

Section 3:

* "... assigns host IDs, see the definition ..." -- comma splice; "see" should begin a new sentence, or the comma should be a semi-colon

Section 3.1:

* "... in a previous TLV, (such as in ..." -- remove the comma

Section 3.2.1:

* "Given a seqno s and a non-negative integer n, the sum ..." -- It feels to me as if things like "s" and "n" should be quoted when in prose.  I don't know what the RFC Editor prefers, but I keep thinking this as I read through the whole document; I hit it again in 3.5.2, for example.

Section 4.6.5:

* "neighbor" is spelled the American way here, but the British way everywhere else.  Come to think of it, I think the RFC Editor standardizes on the former (see RFC 7322) if you're not internally consistent.  So, if you like your "u"s, don't give them this ammunition to change them.

Section 5:

* I suggest replacing with "IANA has {registered, created} ..." with "IANA previously {registered, created} ..." to make it clear that the prior RFCs did that work, and that they're simply being restated here and/or that this document is assuming authority over them.

* The new registries feel like they're under-specified to me.  It's not clear, for example, whether I could register bit 16 in the "Babel Hello Flags Values" registry.  Maybe add a sentence making it explicit that registrations can only be made from the "Unassigned" range?   Admittedly, this could be experience bias on my part as I typically work with registries that cover unconstrained namespaces, so I'm leaving this only as a comment.

Appendix G:

* Although it's possibly obvious and perhaps unnecessary, I suggest making it explicit that this appendix is not intended to be published and can be removed by the editor.

Thank you for addressing my DISCUSS and COMMENT points, and further explaining the protocol where necessary.

Dear authors,

Thank you for the work put into this document and its companion documents. The text is usually clear and explanations are concise and easy to understand. I have nevertheless some COMMENTs and a few NITs.

Regards,

-éric

== COMMENTS ==


-- Section 1.1 --

In the properties bullet points, "its diameter" is it about the loop diameter or the network diameter? I suspect the latter but this is ambiguous IMHO.

-- Section 3 --

It is unclear by reading "The protocol encoding is slightly more compact when router-ids are assigned in the same manner as the IPv6 layer assigns host IDs." whether EUI-64 is referred here. I also fail to see in section 4.6.7 (router-id TLV) what is the encoding benefit?

-- Section 3.1 --

Should there be a mention of maximum UDP datagram size? and some words on layer-3 fragmentation ? I understand that section 4 has a section on this, so, perhaps refer already to that section for completeness ?

-- Sections 3.2.3 and 3.2.4 --

Does a dual-stack host have to send 2 hello? One on each protocol stack (v6 or v4) ? Unclear from the explanation.

-- Section 3.4.2 --

It is unclear to me whether a link to a neighbor (router-id) can have different cost based on the v6 or v4.

-- Section 4 --

Is there a reason why the well-known ports and multicast group addresses are not spelled out in this section ? They only appear in the IANA considerations section.

Also, should the hello be sent over v6 _AND_ v4 ?

-- Section 4.2 --

No a real comment, just an appreciation of your humor: "The arbitrary but carefully chosen value 42" ;-) you made my Monday morning !

-- Section 4.4 --

Is there any reason why the 'mandatory bit' does not appear in the packet structure?

-- Section 4.6.2 --

Is there any reason why the "MBZ" is not expanded ? Must Be Zero ?

-- Section 4.6.3 --

I wonder how the receiver could estimate the propagation time (in each direction BTW) + queuing time + whatever delay...

-- Section 4.6.7 --

Should the router-id field length be repeated here as well ?

== NITS ==


-- Section 1.1 --

Rather than using 'naive' to describe RIP, let's rather use 'trivial' or 'simple' ;-)

s/the routers involved participate/the involved routers participate/  ?

-- Section 3.2.6 -- 

Suggest to use '0xffff' rather than 'FFFF' and be consistent in the use of lowercase / uppercase for hexadecimal numbers.

-- Section 4.5 --

Parsing "since for correct parsing it must be identical across implementations" is not easy... a comma would be welcome.

-- Section 4.6.3 --

s/receiver send/receiver sends/

Thank you for the patience and the work put into addressing my DISCUSS.

I support the DISCUSS points by Ben and Álvaro.

Thank you for addressing my Discuss points!
I am happy to see the expanded text on update representation and parser
state in Section 4.5 -- that's enough to clear my discuss point, though
I probably would have added even a bit more text if I was writing it
myself.

Section 4.5

   In addition to the above, an Update TLV can omit a prefix of the
   prefix being announced, which is then extracted from the preceding
   Update TLV in the same address family (IPv4 or IPv6).  Finally, as a

nit: from a rhetorical sense, I'd suggest "omit an initial portion of
the prefix", to avoid using the word "prefix" with two different
meanings in the same sentence.

Appendix B

      Link cost: estimated using ETX on wireless links; 2-out-of-3 with
      C=96 on wired links.

Perhaps "ETX as described in Appendix A.2.2".

Appendix C

   At a minimum, they discard routes with a destination prefix in
   fe80::/64, ff00::/8, 127.0.0.1/32, 0.0.0.0/32 and 224.0.0.0/8.

127.0.0.1/32 as opposed to /8?

Appendix F

   There are two optioal features that make the new protocol
   incompatible with its predecessor.  First of all, RFC 6126 did not

nit: "optional"

   Two changes need to be made to an implementation of RFCs 6126 and
   7557 so that it can safely interoperate in all cases with
   implementations of this protocol.  First, it needs to be modified to
   either ignore or process Unicast Hellos.  Second, it needs to be
   modified to parse sub-TLVs of all the TLVs that it understands and
   that allow sub-TLVs, and to ignore the TLV is an unknown mandatory
   sub-TLV is found.  It is not necessary to parse unknown TLVs, since

nit: "if an unknown"

   There are other changes, but these are not of a nature to prevent
   interoperability:
   [...]
   o  the compression state is now specific to an address family rather
      than an address encoding Section 4.5;

It seems like an old implementation that decompresses an update to
different contents than a new implementation does, would have some
effect on routing.  Am I missing something?

One nit:

3.7.2 In the last paragraph, selection of a next hop is listed as both a "MAY" and a "SHOULD NOT". I suppose that this not logically inconsistent, but the context certainly suggests that's both significant and minor, which doesn't make sense.

Thanks for all the work to address my discuss points. I still think that it is important for PS spec to normatively define default values as part of the main spec, also because it is rather uncommon to use normative language in the appendix. However, there are references to the appendix in all relevant places in the spec now and I'm okay to clear my discuss on this basis.

Please note that the following comment from my discuss ballot is not fully addressed:
"In section 4.1.1 the update interval needs a lower limit (e.g. 3 seconds) and a recommend default value would be could as well (Note that there are other part in section 3 where the update value is discussed as well)."
We had a long discuss about this value specifically and the recommended default in the appendix is fine with me. However, section 4.1.1 does not have a pointer to the appendix. Further I think it would be appropriate to add a warning here that low values mean higher network load which can have a negative impact in certain networks.

-------------------------------
Old comments (here for the record; didn't check these points):

1) While this point might not raise discuss-level, it would probably also be good to provide more concrete advise on how to implement jitter:
Sec 3.1.: “   A moderate amount of jitter may be applied to packets sent by a Babel
   speaker: outgoing TLVs are buffered and SHOULD be sent with a small
   random delay.”
Sec 4: “a Babel node SHOULD
   buffer every TLV and delay sending a packet by a small, randomly
   chosen delay [JITTER].”

2) Sec 4.1.2. (Router-Id) should probably state again that the router-id is assumed to be unique within a domain.

3) Sec 4: “The most-significant bit of the sub-TLV, called the mandatory bit,
   indicates how to handle unknown sub-TLVs.”
I would recommend to also indicate this bit in the image.

4) Sec 4.4:
“If a TLV has a self-terminating format, then it MAY
   allow a sequence of sub-TLVs to follow the body.”
Initially I wasn’t quite sure what you wanted to say here. I guess you say that the length would indicate a larger value that needed for the body and therefore a subTLV might be present? I recommend to clarify this here a bit.

5) I recommend to move Appendix C (Considerations for protocol extensions) in the body of the document.

Thanks for including explanatory text for describing expectations and limitations of Backward Compatibility in Appendix F, in order to address my DISCUSS point regarding issues with backward compatibility with RFC6126 implementations.