kernel: allow only one global logging connection #34775

stickies-v commented at 8:35 AM on March 9, 2026: contributor

Kernel logging is inherently global: KernelLogger is a process-wide singleton, so every connection receives every log entry and shares the minimum level. Multiple btck_LoggingConnection handles give the false impression of independent loggers.

Limit btck_global_logging_connection_create to a single connection, so that the globalness is expressed in that one function and the rest of the interface can operate on the connection handle.

A single connection also makes it straightforward to invoke the callback without holding the logger's mutex. Holding it serializes all logging threads on user code, and deadlocks if anything on the callback's thread logs, e.g. a garbage collector running finalizers that destroy kernel objects.

Behaviour changes for consumers:

btck_logging_connection_create is renamed to btck_global_logging_connection_create, and fails if a connection already exists.
btck_logging_set_min_level becomes btck_logging_connection_set_min_level, and each new connection starts at Info.
The "Logger connected." Debug entry is removed, since it can no longer be delivered.
The callback can be invoked from multiple threads at the same time, so it must be thread-safe. Destroying the connection still waits for running invocations to return.

DrahtBot added the label Validation on Mar 9, 2026

DrahtBot commented at 8:36 AM on March 9, 2026: contributor

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

External sites

Code coverage & benchmarks: https://corecheck.dev/bitcoin/bitcoin/pulls/34775
Ralph review <img src=https://review.fish.foo/assets/bitcoin/bitcoin/34775.png alt="Ralph review status"> https://git.fish.foo/bitcoin/bitcoin/pulls/34775#author-ralph

Reviews

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

Type	Reviewers
Concept ACK	sedited
Approach ACK	w0xlt

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36598 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36598.svg"> ([Demo] validation: remove AssumeUTXO by l0rinc)
#36584 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36584.svg"> (kernel: make btck_LogEntry opaque by stickies-v)
#35355 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35355.svg"> (Use atomics for determining whether trace logging is enabled by ajtowns)

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.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

src/test/kernel/test_kernel.cpp: BOOST_CHECK_THROW(Logger{std::make_unique<CountingLog>(rejected_messages, rejected_destroyed)}, std::runtime_error) → Prefer BOOST_CHECK_EXCEPTION(..., std::runtime_error, HasReason{"A logging connection already exists"}) to check the exception message as well as its type.

2026-10-19 21:33:40

sedited commented at 11:12 AM on March 9, 2026: contributor

While I do believe contextualized logging would be a natural fit for the kernel interface, it seems there is currently (in my view) too little demand for the scope of changes necessary.

It really isn't a feature/issue that is sorely missed for the foreseeable future in the kernel library. There are a few applications where it would be a nice to have. Logs coming from different sources, for example in the units tests, can be a little confusing sometimes, but even that has been manageable, and I don't think I've heard many complain about that.

Not having separate connections seems simpler. Clients of the library can still multiplex the single callback on their side in whatever fashion they wish to. There are many other (and more interesting problems imo) to be working on, so this seems like the pragmatic thing to do for now.

Concept ACK

sedited requested review from alexanderwiederin on Mar 9, 2026

purpleKarrot commented at 1:12 PM on March 9, 2026: contributor

I would go a step further. Since the log function is global, there is no need for local user data. It can be a simple function pointer, even in the C++ wrapper.

stickies-v commented at 2:35 PM on March 9, 2026: contributor

Since the log function is global, there is no need for local user data.

Forcing the user to use globals if they need state seems unnecessary, when using user_data is I think a well-established pattern in C APIs? Unless I'm misunderstanding your suggested approach?

ryanofsky commented at 3:09 PM on March 9, 2026: contributor

I think it would be a mistake to limit functionality of the logging/debuging/tracing API prematurely, in a way that seems difficult to reverse and does not seem to provide a noticeable simplification (possible I missed it, I have not reviewed the code much yet!). Anyway I have many more thoughts on this topic and can post them later.

One thing I did want to say is I don't think #30342 is the only alternative to this and other approaches should be possible. Also I am actively working on #30342, and will push a significant update to it today or tomorrow. So if there are problems with that approach, I would like to at least try to pin them down and address them.

My preference in terms of review & development effort would be to undraft your other PR #34374. I rebased that PR recently #34374 (comment) and think is ready for review, and that it makes improvements that should have many more benefits to applications than this PR. Personally, I would like to review it and see it merged ASAP.

sedited added this to a project on Mar 9, 2026

github-project-automation[bot] changed the project status on Mar 9, 2026

sedited changed the project status on Mar 9, 2026

sedited commented at 8:16 PM on June 4, 2026: contributor

@stickies-v I'm not sure how we should proceed here. Are you still looking for review here, or should we work towards #34374?

stickies-v commented at 8:22 PM on June 5, 2026: contributor

My apologies for not responding earlier @ryanofsky

in a way that seems difficult to reverse

It's a +76/-72 diff, I don't think this will be particularly hard to reverse? If you're talking about breaking the interface: it is unversioned and we're quite explicit about breaking it when we want to, so I don't think that's a concern either. Of course, we shouldn't merge this when it seems like there is strong conceptual support for an alternative.

