#35322 logging: streamline Logger state and drop redundant methods
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #35322 logging: streamline Logger state and drop redundant methods | Ready | 0 | Neutral | 87 + 93 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
Replaces the Logger's internal category mask, global log level threshold, and mutex-protected per-category override map with a two-element array of atomic bitmasks corresponding to Trace and Debug levels. It removes obsolete methods like EnableCategory, DisableCategory, LogLevel, and GetCategoryMask in favor of SetCategoryLogLevel and lock-free WillLogCategoryLevel checks.
Problem: Logger redundantly tracked logging state across three separate fields (an atomic bitmask, an atomic level threshold, and a mutex-guarded hash map). Checking if a message should be logged required acquiring a mutex and performing hash lookups, and modifying log settings required coordinating multiple interrelated fields and methods.
Evidence of importance:
- PR description: 'Bitcoin Core has only ~30 log categories and 2 conditional log levels, so a mutex-protected hash map, a bitset, and a separate global level are unnecessary. The added complexity is also harmful: it makes the code harder to follow, and basic operations like setting a category's level require coordinating multiple fields and method calls.'
- Commit fa1702dff4: 'This reduces the number of possible states, removes mutex locking and hash lookups from log-level checks, and simplifies the Logger interface by dropping multiple enable, disable, and query methods that are no longer necessary.'
Scope: Touches logging internals, node initialization argument handling, node interfaces, and test harnesses. Commit messages note the changes were written with Claude Sonnet 4.6.
Risk of a mistake: low (rpc_api, other). A flaw in category bitmask calculations could cause log messages to be erroneously suppressed or emitted at incorrect verbosity, affecting debugging.
Changed since the previous assessment: Added a release note documenting that runtime category enablement via the logging RPC always defaults to debug level, and added unit tests testing combinations of -loglevel and -debug.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: re-review from l0rinc after commits split, test coverage added, and rebased 2026-05-27
- reviewer: l0rinc to re-review after author addressed objections and suggestions with updated commits and tests 2026-05-27
Agreement
Neutral
objections settled, nobody has spoken for the PR
Thread notes: Brought record up to date under current rules. Converted non-harm review requests to suggestions and recorded unlisted suggestions and questions from l0rinc; PR is awaiting re-review following author's updates and rebases. / Brought existing claims into compliance with current schema rules by updating types (reclassifying test request and commit split request as suggestions). Added additional suggestions and questions raised in reviewer comments.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| l0rinc (member) | suggestion, scope | split the PR into smaller, more focused commits or multiple PRs | fixed · fix pushed · author split the PR into separate commits | changed | yes | 2026-05-20: 'If you think it's worth it, could you please split this into smaller, more focused commits (maybe multiple PRs, since the title already suggests it's hard to properly describe the goal of the changes)?' Status from: 2026-05-27: ryanofsky: 'latest push removes each method in separate commits.' |
| l0rinc (member) | suggestion, maintenance | peel off each method separately such as getLogCategories/isAnyDebugLoggingEnabled before category-mask refactor | fixed · fix pushed · author split removal of each method into separate commits | changed | yes | 2026-05-21: 'I would prefer peeling off each method separately to have some progress during review.' Status from: 2026-05-27: ryanofsky: 'latest push removes each method in separate commits.' |
| l0rinc (member) | suggestion, maintenance | drop the string-parsing SetCategoryLogLevel overload with no production callers | fixed · fix pushed · overload removed and test updated to use typed setter | changed | yes | 2026-05-21: 'Could we drop it and have the test use the typed setter directly?' Status from: 2026-05-27: ryanofsky: 'Nice find! This lets another logger method to be dropped. Implemented this change.' |
| l0rinc (member) | suggestion, maintenance | split naming cleanup for UNCONDITIONAL_LOG_LEVEL before atomic-mask change | answered · author explained parsing code was dropped from Logger class and kept Info for now | answered | yes | 2026-05-21: 'Could this naming cleanup be split before the atomic-mask change?' Status from: 2026-05-27: ryanofsky: 'Following your other suggestion ... this -loglevel parsing code is now dropped from the logger class.' |
| l0rinc (member) | suggestion, maintenance | split test save/restore plumbing into the commit where test fixtures need it | fixed · fix pushed · author split the snapshot helper into the commit where test fixtures need it | changed | yes | 2026-05-21: 'Could it be split into the commit where the test fixtures first need to snapshot the level masks?' Status from: 2026-05-27: ryanofsky: 'Yes, this should be done now.' |
| l0rinc (member) | objection, correctness | setting -loglevel unintentionally enables a log category without -debug being set | fixed · fix pushed · fixed by only updating levels of previously enabled categories | changed | yes | 2026-05-21: 'Previously, setting a category level at startup stored the override, but it had no visible effect unless the category was also enabled with -debug. Here, setting `-loglevel=net:trace` appears to enabl' Status from: 2026-05-27: ryanofsky: 'It's fixed by making `-loglevel` only update the levels of previously enabled categories.' |
| l0rinc (member) | objection, approach | confusing design that forces conditional categories and unconditional logging levels into the same model | fixed · fix pushed · changed array declaration and added comment to make this clearer | changed | yes | 2026-05-22: 'Seems we have two fundamentally different ways of logging that we're forcing together here... I find this design confusing for multiple reasons' Status from: 2026-05-27: ryanofsky: 'changed array declaration and added comment to make this clearer' |
| l0rinc (member) | question, maintenance | whether category check taking different branch for info vs debug is an optimization or redundant code smell | fixed · fix pushed · improved in push | answered | yes | 2026-05-22: 'This seems like a code smell (even though it was similar before). Wouldn't the categories already return the same value here - Is this an optimization?' Status from: 2026-05-27: ryanofsky: 'Yeah agree this could be better, should be improved now but let me know.' |
| l0rinc (member) | suggestion, maintenance | make loop dual behavior in SetCategoryLogLevel more obvious in code instead of needing header explanation | answered · author added comment and explained why current loop structure is simpler | answered | yes | 2026-05-22: 'This loop has dual behavior that needed extra explanation in the header. Maybe we could make it more obvious via code instead' Status from: 2026-05-27: ryanofsky: 'I added a comment here to be clearer, and I'm open to feedback on different ways to write this, but I feel like the current version is actually simpler than the suggestions' |
| l0rinc (member) | suggestion, correctness | add a test covering the runtime-enable path when startup configured category at Trace | fixed · fix pushed · tests added covering runtime-enable behavior | changed | yes | 2026-05-22: 'Could we add a test that goes through this runtime-enable path after startup configured the category at Trace?' Status from: 2026-05-27: ryanofsky: 'adding new tests commits before changes to cover more behaviors' |
| l0rinc (member) | suggestion, style | deduplicate libevent enabled check when updating HTTP server logging | fixed · fix pushed · author applied suggestion | changed | yes | 2026-05-22: 'We can probably deduplicate this:' Status from: 2026-05-27: ryanofsky: 'Make sense, appied suggestion' |
| l0rinc (member) | objection, interface | kernel logging API changes behavior by enabling a category when setting its level | fixed · fix pushed · author explained rationale and adjusted kernel logging behavior | changed | yes | 2026-05-22: 'Previously, setting a category level did not enable the category. Now it looks like setting the level also enables it. Should we adjust this to match the old behavior?' Status from: 2026-05-27: ryanofsky: 'In this case I think the new behavior is probably better as there is no point to kernel code setting a level for a categ' |
Participants:
- l0rinc (member, objection, 14 statements 2026-05-20 to 2026-05-22): requested changes over commit structure, unintended -loglevel behavior changes, kernel logging semantics, and missing test coverage
Files
160 lines under test/bench/ci.
- src/test/logging_tests.cpp +87/-61
- src/logging.cpp +26/-62
- src/logging.h +21/-36
- src/init/common.cpp +24/-6
- src/rpc/node.cpp +3/-9
- src/test/i2p_tests.cpp +5/-5
- doc/release-notes-35322.md +8/-0
- src/interfaces/node.h +3/-3
- src/bench/logging.cpp +1/-1
- src/node/interfaces.cpp +1/-1
- src/qt/transactiondesc.cpp +1/-1
Card
The short text the ranking pass compares across PRs.
Streamlines Logger state by replacing a category bitfield, global log level, and mutex-protected override map with a two-element array of atomic category bitmasks for Debug and Trace levels. This eliminates mutex locking and hash lookups from log checks and drops several redundant methods from the logging interface. The only functional change is that dynamically enabling categories via the logging RPC or kernel API now always enables them at debug level rather than restoring startup -loglevel settings.
Data
thread read through 2026-05-27 (28 statements), head d5df90e17f
labels: none
Categories
Utilities (logging, arguments, libraries)
P3 · cleanup (ranking pass: Moved down and grouped with #35355; both convert log-level state to atomic bitmasks.)
Streamlines Logger internals by replacing redundant state tracking with atomic bitmasks and lock-free checks. As noted in the commit message, it 'reduces the number of possible states, removes mutex locking and hash lookups from log-level checks, and simplifies the Logger interface', providing a clean internal improvement to core logging utilities.
Membership: Substantially refactors src/logging.cpp and src/logging.h.
Factors: security/stability 0, bug 0, performance 1, user value 0, leverage 1