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.

Author: ryanofsky

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>
@@ -62,6 +63,7 @@ static std::vector<std::string> parseTestSetupArgs(const ArgsManager& argsman)
 
 int main(int argc, char** argv)
 {
+    BCLog::Logger logger;
     ArgsManager argsman;
     SetupBenchArgs(argsman);
     SHA256AutoDetect();

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>
@@ -1329,6 +1330,7 @@ static int CommandLineRPC(int argc, char *argv[])
 
 MAIN_FUNCTION
 {
+    BCLog::Logger logger;
     SetupEnvironment();
     if (!SetupNetworking()) {
         tfm::format(std::cerr, "Error: Initializing networking failed\n");

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>
@@ -857,6 +858,7 @@ static int CommandLineRawTx(int argc, char* argv[])
 
 MAIN_FUNCTION
 {
+    BCLog::Logger logger;
     SetupEnvironment();
 
     try {

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 {

src/bitcoin-wallet.cpp

@@ -95,6 +95,8 @@ MAIN_FUNCTION
 {
     ArgsManager& args = gArgs;
 
+    BCLog::Logger logger;
+
     int exit_status;
     std::unique_ptr<interfaces::Init> init = interfaces::MakeWalletInit(argc, argv, exit_status);
     if (!init) {

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 @@ static bool AppInit(NodeContext& node)
 
 MAIN_FUNCTION
 {
+    // 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);

src/kernel/bitcoinkernel.cpp

@@ -57,10 +57,16 @@ using util::ImmediateTaskRunner;
 // library aren't required to export this symbol
 extern const TranslateFn G_TRANSLATION_FUN{nullptr};
 
-static const kernel::Context btck_context_static{};
-
 namespace {
 
+// Log instance for kernel applications that don't create any logging
+// connections and don't have any logging.
+static BCLog::Logger& GlobalLogger()
+{
+    static BCLog::Logger g_logger;
+    return g_logger;
+}
+
 bool is_valid_flag_combination(script_verify_flags flags)
 {
     if (flags & SCRIPT_VERIFY_CLEANSTACK && ~flags & (SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS)) return false;
@@ -226,10 +232,7 @@ btck_Warning cast_btck_warning(kernel::Warning warning)
 }
 
 struct LoggingConnection {
-    // Reference to global log instance. This could be replaced with a
-    // per-connection instance (#30342) to give clients more granular control
-    // over logging.
-    BCLog::Logger& m_logger{LogInstance()};
+    BCLog::Logger m_logger;
     std::unique_ptr<std::list<std::function<void(const std::string&)>>::iterator> m_connection;
     void* m_user_data;
     std::function<void(void* user_data)> m_deleter;
@@ -374,7 +377,7 @@ struct ContextOptions {
 class Context
 {
 public:
-    BCLog::Logger* m_logger;
+    BCLog::Logger& m_logger;
 
     std::unique_ptr<kernel::Context> m_context;
 
@@ -389,18 +392,16 @@ class Context
     std::shared_ptr<KernelValidationInterface> m_validation_interface;
 
     Context(const ContextOptions* options, bool& sane)
-        : m_logger{options && options->m_logger ? options->m_logger : nullptr},
+        : m_logger{*(options && options->m_logger ? options->m_logger : &GlobalLogger())},
           m_context{std::make_unique<kernel::Context>()},
           m_interrupt{std::make_unique<util::SignalInterrupt>()}
     {
-        if (!m_logger) {
-            // For efficiency, disable logging globally instead of writing log
-            // messages to temporary buffer if no log callbacks are connected.
-            if (BCLog::Logger& logger{LogInstance()}; logger.NumConnections() == 0 && logger.Enabled()) {
-                logger.DisableLogging();
-            }
-        } else if (!m_logger->StartLogging()) {
-            throw std::runtime_error("Failed to start logging");
+        if (m_logger.NumConnections() > 0) {
+            if (!m_logger.StartLogging()) throw std::runtime_error("Failed to start logging");
+        } else if (m_logger.Enabled()) {
+            // For efficiency, disable logging instead of writing log messages
+            // to temporary buffer if no log callbacks are connected.
+            m_logger.DisableLogging();
         }
 
         if (options) {

src/kernel/bitcoinkernel.h

@@ -746,9 +746,7 @@ BITCOINKERNEL_API void btck_transaction_output_destroy(btck_TransactionOutput* t
 ///@{
 
 /**
- * @brief Set some options for the logger. Currently, this changes global
- * settings and will override settings for all existing @ref
- * btck_LoggingConnection instances.
+ * @brief Set some options for the logger.
  *
  * @param[in] logger  Non-null.
  * @param[in] options Sets formatting options of the log messages.
@@ -758,9 +756,7 @@ BITCOINKERNEL_API void btck_logging_set_options(btck_LoggingConnection* logger,
 /**
  * @brief Set the log level of the logger. This does not
  * enable the selected categories. Use @ref btck_logging_enable_category to
- * start logging from a specific, or all categories. Currently, this changes a global
- * setting and will override settings for all existing
- * @ref btck_LoggingConnection instances.
+ * start logging from a specific, or all categories.
  *
  * @param[in] logger   Non-null.
  * @param[in] category If btck_LogCategory_ALL is chosen, sets both the global fallback log level
@@ -774,19 +770,15 @@ BITCOINKERNEL_API void btck_logging_set_options(btck_LoggingConnection* logger,
 BITCOINKERNEL_API void btck_logging_set_level_category(btck_LoggingConnection* logger, btck_LogCategory category, btck_LogLevel level) BITCOINKERNEL_ARG_NONNULL(1);
 
 /**
- * @brief Enable a specific log category for the logger. Currently, this
- * changes a global setting and will override settings for all existing @ref
- * btck_LoggingConnection instances.
+ * @brief Enable a specific log category for the logger.
  *
  * @param[in] logger   Non-null.
  * @param[in] category If btck_LogCategory_ALL is chosen, all categories will be enabled.
  */
 BITCOINKERNEL_API void btck_logging_enable_category(btck_LoggingConnection* logger, btck_LogCategory category) BITCOINKERNEL_ARG_NONNULL(1);
 
 /**
- * @brief Disable a specific log category for the logger. Currently, this
- * changes a global setting and will override settings for all existing @ref
- * btck_LoggingConnection instances.
+ * @brief Disable a specific log category for the logger.
  *
  * @param[in] logger   Non-null.
  * @param[in] category If btck_LogCategory_ALL is chosen, all categories will be disabled.
@@ -800,10 +792,6 @@ BITCOINKERNEL_API void btck_logging_disable_category(btck_LoggingConnection* log
  * log options and @ref btck_context_options_set_logger to receive log messages
  * generated in a particular context.
  *
- * Currently, most log state is global, so log options applied to other streams
- * may effect this stream, and if there are multiple contexts, log messages from
- * other contexts may be received. These behaviors can be improved internally.
- *
  * @param[in] log_callback               Non-null, function through which messages will be logged.
  * @param[in] user_data                  Nullable, holds a user-defined opaque structure. Is passed back
  *                                       to the user through the callback. If the user_data_destroy_callback

src/logging.cpp

@@ -23,8 +23,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
@@ -40,17 +41,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);

src/logging.h

@@ -185,6 +185,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, SourceLocation&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
             EXCLUSIVE_LOCKS_REQUIRED(!m_cs);