Nuke adjusted time from validation (attempt 2) #28956

dergoegge · 2023-11-28T13:26:59Z

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.

DrahtBot · 2023-11-28T13:27:02Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	naumenkogs, maflcko, stickies-v, achow101
Concept ACK	ajtowns, RandyMcMillan, fanquake, Pttn, ariard, TheCharlatan, sr-gi

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:

#29236 (log: Nuke error(...) by maflcko)
#28339 (validation: improve performance of CheckBlockIndex by mzumsande)

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.

dergoegge · 2023-11-29T10:58:33Z

@ajtowns @darosior @fanquake @maflcko @naumenkogs You all were interested in the previous PR. Would appreciate your review.

ajtowns

Concept ACK

src/net_processing.h

src/rpc/net.cpp

src/net_processing.cpp

src/init.cpp

RandyMcMillan · 2023-11-29T15:36:45Z

Concept ACK

fanquake · 2023-11-29T17:21:12Z

Concept ACK

Pttn · 2023-11-29T22:26:33Z

Concept ACK

src/net_processing.cpp

naumenkogs

Concept ACK

ariard

Concept ACK

I would recommend to add an explicit release note, inciting nodes operators to monitor this new warning, especially if they’re running time-sensitive apps. Exploiting NTP to target bitcoin has been suggested both in NTP security literature and the time-dilation paper (I’ve never seen or heard any in the wild, contrary to some BGP attacks in other cryptos though I believe this is fairly practical). E.g

new local clock monitoring 
==========================

The notion of adjusted time relying on the median of other network nodes clocks is removed.
Instead, a rolling sample of the time offset from local clock of each of the last 10 outbound
peers is maintained and the median computed. If the yielding result is more or less than
70 min, a node warning is issued.

This warning can be consumed by a call to `getblockchaininfo`. If the node is backing an
application or protocol with stringent time-sensitive requirements and high funds at stake
(e.g a lightning or time-enabled wallet)  it is recommended to periodically run a monitoring
job and in presence of warning automatically notify the operator.

src/net_processing.cpp

TheCharlatan · 2024-01-03T15:36:40Z

Concept ACK

sr-gi

Concept ACK

sr-gi · 2024-01-02T18:41:27Z

src/net_processing.cpp

+        LOCK(m_mutex);
+
+        // Only compute the median from 5 or more samples.
+        if (m_index < 4) return 0;


In order to be consistent with how the code used to be what needs updating is the code and not the comment, doesn't it? Otherwise, we will be computing the median as low as with 4 elements when we used to need 5 before.

I don't know what the motivation to require 5 data points previously was (why not 4 or 3?), but my impression by reading the comments in this PR is that we are trying no to change anyting with respect to how the median is computed and leave that for a followup

src/net_processing.cpp

sr-gi · 2024-01-02T19:11:34Z

src/net_processing.cpp

-            // Don't use timedata samples from inbound peers to make it
-            // harder for others to tamper with our adjusted time.


I think a version of this comment may still be relevant to motivate why we only sample outbound nodes

sr-gi · 2024-01-02T19:30:04Z

src/net_processing.h

+struct PeerManagerInfo {
+    int64_t median_outbound_time_offset{0};
+    bool ignores_incoming_txs{false};


Out of curiosity: are you grouping these two so you can query them together, given you'll need both of them in getnetworkinfo if peerman is defined?

sr-gi · 2024-01-02T20:28:04Z

src/net_processing.cpp

+        std::sort(sorted_copy.begin(), sorted_copy.begin() + m_index);
+
+        if (m_index % 2 == 0) {
+            return (sorted_copy[m_index / 2] / 2) +


I was going to suggest doing sorted_copy[m_index / 2] + sorted_copy[m_index / 2 - 1]) / 2)) before seeing this comment since the current approach feels a bit strange. If we keep this I'd say at least add a comment pointing out this is to prevent overflow

src/validation.cpp

dergoegge · 2024-01-22T15:13:29Z

This PR now only removes adjusted time from validation code while keeping all the code for the warning in place as is. A follow-up can change the warning (as it has many problems) but this PR will focus on the removal of adjusted time from consensus.

