Simplify network-adjusted time warning logic

[stickies-v]

An earlier approach in #28956 involved simplifying and refactoring the network-adjusted time calculation logic, but this was eventually left out 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

[DrahtBot]

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	sr-gi, achow101, dergoegge
Concept ACK	fanquake, theStack, glozow
Approach ACK	vasild
Stale ACK	alfonsoromanz, sipa

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:

• #29845 (rpc: return warnings as an array instead of just a single one by stickies-v)
• #29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
• #29494 (build: Assume HAVE_CONFIG_H, Add IWYU pragma keep to bitcoin-config.h includes by maflcko)
• #29346 (Stratum v2 Noise Protocol by Sjors)
• #29015 (kernel: Streamline util library by ryanofsky)
• #27052 (test: rpc: add last block announcement time to getpeerinfo result by LarryRuane)
• #26966 (index: blockfilter initial sync speedup, parallelize process by furszy)
• #15218 (validation: sync chainstate to disk after syncing to tip by andrewtoth)

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.

[DrahtBot]

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/22527379458}

[dergoegge]

Concept ACK

Thanks for picking this up!

[fanquake]

Concept ACK

[stickies-v]

Force pushed to:

• address a thread safety analysis issue by annotating WarnIfOutOfSync() with a EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
• fix failing functional test by resetting node_miner's mocktime before connecting to the next outbound
• rebase to latest master to fix native macos job

[alfonsoromanz]

Code review ACK 55b476b

I also:

• Ran the unit test, which is passing
• Ran the functional test, which is passing
• Verified that the peer window now shows the offset of each peer.

[stickies-v]

Force pushed to:

• remove dependency on C++20's std::ranges::sort which is not yet supported by the no wallet, libbitcoinkernel ci task
• add timeoffsets fuzz target as per review comment
• improve includes and copyright headers

Verified that the peer window now shows the offset of each peer.

FYI this is not new functionality, you should see the same behaviour on master.

[vasild]

Concept ACK, I love simplifications or refactors that are net removing code (+213 −363).

[ariard]

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 minutes (which is an arbitrary number).

on the 2 value selections (50 outbound peers):

• 50 sounds reasonable, to be fair best would be to add timestamp in m_outbound_time_offsets from FEELER connection, it’s an outbound connection type per se. shuffling things more at the AS-level.
• 20 min, i can suggest to pick up 10 min to match average interval-block-time of ~10 min

[sipa]

utACK 5c8e71c

[maflcko]

lgtm 2ef71c7 🛴

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 2ef71c73582be554e565ada3f8a6ca77c2cd79f1   🛴
Q9sgCN5S+/6cPhk6lYNjDonkzAs6Gm8QI7XynAlXDMCM29pQnMy67XGFMTPuBfZCMwdyVCo/S1sFWJI8l5xkCw==

[maflcko]

Reminder for myself: #29623 (comment)

[vasild]

Approach ACK 2ef71c7

Wrt adjusting the peer's offset with local clock corrections in order to stop the warnings immediately after the local clock is fixed (instead of after 4-5h) #29623 (comment), consider this:

[patch] adjust peer time with local clock corrections (untested)

