Merge bitcoin/bitcoin#30058: Encapsulate warnings in generalized node::Warnings and remove globals

260f8da71a refactor: remove warnings globals (stickies-v)
9c4b0b7ce4 node: update uiInterface whenever warnings updated (stickies-v)
b071ad9770 introduce and use the generalized `node::Warnings` interface (stickies-v)
20e616f864 move-only: move warnings from common to node (stickies-v)
bed29c481a 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 260f8da71a
  ryanofsky:
    Code review ACK 260f8da71a. Only change since last review was rebasing
  TheCharlatan:
    Re-ACK 260f8da71a

Tree-SHA512: a3fcedaee0d3ad64e9c111aeb30665162f98e0e72acd6a70b76ff2ddf4f0a34da4f97ce353c322a1668ca6ee4d8a81cc6e6d170c5bbeb7a43cffdaf66646b588

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)()) {

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

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>

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

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;

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)
 {
-    DoWarning(warning);
+    if (m_warnings.Set(id, message)) {
+        AlertNotify(message.original);
+    }
+}
+
+void KernelNotifications::warningUnset(kernel::Warning id)
+{
+    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)

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);

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;
 }

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);

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

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