kernel: allow only one global logging connection #34775

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

Based on #34374. This PR's changes are limited to the top commit.

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. btck_logging_set_min_level becomes btck_logging_connection_set_min_level, and each new connection starts at Info. A second create call fails instead of silently sharing the logger.

Remove the "Logger connected." Debug entry, which can no longer be delivered since a new connection always starts at Info.

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:

#36353 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36353.svg"> (kernel: don't reuse cached CheckBlock results across params by FlashWayne)
#36326 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36326.svg"> (kernel: Use typed errors for fatal and flush error notifications by arejula27)
#36000 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/36000.svg"> (validation: prefetch blocks while connecting by l0rinc)
#35641 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35641.svg"> (kernel: Add script evaluation tracer by sedited)
#35322 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35322.svg"> (logging: streamline Logger state and drop redundant methods by ryanofsky)
#35187 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/35187.svg"> (kernel: Block validation without a complete UTXO set by sedited)
#33847 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/33847.svg"> (kernel: Attach logging connections to contexts, per-connection log levels by ryanofsky)
#28690 <img src="https://drahtbot.space/ack_count/bitcoin/bitcoin/28690.svg"> (build: Introduce internal kernel library by sedited)

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"}).

2026-10-08 15:10:56

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?

kernel: allow only one global logging connection

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.
btck_logging_set_min_level becomes btck_logging_connection_set_min_level,
and each new connection starts at Info. A second create call fails
instead of silently sharing the logger.

Remove the "Logger connected." debug entry, which can no longer be
delivered since a new connection always starts at Info.

5548f4b05f

stickies-v force-pushed on Oct 8, 2026

DrahtBot removed the label CI failed on Oct 8, 2026