Re-implementing the same warning seemed unnecessary (even if it was less code) and prone to bugs.

stickies-v

Approach ACK

I think keeping this diff small and focusing on just the (non-warning) functional behaviour change is the best approach to reduce friction to get more high quality feedback on whether this is indeed a safety improvement (I lean towards yes). Code LGTM ff9039f, but I think this warrants a release note since we are changing some security assumptions, making users aware of that seems like the way to go imo.

nit: latest force push regressed a couple of includes:

git diff on ff9039f

diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index e1dfe10483..e14de004f5 100644
--- a/src/headerssync.cpp
+++ b/src/headerssync.cpp
@@ -5,8 +5,8 @@
 #include <headerssync.h>
 #include <logging.h>
 #include <pow.h>
-#include <timedata.h>
 #include <util/check.h>
+#include <util/time.h>
 #include <util/vector.h>
 
 // The two constants below are computed using the simulation script in
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b426b889cf..8c43377875 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -41,6 +41,7 @@
 #include <txrequest.h>
 #include <util/check.h>
 #include <util/strencodings.h>
+#include <util/time.h>
 #include <util/trace.h>
 #include <validation.h>
 
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 89498fb976..87f40e993f 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -20,8 +20,8 @@
 #include <policy/policy.h>
 #include <pow.h>
 #include <primitives/transaction.h>
-#include <timedata.h>
 #include <util/moneystr.h>
+#include <util/time.h>
 #include <validation.h>
 
 #include <algorithm>
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 069a451089..929247d32b 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -27,11 +27,11 @@
 #include <script/descriptor.h>
 #include <script/script.h>
 #include <script/signingprovider.h>
-#include <timedata.h>
 #include <txmempool.h>
 #include <univalue.h>
 #include <util/strencodings.h>
 #include <util/string.h>
+#include <util/time.h>
 #include <util/translation.h>
 #include <validation.h>
 #include <validationinterface.h>
diff --git a/src/timedata.h b/src/timedata.h
index 90428d071c..17e7539700 100644
--- a/src/timedata.h
+++ b/src/timedata.h
@@ -5,8 +5,6 @@
 #ifndef BITCOIN_TIMEDATA_H
 #define BITCOIN_TIMEDATA_H
 
-#include <util/time.h>
-
 #include <algorithm>
 #include <cassert>
 #include <chrono>

stickies-v · 2024-01-24T12:14:31Z

src/validation.cpp

@@ -3805,7 +3805,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, BlockValidatio
        return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "time-too-old", "block's timestamp is too early");

    // Check timestamp
