diff options
author | Ava Chow <github@achow101.com> | 2024-04-30 18:41:45 -0400 |
---|---|---|
committer | Ava Chow <github@achow101.com> | 2024-04-30 18:49:34 -0400 |
commit | 0c3a3c9394e608c4beb92722ad034648af81dee7 (patch) | |
tree | fe416c62d4190f28dc0f8edbb9df9598612bcb5f /src/node | |
parent | d813ba1bc4b4da3ad1f3812b61ff125d1d664625 (diff) | |
parent | c6be144c4b774a03a8bcab5a165768cf81e9b06b (diff) | |
download | bitcoin-0c3a3c9394e608c4beb92722ad034648af81dee7.tar.xz |
Merge bitcoin/bitcoin#29623: Simplify network-adjusted time warning logic
c6be144c4b774a03a8bcab5a165768cf81e9b06b Remove timedata (stickies-v)
92e72b5d0d49aa395e626c238bc28aba8e4c3d44 [net processing] Move IgnoresIncomingTxs to PeerManagerInfo (dergoegge)
7d9c3ec622d73a98d07ab3cee78751718982a5bc [net processing] Introduce PeerManagerInfo (dergoegge)
ee178dfcc1175e0af8163216c9c024f4bfc97965 Add TimeOffsets helper class (stickies-v)
55361a15d1aa6984051441bce88112000688fb43 [net processing] Use std::chrono for type-safe time offsets (stickies-v)
038fd979effb54ee76ce1b7cf078e920c652326a [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](https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1904214370) 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](https://github.com/bitcoin/bitcoin/pull/29623/commits/c6be144c4b774a03a8bcab5a165768cf81e9b06b)
achow101:
reACK c6be144c4b774a03a8bcab5a165768cf81e9b06b
dergoegge:
utACK c6be144c4b774a03a8bcab5a165768cf81e9b06b
Tree-SHA512: 1063d639542e882186cdcea67d225ad1f97847f44253621a8c4b36c4d777e8f5cb0efe86bc279f01e819d33056ae4364c3300cc7400c087fb16c3f39b3e16b96
Diffstat (limited to 'src/node')
-rw-r--r-- | src/node/timeoffsets.cpp | 69 | ||||
-rw-r--r-- | src/node/timeoffsets.h | 39 |
2 files changed, 108 insertions, 0 deletions
diff --git a/src/node/timeoffsets.cpp b/src/node/timeoffsets.cpp new file mode 100644 index 0000000000..62f527be8a --- /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 <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; +} diff --git a/src/node/timeoffsets.h b/src/node/timeoffsets.h new file mode 100644 index 0000000000..2b12584e12 --- /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 <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 |