refactor: remove warnings globals

stickies-v · stickies-v · commit 260f8da71a35 · 2024-06-13T11:20:49.000+01:00
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
@@ -17,6 +17,7 @@
 #include <kernel/context.h>
 #include <node/context.h>
 #include <node/interface_ui.h>
+#include <node/warnings.h>
 #include <noui.h>
 #include <util/check.h>
 #include <util/exception.h>
@@ -181,6 +182,8 @@ static bool AppInit(NodeContext& node)
             return false;
         }
 
+        node.warnings = std::make_unique<node::Warnings>();
+
         node.kernel = std::make_unique<kernel::Context>();
         node.ecc_context = std::make_unique<ECC_Context>();
         if (!AppInitSanityChecks(*node.kernel))
diff --git a/src/index/base.cpp b/src/index/base.cpp
@@ -30,7 +30,7 @@ template <typename... Args>
 void BaseIndex::FatalErrorf(const char* fmt, const Args&... args)
 {
     auto message = tfm::format(fmt, args...);
-    node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message));
+    node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get());
 }
 
 CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)
diff --git a/src/init.cpp b/src/init.cpp
@@ -1486,7 +1486,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 
     // ********************************************************* Step 7: load block chain
 
-    node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status);
+    node.notifications = std::make_unique<KernelNotifications>(*Assert(node.shutdown), node.exit_status, *Assert(node.warnings));
     ReadNotificationArgs(args, *node.notifications);
     ChainstateManager::Options chainman_opts{
         .chainparams = chainparams,
@@ -1639,7 +1639,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     assert(!node.peerman);
     node.peerman = PeerManager::make(*node.connman, *node.addrman,
                                      node.banman.get(), chainman,
-                                     *node.mempool, peerman_opts);
+                                     *node.mempool, *node.warnings,
+                                     peerman_opts);
     validation_signals.RegisterValidationInterface(node.peerman.get());
 
     // ********************************************************* Step 8: start indexers
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -25,6 +25,7 @@
 #include <node/blockstorage.h>
 #include <node/timeoffsets.h>
 #include <node/txreconciliation.h>
+#include <node/warnings.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <policy/settings.h>
@@ -489,7 +490,7 @@ class PeerManagerImpl final : public PeerManager
 public:
     PeerManagerImpl(CConnman& connman, AddrMan& addrman,
                     BanMan* banman, ChainstateManager& chainman,
-                    CTxMemPool& pool, Options opts);
+                    CTxMemPool& pool, node::Warnings& warnings, Options opts);
 
     /** Overridden from CValidationInterface. */
     void BlockConnected(ChainstateRole role, const std::shared_ptr<const CBlock>& pblock, const CBlockIndex* pindexConnected) override
@@ -790,7 +791,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;
+    node::Warnings& m_warnings;
+    TimeOffsets m_outbound_time_offsets{m_warnings};
 
     const Options m_opts;
 
@@ -2042,14 +2044,14 @@ std::optional<std::string> PeerManagerImpl::FetchBlock(NodeId peer_id, const CBl
 
 std::unique_ptr<PeerManager> PeerManager::make(CConnman& connman, AddrMan& addrman,
                                                BanMan* banman, ChainstateManager& chainman,
-                                               CTxMemPool& pool, Options opts)
+                                               CTxMemPool& pool, node::Warnings& warnings, Options opts)
 {
-    return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, opts);
+    return std::make_unique<PeerManagerImpl>(connman, addrman, banman, chainman, pool, warnings, opts);
 }
 
 PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
                                  BanMan* banman, ChainstateManager& chainman,
-                                 CTxMemPool& pool, Options opts)
+                                 CTxMemPool& pool, node::Warnings& warnings, Options opts)
     : m_rng{opts.deterministic_rng},
       m_fee_filter_rounder{CFeeRate{DEFAULT_MIN_RELAY_TX_FEE}, m_rng},
       m_chainparams(chainman.GetParams()),
