Add TimeOffsets helper class

This helper class is an alternative to CMedianFilter, but without a
lot of the special logic and exceptions that we needed while it was
still used for consensus.

Author: stickies-v

src/Makefile.am

@@ -233,6 +233,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 \
@@ -435,6 +436,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 \

src/Makefile.test.include

@@ -152,6 +152,7 @@ BITCOIN_TESTS =\
   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 \
@@ -382,6 +383,7 @@ test_fuzz_fuzz_SOURCES = \
  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/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>
@@ -753,6 +754,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;
@@ -3673,9 +3676,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
 
         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.
+            // Don't use time offset samples from inbound peers to make it
+            // harder for others to create false warnings about our clock being out of sync.
             AddTimeData(pfrom.addr, Ticks<std::chrono::seconds>(peer->m_time_offset.load()));
+            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/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

src/test/fuzz/timeoffsets.cpp

@@ -0,0 +1,28 @@
+// 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 <node/timeoffsets.h>
+#include <test/fuzz/FuzzedDataProvider.h>
+#include <test/fuzz/fuzz.h>
+#include <test/util/setup_common.h>
+
+#include <chrono>
+#include <cstdint>
+#include <functional>
+
+void initialize_timeoffsets()
+{
+    static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);
+}
+
+FUZZ_TARGET(timeoffsets, .init = initialize_timeoffsets)
+{
+    FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
+    TimeOffsets offsets{};
+    LIMITED_WHILE(fuzzed_data_provider.remaining_bytes() > 0, 4'000) {
+        (void)offsets.Median();
+        offsets.Add(std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<std::chrono::seconds::rep>()});
+        offsets.WarnIfOutOfSync();
+    }
+}

src/test/timeoffsets_tests.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 <node/timeoffsets.h>
+#include <test/util/setup_common.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <chrono>
+#include <vector>
+
+using namespace std::chrono_literals;
+
+static void AddMulti(TimeOffsets& offsets, const std::vector<std::chrono::seconds>& to_add)
+{
+    for (auto offset : to_add) {
+        offsets.Add(offset);
+    }
+}
+
+BOOST_FIXTURE_TEST_SUITE(timeoffsets_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(timeoffsets)
+{
+    TimeOffsets offsets{};
+    BOOST_CHECK(offsets.Median() == 0s);
+
+    AddMulti(offsets, {{0s, -1s, -2s, -3s}});
+    // median should be zero for < 5 offsets
+    BOOST_CHECK(offsets.Median() == 0s);
+
+    offsets.Add(-4s);
+    // we now have 5 offsets: [-4, -3, -2, -1, 0]
+    BOOST_CHECK(offsets.Median() == -2s);
+
+    AddMulti(offsets, {4, 5s});
+    // we now have 9 offsets: [-4, -3, -2, -1, 0, 5, 5, 5, 5]
+    BOOST_CHECK(offsets.Median() == 0s);
+
+    AddMulti(offsets, {41, 10s});
+    // the TimeOffsets is now at capacity with 50 offsets, oldest offsets is discarded for any additional offset
+    BOOST_CHECK(offsets.Median() == 10s);
+
+    AddMulti(offsets, {25, 15s});
+    // we now have 25 offsets of 10s followed by 25 offsets of 15s
+    BOOST_CHECK(offsets.Median() == 15s);
+}
+
+static bool IsWarningRaised(const std::vector<std::chrono::seconds>& check_offsets)
+{
+    TimeOffsets offsets{};
+    AddMulti(offsets, check_offsets);
+    return offsets.WarnIfOutOfSync();
+}
+
+
+BOOST_AUTO_TEST_CASE(timeoffsets_warning)
+{
+    BOOST_CHECK(IsWarningRaised({{-60min, -40min, -30min, 0min, 10min}}));
+    BOOST_CHECK(IsWarningRaised({5, 11min}));
+
+    BOOST_CHECK(!IsWarningRaised({4, 60min}));
+    BOOST_CHECK(!IsWarningRaised({100, 3min}));
+}
+
+
+BOOST_AUTO_TEST_SUITE_END()

src/warnings.cpp

@@ -14,11 +14,13 @@
 #include <util/string.h>
 #include <util/translation.h>
 
+#include <optional>
 #include <vector>
 
 static GlobalMutex g_warnings_mutex;
 static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex);
 static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false;
+static std::optional<bilingual_str> g_timeoffset_warning GUARDED_BY(g_warnings_mutex){};
 
 void SetMiscWarning(const bilingual_str& warning)
 {
@@ -32,6 +34,11 @@ void SetfLargeWorkInvalidChainFound(bool flag)
     fLargeWorkInvalidChainFound = flag;
 }
 
+void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning)
+{
+    LOCK(g_warnings_mutex);
+    g_timeoffset_warning = warning;
+}
 bilingual_str GetWarnings(bool verbose)
 {
     bilingual_str warnings_concise;
@@ -56,6 +63,10 @@ bilingual_str GetWarnings(bool verbose)
         warnings_verbose.emplace_back(warnings_concise);
     }
 
+    if (g_timeoffset_warning) {
+        warnings_verbose.emplace_back(g_timeoffset_warning.value());
+    }
+
     if (verbose) {
         return Join(warnings_verbose, Untranslated("<hr />"));
     }

src/warnings.h

@@ -6,12 +6,15 @@
 #ifndef BITCOIN_WARNINGS_H
 #define BITCOIN_WARNINGS_H
 
+#include <optional>
 #include <string>
 
 struct bilingual_str;
 
 void SetMiscWarning(const bilingual_str& warning);
 void SetfLargeWorkInvalidChainFound(bool flag);
+/** Pass std::nullopt to disable the warning */
+void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning);
 /** Format a string that describes several potential problems detected by the core.
  * @param[in] verbose bool
  * - if true, get all warnings separated by <hr />

test/functional/feature_maxtipage.py

@@ -43,6 +43,10 @@ def test_maxtipage(self, maxtipage, set_parameter=True, test_deltas=True):
         self.generate(node_miner, 1)
         assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], False)
 
+        # reset time to system time so we don't have a time offset with the ibd node the next
+        # time we connect to it, ensuring TimeOffsets::WarnIfOutOfSync() doesn't output to stderr
+        node_miner.setmocktime(0)
+
     def run_test(self):
         self.log.info("Test IBD with maximum tip age of 24 hours (default).")
         self.test_maxtipage(DEFAULT_MAX_TIP_AGE, set_parameter=False)