p2p: remove adjusted time #25908
p2p: remove adjusted time #25908
Conversation
fae674e to
6a860cd
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK, light code review ACK. |
| @@ -638,7 +636,6 @@ static RPCHelpMan getnetworkinfo() | |||
| if (node.peerman) { | |||
| obj.pushKV("localrelay", !node.peerman->IgnoresIncomingTxs()); | |||
| } | |||
| obj.pushKV("timeoffset", GetTimeOffset()); | |||
There was a problem hiding this comment.
Maybe keep this field and instead return the median offset of the currently connected outbounds or so? See also #24606 (comment)
| // harder for others to tamper with our adjusted time. | ||
| AddTimeData(pfrom.addr, nTimeOffset); | ||
| } | ||
| pfrom.nTimeOffset = nTime - GetTime(); |
There was a problem hiding this comment.
Perhaps:
if (!pfrom.IsInboundConn() && std::abs(pfrom.nTimeOffset) >= 1800) {
++m_bad_non_inbound_time_offset;
if (m_bad_non_inbound_time_offset == 3) {
SetMiscWarning("Please check your computer's date and time are correct");
}
}(ie, add a counter to PeerManagerImpl, and on the third outbound connection that has a time offset of 30mins or more, set a warning that will show up in getblockchaininfo, getnetworkinfo, getmininginfo until restart)
There was a problem hiding this comment.
Alternatively, we regularly do new feeler and blocks-only connections, perhaps we could keep a rolling average of abs(peer.nTimeOffset) and report that in getnetworkinfo?
|
Concept ACK |
There was a problem hiding this comment.
By removing the adjustement of our local clock time on the median of offsets from peers clocks, we're lowering the robustness of our operational time against time shifting attacks where an adversary shifts the local time of the host NTP client backward/forward. Before, this change even if the clock drifts, it's at least corrected in the bound of DEFAULT_MAX_TIME_ADJUSTEMENT. After, this change if the clock is controlled by malicious timeservers, I don't think there is corrective action or even warnings issued towards the operators.
With most of Linux distributions, the local time at a client is determined based on the clock readings received from timeservers from the NTP pool, an open set of public servers, segmented in geographical zones for load-balancing. It should be noted the policy of this pool recommends one to distrust time provided if high reliability is sought. An adversary could either fulfill the pool with crappy servers or exploit one of the unpatched vulnerabilities as the high-stratum servers are likely randomly maintained due to being run by volunteers.
For block consensus validation, we're relying on clock time accuracy w.r.t honest network of peers, a block timestamp too far in the future from the viewpoint of the local clock will be rejected (L3472 in ContextualCheckBlockHeader()). A successful time shifting attack against your time could fork you off from the network and from then escalate to a time-dilation attack variant against your Lightning node.
While I think this is valuable to overhaul or remove GetAdjustedTime(), I would prefer first that we introduce a warning system based on discrepancies between local clock and outbound peers ones, or allocate more thoughts if we can come up with better mitigations against NTP attacks at the application-level.
cc @TheBlueMatt @naumenkogs with whom I had discussions in the past on how fucking up with your NTP syncing can compromise your Lightning node funds security.
No correction will happen if your time drifts and you only ever cycle through at most 4 distinct honest outbound connections. Also, no correction will happen if your time drifts after you made more than I don't see a way that adjusted time can be helping some without also harming others. Every instance where it provides a "fix" for some users, it opens the attack surface for users that are not affected by the problem. For example, if an attacker can influence enough (not all) outgoing connections of your node, they can likely get you out of consensus with a perfectly synced system clock. This also breaks the assumption that a single honest connection would be sufficient for you to stay in consensus. |
From my understanding, there is at least a correction brought to your system clock once you have at most 4 distinct honest outbound connections and before you have made more than BITCOIN_TIMEDATA_MAX_SAMPLES/2+1, which I think made my point correct that the existence of a correction hardens the attack game for an adversary targeting your NTP syncing, even minimally. Noting this correction is featherweight if you're facing an adversary with BGP capabilities, I don't deny it. I would agree a better protection would be effectively for the system operator to correct the system clock, a good question being if we can achieve this at the bitcoind-level with a warning system potentially based on peers clocks, or we should recommend high-stakes users to be their own stratum 0 servers, and directly get time from GPS or a non-Internet time source.
Assuming you're starting with a perfectly synced system clock, as we're bounded with |
|
They could delay your time by 70min and then mine blocks 2h ahead (or advance the time of another miner by 70min), which would make you lag on consensus and throw away your hashrate (if you have any) for as long as the latest block is too far ahead. |
6a860cd to
769a6af
Compare
|
Concept ACK. |
769a6af to
8f3b09e
Compare
8f3b09e to
a9a615d
Compare
a9a615d to
d29b81e
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
d29b81e to
644f93f
Compare
ff9039f Remove GetAdjustedTime (dergoegge) Pull request description: This picks up parts of #25908. The use of adjusted time is removed from validation code while the warning to users if their clock is out of sync with the rest of the network remains. ACKs for top commit: naumenkogs: ACK ff9039f achow101: ACK ff9039f maflcko: lgtm ACK ff9039f 🤽 stickies-v: ACK ff9039f Tree-SHA512: d1f6b9445c236915503fd2ea828f0d3b92285a5dbc677b168453276115e349972edbad37194d8becd9136d8e7219b576af64ec51c72bdb1923e57e405c0483fc
After recent refactoring, the changes required to remove adjusted time are now quite straight forward. This PR removes the notion of adjusted time, along with the
-maxtimeadjustmentoption. It doesn't do any other cleanup.Opening for discussion of gotchas / brainstorming / concept (N)ACKs.
1 question from a reviewer was whether we should keep some sort of warning around for if our clock differs too much from our peers. Although it's unclear how useful that might be.
Commits can be cleaned up / sqaushed. Missing release notes etc.
Would close #4521.