and does not seem to provide a noticeable simplification

I think having an interface that looks like it makes sense to have multiple logging connections, when it actually doesn't is unnecessarily confusing. While having documentation is good, having a self-explanatory interface is better.

Generally, I don't think this PR should or will prevent future changes that improve contextualized logging. It just seems to me like there currently isn't a lot of support for those improvements, so I think it's better to clean up the interface since that's such a straightforward change. I'm not opposed to contextualized logging, but I won't be reviewing it myself as I think it's currently not worth the effort.

Are you still looking for review here, or should we work towards #34374?

These changes are orthogonal to #34374 (they both use a KernelLogger class, but that's an implementation detail). So yes, I'm still looking for review here.

in src/kernel/bitcoinkernel.cpp:303 in dafe5eec14

 298 | +    }
 299 | +};
 300 | +
 301 | +KernelLogger& g_kernel_logger()
 302 | +{
 303 | +    static KernelLogger* logger{new KernelLogger};

sedited commented at 11:56 AM on June 11, 2026:

Does this have to be a raw pointer?

stickies-v commented at 6:39 AM on September 16, 2026:

I think so, to avoid running consumer code from static destructors. The order of destruction of static objects is undefined, so a plain static would run the consumer's destroy callback at some point during process teardown that they can't control, e.g. after objects their user_data refers to are already destroyed, or after a language runtime like Python has shut down. It's more important for #34374, where we also want to avoid log statements referencing a logger that's already destroyed, but it seems sensible to leak it here too.

in src/test/kernel/test_kernel.cpp:721 in dafe5eec14

 717 | @@ -720,7 +718,7 @@ Context create_context(std::shared_ptr<TestKernelNotifications> notifications, C
 718 |  
 719 |  BOOST_AUTO_TEST_CASE(btck_chainman_tests)
 720 |  {
 721 | -    Logger logger{std::make_unique<TestLog>()};
 722 | +    logging_set_callback(std::make_unique<TestLog>());

sedited commented at 12:03 PM on June 11, 2026:

I think this shows the downside of exposing a global function for this: Even though this function is scoped to this test, it now enables logging for all the the other unit tests too. Maybe add another call to set the callback to null at the end of the test?

w0xlt commented at 12:08 AM on July 16, 2026:

Yes, any test that installs the global logging callback now needs to clear it before returning to prevent state leakage into later tests. logging_tests already does this, so btck_chainman_tests would need to do the same.

diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index cfd02ea495..1413d5be22 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -758,6 +758,7 @@ BOOST_AUTO_TEST_CASE(btck_chainman_tests)
     BOOST_CHECK(chainman_opts.SetWipeDbs(/*wipe_block_tree=*/false, /*wipe_chainstate=*/true));
     BOOST_CHECK(chainman_opts.SetWipeDbs(/*wipe_block_tree=*/false, /*wipe_chainstate=*/false));
     ChainMan chainman{context, chainman_opts};
+    logging_set_callback(nullptr);
 }
 
 std::unique_ptr<ChainMan> create_chainman(TestDirectory& test_directory,

w0xlt commented at 12:22 AM on July 16, 2026:

I think the C++ wrapper could retain RAII while still exposing a single global logging callback.

stickies-v commented at 11:46 AM on September 17, 2026:

I think the C++ wrapper could retain RAII while still exposing a single global logging callback.

Yeah, this seems like the most elegant approach to me. In fact, I think it makes sense to not expose the free logging functions at all in the wrapper. The Logger class should offer all required functionality, in a way that is much harder to abuse.

Thank you both for your ideas and suggestions. I've updated the PR with this approach.

in src/kernel/bitcoinkernel_wrapper.h:849 in dafe5eec14

 853 | +        throw std::runtime_error("Failed to set logging callback");
 854 |      }
 855 | -};
 856 | +}
 857 | +
 858 | +inline void logging_set_callback(std::nullptr_t)

sedited commented at 12:05 PM on June 11, 2026:

How about calling this logging_reset_callback and removing the argument?

stickies-v commented at 11:46 AM on September 17, 2026:

I agree, but this is no longer relevant in the latest version as per #34775 (review).

w0xlt commented at 5:41 PM on June 24, 2026: contributor

Approach ACK

sedited commented at 11:04 AM on August 27, 2026: contributor

@stickies-v what is the status here?

stickies-v force-pushed on Sep 17, 2026

stickies-v commented at 11:43 AM on September 17, 2026: contributor

Force-pushed to rebase onto latest master, and to:

address @w0xlt's suggestion to have an RAII Logger in the C++ wrapper instead of free functions
align naming etc better with the updated approach in #34374

My apologies for once again a slow follow-up here. I wanted to first figure out my approach in #34374, and that took me much longer than I had anticipated.

ryanofsky commented at 5:52 AM on September 25, 2026: contributor

Sorry for going quiet here again, and thanks for the update and earlier reply.

I just ACKed #34374, and I think it changes the picture for this PR. The description says btck_LoggingConnection gives a false impression of independent loggers because LogInstance() is a process-wide singleton and all logging configuration is global. After #34374, the kernel doesn't use LogInstance() at all. KernelLogger already supports multiple callbacks, each with its own user_data and lifetime, registered and removed under a mutex, and the only shared setting left is the minimum level.

