kernel: Simplify logging API

ryanofsky · ryanofsky · commit d0bd114e9745 · 2025-11-10T13:14:19.000-05:00
Current kernel logging API is too complicated and too restrictive. This PR tries to improves it. I'd expect an API that supported logging to allow creating log streams with different options and providing some way to specify which library operations should be logged to which streams. By contrast, the current API allows creating multiple log streams, but all the streams receive the same messages because options can only be set globally and the stream objects can't be passed to any kernel API functions. They are not even referenced by the `btck_Context` struct which holds other shared state. If no log streams are created, log messages are generated anyway, but they are stored in a 1MB buffer and not sent anywhere, unless a log stream is created later, at which point they are sent in bulk to that stream. More log streams can be created after that, but they only receive new messages, not the buffered ones. If log output is not needed, a btck_logging_disable() call is required to prevent log messages from accumulating in the 1MB buffer. Calling this will abort the program if any log streams were created before it was called, and also abort the program if any new log streams are created later. None of these behaviors seem necessary or ideal, and it would be better to provide a simpler logging API that allows creating a log stream, setting options on it, registering it with the `btck_Context` instance and receiving log messages from it. Another advantage of this approach is that it could allow (with #30342) different log streams to be used for different purposes, which would be not be possible with the current interface.
diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp
@@ -151,12 +151,12 @@ int main(int argc, char* argv[])
         .always_print_category_levels = true,
     };
 
-    logging_set_options(logging_options);
-
     Logger logger{std::make_unique<KernelLog>()};
+    logging_set_options(logger, logging_options);
 
     ContextOptions options{};
     ChainParams params{ChainType::MAINNET};
+    options.SetLogger(logger);
     options.SetChainParams(params);
 
     options.SetNotifications(std::make_shared<TestKernelNotifications>());
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
@@ -226,6 +226,10 @@ 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()};
     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;