diff --git i/src/node/timeoffsets.cpp w/src/node/timeoffsets.cpp
index 8488e47ff5..4dce6f9edd 100644
--- i/src/node/timeoffsets.cpp
+++ w/src/node/timeoffsets.cpp
@@ -24,27 +24,31 @@ void TimeOffsets::Add(std::chrono::seconds offset)
 {
     LOCK(m_mutex);
 
     if (m_offsets.size() >= N) {
         m_offsets.pop_front();
     }
-    m_offsets.push_back(offset);
+    m_offsets.emplace_back(offset, std::chrono::steady_clock::now(), std::chrono::system_clock::now());
     LogDebug(BCLog::NET, "Added time offset %+ds, total samples %d\n",
              Ticks<std::chrono::seconds>(offset), m_offsets.size());
 }
 
 std::chrono::seconds TimeOffsets::Median() const
 {
     LOCK(m_mutex);
 
     // Only calculate the median if we have 5 or more offsets
     if (m_offsets.size() < 5) return 0s;
 
+    const auto steady_now = std::chrono::steady_clock::now();
+    const auto system_now = std::chrono::system_clock::now();
     auto sorted_copy = m_offsets;
-    std::sort(sorted_copy.begin(), sorted_copy.end());
-    return sorted_copy[m_offsets.size() / 2];  // approximate median is good enough, keep it simple
+    std::sort(sorted_copy.begin(), sorted_copy.end(), [&](const peer_time_data& a, const peer_time_data& b) {
+        return a.Offset(steady_now, system_now) < b.Offset(steady_now, system_now);
+    });
+    return sorted_copy[m_offsets.size() / 2].Offset(steady_now, system_now);  // approximate median is good enough, keep it simple
 }
 
 bool TimeOffsets::WarnIfOutOfSync() const
 {
     // when median == std::numeric_limits<int64_t>::min(), calling std::chrono::abs is UB
     auto median{std::max(Median(), std::chrono::seconds(std::numeric_limits<int64_t>::min() + 1))};
diff --git i/src/node/timeoffsets.h w/src/node/timeoffsets.h
index 8bccae20e8..1d7a9f0153 100644
--- i/src/node/timeoffsets.h
+++ w/src/node/timeoffsets.h
@@ -21,13 +21,29 @@ private:
     //! Log the last time a GUI warning was raised to avoid flooding user with them
     mutable std::optional<std::chrono::steady_clock::time_point> m_warning_emitted GUARDED_BY(m_mutex){};
     //! Minimum time to wait before raising a new GUI warning
     static constexpr std::chrono::minutes m_warning_wait{60};
 
     mutable Mutex m_mutex;
-    std::deque<std::chrono::seconds> m_offsets GUARDED_BY(m_mutex){};
+    struct peer_time_data {
+        //! The difference between peer's time and our time, at the time of the P2P handshake.
+        std::chrono::seconds handshake_offset;
+        // The following two are used to detect system clock corrections/adjustments.
+        std::chrono::time_point<std::chrono::steady_clock> handshake_steady;
+        std::chrono::time_point<std::chrono::system_clock> handshake_system;
+
+        /// Calculate the offset as of now, taking into account any system clock corrections that
+        /// have happened since the peer connected.
+        std::chrono::seconds Offset(std::chrono::time_point<std::chrono::steady_clock> steady_now,
+                                    std::chrono::time_point<std::chrono::system_clock> system_now) const {
+            return handshake_offset +
+                std::chrono::duration_cast<std::chrono::seconds>(steady_now - handshake_steady) -
+                std::chrono::duration_cast<std::chrono::seconds>(system_now - handshake_system);
+        }
+    };
+    std::deque<peer_time_data> m_offsets GUARDED_BY(m_mutex){};
 
 public:
     /** Add a new time offset sample. */
     void Add(std::chrono::seconds offset) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
 
     /** Compute and return the median of the collected time offset samples. */

[glozow]

approach ACK

[stickies-v]

Thank you very much @sr-gi @maflcko @vasild @glozow @mzumsande for your extensive review.

Force pushed to address all review comments:

• in GUI: display the warning along with other warnings, instead of in a messagebox. This prevents the warning from Simplify network-adjusted time warning logic #29623 (comment) (and is a nicer approach in general)
• various nits around improved documentation, variable naming, references, help messages, ...
• I probably won't be addressing nits in this PR anymore to minimize review burden, so if there are any left (please keep posting them if you see any) I'll keep them for a follow-up.

Also, qt/guiutils.h still refers to CNodeCombinedStats.nTimeOffset

References updated, thanks

Wrt adjusting the peer's offset with local clock corrections in order to stop the warnings immediately after the local clock is fixed (instead of after 4-5h) #29623 (comment), consider this:

I like the idea of using steady_clock here, but I would prefer to do it in a follow-up if there's appetite for it. It's nice to have the warning disappear more quickly when the issue is resolved, but I think a downside is that the timeoffset peer stats in GUI and RPC will be inconsistent with the ones we use for the warning. I'd started implementing a PeerClock approach based on your suggestion, and I think it's quite elegant, but it's a lot more code overhaul so not really ideal for this PR. WIP commit at master...stickies-v:bitcoin:2024-03/remove-timedata-keep-clock

[stickies-v]

I think it is for if (m_offsets.size() < 5) return 0s;?

[sr-gi]

Re-ACK c6be144

[achow101]

reACK c6be144

[dergoegge]

utACK c6be144

[hebasto]

Ported to the CMake-based build system in hebasto#186.