kernel: use struct-based logging and simplify logging interface #34374

pull stickies-v wants to merge 6 commits into bitcoin:master from stickies-v:2026-01/kernel-logging-layering-predicate changing 8 files +464 −269
  1. stickies-v commented at 7:56 PM on January 21, 2026: contributor

    tl;dr: kernel logging is cumbersome. This PR delivers log entries as a struct instead of a formatted string, and simplifies the kernel logging interface. Closes #34062.

    Motivation

    The bitcoinkernel library (#27587) exposes functionality to interface with the kernel logging. This includes registering callbacks for log statements, level/category filtering, string formatting options, and more.

    Kernel logging has a few problems:

    • callbacks operate on formatted strings, so users need to parse the string to get the timestamp, category, level, ... based on which options are set. This is cumbersome, brittle, and inefficient.
    • the filtering interface is not really intuitive, requiring users to call combinations of btck_logging_set_level_category and btck_logging_enable_category when they want to produce debug or trace logs. The level/category system makes sense for node, because it directly controls what gets written to disk and stdout, and there are quite a lot more categories producing logs. Kernel doesn't really need this - users control what happens to the logs, and can do any filtering/manipulation in the callback they provide.
    • the node logging infrastructure has quite a bit more functionality than is necessary for a library, including ratelimiting, log formatting, outputting, buffering, ... This introduces unnecessary code and interface complexity.

    Approach

    Log generation (util/log.h: macros, util::log::Entry, and the ShouldDebugLog/ShouldTraceLog/Log hooks an application must provide) was already separated from log handling (logging.h) in #34465 and subsequent PRs. This PR gives bitcoinkernel its own implementation of those hooks, so it no longer depends on logging.cpp, and upgrades the C API to deliver struct-based entries. Node logging is not changed.

    1. Preparatory work: expose the missing levels and categories (WARNING, ERROR, TXPACKAGES, LOCK) and the btck_LogEntry struct, add Name() helpers to the C++ wrapper, and remove btck_logging_set_options and btck_logging_disable (with struct-based entries there is no kernel-side format to configure, and without buffering there is nothing to disable).
    2. Add KernelLogger: a kernel-owned backend that holds the registered callbacks and the minimum level, and converts each util::log::Entry into a btck_LogEntry. Introduced in a separate commit to keep the scope of the behaviour-changing commit smaller.
    3. Update the bitcoinkernel C API: btck_LogCallback receives a btck_LogEntry instead of a string, the util::log hooks are implemented on KernelLogger, logging.cpp is dropped from the kernel build, and the logging configuration interface is reduced to btck_logging_set_min_level().

    Behaviour changes for consumers (also described in each commit message):

    • Logging callbacks deliver structs instead of formatted strings, and the entire logging interface is simplified.
    • Filtering is levels-based only and can also filter Info and above. Debug/Trace apply to all categories, consumers can filter on btck_LogEntry::category (but string formatting is done for the entire level, instead of per enabled category).
    • Entries logged before the first connection are dropped instead of buffered.
    • Messages are delivered unescaped and without a trailing newline.

    Appendix

    bitcoinkernel C logging interface

    typedef struct {
        const char* message;       //!< Log message text (not null-terminated).
        size_t message_len;
        const char* thread_name;   //!< Name of the thread that produced the log message.
        size_t thread_name_len;
        int64_t timestamp_ns;      //!< Timestamp in nanoseconds since the Unix epoch.
        int64_t mocktime;          //!< Mock time in seconds since the Unix epoch, or 0 if not set.
        const char* file_name;     //!< Source file name.
        size_t file_name_len;
        const char* function_name; //!< Source function name.
        size_t function_name_len;
        uint32_t line;             //!< Source line number.
        btck_LogLevel level;       //!< Log severity level.
        btck_LogCategory category; //!< Log category.
    } btck_LogEntry;
    
    typedef void (*btck_LogCallback)(void* user_data, const btck_LogEntry* entry);
    
    BITCOINKERNEL_API void btck_logging_set_min_level(btck_LogLevel level);
    
    BITCOINKERNEL_API btck_LoggingConnection* BITCOINKERNEL_WARN_UNUSED_RESULT btck_logging_connection_create(
        btck_LogCallback log_callback,
        void* user_data,
        btck_DestroyCallback user_data_destroy_callback) BITCOINKERNEL_ARG_NONNULL(1);
    
    BITCOINKERNEL_API void btck_logging_connection_destroy(btck_LoggingConnection* logging_connection);
    
  2. DrahtBot added the label Validation on Jan 21, 2026
  3. DrahtBot commented at 7:56 PM on January 21, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34374.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    Approach ACK ryanofsky

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #36244 (validation, net: Process blocks asynchronously and reduce cs_main contention by w0xlt)
    • #36000 (validation: prefetch blocks while connecting by l0rinc)
    • #35641 (kernel: Add script evaluation tracer by sedited)
    • #35322 (logging: streamline Logger state and drop redundant methods by ryanofsky)
    • #34775 (kernel: make logging callback global by stickies-v)
    • #29700 (kernel, refactor: return error status on all fatal errors by ryanofsky)
    • #28690 (build: Introduce internal kernel library by sedited)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. stickies-v force-pushed on Jan 21, 2026
  5. DrahtBot added the label CI failed on Jan 21, 2026
  6. DrahtBot commented at 8:16 PM on January 21, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task tidy: https://github.com/bitcoin/bitcoin/actions/runs/21223722631/job/61065673932</sub> <sub>LLM reason (✨ experimental): Linker errors due to unresolved detail::InitLogger() from logging.cpp.o, causing the build/test script to fail.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  7. stickies-v force-pushed on Jan 21, 2026
  8. DrahtBot removed the label CI failed on Jan 21, 2026
  9. sedited added this to a project on Jan 22, 2026
  10. github-project-automation[bot] changed the project status on Jan 22, 2026
  11. sedited changed the project status on Jan 22, 2026
  12. stickies-v force-pushed on Jan 22, 2026
  13. in src/logging.h:317 in 0d075e3ff6
     319 | + * from a raw pointer to a std::unique_ptr.
     320 | + *
     321 | + * This method of initialization was originally introduced in
     322 | + * ee3374234c60aba2cc4c5cd5cac1c0aefc2d817c.
     323 | + */
     324 | +inline BCLog::Logger* g_logger{InitLogger()};
    


    ajtowns commented at 10:48 AM on January 23, 2026:

    This seems like a regression. With this approach you can't assume LogInfo called during global initialization won't be called prior to InitLogger(), as it now only needs to pass through GetDispatcher() and g_dispatcher().

    As far as I can see, g_dispatcher() should just have independent implementations for bitcoin-kernel vs bitcoind, bitcoin-qt, bitcoin-cli etc.


    stickies-v commented at 4:37 PM on January 23, 2026:

    Yes, I think that's a much better approach, thanks for the suggestion. I'll incorpocate that and update shortly.

    As far as I can see, g_dispatcher() should just have independent implementations for bitcoin-kernel vs bitcoind, bitcoin-qt, bitcoin-cli etc.

    I think it should suffice to declare g_dispatcher() in util/log.h, and have one implementation in bitcoinkernel.cpp, and one in logging.cpp (which just calls LogInstance().GetDispatcher()). All non-kernel binaries can (at least for now) just use the same implementation, I think? Lmk if I'm misunderstanding something.


    stickies-v commented at 8:37 PM on January 26, 2026:

    I've changed the approach to your suggestion, so marking this as resolved.

  14. in src/util/log.h:78 in 0d075e3ff6 outdated
      73 | +    //! Type for callbacks invoked for each log entry that passes filtering.
      74 | +    using Callback = std::function<void(const Entry&)>;
      75 | +    //! Type for opaque handles returned by RegisterCallback(), used to unregister.
      76 | +    using CallbackHandle = std::list<Callback>::iterator;
      77 | +    //! Type for predicates called before logging; returns true if entry should be dispatched.
      78 | +    using FilterFunc = std::function<bool(Level level, uint64_t category)>;
    


    ajtowns commented at 10:50 AM on January 23, 2026:

    Jumping through a std::function when we could just be checking an atomic bitfield doesn't seem like a good approach.


    stickies-v commented at 2:03 PM on January 23, 2026:

    when we could just be checking an atomic bitfield

    I think that's only possible if we either 1) have util::log::Dispatcher expose all of BCLog::Logger's category/level setting logic, or 2) make Dispatcher::WillLog virtual and let each logging sink create its own Dispatcher subtype. Were you thinking of another approach?

    I don't think either approach is warranted to justify the minimal overhead that std::function incurs. 1) would just make Dispatcher's scope way too big in my view, and 2) would require inheritance and still have some (although probably less) overhead from the virtual table lookup.


    ajtowns commented at 4:22 AM on January 28, 2026:

    I don't know that Dispatcher needs to make the category/levels modifiable -- that seems like something the g_dispatcher implementation could handle via marking them as protected in Dispatcher. They should be directly testable via Dispatcher, so that disabled debug/trace categories are trivial to test, without needing indirection.

    I think your latest implementation (a4e8f3c8d6763a95c1804fc40781b379e6127b66) is dangerously wrong, btw -- LogInfo() etc currently always evaluate their arguments, whereas you're making that conditional on LogAcceptCategory which calls WillLog which is conditional on Enabled().


    stickies-v commented at 12:36 PM on January 28, 2026:

    I think your latest implementation (a4e8f3c) is dangerously wrong, btw -- LogInfo() etc currently always evaluate their arguments, whereas you're making that conditional on LogAcceptCategory which calls WillLog which is conditional on Enabled().

    I didn't realize developer-notes.md says to only avoid relying on side-effects for LogDebug and LogTrace, I thought this was true for logging in general. I think it's very brittle that program execution potentially relies on side-effects in logging statements, and I think we should move away from that completely. A cursory search didn't show me any {Info,Warning,Error} statements that have such side-effects, but I agree that it's possible and that makes reviewing this PR much more difficult, so I'll revert that behaviour to minimize scope and leave room for a follow-up. Thanks for pointing this out.

    They should be directly testable via Dispatcher, so that disabled debug/trace categories are trivial to test, without needing indirection.

    I'm not sure I understand your concern here. They are testable via Dispatcher::WillLog(). Is it purely the indirection performance overhead you're worried about? When running the benches on main and on a4e8f3c8d6763a95c1804fc40781b379e6127b66 (with small change to run this->Enabled() after this->WillLogCategoryLevel(static_cast<BCLog::LogFlags>(cat), level) which I'll include in next force-push, it doens't look like we have anything to worry about?

    On master (34a5ecadd7203121b03ac692d22a752d2a364111):

    |               ns/op |                op/s |    err% |     total | benchmark
    |--------------------:|--------------------:|--------:|----------:|:----------
    |            1,837.04 |          544,353.35 |    2.6% |      0.01 | `LogWithDebug`
    |            1,926.63 |          519,041.49 |    0.2% |      0.01 | `LogWithThreadNames`
    |                2.81 |      355,468,404.18 |    3.1% |      0.01 | `LogWithoutDebug`
    |            1,828.42 |          546,919.86 |    1.1% |      0.01 | `LogWithoutThreadNames`
    |               18.31 |       54,600,726.18 |    1.1% |      0.01 | `LogWithoutWriteToFile`
    

    On a4e8f3c8d6763a95c1804fc40781b379e6127b66 (with small unpushed modification):

    |               ns/op |                op/s |    err% |     total | benchmark
    |--------------------:|--------------------:|--------:|----------:|:----------
    |            1,826.40 |          547,524.98 |    0.7% |      0.01 | `LogWithDebug`
    |            1,863.31 |          536,680.71 |    1.0% |      0.01 | `LogWithThreadNames`
    |                4.23 |      236,235,773.60 |    0.4% |      0.01 | `LogWithoutDebug`
    |            1,800.52 |          555,395.22 |    1.3% |      0.01 | `LogWithoutThreadNames`
    |               19.92 |       50,188,600.76 |    1.0% |      0.01 | `LogWithoutWriteToFile`
    

    stickies-v commented at 4:09 PM on January 28, 2026:

    Latest force-push (dca56e0237abb485edd1cfe870069e2d75f08e42) reverts the unconditional argument evaluation for Info and higher levels, and adds a unit test to ensure behaviour before and after this PR remains the same. Thanks for raising this. Would be nice to remove logging side-effects entirely in a follow-up (probably just a documentation change and maybe some fix-ups).


    ajtowns commented at 7:12 AM on January 31, 2026:

    I didn't realize developer-notes.md says to only avoid relying on side-effects for LogDebug and LogTrace, I thought this was true for logging in general. I think it's very brittle that program execution potentially relies on side-effects in logging statements,

    Would be nice to remove logging side-effects entirely in a follow-up

    The easiest way to ensure any potential/accidental side-effects don't make the behaviour brittle is to always evaluate the logging statements, which is what our code currently does -- it's similar logic as always evaluating assertions. Changing that is not a nice follow-up.


    stickies-v commented at 4:11 PM on September 10, 2026:

    Marking this as resolved, as the PR has changed a lot since then and the conversation no longer applies I think. The indirection is gone, and this PR now doesn't touch util/node logging (including logging statement evaluation) at all (except for a documentation change). Happy to re-open if I've missed anything.

  15. DrahtBot added the label Needs rebase on Jan 23, 2026
  16. stickies-v force-pushed on Jan 26, 2026
  17. stickies-v force-pushed on Jan 26, 2026
  18. DrahtBot added the label CI failed on Jan 26, 2026
  19. DrahtBot commented at 6:51 PM on January 26, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/21368395725/job/61506234805</sub> <sub>LLM reason (✨ experimental): IWYU reported include-what-you-use changes and caused the CI step to fail.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  20. stickies-v force-pushed on Jan 26, 2026
  21. DrahtBot removed the label Needs rebase on Jan 26, 2026
  22. stickies-v commented at 8:36 PM on January 26, 2026: contributor

    Force-pushed to:

    • address merge conflict from #33822
    • address @ajtowns comment: removed SetDispatcher and GetDispatcher and instead implemented different g_dispatcher functions for logging.cpp and bitcoinkernel.cpp. This required quite a bit of code re-organization, including splitting out the kernel API changes into multiple commits (one for structured logging, one for levels-based filtering) - which I think improve the overall commit organization, but will make range-diff reviews pretty hard for this force-push.
    • improved logging callback handling: checking for nullptr and catching exceptions.
    • use BCLog::Logger::Enabled() in the filter function
    • address drahtbot linter suggestions, incl typo and named args, and a couple of other small touch-ups here and there.
  23. DrahtBot removed the label CI failed on Jan 26, 2026
  24. purpleKarrot commented at 5:32 AM on January 27, 2026: contributor

    I doubt that there is an actual use case for multiple logging connections. The logger is global internally and can be handled as global by clients as well (no need for userdata). Consider modelling the logging API after set_terminate in the C++ standard library:

    typedef void (*btck_LogHandler)(btck_LogEntry const* entry);
    
    BITCOINKERNEL_API btck_LogHandler btck_set_log(btck_LogHandler handler);
    

    I also think that clients are able to apply their own filtering, so the btck_logging_set_min_level function can be removed. It is worth noting that this function was never designed to filter per-connection, which was leaking the implementation detail that the logger is global.

  25. in src/kernel/bitcoinkernel.cpp:71 in a4e8f3c8d6 outdated
      68 | +// static destruction.
      69 | +static KernelLogger& g_kernel_logger()
      70 | +{
      71 | +    static KernelLogger* p{new KernelLogger{}};
      72 | +    return *p;
      73 | +}
    


    purpleKarrot commented at 5:47 AM on January 27, 2026:

    I interpret that as "We have to use a singleton here because of other singletons". Now everybody should understand why singletons are viral and therefore should be forbidden. I have zero tolerance for such code.


    stickies-v commented at 11:24 AM on January 27, 2026:

    I have zero tolerance for such code.

    I'm sorry to hear. What do you suggest as an alternative?

  26. stickies-v commented at 11:28 AM on January 27, 2026: contributor

    I doubt that there is an actual use case for multiple logging connections.

    I think this concern is orthogonal to the changes proposed in this PR, so I think that conversation might be better had e.g. in #30342 ?

    I also think that clients are able to apply their own filtering, so the btck_logging_set_min_level function can be removed.

    They are, but I think it's reasonable to offer a (minimal) way for clients to avoid paying for overhead (mostly string formatting, lambda evaluation, and expensive operations like memory usage calculations) for logs from levels that will be dropped anyway. Keeping this as-is for now, happy to reconsider if this is a general point of contention.

  27. stickies-v force-pushed on Jan 28, 2026
  28. stickies-v commented at 4:12 PM on January 28, 2026: contributor

    Latest force-push:

    • reverts the unconditional argument evaluation for Info and higher levels, and adds a unit test to ensure behaviour before and after this PR remains the same. Addresses @ajtown's comment.
    • BCLog::Logger's callback now first evaluates WillLogCategoryLevel() before verifying the logger is Enabled(), so we can keep the operation lock-free when the category isn't disabled. No behaviour change, just a small optimization.
  29. DrahtBot added the label CI failed on Jan 28, 2026
  30. DrahtBot commented at 7:03 PM on January 28, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/21445810981/job/61765316670</sub> <sub>LLM reason (✨ experimental): IWYU failure: include-what-you-use detected issues and exited non-zero.</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  31. ryanofsky commented at 8:18 PM on January 28, 2026: contributor

    Code review a4e8f3c8d6763a95c1804fc40781b379e6127b66. Major concept and approach ACK. This seems thoughtfully implemented to be minimally disruptive to existing code while rationalizing the kernel logging API:

    • Providing stuctured log entries instead of strings that need to be parsed to get information like timestamps and source locations.
    • Dropping set_options function with obscure options like log_time_micros.
    • Dropping filtering functions like set_level_category enable_category disable_category that can interact in unexpected ways. Just providing basic level filtering, while leaving space to add other filtering options later.
    • Dropping logging_disable function that needs to be called by applications that don't use logging. (Note: #33847 also eliminates this).
    • Eliminating global logging functions not tied to logging connections (also addressed by #33847).

    Suggestion to remove extra overhead

    I do agree with AJ that requiring a call through a function pointer #34374 (review) just to detect whether to log something does not seem good. Fortunately, there should be no need to do this for bitcoin core logging, though we may want to preserve it as an option for kernel logging. a4e8f3c8d6763a95c1804fc40781b379e6127b66 already has a statically linked g_dispatcher() function with different implementations in kernel/bitcoinkernel.cpp and logging.cpp, so it would only need to add a second g_dispatcher_should_log hook, defined differently for kernel and non-kernel binaries to avoid std::function overhead:

    <details><summary>diff</summary> <p>

    --- a/src/kernel/bitcoinkernel.cpp
    +++ b/src/kernel/bitcoinkernel.cpp
    @@ -57,9 +57,7 @@
     
     struct KernelLogger {
         std::atomic<util::log::Level> min_level{util::log::Level::Info};
    -    util::log::Dispatcher dispatcher{[this](util::log::Level level, uint64_t) {
    -        return level >= min_level.load(std::memory_order_relaxed);
    -    }};
    +    util::log::Dispatcher dispatcher{this};
     };
     
     // Kernel logging state. Intentionally leaked to avoid use-after-destroy if logging occurs during
    @@ -75,6 +73,12 @@ util::log::Dispatcher& util::log::g_dispatcher()
         return g_kernel_logger().dispatcher;
     }
     
    +bool util::log::g_dispatcher_should_log(void* context, util::log::Level level, uint64_t category)
    +{
    +    KernelLogger& logger{*static_cast<KernelLogger*>(context)};
    +    return level >= logger.min_level.load(std::memory_order_relaxed);
    +}
    +
     using kernel::ChainstateRole;
     using util::ImmediateTaskRunner;
     
    --- a/src/logging.cpp
    +++ b/src/logging.cpp
    @@ -49,6 +49,12 @@ util::log::Dispatcher& util::log::g_dispatcher()
         return *LogInstance().GetDispatcher();
     }
     
    +bool util::log::g_dispatcher_should_log(void* context, util::log::Level level, uint64_t category)
    +{
    +    BCLog::Logger& logger{*static_cast<BCLog::Logger*>(context)};
    +    return logger.Enabled() && logger.WillLogCategoryLevel(static_cast<BCLog::LogFlags>(category), level);
    +}
    +
     bool fLogIPs = DEFAULT_LOGIPS;
     
     static int FileWriteStr(std::string_view str, FILE *fp)
    @@ -617,10 +623,7 @@ bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::stri
     
     BCLog::Logger::Logger()
     {
    -    auto filter_func = [this](util::log::Level level, uint64_t cat) {
    -        return this->Enabled() && this->WillLogCategoryLevel(static_cast<BCLog::LogFlags>(cat), level);
    -    };
    -    m_dispatcher = std::make_unique<util::log::Dispatcher>(filter_func);
    +    m_dispatcher = std::make_unique<util::log::Dispatcher>(this);
     
         m_callback_handle = m_dispatcher->RegisterCallback([this](const util::log::Entry& entry) {
             LogPrintStr(entry);
    --- a/src/test/util_log_tests.cpp
    +++ b/src/test/util_log_tests.cpp
    @@ -50,7 +50,7 @@ BOOST_AUTO_TEST_CASE(dispatcher_filter)
         // Use a simple level filter predicate to only log above a specified min_level
         Level min_level;
         auto level_filter = [&min_level](Level level, uint64_t) { return level >= min_level; };
    -    Dispatcher dispatcher{level_filter};
    +    Dispatcher dispatcher{nullptr, level_filter};
         int callback_count{0};
         auto handle = dispatcher.RegisterCallback([&](const Entry&) { ++callback_count; });
     
    --- a/src/util/log.h
    +++ b/src/util/log.h
    @@ -61,6 +61,9 @@ struct Entry {
         bool should_ratelimit{false}; //!< Hint for consumers if this entry should be ratelimited
     };
     
    +//! Early custom callback to determine whether log message should be produced.
    +bool g_dispatcher_should_log(void* context, Level level, uint64_t category);
    +
     /**
      * Dispatcher is responsible for producing logs. It forwards structured log entries to one or
      * multiple logging sinks (e.g. BCLog::Logger) through its registered callbacks.
    @@ -79,12 +82,13 @@ public:
     
         /**
          * Construct a Dispatcher.
    +     * [@param](/bitcoin-bitcoin/contributor/param/)[in] context Optional context passed to g_dispatcher_should_log()
          * [@param](/bitcoin-bitcoin/contributor/param/)[in] filter  Optional predicate to filter log entries before dispatch to minimize
          *                    overhead (e.g. string formatting) for entries that all of the sinks would
          *                    be discarding anyway (e.g. due to a low logging level).
          *                    If null, all entries are dispatched (when callbacks are registered).
          */
    -    Dispatcher(FilterFunc filter = nullptr) : m_filter{std::move(filter)} {}
    +    Dispatcher(void* context = nullptr, FilterFunc filter = nullptr) : m_context{context}, m_filter{std::move(filter)} {}
     
         /**
          * Register a callback to receive log entries.
    @@ -103,7 +107,7 @@ public:
         bool Enabled() const { return m_callback_count.load(std::memory_order_acquire) > 0; }
     
         /** [@return](/bitcoin-bitcoin/contributor/return/) true if Enabled() and the filter (if set) passes for the provided level and category. */
    -    bool WillLog(Level level, uint64_t category) const { return Enabled() && (!m_filter || m_filter(level, category)); }
    +    bool WillLog(Level level, uint64_t category) const { return Enabled() && g_dispatcher_should_log(m_context, level, category) && (!m_filter || m_filter(level, category)); }
     
         /**
          * Format message and dispatch to all registered callbacks. No-op if WillLog() doesn't pass.
    @@ -148,6 +152,8 @@ private:
         std::list<Callback> m_callbacks GUARDED_BY(m_mutex);
         //! Lock-free size of m_callbacks for fast checks.
         std::atomic<size_t> m_callback_count{0};
    +    //! Optional context argument set in constructor.
    +    void* m_context;
         //! Optional filter set in constructor; [@see](/bitcoin-bitcoin/contributor/see/) WillLog().
         const FilterFunc m_filter{nullptr};
     };
    

    </p> </details>

    (Note: this could be rearranged and simplified more, it is just meant to provide a minimal change to consider.)

    Suggestion to split up PR

    My main feedback on the PR is that it is making a lot of trivial / move-only changes that I think would be better to move to a base PR:

    • Portion of first commit moving SourceLocation from logging.h to util/log.h (base PR could introduce util/log.h)
    • Commit "test: verify log argument evaluation semantics"
    • Portion of third commit moving BCLog::Level from logging.h to util/log.h
    • Commit "kernel: add log level/category name getters"
    • Portion of commit "kernel: implement structured logging C API" adding missing category and level mappings.
    • Commit "move-only: move logging categories to separate header"
    • Commit "logging: move LogAcceptCategory to util" (modulo g_dispatcher call)
    • Commit "logging: move macros to util"

    These are all worthwhile changes on their own, and moving them out of this PR should make it substantially smaller and easier to navigate.

    I plan to review this PR more and also rebase #30342 on top of this. I think the PR's should complement each other well.

  32. DrahtBot added the label Needs rebase on Jan 30, 2026
  33. stickies-v commented at 12:34 PM on January 30, 2026: contributor

    Thanks for the in-depth review and suggestions, @ryanofsky !

    so it would only need to add a second g_dispatcher_should_log hook, defined differently for kernel and non-kernel binaries to avoid std::function overhead:

    I hadn't considered this approach yet, and I think it's great. If we already have a g_should_log() global hook, I think it makes more sense to just scrap the filtering from Dispatcher entirely and let the user manage it completely. This simplifies the code, improves performance (e.g. if filtering is already done before arg evaluation, it doesn't have to be done again before dispatching) and avoids the pattern of having Dispatcher instances opaquely call a global function, when users would probably expect it to be local.

    My main feedback on the PR is that it is making a lot of trivial / move-only changes that I think would be better to move to a base PR:

    I've adopted some of your carve-out suggestions, and identified a few other places to make commits more straightforward. I prefer bundling (manageable) amount of clean-up with an actual functional improvement, so I'll open up a new PR that introduces the struct-based logging (i.e. the first commits), and convert this (now draft) follow-up PR to be about levels-based logging and removing logging.cpp from kernel.

  34. stickies-v marked this as a draft on Jan 30, 2026
  35. ryanofsky commented at 2:22 PM on January 30, 2026: contributor

    I prefer bundling (manageable) amount of clean-up with an actual functional improvement

    Your approach sounds reasonable. That said, I think it would be nice to first disentangle the move-only changes and make an initial PR that simply moves the log-emitting code from logging.h to util/log.h (logging macros, level and category constants, SourceLocation, LogAcceptCategory). This could likely be merged quickly, making the more substantive changes that follow easier to review.

    This also makes sense conceptually: log-emitting and log-handling are separate concerns, and there’s no real reason code that only emits log messages should need to include a header defining a full log management API.

    Another selfish reason I have for preferring the move-only changes first is that it should significantly reduce conflicts between your changes and my existing PRs, and make it easier to combine my logging work with yours.

  36. stickies-v force-pushed on Jan 30, 2026
  37. stickies-v force-pushed on Jan 30, 2026
  38. DrahtBot removed the label Needs rebase on Jan 30, 2026
  39. ryanofsky commented at 6:04 PM on January 30, 2026: contributor

    re: #34374 (comment)

    I think it would be nice to first disentangle the move-only changes and make an initial PR that simply moves the log-emitting code from logging.h to util/log.h (logging macros, level and category constants, SourceLocation, LogAcceptCategory). This could likely be merged quickly, making the more substantive changes that follow easier to review.

    I implemented this in #34465 (after chatting with stickies in IRC). The changes in the first 4 commits were taken directly from this PR. Only the last commit adds a bit of new code.

  40. DrahtBot removed the label CI failed on Jan 30, 2026
  41. DrahtBot added the label Needs rebase on Jan 31, 2026
  42. ryanofsky referenced this in commit e500550084 on Jan 31, 2026
  43. sedited referenced this in commit 8f0e1f6540 on Feb 7, 2026
  44. ryanofsky referenced this in commit fa06bdf0d0 on Feb 9, 2026
  45. ryanofsky referenced this in commit 10d36911c0 on Feb 9, 2026
  46. ryanofsky referenced this in commit 82bee29a79 on Mar 2, 2026
  47. ryanofsky referenced this in commit 1987fb2fb0 on Mar 2, 2026
  48. ryanofsky referenced this in commit d81dcb5d3b on Mar 4, 2026
  49. ryanofsky commented at 3:14 PM on March 6, 2026: contributor

    Hi stickies, it'd be really nice to have this rebased & start review. Here is a rebase branch I made: https://github.com/ryanofsky/bitcoin/commits/review.34374.2-edit with minimal changes.

    There are more simplifications that can be made, but it's already much smaller and simpler after #34465

  50. ryanofsky referenced this in commit 449fc7ce3b on Mar 9, 2026
  51. ryanofsky referenced this in commit e983d97a91 on Mar 9, 2026
  52. stickies-v commented at 5:47 AM on March 16, 2026: contributor

    Hi stickies, it'd be really nice to have this rebased & start review.

    Apologies for the slow follow-up here @ryanofsky. My priorities were elsewhere for a while, but I've now picked this up again.

    Since we now have the {Should}Log interface in util/log.h, I think also having util::log::Dispatcher doesn't really make sense anymore, so I've been reworking that into a kernel-specific type that implements the {Should}Log interface. I've been looking at a couple of different approaches and commit orderings to make reviewing as smooth as possible and think I'm close to having a good solution.

  53. ryanofsky commented at 1:26 PM on March 16, 2026: contributor

    I think also having util::log::Dispatcher doesn't really make sense anymore, so I've been reworking that into a kernel-specific type that implements the {Should}Log interface.

    FWIW I did this in the linked branch:

    https://github.com/bitcoin/bitcoin/blob/8611ff533c5c384a2a8fd32000fc9d6619d46b48/src/kernel/bitcoinkernel.cpp#L85-L93

    The Dispatcher type is kernel-specific there and not used outside the kernel. The only thing I didn't do was move the dispatcher class definition and strip out some unused code because I was avoiding extraneous changes in the rebase. Seems fine to reimplement, though. Should be equivalent to using the branch and the stripping bits of code that are no longer needed.

  54. ryanofsky referenced this in commit 3ea39b9f21 on Apr 1, 2026
  55. ryanofsky referenced this in commit fbc43e5b66 on Apr 1, 2026
  56. ryanofsky referenced this in commit ebeb36aa6e on Apr 1, 2026
  57. ryanofsky referenced this in commit 31612a7f5f on Apr 3, 2026
  58. ryanofsky commented at 11:59 AM on April 10, 2026: contributor

    Here is a rebased version of this PR with the logger cleanups from #34865 dropped: ec9e622d2da96ac482cf1bf599e20292d2f78759 (branch)

    Now there's basically no overlap between this PR and #34865 so both could be reviewed in parallel.

    Aside from the logger change I also dropped some dead code in the dispatcher class from the previous rebase and rewrote one commit message. Otherwise the branch is unchanged and the commits are straightforward:

    • 6c148e8c405532338a41f8787ddf76ac299b2b73 util: add log::Dispatcher for struct-based log dispatch (1/10)
    • 1f861b7f46ae1227208c35b880a6c8ebdcafee35 kernel: add log level/category name getters (2/10)
    • ddd655b6dd31b26a414f948a25e48c3366c50fba kernel: add missing log levels and categories (3/10)
    • d73cc47d9804a63b6ff9b3c88ab772032e4188a0 kernel: expose btck_LogEntry (4/10)
    • 8efdf8504959404326970610389dcd3130a31ec3 kernel: make logging struct-based (5/10)
    • 18a9a3c0cad3fb6c3e3c3d091879e75c629e519d kernel: remove unused logging functions (6/10)
    • d4e3282a593b646cb0a6a4d167394bf18e9904c4 logging: declare LogAcceptCategory in util (7/10)
    • ef451c810031168acc37ae52403f558abf72715d doc: advise on log rate limiting in developer notes (8/10)
    • c0cda82e443f2730ef423041d43551c4847ac590 logging: use util/log.h where possible (9/10)
    • ec9e622d2da96ac482cf1bf599e20292d2f78759 kernel: implement levels-based logging C API (10/10)
  59. sedited referenced this in commit 8a843a1b7c on Apr 23, 2026
  60. ryanofsky referenced this in commit 172f66c75f on May 6, 2026
  61. ryanofsky referenced this in commit af3ca8272d on May 6, 2026
  62. ryanofsky referenced this in commit 963ed23bc8 on May 26, 2026
  63. ryanofsky referenced this in commit 31f9929f3d on May 26, 2026
  64. ryanofsky referenced this in commit a76b13796a on May 26, 2026
  65. ryanofsky referenced this in commit a7c3bcd452 on May 27, 2026
  66. ryanofsky referenced this in commit ebd6270bee on May 29, 2026
  67. ryanofsky referenced this in commit 8eddd5aaf5 on Jun 1, 2026
  68. ryanofsky referenced this in commit 681c56066c on Jun 8, 2026
  69. ryanofsky commented at 1:57 PM on June 8, 2026: contributor

    Here is a rebased version of this pr: 683056775c58f05f9603f3f7023467ab0e7f723e (branch) again with minimal changes from the original.

    I'd really like to review and be able to merge this change because it makes the kernel logging API simpler and more usable.

    When I rebased this, I made as few changes as possible so comparison to the current version would be simple. But there are a few additional cleanups that could be made on top of this:

    • Dispatcher class should probably be moved out of util/log since dispatching is a log-handling function not a log-generating function. As mentioned #34865 (comment), the Dispatcher class could also be dropped and replaced with a btcsignals::signal. But it is a very simple class so it seems fine to keep.

    • A few simplifications / renames in bitcoinkernel.cpp would probably be appropriate like inlining g_dispatcher. I just kept current names for things so diffs would be minimal.

  70. ryanofsky referenced this in commit 1cfb06db14 on Jun 8, 2026
  71. ryanofsky referenced this in commit ecd24447df on Jun 24, 2026
  72. ryanofsky referenced this in commit 70d83491c8 on Jun 24, 2026
  73. ryanofsky referenced this in commit d1bd90bd3a on Jul 14, 2026
  74. ryanofsky referenced this in commit 8cd1ee83d1 on Aug 15, 2026
  75. stickies-v force-pushed on Sep 8, 2026
  76. DrahtBot removed the label Needs rebase on Sep 8, 2026
  77. DrahtBot added the label CI failed on Sep 8, 2026
  78. DrahtBot commented at 11:05 PM on September 8, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/34283939029/job/102254999690</sub> <sub>LLM reason (✨ experimental): CI failed because the lint check detected a new circular dependency: kernel/bitcoinkernel -> kernel/logger -> kernel/bitcoinkernel (lint-circular-dependencies).</sub>

    <details><summary>Hints</summary>

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

    </details>

  79. stickies-v force-pushed on Sep 9, 2026
  80. stickies-v force-pushed on Sep 9, 2026
  81. stickies-v renamed this:
    kernel: use structured logging and simplify logging interface
    kernel: use struct-based logging and simplify logging interface
    on Sep 9, 2026
  82. DrahtBot removed the label CI failed on Sep 9, 2026
  83. stickies-v commented at 5:14 PM on September 9, 2026: contributor

    Apologies for the months of radio silence here. I've had many cycles of exploring various architectures and ways to minimize review burden, to then kind of get stuck on something I really didn't like, give up, and repeat a while later. While I've found pretty much every approach to have its own downsides, I believe the current version has the best set of trade-offs. kernel and node logging having to co-exist complicates many things, but is a reality we have to work around.

    The main changes in this latest version are:

    • rebased onto latest master
    • util::log::Dispatcher is gone. Instead, this PR implements the Log and Should{Debug,Trace}Log interface, and removes the std::function indirection callbacks this PR used to have. The new KernelLogger that keeps state (minimum level and callbacks) is small and kernel-specific, and an implementation detail.
    • mocktime is added to btck_LogEntry
    • removed the btck_log_level_get_name/btck_log_category_get_name C functions from the earlier version; the C++ wrapper has Name() helpers instead. Consumers know the constants, so I don't think this belongs in the C API. It removes a lot of the churn in the previous approach wrt using arrays for the btck/util/name mapping.
    • when kernel code emits an unmapped category, this will now only crash in debug builds (with Assume). We can't currently enforce a complete mapping at compile time, and it's fairly easy for a new category to accidentally slip into kernel code. If that's in a rare code path, it'd be dangerous for that to crash in non-debug builds.

    Thoughts on things I've explored:

    • the top commit is fairly large. In previous approaches, I had separated this into smaller commits, but I've decided to revert it to two big ones to minimize churn. Because the util::log interface requires exactly one implementation, certain gradual changes are not really possible without workarounds that then add review complexity.
    • there's fairly minimal test coverage. Since util::log needs exactly one implementation per binary, test_bitcoin can't link the kernel library, and test_kernel only sees the exported C API, so KernelLogger isn't reachable from either test binary without workarounds. I've tried moving KernelLogger into its own header so test_bitcoin can compile it directly. That runs into the circular dependency linter and, when header-only, into clang's -Wunique-object-duplication for the singleton. Working around that means moving the logging C functions out of bitcoinkernel.cpp and sharing the Handle template through yet another header, which is even more churn. I think we should improve this through #28690 and then a new organization of test binaries (e.g. test_node, test_kernel, and test_kernel_interface), but I'm very hesitant to add that to the scope of this PR, which I feel is already fairly large.

    Despite my hesitations, I think these changes would massively improve the kernel logging interface, and help with better separation between kernel and node.

  84. stickies-v marked this as ready for review on Sep 9, 2026
  85. in src/kernel/bitcoinkernel.cpp:952 in 8677c26229
     967 |  }
     968 |  
     969 |  btck_LoggingConnection* btck_logging_connection_create(btck_LogCallback callback, void* user_data, btck_DestroyCallback user_data_destroy_callback)
     970 |  {
     971 | +    assert(callback);
     972 | +    auto handle{GetKernelLogger().RegisterCallback(
    


    w0xlt commented at 11:15 PM on September 10, 2026:

    RegisterCallback() and LogDebug() can also throw std::bad_alloc.

    btck_LoggingConnection can be a smart pointer. Holding the connection in a std::unique_ptr prevents its allocation from leaking if logging throws.

    Shouldn't user_data be destroyed if the callback registration fails ?

    diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
    index 72c1587450..635f1bacaf 100644
    --- a/src/kernel/bitcoinkernel.cpp
    +++ b/src/kernel/bitcoinkernel.cpp
    @@ -949,17 +949,20 @@ void btck_logging_set_min_level(btck_LogLevel level)
     btck_LoggingConnection* btck_logging_connection_create(btck_LogCallback callback, void* user_data, btck_DestroyCallback user_data_destroy_callback)
     {
         assert(callback);
    -    auto handle{GetKernelLogger().RegisterCallback(
    -        callback, user_data, user_data_destroy_callback)};
    -    btck_LoggingConnection* connection;
    +    std::optional<KernelLogger::CallbackHandle> handle;
         try {
    -        connection = btck_LoggingConnection::create(handle);
    +        handle = GetKernelLogger().RegisterCallback(callback, user_data, user_data_destroy_callback);
    +        std::unique_ptr<btck_LoggingConnection> connection{btck_LoggingConnection::create(*handle)};
    +        LogDebug(BCLog::KERNEL, "Logger connected.");
    +        return connection.release();
         } catch (...) {
    -        GetKernelLogger().UnregisterCallback(handle);
    +        if (handle) {
    +            GetKernelLogger().UnregisterCallback(*handle);
    +        } else if (user_data && user_data_destroy_callback) {
    +            user_data_destroy_callback(user_data);
    +        }
             return nullptr;
         }
    -    LogDebug(BCLog::KERNEL, "Logger connected.");
    -    return connection;
     }
     
     void btck_logging_connection_destroy(btck_LoggingConnection* connection)
    

    stickies-v commented at 10:18 AM on September 11, 2026:

    Good point, thanks. I think btck_logging_connection_destroy also needs similar improvements.


    stickies-v commented at 3:16 PM on September 14, 2026:

    Fixed by wrapping user data in a RAII UserData wrapper, and the callback in a RAII CallbackHandle wrapper. It's a larger diff, but I think it makes for cleaner code. The UserData can be reused in other places too, but I'd prefer doing that in a follow-up as it's unrelated to this change. Does this address your concern? You're right that LogDebug also could potentially throw, but this is true in the bitcoinkernel code in general and I feel might be out of scope here?

  86. stickies-v force-pushed on Sep 14, 2026
  87. stickies-v force-pushed on Sep 14, 2026
  88. DrahtBot added the label CI failed on Sep 14, 2026
  89. DrahtBot removed the label CI failed on Sep 14, 2026
  90. stickies-v commented at 11:19 AM on September 15, 2026: contributor

    Force-pushed to wrap user data and the callbacks in RAII wrappers, addressing @w0xlt's comment and making it harder to introduce similar issues in the future. Also moved KernelLogger into the anonymous namespace in the final commit.

  91. DrahtBot added the label Needs rebase on Sep 15, 2026
  92. kernel: add missing log levels and categories
    There are log levels and categories emitted by kernel code that
    are not yet part of the C API. Once entries are delivered as
    structs in a future commit, every emitted value must map to a
    constant, so add the missing ones first in this preparatory commit.
    
    The LOCK constant is unconditional so the C API is the same in every
    build.
    
    No behaviour change. Part of a series of commits to make kernel logging
    struct-based instead of string-based.
    3a72f3040c
  93. kernel: expose btck_LogEntry
    Introduce the btck_LogEntry struct the logging callback will use in
    a future commit (instead of the current string argument). The struct
    is not yet used.
    
    The level and category definitions move up so the callback typedef can
    reference btck_LogEntry; review with --color-moved.
    
    No behaviour change. Part of a series of commits to make kernel logging
    struct-based instead of string-based.
    94cf10c4bf
  94. kernel: add log level and category name helpers to wrapper
    Convenience functions for consumers that want to format levels and
    categories.
    
    No behaviour change. Part of a series of commits to make kernel logging
    struct-based instead of string-based.
    4469d1f57e
  95. kernel: remove API to format and disable logging
    Both functions lose their purpose once the kernel logger is no longer
    coupled to the node logger: formatting becomes the consumer's job, and
    nothing is buffered before a connection exists, so there is nothing to
    disable. Remove them early to simplify the next commits.
    
    Behaviour change until the backend switch later in this series:
    - strings are delivered in BCLog::Logger's default format
    - the 1MB pre-connection buffer cannot be turned off.
    
    Part of a series of commits to make kernel logging struct-based instead
    of string-based.
    b4edc98745
  96. stickies-v force-pushed on Sep 15, 2026
  97. stickies-v commented at 2:15 PM on September 15, 2026: contributor

    Force-pushed to address merge conflict with #36207, no other changes.

  98. DrahtBot removed the label Needs rebase on Sep 15, 2026
  99. kernel: add KernelLogger
    Add the kernel's own logging backend, unused until the next commit
    switches the C API over to it.
    
    Callbacks run while the logger's mutex is held: invocations are
    serialized, and unregistering waits for an in-flight callback.
    
    Callbacks are typed with a local LogCallback alias carrying the
    struct-based signature, because btck_LogCallback still delivers a
    string at this point. The next commit changes the typedef and replaces
    the alias.
    
    No behaviour change. Part of a series of commits to make kernel logging
    struct-based instead of string-based.
    95e63e954a
  100. kernel: make logging struct-based and simplify configuration
    Update btck_LogCallback to use struct-based logging and drop logging.cpp
    from the kernel build.
    
    Since consumers can now easily filter by category, simplify the logging
    configuration to just btck_logging_set_min_level.
    
    Behaviour changes for consumers:
    - Filtering is now purely levels-based, and consumers can choose any
      level they like. Debug and Trace logging applies to all categories,
      further filtering should be done client-side.
      Note: evaluation remains unchanged: Debug and Trace statements are
      only evaluated when they are requested, Info and above is always
      evaluated.
    - Entries logged before the first connection are dropped, not buffered.
    - Trailing newlines are stripped, callback exceptions are swallowed,
      and connection create and destroy no longer take cs_main.
    - Log messages are no longer escaped, this can be done client-side.
    
    KernelLogger moves into the anonymous namespace now that every member
    is used. It stayed outside in the previous commit to avoid unused
    member warnings.
    
    Node logging is unchanged. Final commit of the series to make kernel
    logging struct-based instead of string-based.
    cfd2312f0f
  101. in src/kernel/bitcoinkernel.cpp:567 in 398b2e2423 outdated
     562 | +    }
     563 | +};
     564 | +
     565 | +KernelLogger::CallbackHandle KernelLogger::RegisterCallback(btck_LogCallback fn, UserData user_data)
     566 | +{
     567 | +    STDLOCK(m_mutex);
    


    w0xlt commented at 8:38 PM on September 16, 2026:

    There is a narrow failure case here, related to the concerns about callbacks running under locks discussed in #36197.

    Registration creates a temporary Callback while holding the logger mutex. That temporary owns user_data. If allocating the list node throws std::bad_alloc, the temporary is destroyed before the mutex is released, invoking user_data_destroy_callback.

    If that callback destroys another logging connection, it tries to acquire the same mutex and deadlocks, preventing btck_logging_connection_create from returning nullptr.

    Suggestion: the callback can be constructed before the lock guard, so exception cleanup releases the mutex before destroying the user data, consistent with UnregisterCallback.

    diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
    index 0a4b5e30cb..4f512df146 100644
    --- a/src/kernel/bitcoinkernel.cpp
    +++ b/src/kernel/bitcoinkernel.cpp
    @@ -564,8 +564,10 @@ public:
     
     KernelLogger::CallbackHandle KernelLogger::RegisterCallback(btck_LogCallback fn, UserData user_data)
     {
    +    // Destroy user_data after releasing m_mutex if insertion throws.
    +    Callback callback{fn, std::move(user_data)};
         STDLOCK(m_mutex);
    -    m_callbacks.push_back({fn, std::move(user_data)});
    +    m_callbacks.push_back(std::move(callback));
         return {*this, std::prev(m_callbacks.end())};
     }
    

    stickies-v commented at 7:26 AM on September 17, 2026:

    Good catch, fixed. You're right to point out we need to be careful with executing callbacks when under a lock. I think we should work to making Log execute the logging callbacks without lock too, but I think that's best kept for a follow-up.

    If that callback destroys another logging connection

    Even worse, it can be triggered if the callback interacts in any way with the logger, including producing a logging statement. (edit: of course, this is still a very narrow failure case, requiring allocation failure and an I think unusual path where cleaning up user data interacts with the kernel).

  102. stickies-v force-pushed on Sep 17, 2026
  103. stickies-v commented at 9:22 AM on September 17, 2026: contributor

    Force-pushed to address a (hard to reach) deadlock issue flagged by @w0xlt


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-09-20 21:52 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me