AddTimeData will never update nTimeOffset past 199 samples #4521

1. 
   jprupp commented at 5:03 pm on July 13, 2014: none

   Reading the function AddTimeData, I came to notice two things:

   • The structure vTimeOffsets contains up to 200 elements, after which any new element added to it will not increase its size, replacing the oldest element.
   • The condition to update nTimeOffset includes checking whether the number of elements in vTimeOffsets is odd, which will never happen after there are 200 elements.

   This will lead to nTimeOffset not updating past 199 calls to AddTimeData, remaining forever stuck in whatever value it had then. I don’t believe this is the inteded behaviour. I would suggest removing the oddness check, since vTimeOffsets is capable of returning the median value of an even list according to the median method of the CMedianFilter type.

   In current master, the offending code is in src/timedata.cpp, line 52, columns 34-64:

   0if (vTimeOffsets.size() >= 5 && vTimeOffsets.size() % 2 == 1)
   

   This should better be:

   0if (vTimeOffsets.size() >= 5)
   

2. laanwj added the label Bug on Jul 14, 2014
3. laanwj added the label P2P on Jul 14, 2014
4. 
   laanwj commented at 8:48 am on July 16, 2014: member

   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)

5. laanwj referenced this in commit 93659379bd on Jul 24, 2014
6. 
   dgenr8 commented at 4:59 pm on August 30, 2014: contributor

   #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.
7. 
   simondlr commented at 12:33 pm on September 5, 2014: contributor

   protective against some attacks.

   What are they? Can’t seem to find any details on it.

8. 
   laanwj commented at 12:54 pm on September 5, 2014: member

   @dgenr8 the problem is that the naive ways to fix this bug would make it even easier to convince a node it has the wrong time offset at any time. At least now the ‘vulnerability’ is limited to a window after startup.

   This bug should be fixed, but as a by-effect of rewriting the time offset determination code.

9. 
   dgenr8 commented at 9:14 pm on September 5, 2014: contributor

   It’s not clear to me which is worse. Attacks aside, a slow or fast system clock isn’t getting corrected dynamically as designed. Maybe it would be better to use “one chronometer” for now.
10. 
    gmaxwell commented at 9:18 pm on September 5, 2014: contributor

    @dgenr8 Making running systems trivially vulnerable to attack is worse. I shouldn’t have to point this out.

    The purpose of the time correction is primarily to fix small time zone / dst error or slight mis-setting. Even the crappy oscillators commonly shipped in Bitcoin do not frequently drift fast enough to cause problems for Bitcoins operation (which are tolerant of up to ~2 hours of offset) if set correctly just once.

11. 
    dgenr8 commented at 9:40 pm on September 5, 2014: contributor

    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.

12. 
    gmaxwell commented at 9:46 pm on September 5, 2014: contributor

    I’d happily ACK a patch that added an optional switch that disabled remote adjustment and added warnings if all your peers disagreed by more than a few minutes. There was one I had floating around previously that queried local NTP and disabled adjustment if NTP thought itself to be in sync (technique used in gnunet)
13. 
    laanwj commented at 4:02 am on September 6, 2014: member

    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…)

14. 
    rebroad commented at 8:47 am on September 7, 2014: contributor

    Is it worth modifying bitcoind or bitcoin-qt to display an alert if the system clock is off (compared to a reliable NTP server)?
15. 
    sipa commented at 11:04 am on September 7, 2014: member

    How do you find a reliable NTP server?
16. 
    simondlr commented at 11:13 am on September 7, 2014: contributor

    Perhaps NTP Pool? Cycles randomly between public NTP servers every hour. http://www.pool.ntp.org/en/use.html
17. 
    sipa commented at 11:17 am on September 7, 2014: member

    We’re really not going to rely on a centralized service in the reference client; even if it is just one that rotates between others. Adding optional NTP support that can be configured to use your own server (and perhaps defaults to one on localhost, if available) would be possible, but is perhaps overkill. We have ~hours wiggle room in local clock variation before there is really a problem…