@@ -234,18 +238,7 @@ struct LoggingConnection {
     {
         LOCK(cs_main);
 
-        auto connection{LogInstance().PushBackCallback([callback, user_data](const std::string& str) { callback(user_data, str.c_str(), str.length()); })};
-
-        // Only start logging if we just added the connection.
-        if (LogInstance().NumConnections() == 1 && !LogInstance().StartLogging()) {
-            LogError("Logger start failed.");
-            LogInstance().DeleteCallback(connection);
-            if (user_data && user_data_destroy_callback) {
-                user_data_destroy_callback(user_data);
-            }
-            throw std::runtime_error("Failed to start logging");
-        }
-
+        auto connection{m_logger.PushBackCallback([callback, user_data](const std::string& str) { callback(user_data, str.c_str(), str.length()); })};
         m_connection = std::make_unique<std::list<std::function<void(const std::string&)>>::iterator>(connection);
         m_user_data = user_data;
         m_deleter = user_data_destroy_callback;
@@ -257,15 +250,7 @@ struct LoggingConnection {
     {
         LOCK(cs_main);
         LogDebug(BCLog::KERNEL, "Logger disconnecting.");
-
-        // Switch back to buffering by calling DisconnectTestLogger if the
-        // connection that we are about to remove is the last one.
-        if (LogInstance().NumConnections() == 1) {
-            LogInstance().DisconnectTestLogger();
-        } else {
-            LogInstance().DeleteCallback(*m_connection);
-        }
-
+        m_logger.DeleteCallback(*m_connection);
         m_connection.reset();
         if (m_user_data && m_deleter) {
             m_deleter(m_user_data);
@@ -380,6 +365,7 @@ class KernelValidationInterface final : public CValidationInterface
 
 struct ContextOptions {
     mutable Mutex m_mutex;
+    BCLog::Logger* m_logger GUARDED_BY(m_mutex) {nullptr};
     std::unique_ptr<const CChainParams> m_chainparams GUARDED_BY(m_mutex);
     std::shared_ptr<KernelNotifications> m_notifications GUARDED_BY(m_mutex);
     std::shared_ptr<KernelValidationInterface> m_validation_interface GUARDED_BY(m_mutex);
@@ -388,6 +374,8 @@ struct ContextOptions {
 class Context
 {
 public:
+    BCLog::Logger* m_logger;
+
     std::unique_ptr<kernel::Context> m_context;
 
     std::shared_ptr<KernelNotifications> m_notifications;
@@ -401,9 +389,20 @@ class Context
     std::shared_ptr<KernelValidationInterface> m_validation_interface;
 
     Context(const ContextOptions* options, bool& sane)
-        : m_context{std::make_unique<kernel::Context>()},
+        : m_logger{options && options->m_logger ? options->m_logger : nullptr},
+          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 (options) {
             LOCK(options->m_mutex);
             if (options->m_chainparams) {
@@ -706,39 +705,38 @@ void btck_txid_destroy(btck_Txid* txid)
     delete txid;
 }
 
-void btck_logging_set_options(const btck_LoggingOptions options)
+void btck_logging_set_options(btck_LoggingConnection* logger, const btck_LoggingOptions options)
 {
+    BCLog::Logger& log{btck_LoggingConnection::get(logger).m_logger};
     LOCK(cs_main);
-    LogInstance().m_log_timestamps = options.log_timestamps;
-    LogInstance().m_log_time_micros = options.log_time_micros;
-    LogInstance().m_log_threadnames = options.log_threadnames;
-    LogInstance().m_log_sourcelocations = options.log_sourcelocations;
-    LogInstance().m_always_print_category_level = options.always_print_category_levels;
+    log.m_log_timestamps = options.log_timestamps;
+    log.m_log_time_micros = options.log_time_micros;
+    log.m_log_threadnames = options.log_threadnames;
+    log.m_log_sourcelocations = options.log_sourcelocations;
+    log.m_always_print_category_level = options.always_print_category_levels;
 }
 
-void btck_logging_set_level_category(btck_LogCategory category, btck_LogLevel level)
+void btck_logging_set_level_category(btck_LoggingConnection* logger, btck_LogCategory category, btck_LogLevel level)
 {
+    BCLog::Logger& log{btck_LoggingConnection::get(logger).m_logger};
     LOCK(cs_main);
     if (category == btck_LogCategory_ALL) {
-        LogInstance().SetLogLevel(get_bclog_level(level));
+        log.SetLogLevel(get_bclog_level(level));
     }
 
-    LogInstance().AddCategoryLogLevel(get_bclog_flag(category), get_bclog_level(level));
-}
-
-void btck_logging_enable_category(btck_LogCategory category)
-{
-    LogInstance().EnableCategory(get_bclog_flag(category));
+    log.AddCategoryLogLevel(get_bclog_flag(category), get_bclog_level(level));
 }
 
-void btck_logging_disable_category(btck_LogCategory category)
+void btck_logging_enable_category(btck_LoggingConnection* logger, btck_LogCategory category)
 {
-    LogInstance().DisableCategory(get_bclog_flag(category));
+    BCLog::Logger& log{btck_LoggingConnection::get(logger).m_logger};
+    log.EnableCategory(get_bclog_flag(category));
 }
 
-void btck_logging_disable()
+void btck_logging_disable_category(btck_LoggingConnection* logger, btck_LogCategory category)
 {
-    LogInstance().DisableLogging();
+    BCLog::Logger& log{btck_LoggingConnection::get(logger).m_logger};
+    log.DisableCategory(get_bclog_flag(category));
 }
 
 btck_LoggingConnection* btck_logging_connection_create(btck_LogCallback callback, void* user_data, btck_DestroyCallback user_data_destroy_callback)
@@ -792,6 +790,12 @@ btck_ContextOptions* btck_context_options_create()
     return btck_ContextOptions::create();
 }
 
+void btck_context_options_set_logger(btck_ContextOptions* options, btck_LoggingConnection* logger)
+{
+    LOCK(btck_ContextOptions::get(options).m_mutex);
+    btck_ContextOptions::get(options).m_logger = &btck_LoggingConnection::get(logger).m_logger;
+}
+
 void btck_context_options_set_chainparams(btck_ContextOptions* options, const btck_ChainParameters* chain_parameters)
 {
     // Copy the chainparams, so the caller can free it again
diff --git a/src/kernel/bitcoinkernel.h b/src/kernel/bitcoinkernel.h
@@ -120,14 +120,6 @@ typedef struct btck_TransactionOutput btck_TransactionOutput;
 
 /**
  * Opaque data structure for holding a logging connection.
- *
- * The logging connection can be used to manually stop logging.
- *
- * Messages that were logged before a connection is created are buffered in a
- * 1MB buffer. Logging can alternatively be permanently disabled by calling
- * @ref btck_logging_disable. Functions changing the logging settings are
- * global and change the settings for all existing btck_LoggingConnection
- * instances.
  */
 typedef struct btck_LoggingConnection btck_LoggingConnection;
 
@@ -690,31 +682,23 @@ BITCOINKERNEL_API void btck_transaction_output_destroy(btck_TransactionOutput* t
 ///@{
 
 /**
- * @brief This disables the global internal logger. No log messages will be
- * buffered internally anymore once this is called and the buffer is cleared.
- * This function should only be called once and is not thread or re-entry safe.
- * Log messages will be buffered until this function is called, or a logging
- * connection is created. This must not be called while a logging connection
- * already exists.
- */
-BITCOINKERNEL_API void btck_logging_disable();
-
-/**
- * @brief Set some options for the global internal logger. This changes global
+ * @brief Set some options for the logger. Currently, this changes global
  * settings and will override settings for all existing @ref
  * btck_LoggingConnection instances.
  *
+ * @param[in] logger  Non-null.
  * @param[in] options Sets formatting options of the log messages.
  */
-BITCOINKERNEL_API void btck_logging_set_options(const btck_LoggingOptions options);
+BITCOINKERNEL_API void btck_logging_set_options(btck_LoggingConnection* logger, const btck_LoggingOptions options) BITCOINKERNEL_ARG_NONNULL(1);
 
 /**
- * @brief Set the log level of the global internal logger. This does not
+ * @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. This changes a global
+ * 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.
  *
+ * @param[in] logger   Non-null.
  * @param[in] category If btck_LogCategory_ALL is chosen, sets both the global fallback log level
  *                     used by all categories that don't have a specific level set, and also
  *                     sets the log level for messages logged with the btck_LogCategory_ALL category itself.
@@ -723,25 +707,27 @@ BITCOINKERNEL_API void btck_logging_set_options(const btck_LoggingOptions option
 
  * @param[in] level    Log level at which the log category is set.
  */
-BITCOINKERNEL_API void btck_logging_set_level_category(btck_LogCategory category, btck_LogLevel level);
+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 global internal logger. This
+ * @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.
  *
+ * @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_LogCategory category);
+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 global internal logger. This
+ * @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.
  *
+ * @param[in] logger   Non-null.
  * @param[in] category If btck_LogCategory_ALL is chosen, all categories will be disabled.
  */
-BITCOINKERNEL_API void btck_logging_disable_category(btck_LogCategory category);
+BITCOINKERNEL_API void btck_logging_disable_category(btck_LoggingConnection* logger, btck_LogCategory category) BITCOINKERNEL_ARG_NONNULL(1);
 
 /**
  * @brief Start logging messages through the provided callback. Log messages
@@ -806,6 +792,16 @@ BITCOINKERNEL_API void btck_chain_parameters_destroy(btck_ChainParameters* chain
  */
 BITCOINKERNEL_API btck_ContextOptions* BITCOINKERNEL_WARN_UNUSED_RESULT btck_context_options_create();
 
+/**
+ * @brief Sets log instance for the kernel context to use.
+ *
+ * @param[in] context_options  Non-null, previously created by @ref btck_context_options_create.
+ * @param[in] logger           Is set to the context options.
+ */
+BITCOINKERNEL_API void btck_context_options_set_logger(
+    btck_ContextOptions* context_options,
+    btck_LoggingConnection* logger) BITCOINKERNEL_ARG_NONNULL(1, 2);
+
 /**
  * @brief Sets the chain params for the context options. The context created
  * with the options will be configured for these chain parameters.
diff --git a/src/kernel/bitcoinkernel_wrapper.h b/src/kernel/bitcoinkernel_wrapper.h
@@ -735,38 +735,13 @@ class Block : public Handle<btck_Block, btck_block_copy, btck_block_destroy>
     }
 };
 
-inline void logging_disable()
-{
-    btck_logging_disable();
-}
-
-inline void logging_set_options(const btck_LoggingOptions& logging_options)
-{
-    btck_logging_set_options(logging_options);
-}
-
-inline void logging_set_level_category(LogCategory category, LogLevel level)
-{
-    btck_logging_set_level_category(static_cast<btck_LogCategory>(category), static_cast<btck_LogLevel>(level));
-}
-
-inline void logging_enable_category(LogCategory category)
-{
-    btck_logging_enable_category(static_cast<btck_LogCategory>(category));
-}
-
-inline void logging_disable_category(LogCategory category)
-{
-    btck_logging_disable_category(static_cast<btck_LogCategory>(category));
-}
-
 template <typename T>
 concept Log = requires(T a, std::string_view message) {
     { a.LogMessage(message) } -> std::same_as<void>;
 };
 
 template <Log T>
-class Logger : UniqueHandle<btck_LoggingConnection, btck_logging_connection_destroy>
+class Logger : public UniqueHandle<btck_LoggingConnection, btck_logging_connection_destroy>
 {
 public:
     Logger(std::unique_ptr<T> log)
@@ -778,6 +753,30 @@ class Logger : UniqueHandle<btck_LoggingConnection, btck_logging_connection_dest
     }
 };
 
+template <Log T>
+void logging_set_options(Logger<T>& logger, const btck_LoggingOptions& logging_options)
+{
+    btck_logging_set_options(logger.get(), logging_options);
+}
+
+template <Log T>
+void logging_set_level_category(Logger<T>& logger, LogCategory category, LogLevel level)
+{
+    btck_logging_set_level_category(logger.get(), static_cast<btck_LogCategory>(category), static_cast<btck_LogLevel>(level));
+}
+
+template <Log T>
+void logging_enable_category(Logger<T>& logger, LogCategory category)
+{
+    btck_logging_enable_category(logger.get(), static_cast<btck_LogCategory>(category));
+}
+
+template <Log T>
+void logging_disable_category(Logger<T>& logger, LogCategory category)
+{
+    btck_logging_disable_category(logger.get(), static_cast<btck_LogCategory>(category));
+}
+
 class BlockTreeEntry : public View<btck_BlockTreeEntry>
 {
 public:
@@ -874,6 +873,12 @@ class ContextOptions : public UniqueHandle<btck_ContextOptions, btck_context_opt
 public:
     ContextOptions() : UniqueHandle{btck_context_options_create()} {}
 
+    template <Log T>
+    void SetLogger(Logger<T>& logger)
+    {
+        btck_context_options_set_logger(get(), logger.get());
+    }
+
     void SetChainParams(ChainParams& chain_params)
     {
         btck_context_options_set_chainparams(get(), chain_params.get());
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
@@ -534,20 +534,19 @@ BOOST_AUTO_TEST_CASE(logging_tests)
         .always_print_category_levels = true,
     };
 
-    logging_set_options(logging_options);
-    logging_set_level_category(LogCategory::BENCH, LogLevel::TRACE_LEVEL);
-    logging_disable_category(LogCategory::BENCH);
-    logging_enable_category(LogCategory::VALIDATION);
-    logging_disable_category(LogCategory::VALIDATION);
+    Logger logger{std::make_unique<TestLog>()};
+    logging_set_options(logger, logging_options);
+    logging_set_level_category(logger, LogCategory::BENCH, LogLevel::TRACE_LEVEL);
+    logging_disable_category(logger, LogCategory::BENCH);
+    logging_enable_category(logger, LogCategory::VALIDATION);
+    logging_disable_category(logger, LogCategory::VALIDATION);
 
-    // Check that connecting, connecting another, and then disconnecting and connecting a logger again works.
+    // Check that setting options on a different logger works.
     {
-        logging_set_level_category(LogCategory::KERNEL, LogLevel::TRACE_LEVEL);
-        logging_enable_category(LogCategory::KERNEL);
-        Logger logger{std::make_unique<TestLog>()};
         Logger logger_2{std::make_unique<TestLog>()};
+        logging_set_level_category(logger_2, LogCategory::KERNEL, LogLevel::TRACE_LEVEL);
+        logging_enable_category(logger_2, LogCategory::KERNEL);
     }
-    Logger logger{std::make_unique<TestLog>()};
 }
 
 BOOST_AUTO_TEST_CASE(btck_context_tests)
