#34374 kernel: use struct-based logging and simplify logging interface
https://github.com/bitcoin/bitcoin/pull/34374 · · +464/-269 in 8 files, 6 commits · labels: Validation
Goal
- Let kernel library consumers receive structured log data instead of parsing formatted text strings
- Decouple the kernel library from internal node logging machinery
Replaces string-based log callback delivery in the bitcoinkernel C API with a structured `btck_LogEntry` and simplifies logging configuration to level filtering. Implements the `util/log.h` hooks directly in a kernel-specific `KernelLogger`, allowing `logging.cpp` to be dropped entirely from the kernel build.
Problem: External consumers of `bitcoinkernel` currently receive logs as pre-formatted strings, forcing brittle string parsing to extract timestamps, categories, or severity levels. Additionally, linking the kernel library pulls in node-specific logging machinery like file writing, rate limiting, and log buffering.
Category: Kernel (libbitcoinkernel) (#4 of 18)
P2 · new feature
- P2 because downstream kernel consumers get structured metadata without parsing formatted log strings
- Removes node-internal logging dependencies to advance clean process separation
P2 because it addresses a fundamental usability limitation of the kernel API, delivering structured log records to external consumers instead of strings, and removes the node's logging.cpp dependency from the kernel library as part of #27587.
Membership: Substantively modifies the bitcoinkernel C API and removes node logging dependencies from the kernel library.
Factors: security/stability 0, bug 0, performance 1, user value 2, leverage 2
Reviewability: Ready
- Ready for review, rebased on master with passing CI and addressed feedback
Branch is clean, CI passes, and the author addressed recent review comments from w0xlt.
Author status: active, addressed feedback and force-pushed on 2026-09-17
Resolved concerns:
- ajtowns noted indirection overhead from std::function and risk with logging argument side-effects; resolved by removing the dispatcher abstraction from util and isolating the implementation inside kernel.
- purpleKarrot criticized the multi-connection logging model and singleton usage; author noted the connection model is preexisting and orthogonal to the structured logging change.
- w0xlt flagged potential leaks and a deadlock risk if bad_alloc occurs during callback registration; author addressed both with RAII wrappers and releasing locks before user data destruction.
Agreement: Mild
- Strong concept and approach support for rationalizing the kernel logging API (ryanofsky)
- Identified indirection overhead and argument evaluation regressions that were addressed (ajtowns)
- Flagged callback registration deadlock and leak hazards which the author fixed (w0xlt)
Mild: nonblocking objection open (purpleKarrot)
ryanofsky provided a substantive Concept and Approach ACK, praising the simplification of the kernel API. Technical objections regarding exception safety and overhead were resolved in pushes.
- ryanofsky: 'Major concept and approach ACK. This seems thoughtfully implemented to be minimally disruptive to existing code while rationalizing the kernel logging API'
Objections:
| Reviewer | Kind | Harm | Status | Blocking | Author replied | Quote |
|---|---|---|---|---|---|---|
| ajtowns | approach | Performance overhead from std::function indirection and unexpected evaluation differences for log arguments. | resolved | no | yes | 2026-01-23: 'Jumping through a std::function when we could just be checking an atomic bitfield doesn't seem like a good approach.' Settled: 2026-09-09: 'util::log::Dispatcher is gone. Instead, this PR implements the Log and Should{Debug,Trace}Log interface, and removes the std::function indirection callbacks' |
| purpleKarrot | approach | Encourages singleton pattern usage and needlessly complex multi-connection C API. | open | no | yes | 2026-01-27: 'I doubt that there is an actual use case for multiple logging connections... Now everybody should understand why singletons are viral and therefore should be forbidden.' Settled: 2026-01-27: author replied that connection model changes are orthogonal and better discussed in #30342; reviewer did not follow up. |
| w0xlt | correctness | Deadlock or leaked resources if callback registration throws std::bad_alloc. | resolved | no | yes | 2026-09-16: 'If that callback destroys another logging connection, it tries to acquire the same mutex and deadlocks' Settled: 2026-09-17: 'Good catch, fixed. You're right to point out we need to be careful with executing callbacks when under a lock.' |
| ajtowns | correctness | LogInfo arguments were made conditionally evaluated, which breaks code relying on side-effects in logging statements | resolved | no | yes | 2026-01-28: 'I think your latest implementation (a4e8f3c8d6763a95c1804fc40781b379e6127b66) is dangerously wrong, btw -- LogInfo() etc currently always evaluate their arguments, whereas you're making that conditional on LogAcceptCategory which calls WillLog which is conditional on Enabled().' Settled: 2026-01-28: 'Latest force-push (dca56e0237abb485edd1cfe870069e2d75f08e42) reverts the unconditional argument evaluation for Info and higher levels, and adds a unit test to ensure behaviour before and after this PR remains the same.' |
| w0xlt | correctness | Memory leak of user_data and potential connection allocation leak if RegisterCallback throws std::bad_alloc | resolved | no | yes | 2026-09-10: 'RegisterCallback() and LogDebug() can also throw std::bad_alloc. btck_LoggingConnection can be a smart pointer. Holding the connection in a std::unique_ptr prevents its allocation from leaking if logging throws. Shouldn't user_data be destroyed if the callback registration fails ?' Settled: 2026-09-14: 'Fixed by wrapping user data in a RAII UserData wrapper, and the callback in a RAII CallbackHandle wrapper.' |
Support:
- ryanofsky: Major concept and approach ACK for rationalizing the kernel logging API into structured entries and dropping unnecessary configuration functions.
Participants: ryanofsky (support), ajtowns (objection), purpleKarrot (objection), w0xlt (objection)
State derived from the lists: nonblocking objection open (purpleKarrot) (model's own read: Strong)
Review verdicts (DrahtBot): 0
- Approach ACK: ryanofsky
Dependencies
Files
File list not available for this run.
Card
This PR updates the bitcoinkernel C API to deliver log entries via a structured btck_LogEntry struct rather than pre-formatted strings, and simplifies the API to level-based filtering. It implements the log hooks in a dedicated KernelLogger, allowing the kernel build to drop its dependency on logging.cpp. The change directly benefits developers building on libbitcoinkernel by removing brittle log-string parsing and decoupling kernel logging from the node. ryanofsky has provided a Concept and Approach ACK, and recent review comments on callback exception handling have been resolved.