https://github.com/bitcoin/bitcoin/pull/28956

Motivation

In order to prevent attacks where the network vision of time can be manipulated by an attacker Bitcoin has used, since it’s first released version IIRC, the concept of network-adjusted time. This adjustment defines how our own view of time can be influenced by the one of our peers, so we can roughly agree on what time it is (or what time is acceptable). With time, the way this has been computed has evolved, from using data from all our peers to only the one provided by our outbound connections, so tampering with time becomes harder (recall all inbounds are treated as potential attackers whereas outbound are randomly picked by us on startup, so the security model relies on outbounds and not inbounds).

However, the used of network-adjusted time has been seen as not that useful for a long time. Tamping with our time, even after the hardening protections, is still possible, and NTP based attacks are not prevented by using it. Hence, this PR attempts to get rid of it.

Thoughts/Questions

• I think this makes sense. The code changes, while spreads trough multiple files, are minimal. We still keep track of the time reported by peers but not to correct our time anymore, just to make sure we are not too far off, and to warn the user if so.
• Look like with the current approach we cannot get out of the warning state if the time is off and it is not our fault (given the data is computed using the first 199 outbound peer timestamps) and our time. If our time is not the issue, the warning will persist.
• Something I noticed that may be somewhat unrelated, but struck me as odd, is that the PR introduced the use of GetTime() (or does not remove them). GetTime() is documented as deprecated, so I wonder whether this is because the readings by it and NodeClock::now() may differ and cause some time inconsistencies, which may lead to consensus discrepancies?
  • Followup: can we get rid of GetTime(), or why is it still part of the codebase if it has been documented as deprecated?
  • Deprecated means it should not be used anymore, and we should replace it by NodeClock::now(), but it does not know it is invalid
• The has been some discussion with respect to how this could lead to a chain split (a la LevelDB update) if nodes do not update and there are time restriction inconsistencies between client versions. I’m curious how likely this is and what the approach moving forward with this should be. Is the suggestion of giving a clear release schedule enough.

Review

Nuke adjusted time (attempt 2) by dergoegge · Pull Request #28956 · bitcoin/bitcoin

PR club

Questions

1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? What was your review approach?

   Check previous section

2. Is it necessary for block headers to have a timestamp? If so, why?

   It is. Timestamps in block headers are still used to compute how long it took to mine blocks, which in turn is used for the difficulty adjustment. The same applies for time based locks. The system sense of time is defined by those timestamps.

3. What is the difference between Median Time Past (MTP) and network-adjusted time? Which of these are relevant to the PR?

   MTP is the median time of the last 11 blocks, and it is used as a lower bound for the timestamps of the subsequent block, that is, the next mined block timestamps cannot be smaller than the MTP. The network-adjusted time, on the other hand, was (prior to this PR) the corrected time computed using timestamp data from the first 199 outbound peer interactions we’ve had (and was not persisted over restarts). This PR does remove the need of the latter, replacing it by our own, non-adjusted, view of time.

4. Why are limits enforced on how far “off” a block header’s timestamp is allowed to be from a node’s internal clock? And since we don’t require exact agreement on time, can these limits be made more strict?

   The restrictions are in place to prevent tampering with the difficulty adjustments. If timestamps are to far off the future, when computing the next difficulty it would look like blocks took longer to be mined, wrongly correcting the difficulty down, the same applies, in the opposite way, if timestamps are too far into the past. This limits could be made more strict, given we are giving some room for discrepancies (maxtimeadjustment) but it we made them too strict valid blocks could be rejected, creating a consensus issue.

5. Prior to this PR, why would an attacker try to manipulate a node’s network-adjusted time?

   For the same reasoning explained in 4. Manipulating the network-adjusted time can slightly affect how the difficulty of the next epoch is adjusted. Furthermore, for time based locks, it could make them trigger slightly before/after than expected.

6. Prior to this PR, how could an attacker try to manipulate a node’s network-adjusted time? Which network message(s) would they use? Hint: network messages are processed in net_processing.cpp

   Manipulating the network-adjusted time would have been hard to pull, but theoretically possible. Initially, all node’s reported time was taken into account when computing the network-adjusted time, however, since https://github.com/bitcoin/bitcoin/pull/23631 this was changed to only consider outbound connections to reduce the potential risk of attacks. The time is reported during the peer handshake, using the VERSION message.

7. Does this PR remove any attack vectors? Does it introduce new ones?

   It theoretically removes the attack vector discussed in 6. However, it really just shifts the requirements to pull it off (from trusting your peers to correct your time, to trusting your time server), so NTP attacks are still theoretically possible.