Reduce systematic error of time offset measurements #6642
Conversation
Incoming peers can OpenNetworkConnection(), thus PushVersion() as soon as the server is done with init step 6. Consequently, nTimeOffset can grow arbitrarily large. Executing init step 6 in the end mitigates this issue.
GetTime() can only be larger than the received time; Possibly between 200-20000 microseconds.
|
Our time system has a resolution of one second and huge inherent systemic errors vs UTC (e.g. we do not handle leap seconds). In blocks, latencies in the mining process usually make their times tens of seconds off (and misconfigurations make them minutes off). The network also has infinite gain, e.g. there is no inherent bias to drive it back in sync with UTC except for corner case limitations in the correction procedure. Fortunately, we do not use the time in any ways in which precision is important. (which is also fortunate because making it precise and consistent without centralization is an unsolved problem!) Given that, I am not sure that an offset of even as much as 20000 microseconds is at all interesting. Are you aware of a case where this matters? |
|
I did a cursory check for step 6 side effects having any effect before the end and did not find any. Before accepting that change a more careful examination should be done in review. I think deferring binding until later in setup is likely reasonable regardless of the timing handling. |
|
Indeed, e12d0ad is relevant only in less than 0.5%, correcting the insignificant one second offset on my machine. To check if 0ac42c4 works, you could pick a server with a public IP address which some other nodes actively try to connect to. Then just restart the server (or even do something costly like reindex). In the meantime you can see the other nodes |
|
As it is not clear to me what practical issue this solves - the time sync logic is a bit dodgy in the first place, and as @gmaxwell already says absolute precision is not important for our use caes - I'm not willing to take the risk of breaking the initialization sequence, sorry. |
Assuming all nodes perfectly synced their clocks,
nTimeOffset≤ 0. This PR's commits bring it closer to zero.