From 493dd26b75d0e35422b3230be3c72a0f96eb5e8a Mon Sep 17 00:00:00 2001
From: Ryan Ofsky <ryan@ofsky.org>
Date: Wed, 3 Jul 2024 14:19:20 -0400
Subject: [PATCH] kernel: Drop global Logger instance

Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger
instance. Instead allow kernel applications to initialize their own logging
instances that can be returned by LogInstance().

The LogInstance() function is not actually used for the majority of logging in
kernel code. Most kernel log output goes directly to BCLog::Logger instances
specified when kernel objects like ChainstateManager and CTxMemPool are
constructed, which allows applications to create multiple Logger instances and
control which log output is sent to them.

The only kernel code that does rely on LogInstance() is certain low level code in
the util library, like the RNG seeder, that is not passed a specific instance
and needs to rely on the global LogInstance() function.

Other code outside the kernel library uses LogInstance() heavily, and may
continue to do so. bitcoind and test code now just create a log instance before
the first LogInstance() call, which it returns, so all behavior is the same as
before.
---
 src/bench/bench_bitcoin.cpp       |  2 ++
 src/bitcoin-cli.cpp               |  2 ++
 src/bitcoin-tx.cpp                |  2 ++
 src/bitcoin-util.cpp              |  2 ++
 src/bitcoin-wallet.cpp            |  2 ++
 src/bitcoind.cpp                  |  4 ++++
 src/logging.cpp                   | 27 ++++++++++++++++++++-------
 src/logging.h                     |  3 +++
 src/qt/bitcoin.cpp                |  3 +++
 src/qt/test/test_main.cpp         |  2 ++
 src/test/blockfilter_tests.cpp    |  2 +-
 src/test/fuzz/block_index.cpp     |  4 ++--
 src/test/fuzz/fuzz.cpp            |  4 ++++
 src/test/fuzz/mini_miner.cpp      |  4 ++--
 src/test/fuzz/script_sigcache.cpp |  4 ++--
 src/test/reverselock_tests.cpp    |  2 +-
 src/test/scheduler_tests.cpp      |  3 ++-
 src/test/sync_tests.cpp           |  2 +-
 src/test/util/setup_common.cpp    |  4 +++-
 src/test/util/setup_common.h      |  2 +-
 20 files changed, 61 insertions(+), 19 deletions(-)

diff --git a/src/bench/bench_bitcoin.cpp b/src/bench/bench_bitcoin.cpp
index 88afe68a1a992..ce9ae69ef1d99 100644
--- a/src/bench/bench_bitcoin.cpp
+++ b/src/bench/bench_bitcoin.cpp
@@ -5,6 +5,7 @@
 #include <bench/bench.h>
 #include <common/args.h>
 #include <crypto/sha256.h>
+#include <logging.h>
 #include <tinyformat.h>
 #include <util/fs.h>
 #include <util/string.h>
