Re: [Ntp] Comments on Miroslav's NTP v5 proposal.

[Miroslav Lichvar <[e-mail]>]

On Fri, Sep 16, 2022 at 09:51:11AM +0200, Ulrich Windl wrote:
> A pull request with comments on the draft will follow shortly on github, but for the git(hub)-agnostic, I'm attaching a plain old unified diff (ignoring the white space changes that the editor applied automatically in some cases).

Thanks for the comments and suggestions. I'm glad someone looked at it
in such detail. Most of your suggestions make sense to me and I'll try
to address them in the next version of the draft.

Here are my comments and questions for the rest. 

> @@ -74,6 +77,9 @@
>              and server modes remain in NTPv5.</t>
>  
>              <t>Timestamps are clearly separated from values used as cookies.</t>
> +	    <!--UW: In general it would be advisable to define phrases before
> +		using them; "cookies", "MAC field" and "timescales" for
> +		example. -->

This is just a part of the introduction comparing NTPv5 with NTPv4. Do
we need to go in such detail and define these basic terms?

>          <list style="hanging">
>            <t hangText="time16"><vspace/>
> -            A 16-bit fixed-point type containing values in seconds. It has 1
> -            signed integer bit (i.e. it is just the sign) and 15 fractional
> -            bits. The minimum value is the fraction -32767/32768 (almost -1
> +            A 16-bit signed fixed-point type containing values in seconds. It has 1
> +            sign bit (i.e. it is just the sign) and 15 bits describing the fractional
> +            part. The minimum value is the fraction -32767/32768 (almost -1

I think we might want to drop this type completely due to the
possibility that UTC will be decoupled from UT1, so it couldn't
represent larger offsets. It might work better as an extension field.

> +      <!--UW: I would add more types like s_log8 (signed number expressed as a
> +          power of two using 8 bits, maybe including an undefined value),
> +          u_log8 (unsigned number expressed as a power of two using 8 bits,
> +          s_i16 (signed interger using 16 bits) -->

Where would be u_log8 used and do we need to define a type for each
width of a plain integer? There are also 2-bit and 3-bit fields.

> @@ -191,6 +210,9 @@
>  
>        <figure align="center" anchor="message-format"
>                title="Format of NTPv5 messages">
> +	<!--UW: I'd assign a short label for the fields with a long name, used
> +	     to reference them uniquely in the main text (like "CC" for
> +	     "Client Cookie") -->

I suspect that could be confusing. I prefer the longer capitalized
forms.

> @@ -306,21 +334,32 @@
>              timestamp. In requests it is always 0.</t>
>  
>              <t hangText="Timescale Offset"><vspace/>
> +	    <!--UW: This could be the s_i16 type -->
>              A 16-bit value specific to the selected timescale, which is
>              referenced to the receive timestamp. In requests it is always
>              0.
> +	    <!--UW: A high-level description is missing -->

What would you describe here?

>              <list style="symbols">
>                <t>In the UTC (0) and TAI (1) timescales it is the TAI-UTC offset
>                  (TAI minus UTC) as a signed integer, or 0x8000 if unknown.</t>
>                <t>In the UT1 timescale (2) it is the UT1-UTC offset (UT1 minus
>                UTC) using the time16 type (0x8000 if unknown).</t>
> +	      <!--UW: Shouldn't it be something like UT1-TAI instead (or
> +	          TAI-UT1?)? -->

That's how DUT1 is defined and I'd say UTC is the most important
timescale so everything else should be against it.

> +	      <!--UW: I also think it's bad design to use two different data
> +		  types in one field -->

Would it help if you considered it as two different fields sharing the
same position?

> @@ -428,6 +477,7 @@
>          <t>As each client can be synchronized to an unlimited number of
>            servers (and there can be up to 15 strata of servers), the reference
>            IDs are exchanged as a Bloom filter instead of a list to limit the
> +	  <!--UW: Add a reference for "Bloom Filter" -->
>            amount of data that needs to be exchanged.</t>

Any examples of a reference I could use?

>            synchronization loop with one of its servers, it SHOULD stop using
> -          the server for synchronization. When the client's reference ID is
> +	  <!--UW: Formally define "synchronization loop"! -->

FWIW, RFCs for previous NTP versions didn't define it.

> @@ -461,12 +517,12 @@
>  
>          <t>False positives are possible. The probability of a collision grows
>            with the number of reference IDs in the filter. With 26 reference IDs
> -          it is about 1e-12. With 118 IDs it is about 1e-6. The client MAY avoid
> +          it is about 1e-12.<!-- please explain giving a formula! --> With 118 IDs it is about 1e-6. The client MAY avoid

That formula isn't very nice and I don't think it's important to know.
Curious readers can find it in the reference which will be provided :).

> @@ -614,15 +673,21 @@
>                 Transmit with all zeros.</t>
>  
>                 <t hangText="Delay Correction"><vspace/>
> +	       <!--UW: Is there a specific reason to add yet another datatype
> +		   instead of using "timestamp64"? -->

Better resolution and compatibility with existing hardware.

>                 A signed fixed-point number of nanoseconds with 48 integer
>                 bits and 16 binary fractional bits, which represents the current
>                 correction of the network delay that has accumulated for this
>                 packet on the path from the source to the destination. The
>                 format of this field is identical to the PTP correctionField.</t>
> +	       <!--UW: Is it necessary to use the PTP format here? The
> +		   hardware owuld have to be adapted for NTP anyway -->

The conversion to NTP format is expensive and would make it more
difficult to implement the support in the hardware.

> @@ -633,12 +698,12 @@
>            </list>
>          </t>
>  
> -        <t>A correction capable client SHALL transmit the request with the
> +        <t>A correction capable client system SHALL transmit the request with the

Can you please explain the difference here?

> @@ -747,6 +813,8 @@
>  
>      <section title="Measurement Modes" anchor="measurement-modes">
>  
> +      <!--UW: Shouldn't you give at least a sketch of the NTP algorithms here?
> +      -->

The algorithms from RFC 5905? I think the idea for this document was to
avoid describing those.

> @@ -787,10 +858,15 @@
>          interleaved mode enables the server to provide the client with a more
>          accurate transmit timestamp which is available only after the
>        response was formed or sent.</t>
> +      <!--UW: Explain interleave mode in greater detail; leaving out the
> +	  algorithms and just claiming the "correct" values should be in the
> +	  packet is a strange concept. -->

I don't follow. Please elaborate.

> @@ -833,6 +911,7 @@ Tx  | 0  |    | t3 |      | 0  |    | t7 |      | 0  |    |t11 |
>          The messages in the basic and interleaved mode are indicated with B and
>          I respectively. The timestamps t3' and t11' correspond to the same
>          transmissions as t3 and t11, but they may be less accurate. The first
> +	<!--UW: It's not obvious what the reason for t3' and T11' is -->

Do you mean something else that nothing better being available?

> @@ -884,6 +963,7 @@ Tx  | 0  |    | t3'|      | 0  |    | t3 |      | 0  |    |t11'|
>          stability of the clock. When polling a public server on Internet, the
>          client SHOULD use at least a polling interval of 64 seconds, increasing
>        up to at least 1024 seconds.</t>
> +      <!--UW: Why exactly those numbers; explain! -->

It's just a recommendation matching the current defaults of the known
clients. No math behind it.

> @@ -893,6 +973,11 @@ Tx  | 0  |    | t3'|      | 0  |    | t3 |      | 0  |    |t11'|
>  
>          <list style="numbers">
>            <t>Generates a new random cookie.</t>
> +	  <!--UW: Is it a "new" coockie in the sense of NONCE, or is it a
> +	      random cookie (which is also new in most cases); if it has to be
> +	      random AND unique, say so!-->

Ideally we want it to be a nonce, but that is difficult to do in the
64-bit range of values, so I think cookie is a better description.

> +	  <!--UW: Also: Shouldn't the list start with the second item, and
> +	      talking about the cookie when it it needed? -->

I don't see the advantage. It can work either way.

> @@ -973,6 +1062,8 @@ Tx  | 0  |    | t3'|      | 0  |    | t3 |      | 0  |    |t11'|
>              dispersion. Otherwise, it MUST be 0, 1, 2, depending on whether a
>              leap second is pending in the next 14 day and, if it is, whether it
>            will be inserted or deleted.</t>
> +	  <!--UW: Should there be an extension field describing the time (at
> +	      least) of the leap event; maybe more details could be added. -->

The time of the event is the last day of the month. What other
information do you think would be useful?

>  
> -              <t>Era is set to the NTP era of the receive timestamp.</t>
> +              <t>Era is set to the NTP era of the receive timestamp if it was valid.</t>

Do you mean the case when the server is not synchronized and responding
with leap=3? The receive timestamp is not expected to be zero.

> @@ -1039,6 +1130,9 @@ Tx  | 0  |    | t3'|      | 0  |    | t3 |      | 0  |    |t11'|
>                  identifies a more accurate transmit timestamp of the response,
>                  which can be retrieved by the client later with another
>                request. The cookie generation is implementation-specific.</t>
> +	      <!--UW: I'd expect "random" or "NONCE", but is this for
> +		  compatibility where it would just be a timestamp? This
> +		  should be discussed IMHO -->

In the NTPv4 interleaved mode it's the (unique in short term) receive
timestamp. In NTPv5 it could be anything that allows the server to
identify the previous request. It could be a simple counter, the same
timestamp as in NTPv4, or something encrypted if there was a reason to
make it more difficult for off-path attackers to access to the stored
timestamps. Unlike in NTPv4 servers don't have to drop it after first
use, so I'm not sure how important that would be.

-- 
Miroslav Lichvar