Skip to content

Commit ee178df

Browse files
stickies-vstickies-v
committed
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.
1 parent 55361a1 commit ee178df

File tree

10 files changed

+234
-2
lines changed

10 files changed

+234
-2
lines changed

‎src/Makefile.am‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,7 @@ BITCOIN_CORE_H = \
233233
node/peerman_args.h \
234234
node/protocol_version.h \
235235
node/psbt.h \
236+
node/timeoffsets.h \
236237
node/transaction.h \
237238
node/txreconciliation.h \
238239
node/utxo_snapshot.h \
@@ -435,6 +436,7 @@ libbitcoin_node_a_SOURCES = \
435436
node/minisketchwrapper.cpp \
436437
node/peerman_args.cpp \
437438
node/psbt.cpp \
439+
node/timeoffsets.cpp \
438440
node/transaction.cpp \
439441
node/txreconciliation.cpp \
440442
node/utxo_snapshot.cpp \

‎src/Makefile.test.include‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -152,6 +152,7 @@ BITCOIN_TESTS =\
152152
test/sync_tests.cpp \
153153
test/system_tests.cpp \
154154
test/timedata_tests.cpp \
155+
test/timeoffsets_tests.cpp \
155156
test/torcontrol_tests.cpp \
156157
test/transaction_tests.cpp \
157158
test/translation_tests.cpp \
@@ -382,6 +383,7 @@ test_fuzz_fuzz_SOURCES = \
382383
test/fuzz/strprintf.cpp \
383384
test/fuzz/system.cpp \
384385
test/fuzz/timedata.cpp \
386+
test/fuzz/timeoffsets.cpp \
385387
test/fuzz/torcontrol.cpp \
386388
test/fuzz/transaction.cpp \
387389
test/fuzz/tx_in.cpp \

‎src/net_processing.cpp‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include <netbase.h>
2424
#include <netmessagemaker.h>
2525
#include <node/blockstorage.h>
26+
#include <node/timeoffsets.h>
2627
#include <node/txreconciliation.h>
2728
#include <policy/fees.h>
2829
#include <policy/policy.h>
@@ -753,6 +754,8 @@ class PeerManagerImpl final : public PeerManager
753754
/** Next time to check for stale tip */
754755
std::chrono::seconds m_stale_tip_check_time GUARDED_BY(cs_main){0s};
755756

757+
TimeOffsets m_outbound_time_offsets;
758+
756759
const Options m_opts;
757760

758761
bool RejectIncomingTxs(const CNode& peer) const;
@@ -3673,9 +3676,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
36733676

36743677
peer->m_time_offset = NodeSeconds{std::chrono::seconds{nTime}} - Now<NodeSeconds>();
36753678
if (!pfrom.IsInboundConn()) {
3676-
// Don't use timedata samples from inbound peers to make it
3677-
// harder for others to tamper with our adjusted time.
3679+
// Don't use time offset samples from inbound peers to make it
3680+
// harder for others to create false warnings about our clock being out of sync.
36783681
AddTimeData(pfrom.addr, Ticks<std::chrono::seconds>(peer->m_time_offset.load()));
3682+
m_outbound_time_offsets.Add(peer->m_time_offset);
3683+
m_outbound_time_offsets.WarnIfOutOfSync();
36793684
}
36803685

36813686
// If the peer is old enough to have the old alert system, send it the final alert.

‎src/node/timeoffsets.cpp‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// Copyright (c) 2024-present The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <logging.h>
6+
#include <node/interface_ui.h>
7+
#include <node/timeoffsets.h>
8+
#include <sync.h>
9+
#include <tinyformat.h>
10+
#include <util/time.h>
11+
#include <util/translation.h>
12+
#include <warnings.h>
13+
14+
#include <algorithm>
15+
#include <chrono>
16+
#include <cstdint>
17+
#include <deque>
18+
#include <limits>
19+
#include <optional>
20+
21+
using namespace std::chrono_literals;
22+
23+
void TimeOffsets::Add(std::chrono::seconds offset)
24+
{
25+
LOCK(m_mutex);
26+
27+
if (m_offsets.size() >= MAX_SIZE) {
28+
m_offsets.pop_front();
29+
}
30+
m_offsets.push_back(offset);
31+
LogDebug(BCLog::NET, "Added time offset %+ds, total samples %d\n",
32+
Ticks<std::chrono::seconds>(offset), m_offsets.size());
33+
}
34+
35+
std::chrono::seconds TimeOffsets::Median() const
36+
{
37+
LOCK(m_mutex);
38+
39+
// Only calculate the median if we have 5 or more offsets
40+
if (m_offsets.size() < 5) return 0s;
41+
42+
auto sorted_copy = m_offsets;
43+
std::sort(sorted_copy.begin(), sorted_copy.end());
44+
return sorted_copy[sorted_copy.size() / 2]; // approximate median is good enough, keep it simple
45+
}
46+
47+
bool TimeOffsets::WarnIfOutOfSync() const
48+
{
49+
// when median == std::numeric_limits<int64_t>::min(), calling std::chrono::abs is UB
50+
auto median{std::max(Median(), std::chrono::seconds(std::numeric_limits<int64_t>::min() + 1))};
51+
if (std::chrono::abs(median) <= WARN_THRESHOLD) {
52+
SetMedianTimeOffsetWarning(std::nullopt);
53+
uiInterface.NotifyAlertChanged();
54+
return false;
55+
}
56+
57+
bilingual_str msg{strprintf(_(
58+
"Your computer's date and time appear to be more than %d minutes out of sync with the network, "
59+
"this may lead to consensus failure. After you've confirmed your computer's clock, this message "
60+
"should no longer appear when you restart your node. Without a restart, it should stop showing "
61+
"automatically after you've connected to a sufficient number of new outbound peers, which may "
62+
"take some time. You can inspect the `timeoffset` field of the `getpeerinfo` and `getnetworkinfo` "
63+
"RPC methods to get more info."
64+
), Ticks<std::chrono::minutes>(WARN_THRESHOLD))};
65+
LogWarning("%s\n", msg.original);
66+
SetMedianTimeOffsetWarning(msg);
67+
uiInterface.NotifyAlertChanged();
68+
return true;
69+
}

