storj-archived / core

Deprecated. Implementation of the Storj v2 protocol for Node.js.
https://storj.io
Other
395 stars 88 forks source link

Wrong ntp delta calculation #657

Closed littleskunk closed 7 years ago

littleskunk commented 7 years ago

Package Versions

Replace the values below using the output from npm list storj. Use npm list -g storj if installed globally.

  * daemon: 2.4.3, core: 6.2.0, protocol: 1.1.0

Replace the values below using the output from node --version.

v6.9.5

Expected Behavior

Please describe the program's expected behavior. Include an example of your usage code in the back ticks below if applicable.

Simple example. Lets say I have a WLAN connection with a high ping. 4 second latency. My clock is perfect synced.

Send out the NTP request at second 0. Hit the time server at second 2. Get the timestamp at second 4.

I would expect a delta of more or less 0 ms

Actual Behavior

Please describe the program's actual behavior. Please include any stack traces or log output in the back ticks below.

The core is caluclating the latency wrong and will return a delta of 2 seconds and the user can't correct this. The problem is this line of code: https://github.com/Storj/core/blob/891148432f1a072a225e011e300815de0f1252ca/lib/utils.js#L291

It should be latency/2.

littleskunk commented 7 years ago

I will work on a pull request.

littleskunk commented 7 years ago

It is working. The new delta is more stable and looks better.

Old delta:

{"level":"info","message":"clock is synchronized with ntp, delta: 580 ms","timestamp":"2017-02-21T16:04:47.330Z"}
{"level":"info","message":"clock is synchronized with ntp, delta: 240 ms","timestamp":"2017-02-21T16:05:34.213Z"}
{"level":"info","message":"clock is synchronized with ntp, delta: 601 ms","timestamp":"2017-02-21T16:06:12.671Z"}

new delta:

{"level":"info","message":"clock is synchronized with ntp, delta: -274 ms","timestamp":"2017-02-21T16:01:22.553Z"}
{"level":"info","message":"clock is synchronized with ntp, delta: -256 ms","timestamp":"2017-02-21T16:02:24.338Z"}
{"level":"info","message":"clock is synchronized with ntp, delta: -268 ms","timestamp":"2017-02-21T16:03:11.304Z"}
spiccioli commented 7 years ago

Thanks littleskunk for following up on this issue.