@@ -76,6 +77,7 @@ static std::vector<std::string> parseTestSetupArgs(const ArgsManager& argsman)
 
 int main(int argc, char** argv)
 {
+    BCLog::Logger logger;
     ArgsManager argsman;
     SetupBenchArgs(argsman);
     SHA256AutoDetect();
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 5c5965245bc6a..17b703ad5dfd7 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -11,6 +11,7 @@
 #include <common/system.h>
 #include <compat/compat.h>
 #include <compat/stdin.h>
+#include <logging.h>
 #include <policy/feerate.h>
 #include <rpc/client.h>
 #include <rpc/mining.h>
@@ -1331,6 +1332,7 @@ MAIN_FUNCTION
     common::WinCmdLineArgs winArgs;
     std::tie(argc, argv) = winArgs.get();
 #endif
+    BCLog::Logger logger;
     SetupEnvironment();
     if (!SetupNetworking()) {
         tfm::format(std::cerr, "Error: Initializing networking failed\n");
diff --git a/src/bitcoin-tx.cpp b/src/bitcoin-tx.cpp
index a627c24e86839..ef3a310d4b733 100644
--- a/src/bitcoin-tx.cpp
+++ b/src/bitcoin-tx.cpp
@@ -14,6 +14,7 @@
 #include <consensus/consensus.h>
 #include <core_io.h>
 #include <key_io.h>
+#include <logging.h>
 #include <policy/policy.h>
 #include <primitives/transaction.h>
 #include <script/script.h>
@@ -864,6 +865,7 @@ static int CommandLineRawTx(int argc, char* argv[])
 
 MAIN_FUNCTION
 {
+    BCLog::Logger logger;
     SetupEnvironment();
 
     try {
diff --git a/src/bitcoin-util.cpp b/src/bitcoin-util.cpp
index ff2e4fb800560..2853a24ba7216 100644
--- a/src/bitcoin-util.cpp
+++ b/src/bitcoin-util.cpp
@@ -13,6 +13,7 @@
 #include <common/system.h>
 #include <compat/compat.h>
 #include <core_io.h>
+#include <logging.h>
 #include <streams.h>
 #include <util/exception.h>
 #include <util/strencodings.h>
@@ -152,6 +153,7 @@ static int Grind(const std::vector<std::string>& args, std::string& strPrint)
 MAIN_FUNCTION
 {
     ArgsManager& args = gArgs;
+    BCLog::Logger logger;
     SetupEnvironment();
 
     try {
diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
index 033cf7a0f3932..c5139aa9ad50e 100644
--- a/src/bitcoin-wallet.cpp
+++ b/src/bitcoin-wallet.cpp
@@ -104,6 +104,8 @@ MAIN_FUNCTION
     std::tie(argc, argv) = winArgs.get();
 #endif
 
+    BCLog::Logger logger;
+
     int exit_status;
     std::unique_ptr<interfaces::Init> init = interfaces::MakeWalletInit(argc, argv, exit_status);
     if (!init) {
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index 5d5f06127d213..c22cb4f3332ac 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -14,6 +14,7 @@
 #include <init.h>
 #include <interfaces/chain.h>
 #include <interfaces/init.h>
+#include <logging.h>
 #include <kernel/context.h>
 #include <node/context.h>
 #include <node/interface_ui.h>
@@ -259,6 +260,9 @@ MAIN_FUNCTION
     std::tie(argc, argv) = winArgs.get();
 #endif
 
+    // Intentionally leaked! See BCLog::g_logger description for rationale.
+    new BCLog::Logger;
+
     NodeContext node;
     int exit_status;
     std::unique_ptr<interfaces::Init> init = interfaces::MakeNodeInit(node, argc, argv, exit_status);
diff --git a/src/logging.cpp b/src/logging.cpp
index 5f055566ef5d8..3f8698faa76fa 100644
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@ -21,8 +21,9 @@ using util::RemovePrefixView;
 const char * const DEFAULT_DEBUGLOGFILE = "debug.log";
 constexpr auto MAX_USER_SETABLE_SEVERITY_LEVEL{BCLog::Level::Info};
 
-BCLog::Logger& LogInstance()
-{
+bool fLogIPs = DEFAULT_LOGIPS;
+
+namespace BCLog {
 /**
  * NOTE: the logger instances is leaked on exit. This is ugly, but will be
  * cleaned up by the OS/libc. Defining a logger as a global object doesn't work
@@ -38,17 +39,29 @@ BCLog::Logger& LogInstance()
  * This method of initialization was originally introduced in
  * ee3374234c60aba2cc4c5cd5cac1c0aefc2d817c.
  */
-    static BCLog::Logger* g_logger{new BCLog::Logger()};
-    return *g_logger;
-}
-
-bool fLogIPs = DEFAULT_LOGIPS;
+Logger* g_logger{nullptr};
+} // namespace BCLog
 
 static int FileWriteStr(std::string_view str, FILE *fp)
 {
     return fwrite(str.data(), 1, str.size(), fp);
 }
 
+BCLog::Logger& LogInstance()
+{
+    return *Assert(BCLog::g_logger);
+}
+
+BCLog::Logger::Logger()
+{
+    if (!g_logger) g_logger = this;
+}
+
+BCLog::Logger::~Logger()
+{
+    if (g_logger == this) g_logger = nullptr;
+}
+
 bool BCLog::Logger::StartLogging()
 {
     StdLockGuard scoped_lock(m_cs);
diff --git a/src/logging.h b/src/logging.h
index 9f5ff34dbe589..7af8cf10f2745 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -153,6 +153,9 @@ namespace BCLog {
         fs::path m_file_path;
         std::atomic<bool> m_reopen_file{false};
 
+        Logger();
+        ~Logger();
+
         /** Send a string to the log output */
         void LogPrintStr(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
             EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
diff --git a/src/qt/bitcoin.cpp b/src/qt/bitcoin.cpp
index 59eeeb02cd026..abd592542b1cb 100644
--- a/src/qt/bitcoin.cpp
+++ b/src/qt/bitcoin.cpp
@@ -492,6 +492,9 @@ int GuiMain(int argc, char* argv[])
     std::tie(argc, argv) = winArgs.get();
 #endif
 
+    // Intentionally leaked! See BCLog::g_logger description for rationale.
+    new BCLog::Logger;
+
     std::unique_ptr<interfaces::Init> init = interfaces::MakeGuiInit(argc, argv);
 
     SetupEnvironment();
diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp
index f3430c7822a49..01f69b5b3828c 100644
--- a/src/qt/test/test_main.cpp
+++ b/src/qt/test/test_main.cpp
@@ -6,6 +6,7 @@
 
 #include <interfaces/init.h>
 #include <interfaces/node.h>
+#include <logging.h>
 #include <qt/bitcoin.h>
 #include <qt/guiconstants.h>
 #include <qt/test/apptests.h>
@@ -37,6 +38,7 @@ const std::function<std::string()> G_TEST_GET_FULL_NAME{};
 // This is all you need to run all the tests
 int main(int argc, char* argv[])
 {
+    BCLog::Logger logger;
     // Initialize persistent globals with the testing setup state for sanity.
     // E.g. -datadir in gArgs is set to a temp directory dummy value (instead
     // of defaulting to the default datadir), or globalChainParams is set to
diff --git a/src/test/blockfilter_tests.cpp b/src/test/blockfilter_tests.cpp
index 470fdde30a3e6..f4d00ef43576d 100644
--- a/src/test/blockfilter_tests.cpp
+++ b/src/test/blockfilter_tests.cpp
@@ -16,7 +16,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-BOOST_AUTO_TEST_SUITE(blockfilter_tests)
+BOOST_FIXTURE_TEST_SUITE(blockfilter_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(gcsfilter_test)
 {
diff --git a/src/test/fuzz/block_index.cpp b/src/test/fuzz/block_index.cpp
index a2b1df8060afd..02f08a78e9be6 100644
--- a/src/test/fuzz/block_index.cpp
+++ b/src/test/fuzz/block_index.cpp
@@ -14,7 +14,7 @@
 
 namespace {
 
-const BasicTestingSetup* g_setup;
+BasicTestingSetup* g_setup;
 
 // Hardcoded block hash and nBits to make sure the blocks we store pass the pow check.
 uint256 g_block_hash;
@@ -46,7 +46,7 @@ CBlockHeader ConsumeBlockHeader(FuzzedDataProvider& provider)
 
 void init_block_index()
 {
-    static const auto testing_setup = MakeNoLogFileContext<>(ChainType::MAIN);
+    static const auto testing_setup = MakeNoLogFileContext<BasicTestingSetup>(ChainType::MAIN);
     g_setup = testing_setup.get();
     g_block_hash = Params().GenesisBlock().GetHash();
 }
diff --git a/src/test/fuzz/fuzz.cpp b/src/test/fuzz/fuzz.cpp
index 3c81e0f3fe8aa..65d4499e43dda 100644
--- a/src/test/fuzz/fuzz.cpp
+++ b/src/test/fuzz/fuzz.cpp
@@ -4,6 +4,7 @@
 
 #include <test/fuzz/fuzz.h>
 
+#include <logging.h>
 #include <netaddress.h>
 #include <netbase.h>
 #include <test/util/random.h>
@@ -103,6 +104,9 @@ void ResetCoverageCounters() {}
 
 void initialize()
 {
+    // Initialize logger first because RNG startup uses it.
+    static BCLog::Logger g_logger;
+
     // By default, make the RNG deterministic with a fixed seed. This will affect all
     // randomness during the fuzz test, except:
     // - GetStrongRandBytes(), which is used for the creation of private key material.
diff --git a/src/test/fuzz/mini_miner.cpp b/src/test/fuzz/mini_miner.cpp
index 4a183836f135c..f7be182644d5a 100644
--- a/src/test/fuzz/mini_miner.cpp
+++ b/src/test/fuzz/mini_miner.cpp
@@ -20,11 +20,11 @@
 
 namespace {
 
-const TestingSetup* g_setup;
+TestingSetup* g_setup;
 std::deque<COutPoint> g_available_coins;
 void initialize_miner()
 {
-    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    static const auto testing_setup = MakeNoLogFileContext<TestingSetup>();
     g_setup = testing_setup.get();
     for (uint32_t i = 0; i < uint32_t{100}; ++i) {
         g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);
diff --git a/src/test/fuzz/script_sigcache.cpp b/src/test/fuzz/script_sigcache.cpp
index fdb89c6ab2955..6b2aae3e7707e 100644
--- a/src/test/fuzz/script_sigcache.cpp
+++ b/src/test/fuzz/script_sigcache.cpp
@@ -19,12 +19,12 @@
 #include <vector>
 
 namespace {
-const BasicTestingSetup* g_setup;
+BasicTestingSetup* g_setup;
 } // namespace
 
 void initialize_script_sigcache()
 {
-    static const auto testing_setup = MakeNoLogFileContext<>();
+    static const auto testing_setup = MakeNoLogFileContext<BasicTestingSetup>();
     g_setup = testing_setup.get();
 }
 
diff --git a/src/test/reverselock_tests.cpp b/src/test/reverselock_tests.cpp
index 0a9ff5f294fc3..88ed7a1566731 100644
--- a/src/test/reverselock_tests.cpp
+++ b/src/test/reverselock_tests.cpp
@@ -9,7 +9,7 @@
 
 #include <stdexcept>
 
-BOOST_AUTO_TEST_SUITE(reverselock_tests)
+BOOST_FIXTURE_TEST_SUITE(reverselock_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(reverselock_basics)
 {
diff --git a/src/test/scheduler_tests.cpp b/src/test/scheduler_tests.cpp
index 95e725de46853..cba61c484cf78 100644
--- a/src/test/scheduler_tests.cpp
+++ b/src/test/scheduler_tests.cpp
@@ -4,6 +4,7 @@
 
 #include <random.h>
 #include <scheduler.h>
+#include <test/util/setup_common.h>
 #include <util/time.h>
 
 #include <boost/test/unit_test.hpp>
@@ -13,7 +14,7 @@
 #include <thread>
 #include <vector>
 
-BOOST_AUTO_TEST_SUITE(scheduler_tests)
+BOOST_FIXTURE_TEST_SUITE(scheduler_tests, BasicTestingSetup)
 
 static void microTask(CScheduler& s, std::mutex& mutex, int& counter, int delta, std::chrono::steady_clock::time_point rescheduleTime)
 {
diff --git a/src/test/sync_tests.cpp b/src/test/sync_tests.cpp
index 0576bf1633fbe..f5be31fcbba94 100644
--- a/src/test/sync_tests.cpp
+++ b/src/test/sync_tests.cpp
@@ -77,7 +77,7 @@ void TestInconsistentLockOrderDetected(MutexType& mutex1, MutexType& mutex2) NO_
 }
 } // namespace
 
-BOOST_AUTO_TEST_SUITE(sync_tests)
+BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
 {
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index 6027db0b40db8..cf1983cc315d1 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -78,6 +78,8 @@ constexpr inline auto TEST_DIR_PATH_ELEMENT{"test_common bitcoin"}; // Includes
 /** Random context to get unique temp data dirs. Separate from m_rng, which can be seeded from a const env var */
 static FastRandomContext g_rng_temp_path;
 static const bool g_rng_temp_path_init{[] {
+    // Make sure there is a global logger SeedStartup() can use.
+    BCLog::Logger logger;
     // Must be initialized before any SeedRandomForTest
     (void)g_rng_temp_path.rand64();
     return true;
@@ -106,7 +108,7 @@ static void ExitFailure(std::string_view str_err)
 }
 
 BasicTestingSetup::BasicTestingSetup(const ChainType chainType, TestOpts opts)
-    : m_logger{LogInstance()}, m_args{}
+    : m_args{}
 {
     m_node.shutdown_signal = &m_interrupt;
     m_node.shutdown_request = [this]{ return m_interrupt(); };
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index 7700784b94d6f..af046728e1db2 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -62,7 +62,7 @@ struct TestOpts {
  * This just configures logging, data dir and chain parameters.
  */
 struct BasicTestingSetup {
-    BCLog::Logger& m_logger;
+    BCLog::Logger m_logger;
     util::SignalInterrupt m_interrupt;
     node::NodeContext m_node; // keep as first member to be destructed last