‎src/node/timeoffsets.h‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
// Copyright (c) 2024-present The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#ifndef BITCOIN_NODE_TIMEOFFSETS_H
6+
#define BITCOIN_NODE_TIMEOFFSETS_H
7+
8+
#include <sync.h>
9+
10+
#include <chrono>
11+
#include <cstddef>
12+
#include <deque>
13+
14+
class TimeOffsets
15+
{
16+
//! Maximum number of timeoffsets stored.
17+
static constexpr size_t MAX_SIZE{50};
18+
//! Minimum difference between system and network time for a warning to be raised.
19+
static constexpr std::chrono::minutes WARN_THRESHOLD{10};
20+
21+
mutable Mutex m_mutex;
22+
/** The observed time differences between our local clock and those of our outbound peers. A
23+
* positive offset means our peer's clock is ahead of our local clock. */
24+
std::deque<std::chrono::seconds> m_offsets GUARDED_BY(m_mutex){};
25+
26+
public:
27+
/** Add a new time offset sample. */
28+
void Add(std::chrono::seconds offset) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
29+
30+
/** Compute and return the median of the collected time offset samples. The median is returned
31+
* as 0 when there are less than 5 samples. */
32+
std::chrono::seconds Median() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
33+
34+
/** Raise warnings if the median time offset exceeds the warnings threshold. Returns true if
35+
* warnings were raised. */
36+
bool WarnIfOutOfSync() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);
37+
};
38+
39+
#endif // BITCOIN_NODE_TIMEOFFSETS_H

‎src/test/fuzz/timeoffsets.cpp‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright (c) 2024-present The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+
#include <node/timeoffsets.h>
6+
#include <test/fuzz/FuzzedDataProvider.h>
7+
#include <test/fuzz/fuzz.h>
8+
#include <test/util/setup_common.h>
9+
10+
#include <chrono>
11+
#include <cstdint>
12+
#include <functional>
13+
14+
void initialize_timeoffsets()
15+
{
16+
static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);
17+
}
18+
19+
FUZZ_TARGET(timeoffsets, .init = initialize_timeoffsets)
20+
{
21+
FuzzedDataProvider fuzzed_data_provider(buffer.data(), buffer.size());
22+
TimeOffsets offsets{};
23+
LIMITED_WHILE(fuzzed_data_provider.remaining_bytes() > 0, 4'000) {
24+
(void)offsets.Median();
25+
offsets.Add(std::chrono::seconds{fuzzed_data_provider.ConsumeIntegral<std::chrono::seconds::rep>()});
26+
offsets.WarnIfOutOfSync();
27+
}
28+
}

