#33847 kernel: Attach logging connections to contexts, per-connection log levels
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #33847 kernel: Attach logging connections to contexts, per-connection log levels | Ready | 0 -1 | Disputed | 142 + 45 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Updates the libbitcoinkernel C API and C++ wrapper so logging connections attach to contexts via btck_context_options_set_logger and minimum log levels are set per connection. Logging connections receive no output until attached to an active context, and destroying a connection while an attached context still exists aborts the process.
Problem: Every kernel logging connection receives an interleaved global log stream at a single process-wide level, preventing applications running multiple chainstate contexts (such as mainnet and signet or multi-context tests) from separating output. The C API also provided no way to pass connections to context creation, meaning future per-context routing would force breaking API changes and caller restructuring.
Evidence of importance:
- PR description: 'An application that opens two chainstates (say mainnet and signet, or a test creating several contexts) gets one interleaved log stream and can't tell which context a line came from. #30342 can separate them, but only if the application says which logging connection belongs to which context, and the current API has no way to do that'
- PR description: 'Per-context output would then mean restructuring every application and binding to get a connection to each place that creates a context. With a connection in the API from the start it costs one extra call'
Scope: Modifies the libbitcoinkernel public C API and introduces strict lifetime requirements where a logging connection must outlive any context options and contexts referencing it.
Risk of a mistake: medium (other). Mistakes in refcounting context usage or synchronization within KernelLogger could cause race conditions or crash kernel library consumers.
Changed since the previous assessment: Rebased following upstream changes; the approach and API surface remain unchanged.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: reviewers to look at the rebased PR and revised approach 2025-11-11
Agreement
Disputed
objection that needs a change, the objector pushed back after a reply (stringintech, stickies-v)
Thread notes: Author updated the PR implementation and rebased on top of merged #34374, replacing the approach with per-connection minimum log levels and context association.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| sedited (member) | suggestion, approach | Push forward #30342 before changing the logging API | answered · author argued this PR should take priority because it changes the API incompatibly while #30342 is backwards compatible | answered | yes | 2025-11-11: 'Maybe it would be preferable to push forward #30342 before this is done?' Status from: 2025-11-11: ryanofsky: 'On the question of whether to push forward with this PR or #30342 first: I think this PR should have much higher priorit' |
| stringintech (contributor) | objection, interface | Accepting per-connection handles on logging configuration functions suggests per-connection isolation while calling them still overrides settings globally across all connections | contested · stringintech reiterated preference to wait for #30342 after author's reply | changed | yes | 2025-11-13: 'keep the API consistent with what it's currently capable of' Status from: 2025-11-14: stringintech: 'My preference was to wait for #30342' |
| stringintech (contributor) | question, correctness | Whether delaying StartLogging() to context creation means context-free operations miss logs and whether it would need to move back if such operations log in the future | answered · author replied that non-trivial operations producing logs should have an associated context object | answered | yes | 2025-11-14: 'This delay could mean logs won't be produced by context-free operations (like btck_script_pubkey_verify())' Status from: 2025-11-14: ryanofsky: 'I am a little surprised there would be any context-free operations except very basic ones that shouldn't log' |
| stickies-v (member) | objection, approach | Exposing a new public interface before internal logging refactorings like #30342 proceed risks unexpected behavior and misses insights that internal changes could provide for interface design | contested · stickies-v pushed back with an Approach NACK, arguing interface and functional changes should happen atomically in #30342 | changed | yes | 2025-11-14: 'internal logging refactorings / changes (e.g. #30342, but also others) may surface roadblocks or ideas that inform us about a better interface design, so waiting a bit more seems helpful to me' Status from: 2026-10-06: stickies-v: 'Approach NACK' |
| stickies-v (member) | objection, safety | If context does not own the logging connection, object lifetime management becomes brittle and destroying a logging connection could leave the associated context in an invalid state | accepted · stickies-v agreed that context does not need to own the logging connection | changed | yes | 2025-12-10: 'managing lifetimes becomes more brittle, i.e. users must ensure `logger` is kept alive for the duration of `context`' Status from: 2025-12-10: stickies-v: 'I agree then that there is no need for context to own the logging connection.' |
Support:
- sedited (member) (Concept ACK, 2025-11-11) [read]: This is close to how I hoped to implement the logging capabilities
- stickies-v (member) (Concept ACK, 2025-11-14) [read]: making the logging interface local instead of global
Participants:
- sedited (member, support, 1 statements 2025-11-11 to 2025-11-11): Concept ACK; suggested prioritizing #30342 first
- stringintech (contributor, objection, 2 statements 2025-11-13 to 2025-11-14): prefers waiting for #30342 so API does not falsely suggest per-connection isolation
- stickies-v (member, objection, 5 statements 2025-11-14 to 2026-10-06): Approach NACK; objects to exposing contextualized logging before #30342 is ready
Dependencies
Files
58 lines under test/bench/ci.
- src/kernel/bitcoinkernel.cpp +84/-17
- src/test/kernel/test_kernel.cpp +45/-13
- src/kernel/bitcoinkernel.h +41/-13
- src/kernel/bitcoinkernel_wrapper.h +16/-5
- src/bitcoin-chainstate.cpp +1/-0
Card
The short text the ranking pass compares across PRs.
Updates the libbitcoinkernel C API and wrapper to associate logging connections with contexts and set log levels per connection rather than globally. Currently, all kernel logging connections receive an interleaved, process-wide log stream at a single level, making multi-context applications unable to isolate log output. This PR establishes the API hooks and connection lifetime tracking so subsequent work can isolate each context's log stream without further API breaks. Depends on #34374 and enables #30342.
Data
thread read through 2026-10-06 (16 statements), head 20ab175656
labels: Validation · draft
Categories
Kernel (libbitcoinkernel)
P2 · unblocks #30342
Establishes logging connection interfaces on context creation to prevent future breaking API changes and unblock per-context logging in #30342. The author notes that without this API hook, 'future per-context routing would force breaking API changes and caller restructuring' across all kernel consumers and bindings.
Membership: Modifies libbitcoinkernel public C API headers, implementations, and C++ wrapper (src/kernel/bitcoinkernel.{h,cpp}) to route logging connections through contexts.
Factors: security/stability 0, bug 0, performance 0, user value 1, leverage 2