kernel, logging: Pass Logger instances to kernel objects #30342
+756
−417
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30342. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
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 typos and grammar issues:
2025-12-12 |
This was referenced Jun 26, 2024
Closed
Contributor
|
Concept ACK. |
Contributor
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. Debug: https://github.com/bitcoin/bitcoin/runs/26722091796 |
Contributor
Author
|
Updated 4542335 -> db1b9f7 ( |
This was referenced Jun 27, 2024
Closed
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Jul 3, 2024
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger instance. Instead allow kernel applications to initialize their own logging instances that can be returned by LogInstance(). This change is somewhat useful by itself, but more useful in combination with bitcoin#30342. By itself, it gives kernel applications control over when the Logger is created and destroyed and allows new instances to replace old ones. In combination with bitcoin#30342 it allows multiple log instances to exist at the same time, and different output to be sent to different instances. This commit is built on top of bitcoin#30141 since it simplifies the implementation somewhat.
This was referenced Nov 11, 2025
Test will be extended in next commit to cover support for context objects.
Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to accept context arguments to provide more information in log messages and more control over logging to callers. This functionality is used in followup PRs: - bitcoin#30342 - To let libbitcoinkernel send output to specfic `BCLog::Logger` instances instead of a global instance, so output can be disambiguated and applications can have more control over logging. - bitcoin#30343 - To replace custom `WalletLogPrintf` calls with standard logging calls that automatically include wallet names and don't log everything at info level. This commit does not change behavior of current log prints or require them to be updated. It includes tests and documentation covering the new functionality. Co-Authored-By: Hodlinator <[email protected]>
Custom log contexts allow overridding log formatting and adding metadata such as request ids or wallet names to log messages while still using standard macros for logging. This is used to replace WalletLogPrintf() functions with LogInfo() calls in followup PR bitcoin#30343.
Needed to fix errors like: const Context &GetContext(const Context &)': expects 1 arguments - 3 provided due to a compiler bug: https://stackoverflow.com/questions/5134523/msvc-doesnt-expand-va-args-correctly/5134656#5134656 Example CI failure: https://github.com/bitcoin/bitcoin/actions/runs/8072891468/job/22055528830?pr=29256
Disallow passing BCLog category constants to LogInfo(), LogWarning(), and LogError() macros, and disallow omitting BCLog categories when calling LogDebug() and LogTrace() macros. Additionally, drop category information from log output at higher levels as suggested by Hodlinator bitcoin#29256 (comment). None of these restrictions are technically necessary, and not everybody believes they are good. However, they have existed since the Log{Trace,Debug,Info,Warning,Error} macros were introduced in bitcoin#28318, and removing these restrictions is orthogonal to the goals of this PR which are: (1) to allow the Log macros to work without accessing global state and (2) to support local logging customization with additional metadata. This change just more clearly documents the current logging restrictions and provides better error messages to developers when enforcing them. Co-Authored-By: Hodlinator <[email protected]>
Current kernel logging API is too complicated and too restrictive. This PR tries to improves it. I'd expect an API that supported logging to allow creating log streams with different options and providing some way to specify which library operations should be logged to which streams. By contrast, the current API allows creating multiple log streams, but all the streams receive the same messages because options can only be set globally and the stream objects can't be passed to any kernel API functions. They are not even referenced by the `btck_Context` struct which holds other shared state. If no log streams are created, log messages are generated anyway, but they are stored in a 1MB buffer and not sent anywhere, unless a log stream is created later, at which point they are sent in bulk to that stream. More log streams can be created after that, but they only receive new messages, not the buffered ones. If log output is not needed, a btck_logging_disable() call is required to prevent log messages from accumulating in the 1MB buffer. Calling this will abort the program if any log streams were created before it was called, and also abort the program if any new log streams are created later. None of these behaviors seem necessary or ideal, and it would be better to provide a simpler logging API that allows creating a log stream, setting options on it, registering it with the `btck_Context` instance and receiving log messages from it. Another advantage of this approach is that it could allow (with bitcoin#30342) different log streams to be used for different purposes, which would be not be possible with the current interface.
Pass Logger instances to BlockManager, CCoinsViewDB, CDBWrapper, ChainstateManager, and CoinsViews instances so libbitcoinkernel applications and test code have the option to control where log output goes instead of having all output sent to the global logger. This commit just passes the logger objects without using them. The next commit updates log print statements to use the new objects.
This is a mechanical change updating kernel code that currently uses the global log instance to log to local instances instead.
This allows libbitcoinkernel applications and test code to have the option to control where log output goes instead of having all output sent to the global logger.
Change LogInstance() function to no longer allocate (and leak) a BCLog::Logger instance. Instead allow kernel applications to initialize their own logging instances that can be returned by LogInstance(). The LogInstance() function is not actually used for the majority of logging in kernel code. Most kernel log output goes directly to BCLog::Logger instances specified when kernel objects like ChainstateManager and CTxMemPool are constructed, which allows applications to create multiple Logger instances and control which log output is sent to them. The only kernel code that does rely on LogInstance() is certain low level code in the util library, like the RNG seeder, that is not passed a specific instance and needs to rely on the global LogInstance() function. Other code outside the kernel library uses LogInstance() heavily, and may continue to do so. bitcoind and test code now just create a log instance before the first LogInstance() call, which it returns, so all behavior is the same as before.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Dec 12, 2025
Current kernel logging API is too complicated and too restrictive. This PR tries to improves it. I'd expect an API that supported logging to allow creating log streams with different options and providing some way to specify which library operations should be logged to which streams. By contrast, the current API allows creating multiple log streams, but all the streams receive the same messages because options can only be set globally and the stream objects can't be passed to any kernel API functions. They are not even referenced by the `btck_Context` struct which holds other shared state. If no log streams are created, log messages are generated anyway, but they are stored in a 1MB buffer and not sent anywhere, unless a log stream is created later, at which point they are sent in bulk to that stream. More log streams can be created after that, but they only receive new messages, not the buffered ones. If log output is not needed, a btck_logging_disable() call is required to prevent log messages from accumulating in the 1MB buffer. Calling this will abort the program if any log streams were created before it was called, and also abort the program if any new log streams are created later. None of these behaviors seem necessary or ideal, and it would be better to provide a simpler logging API that allows creating a log stream, setting options on it, registering it with the `btck_Context` instance and receiving log messages from it. Another advantage of this approach is that it could allow (with bitcoin#30342) different log streams to be used for different purposes, which would be not be possible with the current interface.
ryanofsky
added a commit
to ryanofsky/bitcoin
that referenced
this pull request
Dec 12, 2025
Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to accept context arguments to provide more information in log messages and more control over logging to callers. This functionality is used in followup PRs: - bitcoin#30342 - To let libbitcoinkernel send output to specfic `BCLog::Logger` instances instead of a global instance, so output can be disambiguated and applications can have more control over logging. - bitcoin#30343 - To replace custom `WalletLogPrintf` calls with standard logging calls that automatically include wallet names and don't log everything at info level. This commit does not change behavior of current log prints or require them to be updated. It includes tests and documentation covering the new functionality. Co-Authored-By: Hodlinator <[email protected]>
Contributor
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
Contributor
|
🐙 This pull request conflicts with the target branch and needs rebase. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pass
Loggerinstances toBlockManager,CCoinsViewDB,CDBWrapper,Chainstate,ChainstateManager,CoinsViews, andCTxMemPoolclasses so libbitcoinkernel applications and tests have the option to control where log output goes instead of having all output sent to the global logger.This PR is an alternative to #30338. It is more limited because it only changes kernel code while leaving other parts of the codebase alone. It also takes the opportunity to migrate legacy LogPrint / LogPrintf calls to the new log macros introduced in #28318.
This is based on #29256 + #33847. The non-base commits are:
2ca8523ec706refactor: Pass Logger instances to kernel objects8550b8d50f7crefactor: Log kernel output to local log instancesbe2de43f2ad5refactor: Pass Logger instance to CTxMemPoolb9dc75a2b5d6kernel: Drop global Logger instance