18. 
    dgenr8 commented at 10:02 pm on September 7, 2014: contributor

    All of the platforms on which core runs have well supported time sync clients, which can be configured to use any server.
19. 
    laanwj commented at 7:11 am on September 8, 2014: member

    As said many times, we have no need for NTP-level precision. Let’s not keep bringing that back into the discussion.

    Giving a warning if the time is significantly different from all peers should be enough. The reason for this check is mostly to find out if people have misconfigured their time zone.

20. 
    jprupp commented at 2:14 pm on September 8, 2014: none

    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.

21. MathyV referenced this in commit aebd5ec3fe on Nov 24, 2014
22. 
    hno commented at 10:09 am on July 29, 2015: none

    A side effect of this bug is that a node that has a drifting clock can will keep on drifting with no warning shown until bitcoind is restarted.

    Node starts up at reasonaly correct time. Collects 199 time offsets from other peers. Local clock drifts. bitcoind silently keeps on using local time + previously calculated network adjustment, not at all compensating for it’s local clock drift or even giving a warning/error when the difference gets too large.

23. hno referenced this in commit 1f4d1977fb on Jul 29, 2015
24. 
    hno commented at 12:53 pm on July 29, 2015: none

    The above is an attempt to address the concerns raised here by adding an option to trust the local clock source, and rework warning conditions somewhat. Please review if you are interested.
25. 
    paveljanik commented at 1:08 pm on August 7, 2015: contributor

    -trustlocalclock: Why not just return 0 in GetTimeOffset()? And warn about the peers offset being higher?
26. 
    paveljanik commented at 1:14 pm on August 7, 2015: contributor

    But in general, I like -trustlocalclock. It allows the user to choose the “take one” part of the “Never go to the sea…” comment in https://github.com/bitcoin/bitcoin/blob/master/src/timedata.cpp#L21. Of course when using NTP, you could still be in bad network partition. Or your other time source could be compromised etc. But you can at least select such option.
27. 
    maflcko commented at 1:53 pm on October 11, 2015: member

    Related PR: #6545
28. hno referenced this in commit a4c6e97bc4 on Nov 13, 2015
29. hno referenced this in commit 4f076f843d on Nov 13, 2015
30. 
    laanwj commented at 5:02 pm on February 16, 2016: member

    “We’re using SPV peer’s time to compute our network offset P2P #6071” is another time correction related issue.
31. 
    laanwj commented at 7:29 am on March 30, 2016: member

    Another issue: #7573 (comment)

    But the implementation has an odd non-monotonicity. E.g. if you have 5 peers claiming ten seconds off, then six more connect claiming 80 minutes. You’ll apply a correction of 0 instead of 10 seconds. Perhaps better to just fix in another PR, since it was an existing misbehavior (though more likely to be triggered when the time is cut down to small numbers).

32. reddink referenced this in commit 0cf0ee85ec on May 27, 2020
33. maflcko removed the label Bug on Nov 30, 2021
34. maflcko added the label Brainstorming on Nov 30, 2021
35. maflcko added the label Feature on Nov 30, 2021
36. fanquake referenced this in commit e457513eb1 on Dec 7, 2021
37. 
    maflcko commented at 4:15 pm on December 7, 2021: member

    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 #4521 (comment)

38. sidhujag referenced this in commit 455720cb71 on Dec 7, 2021
39. maflcko referenced this in commit 28bdaa3f76 on Mar 14, 2022
40. sidhujag referenced this in commit 0daf624c9e on Mar 14, 2022
41. Fabcien referenced this in commit 0eda354e86 on Mar 17, 2023
42. achow101 closed this on Apr 30, 2024

    ---

Contributors
jprupp laanwj dgenr8 simondlr gmaxwell rebroad sipa hno paveljanik maflcko

Labels
Feature Brainstorming P2P