@@ -2058,6 +2060,7 @@ PeerManagerImpl::PeerManagerImpl(CConnman& connman, AddrMan& addrman,
       m_banman(banman),
       m_chainman(chainman),
       m_mempool(pool),
+      m_warnings{warnings},
       m_opts{opts}
 {
     // While Erlay support is incomplete, it must be enabled explicitly via -txreconciliation.
diff --git a/src/net_processing.h b/src/net_processing.h
@@ -16,6 +16,10 @@ class CChainParams;
 class CTxMemPool;
 class ChainstateManager;
 
+namespace node {
+class Warnings;
+} // namespace node
+
 /** Whether transaction reconciliation protocol should be enabled by default. */
 static constexpr bool DEFAULT_TXRECONCILIATION_ENABLE{false};
 /** Default for -maxorphantx, maximum number of orphan transactions kept in memory */
@@ -73,7 +77,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
 
     static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,
                                              BanMan* banman, ChainstateManager& chainman,
-                                             CTxMemPool& pool, Options opts);
+                                             CTxMemPool& pool, node::Warnings& warnings, Options opts);
     virtual ~PeerManager() { }
 
     /**
diff --git a/src/node/abort.cpp b/src/node/abort.cpp
@@ -15,9 +15,9 @@
 
 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)
 {
-    g_warnings.Set(Warning::FATAL_INTERNAL_ERROR, 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
@@ -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
@@ -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
@@ -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
@@ -93,14 +93,15 @@ class NodeImpl : public Node
     explicit NodeImpl(NodeContext& context) { setContext(&context); }
     void initLogging() override { InitLogging(args()); }
     void initParameterInteraction() override { InitParameterInteraction(args()); }
-    bilingual_str getWarnings() override { return Join(node::g_warnings.GetMessages(), 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
     {
         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
@@ -72,25 +72,25 @@ void KernelNotifications::progress(const bilingual_str& title, int progress_perc
 
 void KernelNotifications::warningSet(kernel::Warning id, const bilingual_str& message)
 {
-    if (node::g_warnings.Set(id, message)) {
+    if (m_warnings.Set(id, message)) {
         AlertNotify(message.original);
     }
 }
 
 void KernelNotifications::warningUnset(kernel::Warning id)
 {
-    g_warnings.Unset(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)
diff --git a/src/node/kernel_notifications.h b/src/node/kernel_notifications.h
@@ -25,12 +25,14 @@ class SignalInterrupt;
 
 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;
 
@@ -53,6 +55,7 @@ class KernelNotifications : public kernel::Notifications
 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
@@ -48,7 +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) {
-        node::g_warnings.Unset(node::Warning::CLOCK_OUT_OF_SYNC);
+        m_warnings.Unset(node::Warning::CLOCK_OUT_OF_SYNC);
         return false;
     }
 
@@ -61,6 +61,6 @@ bool TimeOffsets::WarnIfOutOfSync() const
         "RPC methods to get more info."
     ), Ticks<std::chrono::minutes>(WARN_THRESHOLD))};
     LogWarning("%s\n", msg.original);
-    node::g_warnings.Set(node::Warning::CLOCK_OUT_OF_SYNC, msg);
+    m_warnings.Set(node::Warning::CLOCK_OUT_OF_SYNC, msg);
     return true;
 }
diff --git 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
@@ -17,8 +17,6 @@
 #include <vector>
 
 namespace node {
-Warnings g_warnings;
-
 Warnings::Warnings()
 {
     // Pre-release build warning
@@ -54,17 +52,17 @@ std::vector<bilingual_str> Warnings::GetMessages() const
     return messages;
 }
 
-UniValue GetWarningsForRpc(bool use_deprecated)
+UniValue GetWarningsForRpc(const Warnings& warnings, bool use_deprecated)
 {
     if (use_deprecated) {
-        const auto all_warnings{g_warnings.GetMessages()};
-        return all_warnings.empty() ? "" : all_warnings.back().original;
+        const auto all_messages{warnings.GetMessages()};
+        return all_messages.empty() ? "" : all_messages.back().original;
     }
 
-    UniValue warnings{UniValue::VARR};
-    for (auto&& warning : g_warnings.GetMessages()) {
-        warnings.push_back(std::move(warning.original));
+    UniValue messages{UniValue::VARR};
+    for (auto&& message : warnings.GetMessages()) {
+        messages.push_back(std::move(message.original));
     }
-    return warnings;
+    return messages;
 }
 } // namespace node
diff --git a/src/node/warnings.h b/src/node/warnings.h
@@ -7,13 +7,13 @@
 #define BITCOIN_NODE_WARNINGS_H
 
 #include <sync.h>
+#include <util/translation.h>
 
 #include <map>
 #include <variant>
 #include <vector>
 
 class UniValue;
-struct bilingual_str;
 
 namespace kernel {
 enum class Warning;
@@ -79,14 +79,12 @@ class Warnings
 };
 
 /**
- * RPC helper function that wraps g_warnings.GetMessages().
+ * 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(bool use_deprecated);
-
-extern Warnings g_warnings;
+UniValue GetWarningsForRpc(const Warnings& warnings, bool use_deprecated);
 } // namespace node
 
 #endif // BITCOIN_NODE_WARNINGS_H
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -1309,7 +1309,8 @@ RPCHelpMan getblockchaininfo()
         }
     }
 
-    obj.pushKV("warnings", node::GetWarningsForRpc(IsDeprecatedRPCEnabled("warnings")));
+    NodeContext& node = EnsureAnyNodeContext(request.context);
+    obj.pushKV("warnings", node::GetWarningsForRpc(*CHECK_NONFATAL(node.warnings), IsDeprecatedRPCEnabled("warnings")));
     return obj;
 },
     };
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
@@ -455,7 +455,7 @@ static RPCHelpMan getmininginfo()
     obj.pushKV("networkhashps",    getnetworkhashps().HandleRequest(request));
     obj.pushKV("pooledtx",         (uint64_t)mempool.size());
     obj.pushKV("chain", chainman.GetParams().GetChainTypeString());
-    obj.pushKV("warnings", node::GetWarningsForRpc(IsDeprecatedRPCEnabled("warnings")));
+    obj.pushKV("warnings", node::GetWarningsForRpc(*CHECK_NONFATAL(node.warnings), IsDeprecatedRPCEnabled("warnings")));
     return obj;
 },
     };
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
@@ -716,7 +716,7 @@ static RPCHelpMan getnetworkinfo()
         }
     }
     obj.pushKV("localaddresses", std::move(localAddresses));
-    obj.pushKV("warnings", node::GetWarningsForRpc(IsDeprecatedRPCEnabled("warnings")));
+    obj.pushKV("warnings", node::GetWarningsForRpc(*CHECK_NONFATAL(node.warnings), IsDeprecatedRPCEnabled("warnings")));
     return obj;
 },
     };
diff --git a/src/test/blockmanager_tests.cpp b/src/test/blockmanager_tests.cpp
@@ -27,7 +27,7 @@ BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
 BOOST_AUTO_TEST_CASE(blockmanager_find_block_pos)
 {
     const auto params {CreateChainParams(ArgsManager{}, ChainType::MAIN)};
-    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status};
+    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)};
     const BlockManager::Options blockman_opts{
         .chainparams = *params,
         .blocks_dir = m_args.GetBlocksDirPath(),
@@ -134,7 +134,7 @@ BOOST_FIXTURE_TEST_CASE(blockmanager_block_data_availability, TestChain100Setup)
 
 BOOST_AUTO_TEST_CASE(blockmanager_flush_block_file)
 {
-    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status};
+    KernelNotifications notifications{*Assert(m_node.shutdown), m_node.exit_status, *Assert(m_node.warnings)};
     node::BlockManager::Options blockman_opts{
         .chainparams = Params(),
         .blocks_dir = m_args.GetBlocksDirPath(),
diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp
diff --git a/src/test/fuzz/timeoffsets.cpp b/src/test/fuzz/timeoffsets.cpp
diff --git a/src/test/net_peer_connection_tests.cpp b/src/test/net_peer_connection_tests.cpp
diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp
diff --git a/src/test/timeoffsets_tests.cpp b/src/test/timeoffsets_tests.cpp
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
diff --git a/src/test/validation_chainstatemanager_tests.cpp b/src/test/validation_chainstatemanager_tests.cpp

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ template <typename... Args>`
`30`	`30`	`void BaseIndex::FatalErrorf(const char* fmt, const Args&... args)`
`31`	`31`	`{`
`32`	`32`	`auto message = tfm::format(fmt, args...);`
`33`		`- node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message));`
	`33`	`+ node::AbortNode(m_chain->context()->shutdown, m_chain->context()->exit_status, Untranslated(message), m_chain->context()->warnings.get());`
`34`	`34`	`}`
`35`	`35`
`36`	`36`	`CBlockLocator GetLocator(interfaces::Chain& chain, const uint256& block_hash)`