From ee178dfcc1175e0af8163216c9c024f4bfc97965 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Thu, 7 Mar 2024 11:23:00 +0000 Subject: [PATCH] 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. --- src/Makefile.am | 2 + src/Makefile.test.include | 2 + src/net_processing.cpp | 9 +++- src/node/timeoffsets.cpp | 69 ++++++++++++++++++++++++++++ src/node/timeoffsets.h | 39 ++++++++++++++++ src/test/fuzz/timeoffsets.cpp | 28 +++++++++++ src/test/timeoffsets_tests.cpp | 69 ++++++++++++++++++++++++++++ src/warnings.cpp | 11 +++++ src/warnings.h | 3 ++ test/functional/feature_maxtipage.py | 4 ++ 10 files changed, 234 insertions(+), 2 deletions(-) create mode 100644 src/node/timeoffsets.cpp create mode 100644 src/node/timeoffsets.h create mode 100644 src/test/fuzz/timeoffsets.cpp create mode 100644 src/test/timeoffsets_tests.cpp diff --git a/src/Makefile.am b/src/Makefile.am index 1f55bfbafda..383767a5a0a 100644 --- a/src/Makefile.am +++ b/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 \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index 9f9bdbbd0cd..fb703a0b8c5 100644 --- a/src/Makefile.test.include +++ b/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 \ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 363de480228..b549178677e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -753,6 +754,8 @@ private: /** 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(); 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(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. diff --git a/src/node/timeoffsets.cpp b/src/node/timeoffsets.cpp new file mode 100644 index 00000000000..62f527be8a6 --- /dev/null +++ b/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 +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +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(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::min(), calling std::chrono::abs is UB + auto median{std::max(Median(), std::chrono::seconds(std::numeric_limits::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(WARN_THRESHOLD))}; + LogWarning("%s\n", msg.original); + SetMedianTimeOffsetWarning(msg); + uiInterface.NotifyAlertChanged(); + return true; +} diff --git a/src/node/timeoffsets.h b/src/node/timeoffsets.h new file mode 100644 index 00000000000..2b12584e125 --- /dev/null +++ b/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 + +#include +#include +#include + +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 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 diff --git a/src/test/fuzz/timeoffsets.cpp b/src/test/fuzz/timeoffsets.cpp new file mode 100644 index 00000000000..019337a94a5 --- /dev/null +++ b/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 +#include +#include +#include + +#include +#include +#include + +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()}); + offsets.WarnIfOutOfSync(); + } +} diff --git a/src/test/timeoffsets_tests.cpp b/src/test/timeoffsets_tests.cpp new file mode 100644 index 00000000000..008f1a9db95 --- /dev/null +++ b/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 +#include + +#include + +#include +#include + +using namespace std::chrono_literals; + +static void AddMulti(TimeOffsets& offsets, const std::vector& 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& 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() diff --git a/src/warnings.cpp b/src/warnings.cpp index 84b021dad54..d55eecc48de 100644 --- a/src/warnings.cpp +++ b/src/warnings.cpp @@ -14,11 +14,13 @@ #include #include +#include #include 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 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 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("
")); } diff --git a/src/warnings.h b/src/warnings.h index b21e2ea2b8a..866bce6246f 100644 --- a/src/warnings.h +++ b/src/warnings.h @@ -6,12 +6,15 @@ #ifndef BITCOIN_WARNINGS_H #define BITCOIN_WARNINGS_H +#include #include struct bilingual_str; void SetMiscWarning(const bilingual_str& warning); void SetfLargeWorkInvalidChainFound(bool flag); +/** Pass std::nullopt to disable the warning */ +void SetMedianTimeOffsetWarning(std::optional warning); /** Format a string that describes several potential problems detected by the core. * @param[in] verbose bool * - if true, get all warnings separated by
diff --git a/test/functional/feature_maxtipage.py b/test/functional/feature_maxtipage.py index 51f37ef1e01..a1774a53953 100755 --- a/test/functional/feature_maxtipage.py +++ b/test/functional/feature_maxtipage.py @@ -43,6 +43,10 @@ class MaxTipAgeTest(BitcoinTestFramework): 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)