‎src/test/timeoffsets_tests.cpp‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,69 @@
1+
// Copyright (c) 2024-present The Bitcoin Core developers
2+
// Distributed under the MIT software license, see the accompanying
3+
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
//
5+
6+
#include <node/timeoffsets.h>
7+
#include <test/util/setup_common.h>
8+
9+
#include <boost/test/unit_test.hpp>
10+
11+
#include <chrono>
12+
#include <vector>
13+
14+
using namespace std::chrono_literals;
15+
16+
static void AddMulti(TimeOffsets& offsets, const std::vector<std::chrono::seconds>& to_add)
17+
{
18+
for (auto offset : to_add) {
19+
offsets.Add(offset);
20+
}
21+
}
22+
23+
BOOST_FIXTURE_TEST_SUITE(timeoffsets_tests, BasicTestingSetup)
24+
25+
BOOST_AUTO_TEST_CASE(timeoffsets)
26+
{
27+
TimeOffsets offsets{};
28+
BOOST_CHECK(offsets.Median() == 0s);
29+
30+
AddMulti(offsets, {{0s, -1s, -2s, -3s}});
31+
// median should be zero for < 5 offsets
32+
BOOST_CHECK(offsets.Median() == 0s);
33+
34+
offsets.Add(-4s);
35+
// we now have 5 offsets: [-4, -3, -2, -1, 0]
36+
BOOST_CHECK(offsets.Median() == -2s);
37+
38+
AddMulti(offsets, {4, 5s});
39+
// we now have 9 offsets: [-4, -3, -2, -1, 0, 5, 5, 5, 5]
40+
BOOST_CHECK(offsets.Median() == 0s);
41+
42+
AddMulti(offsets, {41, 10s});
43+
// the TimeOffsets is now at capacity with 50 offsets, oldest offsets is discarded for any additional offset
44+
BOOST_CHECK(offsets.Median() == 10s);
45+
46+
AddMulti(offsets, {25, 15s});
47+
// we now have 25 offsets of 10s followed by 25 offsets of 15s
48+
BOOST_CHECK(offsets.Median() == 15s);
49+
}
50+
51+
static bool IsWarningRaised(const std::vector<std::chrono::seconds>& check_offsets)
52+
{
53+
TimeOffsets offsets{};
54+
AddMulti(offsets, check_offsets);
55+
return offsets.WarnIfOutOfSync();
56+
}
57+
58+
59+
BOOST_AUTO_TEST_CASE(timeoffsets_warning)
60+
{
61+
BOOST_CHECK(IsWarningRaised({{-60min, -40min, -30min, 0min, 10min}}));
62+
BOOST_CHECK(IsWarningRaised({5, 11min}));
63+
64+
BOOST_CHECK(!IsWarningRaised({4, 60min}));
65+
BOOST_CHECK(!IsWarningRaised({100, 3min}));
66+
}
67+
68+
69+
BOOST_AUTO_TEST_SUITE_END()

‎src/warnings.cpp‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,13 @@
1414
#include <util/string.h>
1515
#include <util/translation.h>
1616

17+
#include <optional>
1718
#include <vector>
1819

1920
static GlobalMutex g_warnings_mutex;
2021
static bilingual_str g_misc_warnings GUARDED_BY(g_warnings_mutex);
2122
static bool fLargeWorkInvalidChainFound GUARDED_BY(g_warnings_mutex) = false;
23+
static std::optional<bilingual_str> g_timeoffset_warning GUARDED_BY(g_warnings_mutex){};
2224

2325
void SetMiscWarning(const bilingual_str& warning)
2426
{
@@ -32,6 +34,11 @@ void SetfLargeWorkInvalidChainFound(bool flag)
3234
fLargeWorkInvalidChainFound = flag;
3335
}
3436

37+
void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning)
38+
{
39+
LOCK(g_warnings_mutex);
40+
g_timeoffset_warning = warning;
41+
}
3542
bilingual_str GetWarnings(bool verbose)
3643
{
3744
bilingual_str warnings_concise;
@@ -56,6 +63,10 @@ bilingual_str GetWarnings(bool verbose)
5663
warnings_verbose.emplace_back(warnings_concise);
5764
}
5865

66+
if (g_timeoffset_warning) {
67+
warnings_verbose.emplace_back(g_timeoffset_warning.value());
68+
}
69+
5970
if (verbose) {
6071
return Join(warnings_verbose, Untranslated("<hr />"));
6172
}

‎src/warnings.h‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,12 +6,15 @@
66
#ifndef BITCOIN_WARNINGS_H
77
#define BITCOIN_WARNINGS_H
88

9+
#include <optional>
910
#include <string>
1011

1112
struct bilingual_str;
1213

1314
void SetMiscWarning(const bilingual_str& warning);
1415
void SetfLargeWorkInvalidChainFound(bool flag);
16+
/** Pass std::nullopt to disable the warning */
17+
void SetMedianTimeOffsetWarning(std::optional<bilingual_str> warning);
1518
/** Format a string that describes several potential problems detected by the core.
1619
* @param[in] verbose bool
1720
* - if true, get all warnings separated by <hr />

‎test/functional/feature_maxtipage.py‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ def test_maxtipage(self, maxtipage, set_parameter=True, test_deltas=True):
4343
self.generate(node_miner, 1)
4444
assert_equal(node_ibd.getblockchaininfo()['initialblockdownload'], False)
4545

46+
# reset time to system time so we don't have a time offset with the ibd node the next
47+
# time we connect to it, ensuring TimeOffsets::WarnIfOutOfSync() doesn't output to stderr
48+
node_miner.setmocktime(0)
49+
4650
def run_test(self):
4751
self.log.info("Test IBD with maximum tip age of 24 hours (default).")
4852
self.test_maxtipage(DEFAULT_MAX_TIP_AGE, set_parameter=False)

0 commit comments

Comments
 (0)