#34775 kernel: make logging callback global
https://github.com/bitcoin/bitcoin/pull/34775 · · +105/-66 in 5 files, 1 commits · labels: Validation
Goal
- Simplify the kernel logging API by replacing per-connection handles with a global callback
- Avoid confusion for downstream consumers by reflecting that logging is a global singleton
Replaces the per-connection `btck_LoggingConnection` interface in libbitcoinkernel with a single global callback setter `btck_logging_set_callback`. Internally, a leaked `KernelLogger` singleton manages callback state, and the C++ wrapper maintains scoped lifetime via a single-instance RAII `Logger`.
Problem: The underlying node logging system is a process-wide singleton, yet libbitcoinkernel exposed connection handles that gave external callers the misleading impression that multiple independent loggers could be configured.
Category: Kernel (libbitcoinkernel) (#11 of 18)
P3 · cleanup
- P3 because this is an API cleanup aligning the kernel interface with its underlying global logger
- Downstream consumers get a clearer API, though it adds no new validation capabilities
- Unblocks follow-up kernel work in #34374
It simplifies the kernel C API by removing misleading `btck_LoggingConnection` handles that implied per-instance logging when the underlying engine is a global singleton. While worthwhile API hygiene, it is an internal API cleanup rather than an essential feature or fix.
Membership: Directly alters libbitcoinkernel C API headers and C++ wrapper
Factors: security/stability 0, bug 0, performance 0, user value 1, leverage 1
Reviewability: Ready
- Ready to review: CI is passing and recent feedback is addressed
The code is rebased and updated to incorporate reviewer suggestions regarding RAII lifetime in the C++ wrapper.
Author status: Active, recently force-pushed to implement reviewer suggestions.
Resolved concerns:
- ryanofsky cautioned against prematurely limiting the logging API in a way that might be hard to reverse; author noted the kernel C API is explicitly unversioned and an orthogonal step before larger contextual logging refactors.
- purpleKarrot suggested dropping user_data, but author defended it as standard C callback practice.
- sedited and w0xlt noted that a global callback could leak between unit tests; author addressed this by keeping an RAII `Logger` wrapper in the C++ interface that unsets the callback on destruction.
Agreement: Strong
- Support for simplifying the logging interface by removing connection handles (sedited, w0xlt)
- Questioned whether limiting the API is premature compared to contextual logging (ryanofsky)
- Author addressed test isolation concerns by preserving scoped cleanup (sedited, w0xlt)
Strong: supported by sedited and w0xlt; earlier approach concern by ryanofsky has had no follow-up since June
Reviewers with domain context support aligning the interface with the current global logging implementation. Previous concerns around test isolation were addressed in the latest push.
- sedited: 'Not having separate connections seems simpler... Concept ACK'
- w0xlt: 'Approach ACK'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| ryanofsky | approach | Prematurely restricts the kernel logging API in a way that might be difficult to reverse compared to contextualized logging | resolved | no | yes | 2026-03-09: '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' Settled: 2026-06-05: stickies-v replied explaining the API is unversioned, diff is small, and orthogonal to contextual logging; ryanofsky did not follow up |
| w0xlt | correctness | Global callback without RAII scoped cleanup could leak logging state across tests | resolved | no | yes | 2026-07-16: 'any test that installs the global logging callback now needs to clear it before returning to prevent state leakage into later tests' Settled: 2026-09-17: 'address @w0xlt's suggestion to have an RAII Logger in the C++ wrapper instead of free functions' |
| sedited | correctness | global logging callback function leaks state across unit tests | resolved | no | yes | 2026-06-11: "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." Settled: 2026-09-17: stickies-v pushed changes adopting an RAII Logger in the C++ wrapper |
Support:
- sedited: Agrees that multiplexing can be done caller-side and single callback is simpler
- w0xlt: Approach ACK on the overall direction
Participants: sedited (support), purpleKarrot (question), ryanofsky (objection), w0xlt (support)
State derived from the lists: substantive support, no open objection (sedited, w0xlt)
Review verdicts (DrahtBot): 0
Dependencies
Enables:
Files
25 lines under test/bench/ci.
- src/kernel/bitcoinkernel.h +22/-41
- src/kernel/bitcoinkernel.cpp +37/-12
- src/kernel/bitcoinkernel_wrapper.h +27/-7
- src/test/kernel/test_kernel.cpp +12/-6
- test/sanitizer_suppressions/valgrind.supp +7/-0
Card
PR #34775 refactors libbitcoinkernel's logging API to make its single callback global, replacing the misleading btck_LoggingConnection handles. This removes the false impression of independent loggers while underlying logging remains a singleton. The change is supported by kernel contributors, with earlier concerns regarding RAII test isolation addressed in a recent push. It has no hard dependencies and simplifies downstream work on kernel logging structs.