p2p: remove adjusted time

fanquake commented at 10:01 am on August 23, 2022: member

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 -maxtimeadjustment option. 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.

fanquake added the label Brainstorming on Aug 23, 2022

fanquake added the label P2P on Aug 23, 2022

fanquake force-pushed on Aug 23, 2022

fanquake renamed this:
~~[WIP] p2p: remove adjusted time~~
p2p: remove adjusted time
on Aug 23, 2022

DrahtBot commented at 3:30 pm on August 23, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	naumenkogs, MarcoFalke, dergoegge, darosior

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27534 (rpc: add ‘getnetmsgstats’, new rpc to view network message statistics by satsie)
#27491 (refactor: Move chain constants to the util library by TheCharlatan)
#26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)

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.

naumenkogs commented at 7:31 am on August 24, 2022: member

Concept ACK, light code review ACK.

in src/rpc/net.cpp:649 in 6a860cdd4a outdated

635@@ -638,7 +636,6 @@ static RPCHelpMan getnetworkinfo()
636     if (node.peerman) {
637         obj.pushKV("localrelay", !node.peerman->IgnoresIncomingTxs());
638     }
639-    obj.pushKV("timeoffset",    GetTimeOffset());

maflcko commented at 7:50 am on August 24, 2022:

Maybe keep this field and instead return the median offset of the currently connected outbounds or so? See also #24606 (comment)

maflcko commented at 7:55 am on August 24, 2022: member

Concept ACK, but there should be an easy way for users to figure out if their clock is wrong, ideally at startup or early after startup. It wouldn’t be the best UX if they found out by going out of consensus or by seeing their mined blocks be dropped.

in src/net_processing.cpp:3382 in 6a860cdd4a outdated

2992-        if (!pfrom.IsInboundConn()) {
2993-            // Don't use timedata samples from inbound peers to make it
2994-            // harder for others to tamper with our adjusted time.
2995-            AddTimeData(pfrom.addr, nTimeOffset);
2996-        }
2997+        pfrom.nTimeOffset = nTime - GetTime();

ajtowns commented at 8:19 am on August 26, 2022:

Perhaps:

0if (!pfrom.IsInboundConn() && std::abs(pfrom.nTimeOffset) >= 1800) {
1    ++m_bad_non_inbound_time_offset;
2    if (m_bad_non_inbound_time_offset == 3) {
3        SetMiscWarning("Please check your computer's date and time are correct");
4    }
5}

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

ajtowns commented at 8:29 am on October 7, 2022:

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?

dergoegge commented at 10:44 am on September 26, 2022: member

Concept ACK

ariard commented at 10:35 pm on September 30, 2022: member

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.

maflcko commented at 12:30 pm on October 2, 2022: member

Before, this change even if the clock drifts, it’s at least corrected in the bound of DEFAULT_MAX_TIME_ADJUSTEMENT.

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 BITCOIN_TIMEDATA_MAX_SAMPLES/2+1 distinct honest outbound connections. Instead of a false promise that adjusted time will correct an incorrect system clock, it would be better for the system operator to correct the system clock.

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.

ariard commented at 0:49 am on October 7, 2022: member

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 BITCOIN_TIMEDATA_MAX_SAMPLES/2+1 distinct honest outbound connections. Instead of a false promise that adjusted time will correct an incorrect system clock, it would be better for the system operator to correct the system clock.

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.

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.

Assuming you’re starting with a perfectly synced system clock, as we’re bounded with DEFAULT_MAX_TIME_ADJUSTEMENT, of an effective value of 70min, the attacker won’t be able to make you reject block due to seen timestamp being too far in the future. From my comprehension, current adjusted time does not break the assumption that a single honest connection would be sufficient to stay in consensus.

maflcko commented at 7:49 am on October 7, 2022: member

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.

DrahtBot added the label Needs rebase on Oct 19, 2022

fanquake force-pushed on Nov 9, 2022

DrahtBot removed the label Needs rebase on Nov 9, 2022

DrahtBot added the label Needs rebase on Dec 25, 2022

darosior commented at 10:41 am on March 21, 2023: member

Concept ACK.

fanquake force-pushed on Mar 23, 2023

fanquake commented at 3:27 pm on March 23, 2023: member

Rebased on master. @ajtowns putting aside the current comments, any thoughts on including changes like this in bitcoin-inquisition?

DrahtBot removed the label Needs rebase on Mar 23, 2023

ajtowns commented at 9:37 pm on March 23, 2023: contributor

Rebased on master. @ajtowns putting aside the current comments, any thoughts on including changes like this in bitcoin-inquisition?

No objection to it, but not sure why it would be easier to do it that way than just merge into master?

DrahtBot added the label Needs rebase on Mar 30, 2023

fanquake force-pushed on Mar 31, 2023

DrahtBot removed the label Needs rebase on Mar 31, 2023

DrahtBot added the label Needs rebase on Apr 21, 2023

fanquake force-pushed on Apr 21, 2023

DrahtBot removed the label Needs rebase on Apr 21, 2023

DrahtBot added the label CI failed on Apr 21, 2023

DrahtBot added the label Needs rebase on May 9, 2023

DrahtBot commented at 4:34 pm on May 9, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

p2p: stop sampling timedata from peers f30a2fb736

timedata: remove TimeOffset from AdjustedTime b5a55b7747

time: replace GetAdjustedTime() with NodeClock::now() 970996b762

rpc: remove timeoffset from getnetworkinfo() RPC e300fb314d

init: remove -maxtimeadjustment option 7fdea2fc3c

time: remove timedata 644f93f496

fanquake force-pushed on May 17, 2023

fanquake removed the label Needs rebase on May 17, 2023

fanquake added the label Up for grabs on May 17, 2023

fanquake closed this on May 17, 2023

fanquake removed the label Up for grabs on Nov 28, 2023

achow101 referenced this in commit 3c13f5d612 on Jan 31, 2024

fanquake deleted the branch on Apr 4, 2024

p2p: remove adjusted time #25908

Reviews

Conflicts