bitcoin: AddTimeData will never update nTimeOffset past 199 samples

I wonder if it wouldn’t be better to remove the use of GetAdjustedTime. While it works well to cover some cases of misconfigured clocks, it fails to detect others.

Then, replace it with a stronger warning mechanism that also triggers when the time is off by more than ~10 minutes? Currently it only triggers after 70 minutes.

Maybe even add setting (off by default) that shuts down the node if outbound nodes report a different time than ours?

Edit: This is probably roughly the idea from https://github.com/bitcoin/bitcoin/issues/4521#issuecomment-54685781

Since we cannot agree on what is best, I suggest we leave the computer operator to decide. Let the computer trust its own clock, and disable the network time offset altogether. If the computer is setup to sync with NTP, good, if not, then good too. I think it is a good idea to bail if the machine has an offset of more than, say five or ten minutes with respect to the median time of peers it is attempting to connect to.

These are some potential attacks:

• Malicious NTP server can isolate the machine and serve a separate version of the blockchain (already a risk currently, if enough nodes agree on the bad time, regardless of network offset calculation)
• Malicious NTP server can deny the machine a connection no the Bitcoin network (offset too large, a risk with current scheme too)
• Offset could make machine reject a block (this is already a risk on current scheme)
• Offset may prevent the machine from connecting to the Bitcoin network (already a risk)

There surely are some attack vectors that I have not considered. But these that I could come up with are already affecting the current client, even with all the network offset calculation stuff. We could make the code simpler and still run the same level of risk.

All of the platforms on which core runs have well supported time sync clients, which can be configured to use any server.

I can’t help wondering, if we just used the system clock, and this scheme were suggested, would it be shot down for taking a local problem and making it vulnerable to remote attack?

Probably. Personally I’d prefer to have clock sync as just a local problem. A mechanism such as this should be used to warn, not correct. If your time is wrong, fix it. Wrong clocks cause all kinds of issues on a system.

(this excludes drift over time due to oscillators which is outside the user’s control, of course, but correcting for that requires a more advanced algorithm and as said by @gmaxwell is not required in our case as we don’t need time to be very precise…)

I can’t help wondering, if we just used the system clock, and this scheme were suggested, would it be shot down for taking a local problem and making it vulnerable to remote attack?

And I’d better not fix my clock. bitcoind will keep applying the wrong adjustment until restart.

#4526 does not explain the potential attacks that warrant not fixing this bug. While this bug remains in place, a node can become convinced by its peers that its time is off by 70 minutes, and stay that way until restarted.

According to @gmaxwell this is unintended behavior, but protects against worse attacks:

but in this case the ‘bug’ is protective against some attacks (and may actually explain why we’ve never seen the one I’m thinking of exploited, which had surprised me…). So I think we should hold off on fixing this and just clean it up as a result of some timing cleanup the strengthens it in a number of other ways. I should have some time to work on this soon.

(see discussion in #4526)