kernel: Dispatch log callbacks with btcsignals #36768

pull ryanofsky wants to merge 3 commits into bitcoin:master from ryanofsky:pr/logsig changing 8 files +277 −97
  1. ryanofsky commented at 3:19 AM on November 5, 2026: contributor

    Problem: A kernel application whose log callback logs, or destroys a kernel object, hangs. The common way to hit this is a garbage-collected language binding: Python's garbage collector can run finalizers inside the callback, and a finalizer that destroys a chainstate manager flushes and logs (https://github.com/bitcoin/bitcoin/pull/34775#issuecomment-6278257737, #34374 (review)). The cause is that the kernel calls log callbacks while holding the logger's mutex, so any logging from inside a callback blocks on a mutex its own thread already holds. The problem is not new and has existed since logging callbacks were introduced in #30595.

    Solution: This PR delivers log entries through btcsignals::signal, which calls callbacks without holding any lock, replacing the kernel's own callback list and custom dispatching mechanism.

    The change is a usability improvement and a bugfix because with it applications no longer need to reason about deadlocks and blocking behavior from a mutex they can't see. However, they may need to be updated to deal with log callbacks that are no longer serialized and connection functions that no longer block. Compatibility details are in the commit message.

    The main code change is in src/kernel/bitcoinkernel.cpp (+42/-77) accompanied by header docs and new test_kernel cases, which hang without the fix. There is also a commit switching btcsignals.h to use StdMutex instead of Mutex so it is safe to use in logging callbacks.

    This is an alternative to the last commit of #34775, which fixes the same deadlock but only after limiting the process to one logging connection, and still hangs if a callback destroys the connection.

  2. ci: Add -Wno-error=maybe-uninitialized to i686 and Alpine musl tasks
    GCC 15 at -O1 reports a false -Wmaybe-uninitialized for a
    std::optional<btck::Logger<...>> in test_kernel.cpp, in the destructor
    of a logger that is always emplaced before use. Don't fail the build on
    it, as the armhf task already does.
    
    This change was written with Claude Opus 5.5.
    c2a8048549
  3. btcsignals: Use StdMutex so a signal can deliver log entries
    Locking a sync.h Mutex can log: with DEBUG_LOCKCONTENTION it logs lock
    contention, and with DEBUG_LOCKORDER it logs lock order errors. A signal
    that delivers kernel log entries, as the next commit adds, would then
    lock its own mutex again on the same thread while delivering that entry,
    and deadlock. Use a StdMutex, as Logger in logging.h does for the same
    reason.
    
    The mutex only guards the connection list and is never held while
    callbacks run, so lock order checking adds little here.
    
    This change was written with Claude Opus 5.5.
    4b707fa885
  4. kernel: Dispatch log callbacks with btcsignals so they run without the logger mutex
    KernelLogger called log callbacks while holding its mutex, so a callback
    that logged, or destroyed a kernel object that logs, deadlocked. A
    garbage collector running finalizers inside a callback can trigger this
    (https://github.com/bitcoin/bitcoin/pull/34775#issuecomment-6278257737).
    
    Deliver entries through a btcsignals::signal instead, which holds no lock
    while callbacks run. The API is unchanged. Behavior changes:
    
    - Callbacks can run on several threads at the same time, so they must be
      thread-safe.
    - Callbacks may call kernel functions, including creating and destroying
      logging connections, their own included.
    - Destroying a connection no longer waits for invocations already
      running on other threads. The user data is destroyed when the last one
      returns, as with other btcsignals users.
    
    This is the follow-up suggested in
    https://github.com/bitcoin/bitcoin/pull/34374#discussion_r4034240501.
    
    This change was written with Claude Opus 5.5.
    a9f467e8df
  5. ryanofsky force-pushed on Nov 5, 2026
  6. ryanofsky commented at 3:55 AM on November 5, 2026: contributor

    <!-- begin push-2 -->

    Updated 7a5e5b0a4998d79710a1c0babae9e66dac123f0e -> 25eff90bfd61d1dac76627f8b0eda96c23be8973 (pr/logsig.1 -> pr/logsig.2, compare)<!-- end --> Add a commit disabling -Werror for a GCC 15 -Wmaybe-uninitialized false positive in the new test_kernel test, as the armhf task already does https://github.com/bitcoin/bitcoin/actions/runs/41960869850/job/127026663042 https://github.com/bitcoin/bitcoin/actions/runs/41960869850/job/127026663160

  7. nervana21 commented at 2:57 AM on November 6, 2026: contributor

    Approach ACK

  8. DrahtBot commented at 2:57 AM on November 6, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    External sites

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

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

    Type Reviewers
    Approach ACK nervana21

    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:

    • #35355 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35355.svg"></sub> (Use atomics for determining whether trace logging is enabled by ajtowns)
    • #34775 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/34775.svg"></sub> (kernel: allow only one global logging connection by stickies-v)
    • #33847 <sub><img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33847.svg"></sub> (kernel: Attach logging connections to contexts, per-connection log levels 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-->

  9. in ci/test/00_setup_env_i686.sh:22 in e5c00b4bad
      17 | @@ -18,6 +18,7 @@ export CI_LIMIT_STACK_SIZE=1
      18 |  printf -v BITCOIN_CONFIG "%q " \
      19 |   --preset=dev-mode \
      20 |   -DCMAKE_BUILD_TYPE=Debug \
      21 | - -DAPPEND_CPPFLAGS=-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE
      22 | + -DAPPEND_CPPFLAGS=-DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE \
      23 | + -DCMAKE_CXX_FLAGS="-Wno-error=maybe-uninitialized"
    


    maflcko commented at 8:32 AM on November 6, 2026:

    nit: could use a trailing comment # for std::optional::emplace in test_kernel.cpp (or so), and put this in the first line to minimize the diff churn and conflicts with other ci pulls?


    ryanofsky commented at 11:07 PM on November 6, 2026:

    re: #36768 (review)

    Thanks, moved it to the first line with a trailing comment, in the Alpine musl task too.

  10. in src/bitcoin-chainstate.cpp:52 in 25eff90bfd
      51 | -                  << " [" << Name(entry.Category()) << ":"
      52 | -                  << Name(entry.Level()) << "] "
      53 | -                  << entry.Message() << "\n";
      54 | +        // Write each line in one call, since LogMessage() can run on several threads at once.
      55 | +        std::ostringstream line;
      56 | +        line << "kernel: " << entry.Timestamp().time_since_epoch().count()
    


    maflcko commented at 8:33 AM on November 6, 2026:

    unrelated-nit: Obviously doesn't matter, but in theory this should use a hard-coded type in TicksSinceEpoch, not a silently fluent type.


    ryanofsky commented at 11:07 PM on November 6, 2026:

    re: #36768 (review)

    in theory this should use a hard-coded type in TicksSinceEpoch, not a silently fluent type.

    Good point, changed it to std::chrono::nanoseconds{entry.Timestamp().time_since_epoch()}.count(), so a change to the timestamp type would fail to compile instead of silently changing the unit. I used std::chrono instead of TicksSinceEpoch because bitcoin-chainstate is there to show what an external c++ program using the kernel library needs, so it shouldn't depend on internal util/ helpers.

  11. sedited requested review from stickies-v on Nov 6, 2026
  12. sedited added this to a project on Nov 6, 2026
  13. github-project-automation[bot] changed the project status on Nov 6, 2026
  14. sedited changed the project status on Nov 6, 2026
  15. ryanofsky force-pushed on Nov 6, 2026
  16. ryanofsky commented at 11:10 PM on November 6, 2026: contributor

    <!-- begin push-3 -->

    Updated 25eff90bfd61d1dac76627f8b0eda96c23be8973 -> a9f467e8dfeafc137cb2a565fd37d555e24e3136 (pr/logsig.2 -> pr/logsig.3, compare)<!-- end --> with maflcko's suggestions: put the CI flag first with a comment saying what it is for, and print the log timestamp with an explicit nanoseconds unit


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-11-08 19:51 UTC

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