-    if (block.Time() > now + std::chrono::seconds{MAX_FUTURE_BLOCK_TIME}) {
+    if (block.Time() > NodeClock::now() + std::chrono::seconds{MAX_FUTURE_BLOCK_TIME}) {


This latest force-push seems to have accidentally reverted the necessary doc change in chain.h:

git diff on ff9039f

diff --git a/src/chain.h b/src/chain.h index f9121fb861..ab58fad5b9 100644 --- a/src/chain.h +++ b/src/chain.h @@ -19,7 +19,7 @@ /** * Maximum amount of time that a block timestamp is allowed to exceed the - * current network-adjusted time before the block will be accepted. + * current time before the block will be accepted. */ static constexpr int64_t MAX_FUTURE_BLOCK_TIME = 2 * 60 * 60;

sr-gi · 2024-01-26T20:47:15Z

This PR now only removes adjusted time from validation code while keeping all the code for the warning in place as is. A follow-up can change the warning (as it has many problems) but this PR will focus on the removal of adjusted time from consensus.

Re-implementing the same warning seemed unnecessary (even if it was less code) and prone to bugs.

Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?

stickies-v · 2024-01-26T21:02:08Z

Does this mean that the adjusted time is now being collected and used exclusively to raise the warning?

This was the case before the force-push, too. In this current approach, the diff is much smaller so review can focus more on whether we're okay removing network-adjusted time, instead of on the adjusted time calculation refactoring.

naumenkogs · 2024-01-29T09:46:37Z

ACK ff9039f

After a few attempts, I haven't found a way to trigger a consensus fork through adjusted time. It's not like this could "infect" half of the network (legacy) through block propagation or something. Because of that, if there was/is a legacy way to exploit it (say an overflow in nTimeOffset calculation), this new code won't make it worse.

maflcko

lgtm ACK ff9039f 🤽

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK ff9039f6ea876bab2c40a06a93e0dd087f445fa2 🤽
hjCKZbc2j48mFkBbdMp6XYwvC+JpHi9gtElrJyy4RJVJjbMvsAntWQAHbs4leVk6OCxxLaX5X9PRIWR3wRQ/DA==

stickies-v

ACK ff9039f

I would prefer the commit message to contain a rationale for the change, though. It's currently empty.

I now believe that the network-adjusted time implementation in master does not offer any security guarantees against an attacker trying to get a node out of consensus. The existing NTP vulnerabilities (which seem to be very real, even if currently apparently not exploited too often) can just as well be used by an attacker when network-adjusted time is used, given that we limit the network adjustment to 70 minutes. As such, this PR does indeed seem like a strong improvement to me.

Ensuring an accurate clock is a system responsibility, and should not be a Bitcoin Core concern. Removing this logic simplifies the code, and removes the assumption that we need a majority of honest outbound nodes. Moreover, it seems trivial to implement an external daemon that monitors Bitcoin network time and e.g. adjusts the local clock or shuts down Bitcoin Core when a large offset is detected, for users that wish to harden their system this way.

achow101 · 2024-01-31T20:48:37Z

ACK ff9039f

TheCharlatan

ACK ff9039f

ariard · 2024-02-08T23:12:15Z

After a few attempts, I haven't found a way to trigger a consensus fork through adjusted time.

Unclear what has been tested actually (e.g setting sampled time of peers in the past and playing with blocks timestamps).
That said I think it’s a very edge scenario, and most susceptible to affect non-upgraded things like btcd.

I now believe that the network-adjusted time implementation in master does not offer any security guarantees against an attacker trying to get a node out of consensus. The existing NTP vulnerabilities (which seem to be very real, even if currently apparently not exploited too often) can just as well be used by an attacker when network-adjusted time is used, given that we limit the network adjustment to 70 minutes.

NTP has its own data / control plane. Our peers connections going through TCP. You might compromise NTP stack but not TCP and vice-versa.

Ensuring an accurate clock is a system responsibility, and should not be a Bitcoin Core concern

With argument of that kind we can remove assumevalid / assumeutxo and say perf issues poor host not core concern.
More we’re able to compensate weakness from network infrastructure level, better core node ecosystem stronger.

Moreover, it seems trivial to implement an external daemon that monitors Bitcoin network time and e.g. adjusts the local clock or shuts down Bitcoin Core when a large offset is detected, for users that wish to harden their system this way.

I’ll let as an exercise to the reader to define what should be the authoritative source of time of this external monitoring daemon :) My belief we should conserve a a warning system based on other peers nversion announcements.

@dergoegge I think it can be good if you notify btcd and other validation software carrying a minimum of economic traffic of this change, at least to avoid future issues like the bip68 consensus discrepancy you found recently.

maflcko · 2024-02-09T08:37:35Z

My belief we should conserve a a warning system based on other peers nversion announcements.

Yes, this is exactly what this pull request did. See also the description, which said: "while the warning to users if their clock is out of sync with the rest of the network remains."

See also: https://bitcoinops.org/en/newsletters/2024/02/07/#bitcoin-core-28956

ariard · 2024-02-12T00:26:23Z

Yes, this is exactly what this pull request did. See also the description, which said: "while the warning to users if their clock is out of sync with the rest of the network remains.”

That I know. It was more related to using an external daemon and naively using the same NTP source than your Bitcoin Core instance. There is no such thing as Bitcoin network time, just a sampling of peers nversion time. Yes it works if this external daemon consumes Core warning messages and takes corrective actions on this. And even that the correction frequency have to be weighted carefully (are you scaling the correction frequency on internal clock itself or received peers msgs ?)

FYI, some NTP attacks were actually on targeting NTP pool monitoring infrastructure, see this paper.

@dergoegge

c6be144 Remove timedata (stickies-v) 92e72b5 [net processing] Move IgnoresIncomingTxs to PeerManagerInfo (dergoegge) 7d9c3ec [net processing] Introduce PeerManagerInfo (dergoegge) ee178df Add TimeOffsets helper class (stickies-v) 55361a1 [net processing] Use std::chrono for type-safe time offsets (stickies-v) 038fd97 [net processing] Move nTimeOffset to net_processing (dergoegge) Pull request description: [An earlier approach](https://github.com/bitcoin/bitcoin/commits/1d226ae1f984c5c808f5c24c431b959cdefa692e/) in #28956 involved simplifying and refactoring the network-adjusted time calculation logic, but this was eventually [left out](#28956 (comment)) of the PR to make it easier for reviewers to focus on consensus logic changes. Since network-adjusted time is now only used for warning/informational purposes, cleaning up the logic (building on @dergoegge's approach in #28956) should be quite straightforward and uncontroversial. The main changes are: - Previously, we would only calculate the time offset from the first 199 outbound peers that we connected to. This limitation is now removed, and we have a proper rolling calculation. I've reduced the set to 50 outbound peers, which seems plenty. - Previously, we would automatically use the network-adjusted time if the difference was < 70 mins, and warn the user if the difference was larger than that. Since there is no longer any automated time adjustment, I've changed the warning threshold to ~~20~~ 10 minutes (which is an arbitrary number). - Previously, a warning would only be raised once, and then never again until node restart. This behaviour is now updated to 1) warn to log for every new outbound peer for as long as we appear out of sync, 2) have the RPC warning toggled on/off whenever we go in/out of sync, and 3) have the GUI warn whenever we are out of sync (again), but limited to 1 messagebox per 60 minutes - no more globals - remove the `-maxtimeadjustment` startup arg Closes #4521 ACKs for top commit: sr-gi: Re-ACK [c6be144](c6be144) achow101: reACK c6be144 dergoegge: utACK c6be144 Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96

dergoegge marked this pull request as draft November 28, 2023 14:25

dergoegge force-pushed the 2023-11-nuke-adjtime branch from 3dd0968 to cd3d8bd Compare November 28, 2023 14:28

DrahtBot added the CI failed label Nov 28, 2023

DrahtBot mentioned this pull request Nov 28, 2023

p2p: Fill reconciliation sets (Erlay) #28765

Open

dergoegge force-pushed the 2023-11-nuke-adjtime branch from cd3d8bd to aec6b13 Compare November 28, 2023 15:46

dergoegge marked this pull request as ready for review November 28, 2023 15:46

DrahtBot removed the CI failed label Nov 28, 2023

DrahtBot mentioned this pull request Nov 29, 2023

Multiprocess bitcoin #10102

Draft

ajtowns reviewed Nov 29, 2023

View reviewed changes

dergoegge force-pushed the 2023-11-nuke-adjtime branch from aec6b13 to d283c10 Compare November 29, 2023 13:28

sipa reviewed Nov 30, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Dec 1, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

naumenkogs reviewed Dec 1, 2023

View reviewed changes

DrahtBot mentioned this pull request Dec 1, 2023

Stratum v2 Template Provider (take 2) #28983

Closed

30 tasks

ariard reviewed Dec 4, 2023

View reviewed changes

src/net_processing.cpp Outdated Show resolved Hide resolved

dergoegge force-pushed the 2023-11-nuke-adjtime branch 2 times, most recently from 82ac64e to 2d429bc Compare December 6, 2023 15:47

DrahtBot added the CI failed label Dec 6, 2023

DrahtBot mentioned this pull request Dec 6, 2023

kernel: Streamline util library #29015

Open

sr-gi reviewed Jan 3, 2024

View reviewed changes

sipa reviewed Jan 3, 2024

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

Remove GetAdjustedTime

ff9039f

DrahtBot mentioned this pull request Jan 11, 2024

log: Nuke error(...) #29236

Merged

DrahtBot added the CI failed label Jan 16, 2024

DrahtBot mentioned this pull request Jan 21, 2024

refactor: Readable widget naming for debug window bitcoin-core/gui#790

Closed

dergoegge force-pushed the 2023-11-nuke-adjtime branch from 1d226ae to ff9039f Compare January 22, 2024 15:06

dergoegge changed the title ~~Nuke adjusted time (attempt 2)~~ Nuke adjusted time from validation (attempt 2) Jan 22, 2024

stickies-v reviewed Jan 24, 2024

View reviewed changes

DrahtBot removed the CI failed label Jan 27, 2024

DrahtBot requested review from stickies-v, TheCharlatan and sr-gi January 29, 2024 09:46

maflcko approved these changes Jan 29, 2024

View reviewed changes

stickies-v approved these changes Jan 30, 2024

View reviewed changes

achow101 merged commit 3c13f5d into bitcoin:master Jan 31, 2024
16 checks passed

TheCharlatan reviewed Jan 31, 2024

View reviewed changes

stickies-v mentioned this pull request Feb 1, 2024

refactor: Fix timedata includes #29361

Merged

stickies-v mentioned this pull request Mar 11, 2024

Simplify network-adjusted time warning logic #29623

Merged

Extheoisah mentioned this pull request Mar 12, 2024

Simplify network-adjusted time warning logic Extheoisah/bitcoin#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nuke adjusted time from validation (attempt 2) #28956

Nuke adjusted time from validation (attempt 2) #28956

dergoegge commented Nov 28, 2023 •

edited

DrahtBot commented Nov 28, 2023 •

edited

dergoegge commented Nov 29, 2023

ajtowns left a comment

RandyMcMillan commented Nov 29, 2023

fanquake commented Nov 29, 2023

Pttn commented Nov 29, 2023

naumenkogs left a comment

ariard left a comment

TheCharlatan commented Jan 3, 2024

sr-gi left a comment

sr-gi Jan 2, 2024 •

edited

sr-gi Jan 2, 2024

sr-gi Jan 2, 2024

sr-gi Jan 2, 2024

dergoegge commented Jan 22, 2024

stickies-v left a comment

stickies-v Jan 24, 2024

sr-gi commented Jan 26, 2024

stickies-v commented Jan 26, 2024

naumenkogs commented Jan 29, 2024

maflcko left a comment

stickies-v left a comment

achow101 commented Jan 31, 2024

TheCharlatan left a comment

ariard commented Feb 8, 2024

maflcko commented Feb 9, 2024

ariard commented Feb 12, 2024

		// Don't use timedata samples from inbound peers to make it
		// harder for others to tamper with our adjusted time.

Nuke adjusted time from validation (attempt 2) #28956

Nuke adjusted time from validation (attempt 2) #28956

Conversation

dergoegge commented Nov 28, 2023 • edited

DrahtBot commented Nov 28, 2023 • edited

Code Coverage

Reviews

Conflicts

dergoegge commented Nov 29, 2023

ajtowns left a comment

Choose a reason for hiding this comment

RandyMcMillan commented Nov 29, 2023

fanquake commented Nov 29, 2023

Pttn commented Nov 29, 2023

naumenkogs left a comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

TheCharlatan commented Jan 3, 2024

sr-gi left a comment

Choose a reason for hiding this comment

sr-gi Jan 2, 2024 • edited

Choose a reason for hiding this comment

sr-gi Jan 2, 2024

Choose a reason for hiding this comment

sr-gi Jan 2, 2024

Choose a reason for hiding this comment

sr-gi Jan 2, 2024

Choose a reason for hiding this comment

dergoegge commented Jan 22, 2024

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Jan 24, 2024

Choose a reason for hiding this comment

sr-gi commented Jan 26, 2024

stickies-v commented Jan 26, 2024

naumenkogs commented Jan 29, 2024

maflcko left a comment

Choose a reason for hiding this comment

stickies-v left a comment

Choose a reason for hiding this comment

achow101 commented Jan 31, 2024

TheCharlatan left a comment

Choose a reason for hiding this comment

ariard commented Feb 8, 2024

maflcko commented Feb 9, 2024

ariard commented Feb 12, 2024

dergoegge commented Nov 28, 2023 •

edited

DrahtBot commented Nov 28, 2023 •

edited

sr-gi Jan 2, 2024 •

edited