kernel: make logging callback global #34775

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

The underlying LogInstance() is a process-wide singleton, and all logging configuration (categories, levels, options) are already global. The per-connection btck_LoggingConnection handle gives the false impression of independent logger instances.

Fix this by explicitly making the entire kernel logging interface explicitly global, simplifying its usage. Replaces btck_LoggingConnection and its {create,destroy} functions with a single btck_logging_set_callback function. The KernelLogger class is introduced to hold global kernel logging state, which will be used in future work such as #34374.

To an extent, this approach is an alternative to #30342 which contextualizes logging. 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. That's why I've opened this PR as a much more straightforward way to fix kernel's current logging weirdness. If you believe kernel logging should be contextualized right away, please go review #30342 instead.

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.

Code Coverage & Benchmarks

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

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:

#35322 (logging: streamline Logger state and drop redundant methods by ryanofsky)
#34374 (kernel: use struct-based logging and simplify logging interface by stickies-v)

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 named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

g_kernel_logger().SetCallback(nullptr, nullptr, nullptr) in src/kernel/bitcoinkernel.cpp
btck_logging_set_callback(nullptr, nullptr, nullptr) in src/kernel/bitcoinkernel_wrapper.h

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?

kernel: make logging callback global

The underlying LogInstance() is a process-wide singleton, and all
logging configuration (categories, levels, options) are already
global. The per-connection btck_LoggingConnection handle gives the
false impression of independent logger instances.

Replace btck_logging_connection_create/destroy with a single
btck_logging_set_callback that sets or clears the global callback.
Internally, a KernelLogger singleton manages the LoggingConnection
lifetime, intentionally leaked on exit to avoid static destruction
order issues (matching LogInstance).

The C++ wrapper keeps Logger as the RAII owner of the global callback:
it sets the callback on construction and resets it on destruction, so
consumers keep the scoped lifetime the handle gave them, and a test
cannot leak its callback into later tests. Constructing a second Logger
while one exists throws, since they would share the single slot.

f40aeb12ed

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.