diff options
author | Ava Chow <github@achow101.com> | 2024-06-17 17:09:18 -0400 |
---|---|---|
committer | Ava Chow <github@achow101.com> | 2024-06-17 17:09:18 -0400 |
commit | ddf2ebd4659e926c5c6ee6726511b0b88bf7c2e0 (patch) | |
tree | b58d747b2676dff61342636a0faf742847981214 /src/node | |
parent | d97ddbe797f5b8b3bca0ee71b692e542b8990195 (diff) | |
parent | 260f8da71a35232d859d7705861fc1a88bfbbe81 (diff) | |
download | bitcoin-ddf2ebd4659e926c5c6ee6726511b0b88bf7c2e0.tar.xz |
Merge bitcoin/bitcoin#30058: Encapsulate warnings in generalized node::Warnings and remove globals
260f8da71a35232d859d7705861fc1a88bfbbe81 refactor: remove warnings globals (stickies-v)
9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f node: update uiInterface whenever warnings updated (stickies-v)
b071ad9770b7ae7fc718dcbfdc8f62dffbf6cfee introduce and use the generalized `node::Warnings` interface (stickies-v)
20e616f86444d00712ac7eb840666e2b0378af4a move-only: move warnings from common to node (stickies-v)
bed29c481aebeb2b0160450c63c03cc68fb89bc6 refactor: remove unnecessary AppendWarning helper function (stickies-v)
Pull request description:
This PR:
- moves warnings from common to the node library and into the node namespace (as suggested in https://github.com/bitcoin/bitcoin/pull/29845#discussion_r1570069541)
- generalizes the warnings interface to `Warnings::Set()` and `Warnings::Unset()` methods, instead of having a separate function and globals for each warning. As a result, this simplifies the `kernel::Notifications` interface.
- removes warnings.cpp from the kernel library
- removes warning globals
- adds testing for the warning logic
Behaviour change introduced:
- the `-alertnotify` command is executed for all `KernelNotifications::warningSet` calls, which now also covers the `LARGE_WORK_INVALID_CHAIN` warning
- the GUI is updated automatically whenever a warning is (un)set, covering some code paths where it previously wouldn't be, e.g. when `node::AbortNode()` is called, or for the `LARGE_WORK_INVALID_CHAIN` warning
Some discussion points:
- ~is `const std::string& id` the best way to refer to warnings? Enums are an obvious alternative, but since we need to define warnings across libraries, strings seem like a straightforward solution.~ _edit: updated approach to use `node::Warning` and `kernel::Warning` enums._
ACKs for top commit:
achow101:
ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
ryanofsky:
Code review ACK 260f8da71a35232d859d7705861fc1a88bfbbe81. Only change since last review was rebasing
TheCharlatan:
Re-ACK 260f8da71a35232d859d7705861fc1a88bfbbe81
Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588
Diffstat (limited to 'src/node')
-rw-r--r-- | src/node/abort.cpp | 7 | ||||
-rw-r--r-- | src/node/abort.h | 3 | ||||
-rw-r--r-- | src/node/context.cpp | 1 | ||||
-rw-r--r-- | src/node/context.h | 3 | ||||
-rw-r--r-- | src/node/interfaces.cpp | 5 | ||||
-rw-r--r-- | src/node/kernel_notifications.cpp | 29 | ||||
-rw-r--r-- | src/node/kernel_notifications.h | 13 | ||||
-rw-r--r-- | src/node/timeoffsets.cpp | 9 | ||||
-rw-r--r-- | src/node/timeoffsets.h | 10 | ||||
-rw-r--r-- | src/node/warnings.cpp | 68 | ||||
-rw-r--r-- | src/node/warnings.h | 90 |
11 files changed, 207 insertions, 31 deletions
diff --git a/src/node/abort.cpp b/src/node/abort.cpp index b727608384..8a17c41fd2 100644 --- a/src/node/abort.cpp +++ b/src/node/abort.cpp @@ -6,19 +6,18 @@ #include <logging.h> #include <node/interface_ui.h> +#include <node/warnings.h> #include <util/signalinterrupt.h> #include <util/translation.h> -#include <warnings.h> #include <atomic> #include <cstdlib> -#include <string> namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message) +void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings) { - SetMiscWarning(message); + if (warnings) warnings->Set(Warning::FATAL_INTERNAL_ERROR, message); InitError(_("A fatal internal error occurred, see debug.log for details: ") + message); exit_status.store(EXIT_FAILURE); if (shutdown && !(*shutdown)()) { diff --git a/src/node/abort.h b/src/node/abort.h index 1092279142..c881af4634 100644 --- a/src/node/abort.h +++ b/src/node/abort.h @@ -14,7 +14,8 @@ class SignalInterrupt; } // namespace util namespace node { -void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message); +class Warnings; +void AbortNode(util::SignalInterrupt* shutdown, std::atomic<int>& exit_status, const bilingual_str& message, node::Warnings* warnings); } // namespace node #endif // BITCOIN_NODE_ABORT_H diff --git a/src/node/context.cpp b/src/node/context.cpp index e32d21b383..da05fde6ee 100644 --- a/src/node/context.cpp +++ b/src/node/context.cpp @@ -13,6 +13,7 @@ #include <net_processing.h> #include <netgroup.h> #include <node/kernel_notifications.h> +#include <node/warnings.h> #include <policy/fees.h> #include <scheduler.h> #include <txmempool.h> diff --git a/src/node/context.h b/src/node/context.h index a7d92989dd..77838ea99b 100644 --- a/src/node/context.h +++ b/src/node/context.h @@ -39,6 +39,7 @@ class SignalInterrupt; namespace node { class KernelNotifications; +class Warnings; //! NodeContext struct containing references to chain state and connection //! state. @@ -81,6 +82,8 @@ struct NodeContext { //! Issues calls about blocks and transactions std::unique_ptr<ValidationSignals> validation_signals; std::atomic<int> exit_status{EXIT_SUCCESS}; + //! Manages all the node warnings + std::unique_ptr<node::Warnings> warnings; //! Declare default constructor and destructor that are not inline, so code //! instantiating the NodeContext struct doesn't need to #include class diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 9e10f4ca59..2b36f4ceae 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -32,6 +32,7 @@ #include <node/mini_miner.h> #include <node/transaction.h> #include <node/types.h> +#include <node/warnings.h> #include <policy/feerate.h> #include <policy/fees.h> #include <policy/policy.h> @@ -53,7 +54,6 @@ #include <util/translation.h> #include <validation.h> #include <validationinterface.h> -#include <warnings.h> #include <config/bitcoin-config.h> // IWYU pragma: keep @@ -93,7 +93,7 @@ public: explicit NodeImpl(NodeContext& context) { setContext(&context); } void initLogging() override { InitLogging(args()); } void initParameterInteraction() override { InitParameterInteraction(args()); } - bilingual_str getWarnings() override { return Join(GetWarnings(), Untranslated("<hr />")); } + bilingual_str getWarnings() override { return Join(Assert(m_context->warnings)->GetMessages(), Untranslated("<hr />")); } int getExitStatus() override { return Assert(m_context)->exit_status.load(); } uint32_t getLogCategories() override { return LogInstance().GetCategoryMask(); } bool baseInitialize() override @@ -101,6 +101,7 @@ public: if (!AppInitBasicSetup(args(), Assert(context())->exit_status)) return false; if (!AppInitParameterInteraction(args())) return false; + m_context->warnings = std::make_unique<node::Warnings>(); m_context->kernel = std::make_unique<kernel::Context>(); m_context->ecc_context = std::make_unique<ECC_Context>(); if (!AppInitSanityChecks(*m_context->kernel)) return false; diff --git a/src/node/kernel_notifications.cpp b/src/node/kernel_notifications.cpp index 44828b8adb..9894052a3a 100644 --- a/src/node/kernel_notifications.cpp +++ b/src/node/kernel_notifications.cpp @@ -10,15 +10,16 @@ #include <common/args.h> #include <common/system.h> #include <kernel/context.h> +#include <kernel/warning.h> #include <logging.h> #include <node/abort.h> #include <node/interface_ui.h> +#include <node/warnings.h> #include <util/check.h> #include <util/signalinterrupt.h> #include <util/strencodings.h> #include <util/string.h> #include <util/translation.h> -#include <warnings.h> #include <cstdint> #include <string> @@ -28,7 +29,6 @@ using util::ReplaceAll; static void AlertNotify(const std::string& strMessage) { - uiInterface.NotifyAlertChanged(); #if HAVE_SYSTEM std::string strCmd = gArgs.GetArg("-alertnotify", ""); if (strCmd.empty()) return; @@ -46,16 +46,6 @@ static void AlertNotify(const std::string& strMessage) #endif } -static void DoWarning(const bilingual_str& warning) -{ - static bool fWarned = false; - SetMiscWarning(warning); - if (!fWarned) { - AlertNotify(warning.original); - fWarned = true; - } -} - namespace node { kernel::InterruptResult KernelNotifications::blockTip(SynchronizationState state, CBlockIndex& index) @@ -80,20 +70,27 @@ void KernelNotifications::progress(const bilingual_str& title, int progress_perc uiInterface.ShowProgress(title.translated, progress_percent, resume_possible); } -void KernelNotifications::warning(const bilingual_str& warning) +void KernelNotifications::warningSet(kernel::Warning id, const bilingual_str& message) +{ + if (m_warnings.Set(id, message)) { + AlertNotify(message.original); + } +} + +void KernelNotifications::warningUnset(kernel::Warning id) { - DoWarning(warning); + m_warnings.Unset(id); } void KernelNotifications::flushError(const bilingual_str& message) { - AbortNode(&m_shutdown, m_exit_status, message); + AbortNode(&m_shutdown, m_exit_status, message, &m_warnings); } void KernelNotifications::fatalError(const bilingual_str& message) { node::AbortNode(m_shutdown_on_fatal_error ? &m_shutdown : nullptr, - m_exit_status, message); + m_exit_status, message, &m_warnings); } void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications) diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h index f4d97a0fff..e37f4d4e1e 100644 --- a/src/node/kernel_notifications.h +++ b/src/node/kernel_notifications.h @@ -15,18 +15,24 @@ class CBlockIndex; enum class SynchronizationState; struct bilingual_str; +namespace kernel { +enum class Warning; +} // namespace kernel + namespace util { class SignalInterrupt; } // namespace util namespace node { +class Warnings; static constexpr int DEFAULT_STOPATHEIGHT{0}; class KernelNotifications : public kernel::Notifications { public: - KernelNotifications(util::SignalInterrupt& shutdown, std::atomic<int>& exit_status) : m_shutdown(shutdown), m_exit_status{exit_status} {} + KernelNotifications(util::SignalInterrupt& shutdown, std::atomic<int>& exit_status, node::Warnings& warnings) + : m_shutdown(shutdown), m_exit_status{exit_status}, m_warnings{warnings} {} [[nodiscard]] kernel::InterruptResult blockTip(SynchronizationState state, CBlockIndex& index) override; @@ -34,7 +40,9 @@ public: void progress(const bilingual_str& title, int progress_percent, bool resume_possible) override; - void warning(const bilingual_str& warning) override; + void warningSet(kernel::Warning id, const bilingual_str& message) override; + + void warningUnset(kernel::Warning id) override; void flushError(const bilingual_str& message) override; @@ -47,6 +55,7 @@ public: private: util::SignalInterrupt& m_shutdown; std::atomic<int>& m_exit_status; + node::Warnings& m_warnings; }; void ReadNotificationArgs(const ArgsManager& args, KernelNotifications& notifications); diff --git a/src/node/timeoffsets.cpp b/src/node/timeoffsets.cpp index 62f527be8a..002c00d245 100644 --- a/src/node/timeoffsets.cpp +++ b/src/node/timeoffsets.cpp @@ -3,13 +3,12 @@ // file COPYING or http://www.opensource.org/licenses/mit-license.php. #include <logging.h> -#include <node/interface_ui.h> #include <node/timeoffsets.h> +#include <node/warnings.h> #include <sync.h> #include <tinyformat.h> #include <util/time.h> #include <util/translation.h> -#include <warnings.h> #include <algorithm> #include <chrono> @@ -49,8 +48,7 @@ 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(); + m_warnings.Unset(node::Warning::CLOCK_OUT_OF_SYNC); return false; } @@ -63,7 +61,6 @@ bool TimeOffsets::WarnIfOutOfSync() const "RPC methods to get more info." ), Ticks<std::chrono::minutes>(WARN_THRESHOLD))}; LogWarning("%s\n", msg.original); - SetMedianTimeOffsetWarning(msg); - uiInterface.NotifyAlertChanged(); + m_warnings.Set(node::Warning::CLOCK_OUT_OF_SYNC, msg); return true; } diff --git a/src/node/timeoffsets.h b/src/node/timeoffsets.h index 2b12584e12..eba706ac1e 100644 --- a/src/node/timeoffsets.h +++ b/src/node/timeoffsets.h @@ -11,8 +11,16 @@ #include <cstddef> #include <deque> +namespace node { +class Warnings; +} // namespace node + class TimeOffsets { +public: + TimeOffsets(node::Warnings& warnings) : m_warnings{warnings} {} + +private: //! 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. @@ -23,6 +31,8 @@ class TimeOffsets * positive offset means our peer's clock is ahead of our local clock. */ std::deque<std::chrono::seconds> m_offsets GUARDED_BY(m_mutex){}; + node::Warnings& m_warnings; + public: /** Add a new time offset sample. */ void Add(std::chrono::seconds offset) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); diff --git a/src/node/warnings.cpp b/src/node/warnings.cpp new file mode 100644 index 0000000000..b99c845900 --- /dev/null +++ b/src/node/warnings.cpp @@ -0,0 +1,68 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2022 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 <config/bitcoin-config.h> // IWYU pragma: keep + +#include <node/warnings.h> + +#include <common/system.h> +#include <node/interface_ui.h> +#include <sync.h> +#include <univalue.h> +#include <util/translation.h> + +#include <utility> +#include <vector> + +namespace node { +Warnings::Warnings() +{ + // Pre-release build warning + if (!CLIENT_VERSION_IS_RELEASE) { + m_warnings.insert( + {Warning::PRE_RELEASE_TEST_BUILD, + _("This is a pre-release test build - use at your own risk - do not use for mining or merchant applications")}); + } +} +bool Warnings::Set(warning_type id, bilingual_str message) +{ + LOCK(m_mutex); + const auto& [_, inserted]{m_warnings.insert({id, std::move(message)})}; + if (inserted) uiInterface.NotifyAlertChanged(); + return inserted; +} + +bool Warnings::Unset(warning_type id) +{ + auto success{WITH_LOCK(m_mutex, return m_warnings.erase(id))}; + if (success) uiInterface.NotifyAlertChanged(); + return success; +} + +std::vector<bilingual_str> Warnings::GetMessages() const +{ + LOCK(m_mutex); + std::vector<bilingual_str> messages; + messages.reserve(m_warnings.size()); + for (const auto& [id, msg] : m_warnings) { + messages.push_back(msg); + } + return messages; +} + +UniValue GetWarningsForRpc(const Warnings& warnings, bool use_deprecated) +{ + if (use_deprecated) { + const auto all_messages{warnings.GetMessages()}; + return all_messages.empty() ? "" : all_messages.back().original; + } + + UniValue messages{UniValue::VARR}; + for (auto&& message : warnings.GetMessages()) { + messages.push_back(std::move(message.original)); + } + return messages; +} +} // namespace node diff --git a/src/node/warnings.h b/src/node/warnings.h new file mode 100644 index 0000000000..24aeb8a922 --- /dev/null +++ b/src/node/warnings.h @@ -0,0 +1,90 @@ +// Copyright (c) 2009-2010 Satoshi Nakamoto +// Copyright (c) 2009-2021 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_WARNINGS_H +#define BITCOIN_NODE_WARNINGS_H + +#include <sync.h> +#include <util/translation.h> + +#include <map> +#include <variant> +#include <vector> + +class UniValue; + +namespace kernel { +enum class Warning; +} // namespace kernel + +namespace node { +enum class Warning { + CLOCK_OUT_OF_SYNC, + PRE_RELEASE_TEST_BUILD, + FATAL_INTERNAL_ERROR, +}; + +/** + * @class Warnings + * @brief Manages warning messages within a node. + * + * The Warnings class provides mechanisms to set, unset, and retrieve + * warning messages. It updates the GUI when warnings are changed. + * + * This class is designed to be non-copyable to ensure warnings + * are managed centrally. + */ +class Warnings +{ + typedef std::variant<kernel::Warning, node::Warning> warning_type; + + mutable Mutex m_mutex; + std::map<warning_type, bilingual_str> m_warnings GUARDED_BY(m_mutex); + +public: + Warnings(); + //! A warnings instance should always be passed by reference, never copied. + Warnings(const Warnings&) = delete; + Warnings& operator=(const Warnings&) = delete; + /** + * @brief Set a warning message. If a warning with the specified + * `id` is already active, false is returned and the new + * warning is ignored. If `id` does not yet exist, the + * warning is set, the UI is updated, and true is returned. + * + * @param[in] id Unique identifier of the warning. + * @param[in] message Warning message to be shown. + * + * @returns true if the warning was indeed set (i.e. there is no + * active warning with this `id`), otherwise false. + */ + bool Set(warning_type id, bilingual_str message) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** + * @brief Unset a warning message. If a warning with the specified + * `id` is active, it is unset, the UI is updated, and true + * is returned. Otherwise, no warning is unset and false is + * returned. + * + * @param[in] id Unique identifier of the warning. + * + * @returns true if the warning was indeed unset (i.e. there is an + * active warning with this `id`), otherwise false. + */ + bool Unset(warning_type id) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); + /** Return potential problems detected by the node, sorted by the + * warning_type id */ + std::vector<bilingual_str> GetMessages() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex); +}; + +/** + * RPC helper function that wraps warnings.GetMessages(). + * + * Returns a UniValue::VSTR with the latest warning if use_deprecated is + * set to true, or a UniValue::VARR with all warnings otherwise. + */ +UniValue GetWarningsForRpc(const Warnings& warnings, bool use_deprecated); +} // namespace node + +#endif // BITCOIN_NODE_WARNINGS_H |