Simplify network-adjusted time warning logic #29623
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 <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); | ||
| 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 | ||
|
|
@@ -749,6 +753,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; | ||
stickies-v marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| const Options m_opts; | ||
|
|
||
| bool RejectIncomingTxs(const CNode& peer) const; | ||
|
|
@@ -1792,10 +1798,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) | ||
|
|
@@ -3666,12 +3681,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) : "")); | ||
|
|
||
| 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 | ||
|
There was a problem hiding this comment. After 2ef71c7, There was a problem hiding this comment. updated to There was a problem hiding this comment. I don't think this was covered (or it may have been regressed) There was a problem hiding this comment. Sorry, I think it must have regressed with this update in the next line - will fix if I have to retouch. |
||
| // 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. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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; | ||
|
There was a problem hiding this comment. e6d7bcf doesn't look like There was a problem hiding this comment. I think it is for There was a problem hiding this comment. I removed it and this seems to run fine, at least in my system There was a problem hiding this comment. We |
||
|
|
||
| void TimeOffsets::Add(std::chrono::seconds offset) | ||
stickies-v marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| 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 | ||
stickies-v marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (m_offsets.size() < 5) return 0s; | ||
|
|
||
| auto sorted_copy = m_offsets; | ||
stickies-v marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| 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); | ||
|
There was a problem hiding this comment. nit in 9f2904b: This may keep printing the warning, even if the user adjusted the clock, because the offsets are not discarded when the clock changes. There was a problem hiding this comment. The log message has been updated to highlight that the message may persist for a while, thanks. There was a problem hiding this comment. Grepping in my node's log for how frequent outbound connections are made - it would take about 1.5 days to make 200 new outbound connections. Since this is about the median, I guess 101 new connections after fixing the clock will be enough for us to stop the warning. That is like 20ish hours. Quite long. Can we do better? Currently we only save the difference between our time and the peer's time at the start of the connection and we have no clue that this difference has changed due to our clock being adjusted/corrected. If we save the value of the system_clock and steady_clock at the start of the connection, in addition to the peer's offset, at a later time we can deduce that our clock has been adjusted and add that diff/adjustment to the peer's offset to immediately figure out that "now our clock is in sync with that peer's clock". So, instead of Example:Lets say our clock is wrong, 15 minutes ahead. When we connect to a peer at There was a problem hiding this comment. Using both system and steady clock is an interesting idea and one I hadn't considered, thanks. I'm going to look into this a bit more and will follow up shortly.
FYI this PR reduces 200 -> 50 outbounds so your calculations are a bit off. |
||
| SetMedianTimeOffsetWarning(msg); | ||
| uiInterface.NotifyAlertChanged(); | ||
| return true; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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 |
Uh oh!
There was an error while loading. Please reload this page.