So instead of removing the connection handle, I'd suggest going the other way: have btck_logging_set_min_level take a btck_LoggingConnection* and filter per connection. It is a small change on top of #34374: store the level on each callback, and keep the atomic as the lowest level across callbacks so ShouldDebugLog stays cheap. After that, connections really are independent in everything a caller can configure, and the confusion this PR fixes goes away without introducing a process-wide callback that separate components in one process would have to coordinate over. I rebased #33847 onto #34374 to do this, which makes it much smaller than the original version.

re: #34775 (comment)

On reversibility: I agree the diff here is small. My concern is difficultly of updating code written against the API rather than the diff. Code that calls a global set_callback gets no connection object, so going back to connections later means restructuring callers, not just the library, because caller need to track the pointer and pass it to all sites that may need it. In the case of language bindings this could mean a coordinated rollout with bindings first needing to expose connection pointers, and then applications updated to start using them. Udates within applications could be nontrivial as well. Happy to go into this more, but hopefully #33847 shows how these problems can be easily avoided with small tweaks to the connection API making it flexible and future proof. I believe it is a better long term and short term approach.

stickies-v force-pushed on Oct 5, 2026

stickies-v marked this as a draft on Oct 5, 2026

stickies-v renamed this:
~~kernel: make logging callback global~~
kernel: allow only one global logging connection
on Oct 5, 2026

DrahtBot added the label CI failed on Oct 5, 2026

DrahtBot commented at 8:05 PM on October 5, 2026: contributor

🚧 At least one of the CI tasks failed. Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/37360665164/job/111934165167 LLM reason (✨ experimental): CI failed because IWYU modified src/kernel/bitcoinkernel.cpp, leaving a diff that caused the include-check to fail.

<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>

stickies-v force-pushed on Oct 5, 2026

stickies-v commented at 8:19 PM on October 5, 2026: contributor

Force-pushed to change the approach to still let the user manage btck_LoggingConnection* lifetimes, but enforce that only a single btck_LoggingConnection* can exist at any given time (see updated PR description). This keeps the logging interface scoped while still naturally representing that currently we only support a single, global logging connection. I've rebased it on top of #34374, and will keep this as draft until that's merged. #34374 offers many more practical benefits and seems to have more consensus, so prioritizing that makes sense I think. @ryanofsky: I believe this new approach addresses your concern about callers having to restructure if logging is contextualized later, because the connection handle is kept. I don't think allowing multiple connections or setting a connection per context makes sense until #30342 is merged, so I'm leaving that out of scope for here. We can discuss those changes on the relevant PRs?

stickies-v force-pushed on Oct 8, 2026

DrahtBot removed the label CI failed on Oct 8, 2026

refactor: merge KernelLogger::MaybeLog into Log

MaybeLog() is the only caller of Log().

a93f5c019a

kernel: allow only one global logging connection

KernelLogger is a process-wide singleton, so every connection receives
every log entry and shares the minimum level. Multiple
btck_LoggingConnection handles give the false impression of independent
loggers.

Behaviour change: btck_logging_connection_create is renamed to
btck_global_logging_connection_create, and fails if a connection
already exists.

0d1ec522a9

kernel: set the minimum log level on the logging connection

With only one connection, the logging interface can operate on the
connection handle instead of on global state.

Behaviour changes:
- btck_logging_set_min_level becomes
  btck_logging_connection_set_min_level.
- Each new connection starts at Info, instead of at the level of the
  previous connection.
- The "Logger connected." debug entry is removed, since it can no
  longer be delivered.

771f44ed38

kernel: invoke the logging callback without holding the lock

Holding m_mutex while running the callback serializes all logging
threads on user code, and deadlocks if anything on the callback's
thread logs, e.g. a garbage collector running finalizers that destroy
kernel objects.

Behaviour change: the callback can be invoked from multiple threads at
the same time, so it must be thread-safe. Destroying the connection
still waits for running invocations to return.

Update bitcoin-chainstate and the test logger to write each line in a
single call, so concurrent lines don't interleave.

921329000f

stickies-v force-pushed on Oct 19, 2026

stickies-v commented at 9:55 PM on October 19, 2026: contributor

While working on updating the Python language bindings, I noticed that Python's garbage collection can cause deadlocks even when the logging callback itself wouldn't have caused any deadlocks, because the GC can run finalizers inside the callback, and the (usually *_destroy) kernel calls they make log on the same thread. @w0xlt already raised concerns about this too, e.g. here and here.

Having a single callback (or one per context, cfr #30342) makes it a lot easier to invoke the callback without holding a lock, so I decided to include that change in this PR too. It comes with the side-effect that log callback invocations are no longer serialized, and callbacks must be thread-safe.

Restructured the commits a bit for reviewability, and also dropped the MaybeLog function which didn't really make sense anymore, as Log has to be conditional anyway.

I've updated the PR description with the new approach. This PR is now ready for review.

stickies-v marked this as ready for review on Oct 19, 2026