NTP offset definition in book and source code is incorrect

[GabrielBianconi]

In https://github.com/rust-in-action/code/blob/1st-edition/ch9/ch9-clock3/src/main.rs#L40-L50:

impl NTPResult {
  fn offset(&self) -> i64 {
    let delta = self.delay();
    delta.abs() / 2
  }

  fn delay(&self) -> i64 {
    let duration = (self.t4 - self.t1) - (self.t3 - self.t2);
    duration.num_milliseconds()
  }
}

The definition of offset is incorrect (source on Wikipedia). Offset can be positive or negative, whereas delay is always positive. You can see that the equations above always return positive values.

Offset should let delta = (self.t2 - self.t1) - (self.t3 - self.t4);. Currently, it rearranges to let delta = (self.t2 - self.t1) - (self.t4 - self.t3);, i.e. the last two terms are swapped.

(I see there was an attempt to correct in #51, but as the OP later noted, the PR did not fix the issue.)

[ewenmcneill]

I've also run into this issue with the offset calculation being wrong. The code (now) derives the offset from the delay value, but the actual NTP algorithm has entirely different calculations for offset and delay.

It is trivially obvious that the provided code does not work, if one sets the time out by more than a few minutes, and then attempts to use ch9-clock3 to re-synchronise to the correct time. Even if the real time clock is off by minutes, the adjustment made ends up being under 1 second (as described below, I believe the adjustment is clamped to 40ms, for unexplained reasons).

It appears the attempt to "fix #51", with the commit 39e24d7, ended up switching from a somewhat incorrect definition of offset, to the blantantly incorrect definition of offset being "half absolute value of delay" :-/ (As also noted in #51 (comment), after that commit, but it appears there was no followup.)

As noted in this issue (#86) Table 9.2 in the book (page 317 in the eBook PDF) does show two different formulas for the offset and delay, which appear to be almost correct, taking into account the different variable naming (but as noted by the original poster in this issue, the final two offset terms are swapped in Table 9.2).

Unfortunately there's also a mistake in the original post which gives the formula as:

let delta =  (self.t2 - self.t1) - (self.t3 - self.t4);

but actually the delta (offset) needs to be:

let delta = (self.t2 - self.t1) + (self.t3 - self.t4) / 2

Note the "+" in the middle, not the "-": the offset is supposed to be the average of the first server-to-us offset and the second server-to-us offset.

However even with that fixed, the time does not get corrected much, as best I can tell because the correction calculation code assumes that we are milliseconds off:

code/ch9/ch9-clock3/src/main.rs

Lines 339 to 342 in 6ebd519

	let offset = check_time().unwrap() as isize;
	let adjust_ms_ = offset.signum() * offset.abs().min(200) / 5;
	let adjust_ms = ChronoDuration::milliseconds(adjust_ms_ as i64);

and seems to be clamping the offset milliseconds to 200ms (via isize.min()), which it then divides by 5, giving 40ms as the maximum adjustment it is willing to make. So the time had better be really close even before we start, or the adjustment won't help much :-/

Directly using the offset as milliseconds, gets very close to "step to current NTP time":

    let adjust_ms = ChronoDuration::milliseconds(offset as i64);

so it's unclear what the signum() / abs() / divide by 5 dance was intended to do as it is never explained.

Which just leaves the broken OS error handling that assumes any error at all must have been caused by setting the time, resulting in:

Unable to set the time: Os { code: 101, kind: NetworkUnreachable, message: "Network is unreachable" }

because it fails to handle the fact that libc doesn't clear errno between calls (ie, it's never explicitly set to zero); instead the C convention is that you only check libc when you're told that a call failed, and then it should have the latest failure. (But it will have the "latest failure" up until it gets overwritten by something else, which may or may not be a relevant one to the latest call, particularly if the latest call succeeded.) I decided against even trying to fix that misunderstanding.

Also the entire example doesn't compile because the <1>, etc, markers are not commented out, so it's clear it was never tested in released form.

Ewen

Example of actually setting the time by the right offset

ewen@rustdev:~/misc/src/rust/rust-in-action/ch9/ch9-clock3$ date; sudo target/debug/clock check-ntp; date
Wed 11 Jan 2023 14:19:28 NZDT
time.nist.gov => 7151831ms away from local system time
time.apple.com => 7151818ms away from local system time
time.euro.apple.com => 7151817ms away from local system time
time.google.com => 7151818ms away from local system time
time2.google.com => 7151818ms away from local system time
Offset to NTP is: 7151817
Offset signum is: 1
Offset abs is: 7151817
Offset abs min is: 200
Adjusting by Duration { secs: 7151, nanos: 817000000 } ms
Unable to set the time: Os { code: 101, kind: NetworkUnreachable, message: "Network is unreachable" }
2023-01-11T16:18:41.350473735+13:00
Wed 11 Jan 2023 16:18:41 NZDT
ewen@rustdev:~/misc/src/rust/rust-in-action/ch9/ch9-clock3$