Merge bitcoin/bitcoin#29623: Simplify network-adjusted time warning logic

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](bitcoin/bitcoin#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](bitcoin/bitcoin@c6be144)
  achow101:
    reACK c6be144
  dergoegge:
    utACK c6be144

Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96

Author: achow101

src/Makefile.am

@@ -230,6 +230,7 @@ BITCOIN_CORE_H = \
   node/peerman_args.h \
   node/protocol_version.h \
   node/psbt.h \
+  node/timeoffsets.h \
   node/transaction.h \
   node/txreconciliation.h \
   node/utxo_snapshot.h \
@@ -280,7 +281,6 @@ BITCOIN_CORE_H = \
   support/lockedpool.h \
   sync.h \
   threadsafety.h \
-  timedata.h \
   torcontrol.h \
   txdb.h \
   txmempool.h \
@@ -434,6 +434,7 @@ libbitcoin_node_a_SOURCES = \
   node/minisketchwrapper.cpp \
   node/peerman_args.cpp \
   node/psbt.cpp \
+  node/timeoffsets.cpp \
   node/transaction.cpp \
   node/txreconciliation.cpp \
   node/utxo_snapshot.cpp \
@@ -461,7 +462,6 @@ libbitcoin_node_a_SOURCES = \
   rpc/txoutproof.cpp \
   script/sigcache.cpp \
   signet.cpp \
-  timedata.cpp \
   torcontrol.cpp \
   txdb.cpp \
   txmempool.cpp \

src/Makefile.test.include

@@ -153,7 +153,7 @@ BITCOIN_TESTS =\
   test/streams_tests.cpp \
   test/sync_tests.cpp \
   test/system_tests.cpp \
-  test/timedata_tests.cpp \
+  test/timeoffsets_tests.cpp \
   test/torcontrol_tests.cpp \
   test/transaction_tests.cpp \
   test/translation_tests.cpp \
@@ -384,7 +384,7 @@ test_fuzz_fuzz_SOURCES = \
  test/fuzz/string.cpp \
  test/fuzz/strprintf.cpp \
  test/fuzz/system.cpp \
- test/fuzz/timedata.cpp \
+ test/fuzz/timeoffsets.cpp \
  test/fuzz/torcontrol.cpp \
  test/fuzz/transaction.cpp \
  test/fuzz/tx_in.cpp \

src/addrman_impl.h

@@ -11,7 +11,6 @@
 #include <protocol.h>
 #include <serialize.h>
 #include <sync.h>
-#include <timedata.h>
 #include <uint256.h>
 #include <util/time.h>
 

src/init.cpp

@@ -67,7 +67,6 @@
 #include <scheduler.h>
 #include <script/sigcache.h>
 #include <sync.h>
-#include <timedata.h>
 #include <torcontrol.h>
 #include <txdb.h>
 #include <txmempool.h>
@@ -523,7 +522,6 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> automatic connections to peers (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxsendbuffer=<n>", strprintf("Maximum per-connection memory usage for the send buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXSENDBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-maxtimeadjustment", strprintf("Maximum allowed median peer time offset adjustment. Local perspective of time may be influenced by outbound peers forward or backward by this amount (default: %u seconds).", DEFAULT_MAX_TIME_ADJUSTMENT), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxuploadtarget=<n>", strprintf("Tries to keep outbound traffic under the given target per 24h. Limit does not apply to peers with 'download' permission or blocks created within past week. 0 = no limit (default: %s). Optional suffix units [k|K|m|M|g|G|t|T] (default: M). Lowercase is 1000 base while uppercase is 1024 base", DEFAULT_MAX_UPLOAD_TARGET), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
 #if HAVE_SOCKADDR_UN
     argsman.AddArg("-onion=<ip:port|path>", "Use separate SOCKS5 proxy to reach peers via Tor onion services, set -noonion to disable (default: -proxy). May be a local file path prefixed with 'unix:'.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

src/net.cpp

@@ -615,7 +615,6 @@ void CNode::CopyStats(CNodeStats& stats)
     X(m_last_tx_time);
     X(m_last_block_time);
     X(m_connected);
-    X(nTimeOffset);
     X(m_addr_name);
     X(nVersion);
     {

src/net.h

@@ -191,7 +191,6 @@ class CNodeStats
     std::chrono::seconds m_last_tx_time;
     std::chrono::seconds m_last_block_time;
     std::chrono::seconds m_connected;
-    int64_t nTimeOffset;
     std::string m_addr_name;
     int nVersion;
     std::string cleanSubVer;
@@ -703,7 +702,6 @@ class CNode
     std::atomic<std::chrono::seconds> m_last_recv{0s};
     //! Unix epoch time at peer connection
     const std::chrono::seconds m_connected;
-    std::atomic<int64_t> nTimeOffset{0};
     // Address of this peer
     const CAddress addr;
     // Bind address of our side of the connection

src/net_processing.cpp

@@ -23,6 +23,7 @@
 #include <netbase.h>
 #include <netmessagemaker.h>
 #include <node/blockstorage.h>
+#include <node/timeoffsets.h>
 #include <node/txreconciliation.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
@@ -34,7 +35,6 @@
 #include <scheduler.h>
 #include <streams.h>
 #include <sync.h>
-#include <timedata.h>
 #include <tinyformat.h>
 #include <txmempool.h>
 #include <txorphanage.h>
@@ -390,6 +390,10 @@ struct Peer {
     /** Whether this peer wants invs or headers (when possible) for block announcements */
     bool m_prefers_headers GUARDED_BY(NetEventsInterface::g_msgproc_mutex){false};
 
+    /** Time offset computed during the version handshake based on the
+     * timestamp the peer sent in the version message. */
+    std::atomic<std::chrono::seconds> m_time_offset{0s};
+
     explicit Peer(NodeId id, ServiceFlags our_services)
         : m_id{id}
         , m_our_services{our_services}
@@ -513,7 +517,7 @@ class PeerManagerImpl final : public PeerManager
     std::optional<std::string> FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
-    bool IgnoresIncomingTxs() override { return m_opts.ignore_incoming_txs; }
+    PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
     void SetBestBlock(int height, std::chrono::seconds time) override
@@ -788,6 +792,8 @@ class PeerManagerImpl final : public PeerManager
     /** Next time to check for stale tip */
     std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s};
 
+    TimeOffsets m_outbound_time_offsets;
+
     const Options m_opts;
 
     bool RejectIncomingTxs(const CNode& peer) const;
@@ -1864,10 +1870,19 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c
             stats.presync_height = peer->m_headers_sync->GetPresyncHeight();
         }
     }
+    stats.time_offset = peer->m_time_offset;
 
     return true;
 }
 
+PeerManagerInfo PeerManagerImpl::GetInfo() const
+{
+    return PeerManagerInfo{
+        .median_outbound_time_offset = m_outbound_time_offsets.Median(),
+        .ignores_incoming_txs = m_opts.ignore_incoming_txs,
+    };
+}
+
 void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
 {
     if (m_opts.max_extra_txs <= 0)
@@ -3861,12 +3876,12 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                   peer->m_starting_height, addrMe.ToStringAddrPort(), fRelay, pfrom.GetId(),
                   remoteAddr, (mapped_as ? strprintf(", mapped_as=%d", mapped_as) : ""));
 
-        int64_t nTimeOffset = nTime - GetTime();
-        pfrom.nTimeOffset = nTimeOffset;
+        peer->m_time_offset = NodeSeconds{std::chrono::seconds{nTime}} - Now<NodeSeconds>();
         if (!pfrom.IsInboundConn()) {
             // Don't use timedata samples from inbound peers to make it
-            // harder for others to tamper with our adjusted time.
-            AddTimeData(pfrom.addr, nTimeOffset);
+            // harder for others to create false warnings about our clock being out of sync.
+            m_outbound_time_offsets.Add(peer->m_time_offset);
+            m_outbound_time_offsets.WarnIfOutOfSync();
         }
 
         // If the peer is old enough to have the old alert system, send it the final alert.

src/net_processing.h

@@ -9,6 +9,8 @@
 #include <net.h>
 #include <validationinterface.h>
 
+#include <chrono>
+
 class AddrMan;
 class CChainParams;
 class CTxMemPool;
@@ -41,6 +43,12 @@ struct CNodeStateStats {
     bool m_addr_relay_enabled{false};
     ServiceFlags their_services;
     int64_t presync_height{-1};
+    std::chrono::seconds time_offset{0};
+};
+
+struct PeerManagerInfo {
+    std::chrono::seconds median_outbound_time_offset{0s};
+    bool ignores_incoming_txs{false};
 };
 
 class PeerManager : public CValidationInterface, public NetEventsInterface
@@ -83,8 +91,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
     /** Get statistics from node state */
     virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0;
 
-    /** Whether this node ignores txs received over p2p. */
-    virtual bool IgnoresIncomingTxs() = 0;
+    /** Get peer manager info. */
+    virtual PeerManagerInfo GetInfo() const = 0;
 
     /** Relay transaction to all peers. */
     virtual void RelayTransaction(const uint256& txid, const uint256& wtxid) = 0;

src/node/timeoffsets.cpp

@@ -0,0 +1,69 @@
+// Copyright (c) 2024-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <logging.h>
+#include <node/interface_ui.h>
+#include <node/timeoffsets.h>
+#include <sync.h>
+#include <tinyformat.h>
+#include <util/time.h>
+#include <util/translation.h>
+#include <warnings.h>
+
+#include <algorithm>
+#include <chrono>
+#include <cstdint>
+#include <deque>
+#include <limits>
+#include <optional>
+
+using namespace std::chrono_literals;
+
+void TimeOffsets::Add(std::chrono::seconds offset)
+{
+    LOCK(m_mutex);
+
+    if (m_offsets.size() >= MAX_SIZE) {
+        m_offsets.pop_front();
+    }
+    m_offsets.push_back(offset);
+    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;
+
+    auto sorted_copy = m_offsets;
+    std::sort(sorted_copy.begin(), sorted_copy.end());
+    return sorted_copy[sorted_copy.size() / 2];  // 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))};
+    if (std::chrono::abs(median) <= WARN_THRESHOLD) {
+        SetMedianTimeOffsetWarning(std::nullopt);
+        uiInterface.NotifyAlertChanged();
+        return false;
+    }
+
+    bilingual_str msg{strprintf(_(
+        "Your computer's date and time appear to be more than %d minutes out of sync with the network, "
+        "this may lead to consensus failure. After you've confirmed your computer's clock, this message "
+        "should no longer appear when you restart your node. Without a restart, it should stop showing "
+        "automatically after you've connected to a sufficient number of new outbound peers, which may "
+        "take some time. You can inspect the `timeoffset` field of the `getpeerinfo` and `getnetworkinfo` "
+        "RPC methods to get more info."
+    ), Ticks<std::chrono::minutes>(WARN_THRESHOLD))};
+    LogWarning("%s\n", msg.original);
+    SetMedianTimeOffsetWarning(msg);
+    uiInterface.NotifyAlertChanged();
+    return true;
+}

src/node/timeoffsets.h

@@ -0,0 +1,39 @@
+// Copyright (c) 2024-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_NODE_TIMEOFFSETS_H
+#define BITCOIN_NODE_TIMEOFFSETS_H
+
+#include <sync.h>
+
+#include <chrono>
+#include <cstddef>
+#include <deque>
+
+class TimeOffsets
+{
+    //! Maximum number of timeoffsets stored.
+    static constexpr size_t MAX_SIZE{50};
+    //! Minimum difference between system and network time for a warning to be raised.
+    static constexpr std::chrono::minutes WARN_THRESHOLD{10};
+
+    mutable Mutex m_mutex;
+    /** The observed time differences between our local clock and those of our outbound peers. A
+     * positive offset means our peer's clock is ahead of our local clock. */
+    std::deque<std::chrono::seconds> 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. The median is returned
+     * as 0 when there are less than 5 samples. */
+    std::chrono::seconds Median() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+
+    /** Raise warnings if the median time offset exceeds the warnings threshold. Returns true if
+     * warnings were raised. */
+    bool WarnIfOutOfSync() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
+};
+
+#endif // BITCOIN_NODE_TIMEOFFSETS_H