- A new -trace option controls trace-level logging, using the same syntax as -debug. So
-debug=netenables debug logs for net,-trace=netadds trace logs too. This replaces the-logleveloption, which had different syntax and had to be combined with-debugto take effect. - The logging RPC gains trace support: a new trace parameter enables trace-level logging per category, and the output now distinguishes excluded/debug/trace categories. (
-deprecatedrpc=loggingpreserves the old format.)
logging: replace -loglevel with -trace, expose trace logging via RPC #34038
pull ajtowns wants to merge 3 commits into bitcoin:master from ajtowns:202512-log-niceties changing 16 files +263 −129-
ajtowns commented at 5:56 AM on December 10, 2025: contributor
-
DrahtBot commented at 5:56 AM on December 10, 2025: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34038.
<!--021abf342d371248e50ceaed478a90ca-->
Reviews
See the guideline for information on the review process.
Type Reviewers ACK stickies-v Concept ACK ryanofsky If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #34806 (refactor: logging: Various API improvements by ajtowns)
- #21283 (Implement BIP 370 PSBTv2 by achow101)
- #17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
- #17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
- #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
- #17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
- ajtowns force-pushed on Dec 10, 2025
- DrahtBot added the label CI failed on Dec 10, 2025
-
DrahtBot commented at 6:01 AM on December 10, 2025: contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Task
lint: https://github.com/bitcoin/bitcoin/actions/runs/20088899137/job/57632110650</sub> <sub>LLM reason (✨ experimental): Lint failure: the RPC code uses a fatal assert (rpc_assert), triggering the linter to fail.</sub><details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
-
ajtowns commented at 6:04 AM on December 10, 2025: contributor
The reason for the locking change for
ShrinkDebugFileis that the following commit is enough for clang to figure out that theLogWarning("couldn't shrink")message would take the lock, and thus that ShrinkDebugFile should be annotated with!m_cs. But thinking about it, it seemed like it would make more sense to hold the lock and pause any attempted logging until the shrinking was finished anyway, so that's what I did. Because we only shrink on startup, before calling StartLogging, I don't think it has much real impact. - ajtowns force-pushed on Dec 10, 2025
-
in src/rpc/node.cpp:247 in 1df56912a2 outdated
243 | @@ -235,12 +244,26 @@ static RPCHelpMan logging() 244 | { 245 | {"exclude_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the valid logging category"}, 246 | }}, 247 | + {"trace", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to trace logging",
stickies-v commented at 11:19 AM on December 10, 2025:Since we're doing a potentially breaking change (with
-deprecatedrpc) here already, what do you think about renaming "include" to "debug"? Also, the RPC documentation wrt evaluation order needs to be updated a bit to reflect the debug -> trace -> exclude evaluation order.
ajtowns commented at 9:00 PM on December 10, 2025:Seems like a fine idea, but not sure how you'd provide compatibility for
include=["net"]with deprecaterpc?
stickies-v commented at 10:26 PM on December 10, 2025:I think this should work?
<details> <summary>git diff on 77f272c094</summary>
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 0693b60557..25c313c552 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -325,7 +325,7 @@ static const CRPCConvertParam vRPCConvertParams[] = { "psbtbumpfee", 1, "replaceable"}, { "psbtbumpfee", 1, "outputs"}, { "psbtbumpfee", 1, "original_change_index"}, - { "logging", 0, "include" }, + { "logging", 0, "debug" }, { "logging", 1, "exclude" }, { "logging", 2, "trace" }, { "disconnectnode", 1, "nodeid" }, diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp index abca5e69bd..06714bde37 100644 --- a/src/rpc/node.cpp +++ b/src/rpc/node.cpp @@ -225,20 +225,22 @@ static void EnableOrDisableLogCategories(UniValue cats, BCLog::Level new_level) static RPCHelpMan logging() { + const bool use_deprecated = IsDeprecatedRPCEnabled("logging"); + const std::string debug_category_name{use_deprecated ? "include" : "debug"}; return RPCHelpMan{"logging", "Gets and sets the logging configuration.\n" "When called without an argument, returns the the status of the supported logging categories.\n" "When called with arguments, adds or removes categories from debug or trace logging and return the lists above.\n" - "The arguments are evaluated in order \"include\", \"trace\", \"exclude\".\n" - "If an item is both included/traced and excluded, it will thus end up being excluded.\n" + "The arguments are evaluated in order \"" + debug_category_name + "\", \"trace\", \"exclude\".\n" + "If an item is debug and/or trace but also excluded, it will thus end up being excluded.\n" "The valid logging categories are: " + LogInstance().LogCategoriesString() + "\n" "In addition, the following are available as category names with special meanings:\n" " - \"all\", \"1\" : represent all logging categories.\n" , { - {"include", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to debug logging", + {debug_category_name, RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to debug logging", { - {"include_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the valid logging category"}, + {"debug_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the debug logging category"}, }}, {"exclude", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to remove from debug logging", { @@ -249,14 +251,14 @@ static RPCHelpMan logging() {"trace_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the valid logging category"}, }}, }, - { - RPCResult{"If -deprecatedrpc=logging is set", + { use_deprecated ? + RPCResult{ RPCResult::Type::OBJ_DYN, "", "keys are the logging categories, and values indicates its status", { {RPCResult::Type::BOOL, "category", "if being debug logged or not. false:inactive, true:active"}, } - }, - RPCResult{"Otherwise", + } : + RPCResult{ RPCResult::Type::OBJ, "", "", {{ {RPCResult::Type::ARR, "excluded", "excluded categories", {{RPCResult::Type::STR, "", ""}}}, @@ -289,7 +291,6 @@ static RPCHelpMan logging() UpdateHTTPServerLogging(LogInstance().WillLogCategory(BCLog::LIBEVENT)); } - bool use_deprecated = IsDeprecatedRPCEnabled("logging"); UniValue result(UniValue::VOBJ); if (use_deprecated) { for (const auto& info : LogInstance().LogCategoriesInfo()) { diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index fa1de774b7..4dfe13ec99 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -80,10 +80,12 @@ class RpcMiscTest(BitcoinTestFramework): assert('qt' in node.logging()['trace']) node.logging(exclude=['qt']) assert('qt' in node.logging()['excluded']) - node.logging(include=['qt']) + node.logging(debug=['qt']) assert('qt' in node.logging()['debug']) node.logging(trace=['qt']) assert('qt' in node.logging()['trace']) + # "include" is now deprecated in favour of "debug" + assert_raises_rpc_error(-8, "Unknown named parameter include", node.logging, include="net") # Test logging RPC returns the logging categories in alphabetical order. logging = node.logging() @@ -125,6 +127,10 @@ class RpcMiscTest(BitcoinTestFramework): # Specifying an unknown index name returns an empty result assert_equal(node.getindexinfo("foo"), {}) + # Test that deprecatedrpc="logging" maintains behaviour + self.restart_node(0, ["-deprecatedrpc=logging"]) + node.logging(include=["net"]) + assert_raises_rpc_error(-8, "Unknown named parameter debug", node.logging, debug="net") if __name__ == '__main__': RpcMiscTest(__file__).main()</details>
ajtowns commented at 3:47 AM on December 11, 2025:Nice. Left the RPCResult text including both forms of output, as that makes it a little more self-documenting that people can use the deprecatedrpc option if desired. Also left
includein the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds running deprecatedrpc=logging.
stickies-v commented at 10:57 AM on December 11, 2025:Also left
includein the client side so that bitcoin-cli still works with old bitcoinds or bitcoindsI'm not sure that's feasible without also adding a
-deprecatedrpcoption tocli(unless that already exists), which I don't think is worth it? Afaik we don't support disjointbitcoindandbitcoin-cliversions, and I thinkbitcoin-cliis more for manual usage anyway at which point just typing "debug" instead of "include" is trivial? The diff I added passes all tests (locally) and also adds functional tests to ensure both the current and the deprecated approach work fine over RPC (i.e. without cli).
ajtowns commented at 11:56 PM on December 12, 2025:Changed it to just support both include or debug as aliases indefinitely. (Can remove the
include=alias when the deprecatedrpc option is removed)I don't think it makes sense to ship a bitcoind/bitcoin-cli pair that don't work together under some configuration param (ie
bitcoin-cli -named include="['net']"doesn't work because cli won't parse it as json, anddebug=["net"]won't work because you're running withdeprecatedrpc=logging)fanquake added the label Needs release note on Dec 10, 2025stickies-v commented at 11:37 AM on December 10, 2025: contributorConcept ACK. Both the new
BCLog::NO_RATE_LIMITand-loglevelchanges make for a more natural and ergonomic interface for both developers and users, and the changes required are limited enough. The breaking RPC change is probably the biggest downside/cost of this PR.ajtowns force-pushed on Dec 10, 2025ajtowns force-pushed on Dec 10, 2025DrahtBot removed the label CI failed on Dec 11, 2025ajtowns force-pushed on Dec 11, 2025ajtowns force-pushed on Dec 11, 2025DrahtBot added the label CI failed on Dec 11, 2025ajtowns force-pushed on Dec 11, 2025ajtowns force-pushed on Dec 11, 2025ajtowns force-pushed on Dec 11, 2025DrahtBot removed the label CI failed on Dec 11, 2025DrahtBot added the label Needs rebase on Dec 14, 2025ajtowns force-pushed on Dec 15, 2025DrahtBot removed the label Needs rebase on Dec 15, 2025abo-L commented at 1:52 PM on December 15, 2025: nonebc1qhhml75wgm3a3303tuxyvhr74jr4jmhhk6qy5um
DrahtBot added the label Needs rebase on Dec 18, 2025ajtowns force-pushed on Dec 20, 2025DrahtBot added the label CI failed on Dec 20, 2025DrahtBot removed the label Needs rebase on Dec 20, 2025DrahtBot removed the label CI failed on Dec 20, 2025ajtowns commented at 3:06 AM on December 21, 2025: contributorRebased on top of other logging changes, added a release note
in src/threadsafety.h:71 in f565bbce26 outdated
66 | @@ -65,15 +67,19 @@ class LOCKABLE StdMutex : public std::mutex 67 | //! with the ! operator, to indicate that a mutex should not be held. 68 | const StdMutex& operator!() const { return *this; } 69 | #endif // __clang__ 70 | -}; 71 | 72 | -// StdLockGuard provides an annotated version of std::lock_guard for us, 73 | -// and should only be used when sync.h Mutex/LOCK/etc are not usable.
stickies-v commented at 8:05 AM on January 2, 2026:and should only be used when sync.h Mutex/LOCK/etc are not usable.
I think this part of the docstring is still applicable after this PR, should we keep it?
ajtowns commented at 5:52 AM on January 23, 2026:That comment is still there for the
StdMutexclass, andStdMutex::Guardisn't usable withoutStdMutexin src/logging.h:360 in f565bbce26 outdated
354 | @@ -351,8 +355,32 @@ namespace BCLog { 355 | static std::string LogLevelToStr(BCLog::Level level); 356 | 357 | bool DefaultShrinkDebugFile() const; 358 | - }; 359 | 360 | + template <bool should_ratelimit=true, typename... Args> 361 | + inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags flag, BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
stickies-v commented at 8:36 AM on January 2, 2026:nit: can we multi-line these long signatures?
in src/logging.h:383 in 08a624736a outdated
377 | @@ -378,6 +378,9 @@ namespace BCLog { 378 | { 379 | LogPrintFormatInternal<false>(std::move(source_loc), flag, level, std::move(fmt), args...); 380 | } 381 | + 382 | + /** Return true if str parses as a log category and set the flag */ 383 | + static bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
stickies-v commented at 8:54 AM on January 2, 2026:in commit 08a624736a: would be nice to get rid of the out-parameter
<details> <summary>git diff on 08a624736a</summary>
diff --git a/src/logging.cpp b/src/logging.cpp index 494f513865..5765d8f322 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -127,10 +127,11 @@ void BCLog::Logger::EnableCategory(BCLog::LogFlags flag) bool BCLog::Logger::EnableCategory(std::string_view str) { - BCLog::LogFlags flag; - if (!GetLogCategory(flag, str)) return false; - EnableCategory(flag); - return true; + if (auto flag{GetLogCategory(str)}) { + EnableCategory(*flag); + return true; + } + return false; } void BCLog::Logger::DisableCategory(BCLog::LogFlags flag) @@ -140,10 +141,11 @@ void BCLog::Logger::DisableCategory(BCLog::LogFlags flag) bool BCLog::Logger::DisableCategory(std::string_view str) { - BCLog::LogFlags flag; - if (!GetLogCategory(flag, str)) return false; - DisableCategory(flag); - return true; + if (auto flag{GetLogCategory(str)}) { + DisableCategory(*flag); + return true; + } + return false; } bool BCLog::Logger::WillLogCategory(BCLog::LogFlags category) const @@ -216,18 +218,16 @@ static const std::unordered_map<BCLog::LogFlags, std::string> LOG_CATEGORIES_BY_ }(LOG_CATEGORIES_BY_STR) }; -bool BCLog::Logger::GetLogCategory(BCLog::LogFlags& flag, std::string_view str) +std::optional<BCLog::LogFlags> BCLog::Logger::GetLogCategory(std::string_view str) { if (str.empty() || str == "1" || str == "all") { - flag = BCLog::ALL; - return true; + return BCLog::ALL; } auto it = LOG_CATEGORIES_BY_STR.find(str); if (it != LOG_CATEGORIES_BY_STR.end()) { - flag = it->second; - return true; + return it->second; } - return false; + return std::nullopt; } std::string BCLog::Logger::LogLevelToStr(BCLog::Level level) @@ -592,13 +592,13 @@ bool BCLog::Logger::SetLogLevel(std::string_view level_str) bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::string_view level_str) { - BCLog::LogFlags flag; - if (!GetLogCategory(flag, category_str)) return false; + auto flag{GetLogCategory(category_str)}; + if (!flag) return false; const auto level = GetLogLevel(level_str); if (!level.has_value() || level.value() > MAX_USER_SETABLE_SEVERITY_LEVEL) return false; STDLOCK(m_cs); - m_category_log_levels[flag] = level.value(); + m_category_log_levels[flag.value()] = level.value(); return true; } diff --git a/src/logging.h b/src/logging.h index 18c759daa4..d9c02e9e69 100644 --- a/src/logging.h +++ b/src/logging.h @@ -380,7 +380,7 @@ namespace BCLog { } /** Return true if str parses as a log category and set the flag */ - static bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str); + static std::optional<BCLog::LogFlags> GetLogCategory(std::string_view str); }; } // namespace BCLog diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index 0e29fa314d..4de66bcc7a 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -164,10 +164,10 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros_CategoryName, LogSetup) std::vector<std::pair<BCLog::LogFlags, std::string>> expected_category_names; const auto category_names = SplitString(concatenated_category_names, ','); for (const auto& category_name : category_names) { - BCLog::LogFlags category; const auto trimmed_category_name = TrimString(category_name); - BOOST_REQUIRE(BCLog::Logger::GetLogCategory(category, trimmed_category_name)); - expected_category_names.emplace_back(category, trimmed_category_name); + auto category{BCLog::Logger::GetLogCategory(trimmed_category_name)}; + BOOST_REQUIRE(category); + expected_category_names.emplace_back(*category, trimmed_category_name); } std::vector<std::string> expected;</details>
ajtowns commented at 6:07 AM on January 28, 2026:It's only used internally in logging.cpp and in tests, and works fine, I don't think this needs changing.
in src/threadsafety.h:79 in f565bbce26 outdated
82 | + public: 83 | + explicit Guard(StdMutex& cs) EXCLUSIVE_LOCK_FUNCTION(cs) : std::lock_guard<StdMutex>(cs) {} 84 | + ~Guard() UNLOCK_FUNCTION() = default; 85 | + }; 86 | + 87 | + static inline StdMutex& CheckNotHeld(StdMutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }
stickies-v commented at 9:07 AM on January 2, 2026:nit: since this fn is public, the name could be a bit confusing since this doesn't do any runtime checks, and the compile-time checks only work under specific circumstances. Maybe good to move this in
detail_and/or add a brief docstring to avoid misuse?
ajtowns commented at 5:56 AM on January 23, 2026:It matches the
MaybeCheckNotHeldfunctions in sync.h (theMaybethere reflects that the overload for is weakened for Global/Recursive locks), and as an inline function in the class seems pretty self-documentingin src/logging.cpp:123 in f565bbce26 outdated
122 | @@ -123,6 +123,7 @@ void BCLog::Logger::DisableLogging() 123 | void BCLog::Logger::EnableCategory(BCLog::LogFlags flag)
stickies-v commented at 10:16 AM on January 2, 2026:Should we rename this to
DebugCategorynow for consistency?
ajtowns commented at 5:57 AM on January 23, 2026:Seems like a reasonable idea, but doesn't seem necessary to do in this PR
in doc/release-notes-34038.md:7 in f565bbce26
0 | @@ -0,0 +1,14 @@ 1 | + 2 | +Logging 3 | +======= 4 | + 5 | +- A new `-trace` option has been added. The only trace logs currently 6 | + supported are detailed logs about wallet sql statements, which can 7 | + be accessed via `-trace=wallet`. The `-loglevel` configuration option
stickies-v commented at 10:19 AM on January 2, 2026:be accessed via `-trace=walletdb`. The `-loglevel` configuration optionin src/logging.cpp:126 in f565bbce26 outdated
122 | @@ -123,6 +123,7 @@ void BCLog::Logger::DisableLogging() 123 | void BCLog::Logger::EnableCategory(BCLog::LogFlags flag) 124 | { 125 | m_categories |= flag; 126 | + SetCategoryLogLevel(flag, Level::Debug);
stickies-v commented at 10:39 AM on January 2, 2026:Shouldn't
DisableCategoryunset this, too?
ajtowns commented at 6:03 AM on January 23, 2026:EnableCategorydoes it to allow downgrading from trace to debug; butDisableCategorycan just set the flag to disable both trace and debug logging for the category immediately. If it's later re-enabled,EnableCategoryorTraceCategorywill replace any old value at that time.
stickies-v commented at 2:59 AM on March 13, 2026:Yeah, I agree it's not currently necessary, just seems nice to keep things consistent when there's no meaningful cost. But I don't have a strong rationale for it, so you can mark as resolved.
in src/logging.h:341 in f565bbce26 outdated
340 | + void EnableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 341 | + bool EnableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 342 | + void TraceCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 343 | + bool TraceCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 344 | + void DisableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 345 | + bool DisableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
stickies-v commented at 12:40 PM on January 2, 2026:I'm a bit unsure about the merit of these functions. I think just using
SetCategoryLogLevelis a bit more straightforward, which can also be simplified now that we don't have-loglevelanymore. I think the string overloads could also be removed, and move input validation/parsing to the edge rather than inlogging.<details> <summary>git diff on f565bbce26</summary>
diff --git a/src/bench/logging.cpp b/src/bench/logging.cpp index 85e33b035e..c68206fd9b 100644 --- a/src/bench/logging.cpp +++ b/src/bench/logging.cpp @@ -18,7 +18,7 @@ static void Logging(benchmark::Bench& bench, const std::vector<const char*>& extra_args, const std::function<void()>& log) { // Reset any enabled logging categories from a previous benchmark run. - LogInstance().DisableCategory(BCLog::LogFlags::ALL); + LogInstance().SetCategoryLogLevel(BCLog::ALL, BCLog::Level::Info); TestingSetup test_setup{ ChainType::REGTEST, diff --git a/src/init/common.cpp b/src/init/common.cpp index eb1b06adf5..99908581ef 100644 --- a/src/init/common.cpp +++ b/src/init/common.cpp @@ -69,9 +69,11 @@ util::Result<void> SetLoggingCategories(const ArgsManager& args) const auto categories_to_process = (last_negated == categories.rend()) ? categories : std::ranges::subrange(last_negated.base(), categories.end()); for (const auto& cat : categories_to_process) { - if (!LogInstance().EnableCategory(cat)) { + BCLog::LogFlags flags; + if (!BCLog::Logger::GetLogCategory(flags, cat)) { return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)}; } + LogInstance().SetCategoryLogLevel(flags, BCLog::Level::Debug); } // Special-case: Disregard any debugging categories appearing before -trace=0/none @@ -81,16 +83,20 @@ util::Result<void> SetLoggingCategories(const ArgsManager& args) const auto trace_categories_to_process = (trace_negated == trace_categories.rend()) ? trace_categories : std::ranges::subrange(trace_negated.base(), trace_categories.end()); for (const auto& cat : trace_categories_to_process) { - if (!LogInstance().TraceCategory(cat)) { + BCLog::LogFlags flags; + if (!BCLog::Logger::GetLogCategory(flags, cat)) { return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-trace", cat)}; } + LogInstance().SetCategoryLogLevel(flags, BCLog::Level::Trace); } // Now remove the logging categories which were explicitly excluded for (const std::string& cat : args.GetArgs("-debugexclude")) { - if (!LogInstance().DisableCategory(cat)) { + BCLog::LogFlags flags; + if (!BCLog::Logger::GetLogCategory(flags, cat)) { return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debugexclude", cat)}; } + LogInstance().SetCategoryLogLevel(flags, BCLog::Level::Info); } return {}; } diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp index 15812baf00..253f101c4e 100644 --- a/src/kernel/bitcoinkernel.cpp +++ b/src/kernel/bitcoinkernel.cpp @@ -735,12 +735,12 @@ void btck_logging_set_level_category(btck_LogCategory category, btck_LogLevel le void btck_logging_enable_category(btck_LogCategory category) { - LogInstance().EnableCategory(get_bclog_flag(category)); + LogInstance().SetCategoryLogLevel(get_bclog_flag(category), BCLog::Level::Debug); } void btck_logging_disable_category(btck_LogCategory category) { - LogInstance().DisableCategory(get_bclog_flag(category)); + LogInstance().SetCategoryLogLevel(get_bclog_flag(category), BCLog::Level::Info); } void btck_logging_disable() diff --git a/src/logging.cpp b/src/logging.cpp index 870e666c69..6fa0e22db3 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -120,47 +120,6 @@ void BCLog::Logger::DisableLogging() StartLogging(); } -void BCLog::Logger::EnableCategory(BCLog::LogFlags flag) -{ - m_categories |= flag; - SetCategoryLogLevel(flag, Level::Debug); -} - -bool BCLog::Logger::EnableCategory(std::string_view str) -{ - BCLog::LogFlags flag; - if (!GetLogCategory(flag, str)) return false; - EnableCategory(flag); - return true; -} - -void BCLog::Logger::TraceCategory(BCLog::LogFlags flag) -{ - m_categories |= flag; - SetCategoryLogLevel(flag, Level::Trace); -} - -bool BCLog::Logger::TraceCategory(std::string_view str) -{ - BCLog::LogFlags flag; - if (!GetLogCategory(flag, str)) return false; - TraceCategory(flag); - return true; -} - -void BCLog::Logger::DisableCategory(BCLog::LogFlags flag) -{ - m_categories &= ~flag; -} - -bool BCLog::Logger::DisableCategory(std::string_view str) -{ - BCLog::LogFlags flag; - if (!GetLogCategory(flag, str)) return false; - DisableCategory(flag); - return true; -} - bool BCLog::Logger::WillLogCategory(BCLog::LogFlags category) const { return (m_categories.load(std::memory_order_relaxed) & category) != 0; @@ -272,7 +231,7 @@ static std::string LogCategoryToStr(BCLog::LogFlags category) return it->second; } -static std::optional<BCLog::Level> GetLogLevel(std::string_view level_str) +std::optional<BCLog::Level> BCLog::Logger::GetLogLevel(std::string_view level_str) { if (level_str == "trace") { return BCLog::Level::Trace; @@ -625,6 +584,14 @@ bool BCLog::Logger::SetLogLevel(std::string_view level_str) void BCLog::Logger::SetCategoryLogLevel(BCLog::LogFlags flag, BCLog::Level level) { STDLOCK(m_cs); + Assume(level <= BCLog::Level::Info); + if (level >= BCLog::Level::Info) { + m_categories &= ~flag; + m_category_log_levels.erase(flag); + } else { + m_categories |= flag; + } + if (flag == LogFlags::ALL) { SetLogLevel(level); m_category_log_levels.clear(); @@ -632,15 +599,3 @@ void BCLog::Logger::SetCategoryLogLevel(BCLog::LogFlags flag, BCLog::Level level m_category_log_levels[flag] = level; } } - -bool BCLog::Logger::SetCategoryLogLevel(std::string_view category_str, std::string_view level_str) -{ - BCLog::LogFlags flag; - if (!GetLogCategory(flag, category_str)) return false; - - const auto level = GetLogLevel(level_str); - if (!level.has_value() || level.value() > MAX_USER_SETABLE_SEVERITY_LEVEL) return false; - - SetCategoryLogLevel(flag, level.value()); - return true; -} diff --git a/src/logging.h b/src/logging.h index 46ac348ac4..c894288ab4 100644 --- a/src/logging.h +++ b/src/logging.h @@ -325,7 +325,6 @@ namespace BCLog { m_category_log_levels[category] = level; } void SetCategoryLogLevel(LogFlags flag, Level level) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - bool SetCategoryLogLevel(std::string_view category_str, std::string_view level_str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); Level LogLevel() const { return m_log_level.load(); } void SetLogLevel(Level level) { m_log_level = level; } @@ -333,13 +332,6 @@ namespace BCLog { CategoryMask GetCategoryMask() const { return m_categories.load(); } - void EnableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - bool EnableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - void TraceCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - bool TraceCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - void DisableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - bool DisableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - bool WillLogCategory(LogFlags category) const; bool WillLogCategoryLevel(LogFlags category, Level level) const EXCLUSIVE_LOCKS_REQUIRED(!m_cs); @@ -378,6 +370,7 @@ namespace BCLog { LogPrintFormatInternal<false>(std::move(source_loc), flag, level, std::move(fmt), args...); } + static std::optional<BCLog::Level> GetLogLevel(std::string_view level_str); /** Return true if str parses as a log category and set the flag */ static bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str); }; diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp index 301911fa52..79e8df7bd3 100644 --- a/src/rpc/node.cpp +++ b/src/rpc/node.cpp @@ -202,24 +202,11 @@ static void EnableOrDisableLogCategories(UniValue cats, BCLog::Level new_level) for (unsigned int i = 0; i < cats.size(); ++i) { std::string cat = cats[i].get_str(); - bool success{false}; - switch (new_level) { - case BCLog::Level::Trace: - success = LogInstance().TraceCategory(cat); - break; - case BCLog::Level::Debug: - success = LogInstance().EnableCategory(cat); - break; - case BCLog::Level::Info: - success = LogInstance().DisableCategory(cat); - break; - default: - NONFATAL_UNREACHABLE(); - break; - } - if (!success) { + BCLog::LogFlags flags; + if (!BCLog::Logger::GetLogCategory(flags, cat)) { throw JSONRPCError(RPC_INVALID_PARAMETER, "unknown logging category " + cat); } + LogInstance().SetCategoryLogLevel(flags, new_level); } } diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp index b06752c1e2..26fadcbdcc 100644 --- a/src/test/logging_tests.cpp +++ b/src/test/logging_tests.cpp @@ -95,8 +95,8 @@ struct LogSetup : public BasicTestingSetup { LogInstance().SetLogLevel(prev_log_level); LogInstance().SetCategoryLogLevel(prev_category_levels); LogInstance().SetRateLimiting(nullptr); - LogInstance().DisableCategory(BCLog::LogFlags::ALL); - LogInstance().EnableCategory(BCLog::LogFlags{prev_category_mask}); + LogInstance().SetCategoryLogLevel(BCLog::LogFlags::ALL, BCLog::Level::Info); + LogInstance().SetCategoryLogLevel(BCLog::LogFlags{prev_category_mask}, BCLog::Level::Debug); } }; @@ -139,7 +139,7 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintStr, LogSetup) BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup) { - LogInstance().EnableCategory(BCLog::NET); + LogInstance().SetCategoryLogLevel(BCLog::NET, BCLog::Level::Debug); LogTrace(BCLog::NET, "foo6: %s", "bar6"); // not logged LogDebug(BCLog::NET, "foo7: %s", "bar7"); LogInfo("foo8: %s", "bar8"); @@ -157,7 +157,7 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros, LogSetup) BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros_CategoryName, LogSetup) { - LogInstance().EnableCategory(BCLog::LogFlags::ALL); + LogInstance().SetCategoryLogLevel(BCLog::ALL, BCLog::Level::Debug); const auto concatenated_category_names = LogInstance().LogCategoriesString(); std::vector<std::pair<BCLog::LogFlags, std::string>> expected_category_names; const auto category_names = SplitString(concatenated_category_names, ','); @@ -184,8 +184,8 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintMacros_CategoryName, LogSetup) BOOST_FIXTURE_TEST_CASE(logging_SeverityLevels, LogSetup) { LogInstance().SetLogLevel(BCLog::Level::Debug); - LogInstance().EnableCategory(BCLog::LogFlags::ALL); - LogInstance().SetCategoryLogLevel(/*category_str=*/"net", /*level_str=*/"info"); + LogInstance().SetCategoryLogLevel(BCLog::LogFlags::ALL, BCLog::Level::Debug); + LogInstance().SetCategoryLogLevel(BCLog::LogFlags::NET, BCLog::Level::Info); // Global log level LogInfo("info_%s", 1); @@ -431,8 +431,8 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup) LogInstance().m_log_timestamps = false; LogInstance().m_log_sourcelocations = false; LogInstance().m_log_threadnames = false; - LogInstance().DisableCategory(BCLog::LogFlags::ALL); - LogInstance().EnableCategory(BCLog::LogFlags::HTTP); + LogInstance().SetCategoryLogLevel(BCLog::ALL, BCLog::Level::Info); + LogInstance().SetCategoryLogLevel(BCLog::HTTP, BCLog::Level::Debug); constexpr int64_t line_length{1024}; constexpr int64_t num_lines{10};</details>
stickies-v commented at 8:34 AM on January 3, 2026:Not implemented in my diff, but I think just using
SetCategoryLevelwould also make it quite straightforward to deduplicate the debug/trace parsing logic inSetLoggingCategories.
ajtowns commented at 6:06 AM on January 23, 2026:I don't disagree, but I wanted to keep the API changes small in this PR, and tidy up the internal representation (dropping
m_log_leveland replacing them_category_log_levelsmap with a bitfield) in the next one first.
ajtowns commented at 6:10 AM on January 28, 2026:I've added some cleanup commits in this vein to the end of the followup branch at https://github.com/ajtowns/bitcoin/commits/202512-log-atomic/
in test/functional/feature_logging.py:99 in f565bbce26
95 | + self.restart_node(0, ['-trace=0', '-debug=http', '-debug=abc', disable_debug_opt, '-debug=rpc', '-debug=net']) 96 | + logging = self.nodes[0].logging() 97 | + assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace']) 98 | + assert 'abc' not in (logging['excluded'] + logging['debug'] + logging['trace']) 99 | + assert 'rpc' in logging['debug'] and 'rpc' not in (logging['excluded'] + logging['trace']) 100 | + assert 'net' in logging['debug'] and 'rpc' not in (logging['excluded'] + logging['trace'])
stickies-v commented at 8:07 AM on January 3, 2026:assert 'net' in logging['debug'] and 'net' not in (logging['excluded'] + logging['trace'])in test/functional/feature_logging.py:115 in f565bbce26
115 | - assert logging['rpc'] 116 | - assert logging['net'] 117 | + assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace']) 118 | + assert 'abc' not in (logging['excluded'] + logging['debug'] + logging['trace']) 119 | + assert 'rpc' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug']) 120 | + assert 'net' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug'])
stickies-v commented at 8:08 AM on January 3, 2026:assert 'net' in logging['trace'] and 'net' not in (logging['excluded'] + logging['debug'])in src/init/common.cpp:34 in f565bbce26
33 | ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); 34 | - argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories. This takes priority over \"-debug\"", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); 35 | + argsman.AddArg("-trace=<category>", "Output trace logging (default: -notrace, supplying <category> is optional). " 36 | + "If <category> is not supplied or if <category> is 1 or \"all\", output all trace logging. If <category> is 0 or \"none\", any other categories are ignored. Other valid values for <category> are: " + LogInstance().LogCategoriesString() + ". This option can be specified multiple times to output multiple categories.", 37 | + ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); 38 | + argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories. This takes priority over \"-debug\" and \"-trace\".", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
stickies-v commented at 8:15 AM on January 3, 2026:nit
argsman.AddArg("-debugexclude=<category>", "Exclude debug and trace logging for a category. Can be used in conjunction with -debug=1 or -trace=1 to output debug and trace logging for all categories except the specified category. This option can be specified multiple times to exclude multiple categories. This takes priority over \"-debug\" and \"-trace\".", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);in src/init/common.cpp:77 in f565bbce26 outdated
96 | + return util::Error{strprintf(_("Unsupported logging category %s=%s."), "-debug", cat)}; 97 | + } 98 | + } 99 | 100 | - const auto categories_to_process = (last_negated == categories.rend()) ? categories : std::ranges::subrange(last_negated.base(), categories.end()); 101 | + // Special-case: Disregard any debugging categories appearing before -trace=0/none
stickies-v commented at 8:27 AM on January 3, 2026:// Special-case: Disregard any trace categories appearing before -trace=0/nonein src/test/logging_tests.cpp:83 in f565bbce26 outdated
79 | @@ -79,10 +80,7 @@ struct LogSetup : public BasicTestingSetup { 80 | // Prevent tests from failing when the line number of the logs changes. 81 | LogInstance().m_log_sourcelocations = false; 82 | 83 | - LogInstance().SetLogLevel(BCLog::Level::Debug); 84 | - LogInstance().DisableCategory(BCLog::LogFlags::ALL);
stickies-v commented at 8:39 AM on January 3, 2026:nit: not moving
LogInstance().DisableCategory(BCLog::LogFlags::ALL);means we're no longer resetting them_categoriesfield, not a big issue but probably better to keep this? (also probably a good indication that the current interface is still a bit wonky)in src/test/logging_tests.cpp:216 in f565bbce26
214 | { 215 | ResetLogger(); 216 | ArgsManager args; 217 | - args.AddArg("-loglevel", "...", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST); 218 | - const char* argv_test[] = {"bitcoind", "-loglevel=debug"}; 219 | + args.AddArg("-trace", "...", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
stickies-v commented at 8:46 AM on January 3, 2026:nit: this line can just be removed, we're not passing any
traceargsin src/test/logging_tests.cpp:271 in f565bbce26 outdated
285 | - BOOST_CHECK_EQUAL(http_it->second, BCLog::Level::Info); 286 | + BOOST_CHECK_EQUAL(http_it->second, BCLog::Level::Trace); 287 | + 288 | + const auto mempool_it{category_levels.find(BCLog::LogFlags::MEMPOOL)}; 289 | + BOOST_CHECK(mempool_it != category_levels.end()); 290 | + BOOST_CHECK_EQUAL(mempool_it->second, BCLog::Level::Debug);
stickies-v commented at 8:53 AM on January 3, 2026:I think it's also useful to test here that non-set categories don't show up? e.g.
BOOST_CHECK(!category_levels.contains(BCLog::LogFlags::REINDEX));in src/rpc/node.cpp:231 in f565bbce26
226 | @@ -218,29 +227,43 @@ static RPCHelpMan logging() 227 | { 228 | return RPCHelpMan{"logging", 229 | "Gets and sets the logging configuration.\n" 230 | - "When called without an argument, returns the list of categories with status that are currently being debug logged or not.\n" 231 | - "When called with arguments, adds or removes categories from debug logging and return the lists above.\n" 232 | - "The arguments are evaluated in order \"include\", \"exclude\".\n" 233 | - "If an item is both included and excluded, it will thus end up being excluded.\n" 234 | + "When called without an argument, returns the status of the supported logging categories.\n" 235 | + "When called with arguments, adds or removes categories from debug or trace logging and returns the lists above.\n"
stickies-v commented at 9:05 AM on January 3, 2026:nit: "the lists above" is not really clear, would just duplicate from the line above to highlight that the result is the same:
"When called with arguments, adds or removes categories from debug or trace logging and returns the status of the supported logging categories.\n"in test/functional/feature_logging.py:121 in f565bbce26 outdated
121 | + 122 | + # Check that log values can be changed 123 | + logging = self.nodes[0].logging(["mempool"], ["net"], ["http"]) 124 | + assert 'net' in logging['excluded'] and 'net' not in (logging['debug'] + logging['trace']) 125 | + assert 'mempool' in logging['debug'] and 'mempool' not in (logging['excluded'] + logging['trace']) 126 | + assert 'http' in logging['trace'] and 'http' not in (logging['excluded'] + logging['debug'])
stickies-v commented at 9:11 AM on January 3, 2026:Could be useful to test that previous values aren't deleted?
# Check that previous settings without conflict are preserved assert 'rpc' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug'])in src/rpc/node.cpp:262 in f565bbce26 outdated
268 | + } 269 | + }, 270 | + RPCResult{"Otherwise", 271 | + RPCResult::Type::OBJ, "", "", 272 | + {{ 273 | + {RPCResult::Type::ARR, "excluded", "excluded categories", {{RPCResult::Type::STR, "", ""}}},
stickies-v commented at 9:18 AM on January 3, 2026:"exclude" makes sense when configuring categories, but wouldn't "disabled" or "inactive" be a more appropriate name for the result?
ajtowns commented at 9:05 AM on January 23, 2026:I think if you're going to "exclude" something, it's not surprising if its resulting state is "excluded". I could maybe see an argument to making it disable/disabled everywhere, but even that doesn't seem like much of a win? Maybe "silenced" would be better though?
stickies-v commented at 2:57 AM on March 13, 2026:I could maybe see an argument to making it disable/disabled everywhere
I would prefer that. My natural understanding of exclude is something the user actively did (e.g. debug all categories, but exclude net), whereas "disabled" more neutrally represents the state, regardless of how it got there. However, not a native speaker, so my intuition may be wrong here. Regardless, this is probably going to be subjective, so you can ignore if you disagree.
stickies-v commented at 9:27 AM on January 3, 2026: contributorApproach ACK, reviewed code f565bbce26e52ffdbe3f8ecacd015d603d4eb05e
ajtowns force-pushed on Jan 23, 2026DrahtBot added the label Needs rebase on Jan 27, 2026ajtowns force-pushed on Jan 28, 2026DrahtBot removed the label Needs rebase on Jan 28, 2026DrahtBot added the label Needs rebase on Jan 28, 2026ajtowns force-pushed on Jan 29, 2026DrahtBot removed the label Needs rebase on Jan 29, 2026DrahtBot added the label Needs rebase on Feb 7, 2026ajtowns force-pushed on Feb 11, 2026ajtowns renamed this:logging: API improvements
logging: replace -loglevel with -trace, various API improvements
on Feb 11, 2026ajtowns force-pushed on Feb 11, 2026DrahtBot added the label CI failed on Feb 11, 2026DrahtBot commented at 3:44 AM on February 11, 2026: contributor<!--85328a0da195eb286784d51f73fa0af9-->
🚧 At least one of the CI tasks failed. <sub>Task
iwyu: https://github.com/bitcoin/bitcoin/actions/runs/21891649189/job/63198614944</sub> <sub>LLM reason (✨ experimental): IWYU (Include-What-You-Use) check failed, causing the CI to fail.</sub><details><summary>Hints</summary>
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
</details>
DrahtBot removed the label Needs rebase on Feb 11, 2026DrahtBot removed the label CI failed on Feb 11, 2026ajtowns requested review from stickies-v on Feb 13, 2026DrahtBot added the label Needs rebase on Feb 20, 2026ajtowns force-pushed on Feb 20, 2026DrahtBot added the label CI failed on Feb 20, 2026DrahtBot removed the label Needs rebase on Feb 20, 2026DrahtBot removed the label CI failed on Feb 24, 2026ryanofsky commented at 2:05 PM on March 10, 2026: contributorConcept ACK a98dae5c1caa543b20720db850af09145d401890. Have had a look at the changes here and they seem nice and I plan to review.
One tentative suggestion I'd have is to try to make the PR smaller. The STDLOCK thread annotation change might make sense as a separate PR so it could be easily reviewed by people familiar with thread safety annotations (hebasto, vasild, marco) and merged quickly, without reviewers needing to look into more complicated rpc & logging backend changes.
I also don't see the connection here between the logging backend changes and the developer facing ratelimiting and macro changes, so maybe those changes also don't need to be part of this PR. (I'm hoping over time we have a clearer separation between logging frontend & backend, so for example libbitcoinkernel doesn't need to include a logging backend. If this separation also has benefits for code review that would seem like another win.)
stickies-v commented at 8:39 AM on March 11, 2026: contributorOne tentative suggestion I'd have is to try to make the PR smaller. The STDLOCK thread annotation change might make sense as a separate PR so it could be easily reviewed by people familiar with thread safety annotations (hebasto, vasild, marco) and merged quickly, without reviewers needing to look into more complicated rpc & logging backend changes.
I agree. I was pretty close to signing off on the loglevel changes, but then got stuck on the
STDLOCKchanges - which seem sensible, but not trivial to review, and blocked my ack.ajtowns force-pushed on Mar 11, 2026ajtowns force-pushed on Mar 11, 2026ajtowns commented at 4:18 PM on March 11, 2026: contributorI also don't see the connection here between the logging backend changes and the developer facing ratelimiting and macro changes,
Hmm, I thought those were much more tied together than they apparently are. Consider it split!
EDIT: Split into #34806 (internal logging api refactors) and #34809 (STDLOCK).
ajtowns renamed this:logging: replace -loglevel with -trace, various API improvements
logging: replace -loglevel with -trace, expose trace logging via RPC
on Mar 11, 2026DrahtBot added the label CI failed on Mar 11, 2026DrahtBot removed the label CI failed on Mar 11, 2026ajtowns added this to the milestone 32.0 on Mar 12, 2026in doc/release-notes-34038.md:4 in e54609aedc outdated
0 | @@ -0,0 +1,13 @@ 1 | +Logging 2 | +======= 3 | + 4 | +- A new `-trace` option has been added. The only trace logs currently
stickies-v commented at 2:51 AM on March 13, 2026:I think IPC also has trace logs?
ajtowns commented at 5:12 AM on March 20, 2026:Maybe? Doesn't seem documented anywhere, and the implementation doesn't seem to turn into a no-op if tracing is disabled, so not sure I'd call it "supported".
stickies-v commented at 3:00 PM on April 2, 2026:I'm also not getting any logs when running
./build/bin/bitcoin -m node -signet -trace=ipc | grep -i ipc, so yeah seems fair to leave as-is.in test/functional/test_framework/test_node.py:165 in e54609aedc
161 | self.args.append("-logthreadnames") 162 | if self.version_is_at_least(219900): 163 | self.args.append("-logsourcelocations") 164 | - if self.version_is_at_least(239000): 165 | + 166 | + if self.version_is_at_least(309000):
stickies-v commented at 2:53 AM on March 13, 2026:since this is part of the 32 milestone, I think this can be bumped already
if self.version_is_at_least(319000):in src/rpc/node.cpp:232 in e54609aedc outdated
227 | @@ -219,29 +228,44 @@ static RPCHelpMan logging() 228 | { 229 | return RPCHelpMan{"logging", 230 | "Gets and sets the logging configuration.\n" 231 | - "When called without an argument, returns the list of categories with status that are currently being debug logged or not.\n" 232 | - "When called with arguments, adds or removes categories from debug logging and return the lists above.\n" 233 | - "The arguments are evaluated in order \"include\", \"exclude\".\n" 234 | - "If an item is both included and excluded, it will thus end up being excluded.\n" 235 | + "When called without an argument, returns the status of the supported logging categories.\n" 236 | + "When called with arguments, adds or removes categories from debug or trace logging,\n"
stickies-v commented at 3:21 AM on March 13, 2026:We have quite a bit of complexity wrt interface (documentation), implementation, and tests to deal with the interactions and ordering of these
debug,trace,excludeoptions.What if we do away with all of that, and just let the user deal with ordering through multiple calls? For example, with an interface like
logging ( "level" ["category",...] )thecli logging ["all"] ["net"]call is equivalent tocli logging "debug" && cli logging "disable" ["net"]. The latter is slightly more verbose, but is much more simple and doesn't require the user to keep remembering the (awkward, for backward compatibility purposes) ordering of arguments etc and their evaluation. I don't think atomicity is meaningful here?Orthogonally, I think it also wouldn't be unreasonable to just use "info" instead of "disable{d}"/"excluded" language. The fact that we internally choose to not assign categories to info+ levels shouldn't really concern the user. That way
cli logging "info"becomes a natural way to just reset all logging to info.
ajtowns commented at 5:20 AM on March 20, 2026:What if we do away with all of that, and just let the user deal with ordering through multiple calls?
If we were creating the RPC from scratch, maybe; but doesn't seem worth the additional backwards compat break to me (currently this PR changes the output, but the old invocations still work unchanged).
Orthogonally, I think it also wouldn't be unreasonable to just use "info" instead of "disable{d}"/"excluded" language.
There is no "net info" category of logs. The only "net" logs are debug and trace, so the only logs you can expect to have are "both trace and debug", "just debug" or "none" of them. Disabled or excluded is a better description of the latter than "info".
stickies-v commented at 10:07 AM on March 20, 2026:but doesn't seem worth the additional backwards compat break to me
I personally think if we already introduce a
deprecatedrpc(potentially) breaking change that is most likely going to require some engineering attention for users, meaningfully simplifying the interface and cleaning up what feels like tech debt is the right thing to do.There is no "net info" category of logs.
I'm aware, but our users shouldn't have to care about that. If there are no "net info" logs, then setting "net" to "info" will produce 0 logs. I'm not advocating for this because I later on want to start adding categories to info logs (I have no interest in that conversation), I'm suggesting this because i think for a logging user who's not intimately familiar with our codebase, that's much more intuitive.
ajtowns commented at 1:28 PM on March 20, 2026:I personally think if we already introduce a deprecatedrpc (potentially) breaking change that is most likely going to require some engineering attention for users, [...]
I don't expect the deprecatedrpc option this PR introduces to see much if any use; evaluating the result of the
loggingcall is likely something that people only do visually, rather than automate. OTOH, making the call to turn logging off or on seems somewhat more likely to be automated.In any event if you really want to do that, there's plenty of time for you to do so in a followup before 32.x is out if you want to expand the scope of API breakage. I don't think that's a very good use of anyone's time personally (my opinion in general on breaking existing uses in order to clean things up is at an all time low and still sinking fast), but either way I don't think it needs to be a blocker for this PR.
Re: "<category> info" logs: I strongly disagree. Someone who's not intimately familiar with our codebase likely doesn't even know the term "info" for logging, which only appears either in our code (
LogInfo) or if you enable the-loglevelalwaysoption, or if you use the experimental-logleveloption that this PR removes in a way that would hard disable debug/trace logging for all categories.If there are no "net info" logs, then setting "net" to "info" will produce 0 logs.
The same argument would equally well support naming the setting "unicorns".
stickies-v commented at 3:21 PM on April 2, 2026:I don't expect the deprecatedrpc option this PR introduces to see much if any use ... but either way I don't think it needs to be a blocker for this PR.
Fair, not all breaking changes are equally breaking, and I agree we should properly weigh the impact this has on users. I'll see if I want to open a follow-up, I suspect I probably won't.
Someone who's not intimately familiar with our codebase likely doesn't even know the term "info" for logging, which only appears either in our code
The same argument would equally well support naming the setting "unicorns".
"Info" is standard among logging libraries, so I don't think it's unreasonable or a bad idea to generally align with that language instead of our bespoke way of doing things when it's not meaningfully better. Anyway, I understand you don't want to make this change here.
stickies-v commented at 3:37 AM on March 13, 2026: contributorThanks for splitting it up!
DrahtBot added the label Needs rebase on Mar 20, 2026ajtowns force-pushed on Mar 20, 2026DrahtBot removed the label Needs rebase on Mar 20, 2026DrahtBot added the label Needs rebase on Mar 24, 2026ajtowns force-pushed on Mar 24, 2026DrahtBot removed the label Needs rebase on Mar 24, 2026DrahtBot added the label Needs rebase on Mar 31, 2026ajtowns force-pushed on Apr 2, 2026DrahtBot removed the label Needs rebase on Apr 2, 2026in src/logging.h:269 in 6eee953bb8
268 | + void EnableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 269 | + bool EnableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 270 | + void TraceCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 271 | + bool TraceCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 272 | + void DisableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); 273 | + bool DisableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
stickies-v commented at 2:41 PM on April 2, 2026:DisableCategorydoesn't accessm_cs, I think these annotations can be removed?<details> <summary>git diff on 6eee953bb8</summary>
diff --git a/src/logging.h b/src/logging.h index d5c33d5096..ef9b1ecc5d 100644 --- a/src/logging.h +++ b/src/logging.h @@ -265,8 +265,8 @@ namespace BCLog { bool EnableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); void TraceCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); bool TraceCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - void DisableCategory(LogFlags flag) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); - bool DisableCategory(std::string_view str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs); + void DisableCategory(LogFlags flag); + bool DisableCategory(std::string_view str); bool WillLogCategory(LogFlags category) const; bool WillLogCategoryLevel(LogFlags category, Level level) const EXCLUSIVE_LOCKS_REQUIRED(!m_cs);</details>
in src/rpc/node.cpp:317 in 6eee953bb8 outdated
314 | + break; 315 | + case BCLog::Level::Info: 316 | + exc.push_back(info.category); 317 | + break; 318 | + default: 319 | + break;
stickies-v commented at 3:28 PM on April 2, 2026:I think this should also be
NONFATAL_UNREACHABLENONFATAL_UNREACHABLE(); break;in test/functional/rpc_misc.py:80 in 6eee953bb8
76 | @@ -77,15 +77,28 @@ def run_test(self): 77 | self.log.info("test logging rpc and help") 78 | 79 | # Test toggling a logging category on/off/on with the logging RPC. 80 | - assert_equal(node.logging()['qt'], True) 81 | + assert('qt' in node.logging()['trace'])
stickies-v commented at 3:33 PM on April 2, 2026:nit:
assertis a keyword in python, not a function<details> <summary>git diff on 6eee953bb8</summary>
diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py index a867444f66..43cbcbf19c 100755 --- a/test/functional/rpc_misc.py +++ b/test/functional/rpc_misc.py @@ -77,15 +77,15 @@ class RpcMiscTest(BitcoinTestFramework): self.log.info("test logging rpc and help") # Test toggling a logging category on/off/on with the logging RPC. - assert('qt' in node.logging()['trace']) + assert 'qt' in node.logging()['trace'] node.logging(exclude=['qt']) - assert('qt' in node.logging()['excluded']) + assert 'qt' in node.logging()['excluded'] node.logging(debug=['qt']) - assert('qt' in node.logging()['debug']) + assert 'qt' in node.logging()['debug'] node.logging(trace=['qt']) - assert('qt' in node.logging()['trace']) + assert 'qt' in node.logging()['trace'] node.logging(include=['qt']) - assert('qt' in node.logging()['debug']) + assert 'qt' in node.logging()['debug'] # Test logging RPC returns the logging categories in alphabetical order. logging = node.logging()</details>
in test/functional/feature_logging.py:97 in 6eee953bb8
91 | @@ -109,12 +92,35 @@ def run_test(self): 92 | 93 | for disable_debug_opt in disable_debug_options: 94 | # Every category before disable_debug_opt will be ignored, including the invalid 'abc' 95 | - self.restart_node(0, ['-debug=http', '-debug=abc', disable_debug_opt, '-debug=rpc', '-debug=net']) 96 | + self.restart_node(0, ['-trace=0', '-debug=http', '-debug=abc', disable_debug_opt, '-debug=rpc', '-debug=net']) 97 | + logging = self.nodes[0].logging() 98 | + assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace'])
stickies-v commented at 4:11 PM on April 2, 2026:nit: helper functions could clean up this verbosity quite a bit:
<details> <summary>git diff on 6eee953bb8</summary>
diff --git a/test/functional/feature_logging.py b/test/functional/feature_logging.py index 3fcfd71615..07a82debb1 100755 --- a/test/functional/feature_logging.py +++ b/test/functional/feature_logging.py @@ -90,14 +90,21 @@ class LoggingTest(BitcoinTestFramework): '-nodebug' ] + def is_excluded(logging, cat): + return cat in logging['excluded'] and cat not in logging['debug'] and cat not in logging['trace'] + def is_debug(logging, cat): + return cat in logging['debug'] and cat not in logging['excluded'] and cat not in logging['trace'] + def is_trace(logging, cat): + return cat in logging['trace'] and cat not in logging['excluded'] and cat not in logging['debug'] + for disable_debug_opt in disable_debug_options: # Every category before disable_debug_opt will be ignored, including the invalid 'abc' self.restart_node(0, ['-trace=0', '-debug=http', '-debug=abc', disable_debug_opt, '-debug=rpc', '-debug=net']) logging = self.nodes[0].logging() - assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace']) + assert is_excluded(logging, 'http') assert 'abc' not in (logging['excluded'] + logging['debug'] + logging['trace']) - assert 'rpc' in logging['debug'] and 'rpc' not in (logging['excluded'] + logging['trace']) - assert 'net' in logging['debug'] and 'net' not in (logging['excluded'] + logging['trace']) + assert is_debug(logging, 'rpc') + assert is_debug(logging, 'net') self.log.info("Test that -notrace,-trace=0,-trace=none clear previously specified trace options") disable_trace_options = [ @@ -110,17 +117,17 @@ class LoggingTest(BitcoinTestFramework): # Every category before disable_trace_opt will be ignored, including the invalid 'abc' self.restart_node(0, ['-debug=0', '-trace=http', '-trace=abc', disable_trace_opt, '-trace=rpc', '-trace=net']) logging = self.nodes[0].logging() - assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace']) + assert is_excluded(logging, 'http') assert 'abc' not in (logging['excluded'] + logging['debug'] + logging['trace']) - assert 'rpc' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug']) - assert 'net' in logging['trace'] and 'net' not in (logging['excluded'] + logging['debug']) + assert is_trace(logging, 'rpc') + assert is_trace(logging, 'net') # Check that log values can be changed logging = self.nodes[0].logging(["mempool"], ["net"], ["http"]) - assert 'net' in logging['excluded'] and 'net' not in (logging['debug'] + logging['trace']) - assert 'mempool' in logging['debug'] and 'mempool' not in (logging['excluded'] + logging['trace']) - assert 'http' in logging['trace'] and 'http' not in (logging['excluded'] + logging['debug']) - assert 'rpc' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug']) # unchanged + assert is_excluded(logging, 'net') + assert is_debug(logging, 'mempool') + assert is_trace(logging, 'http') + assert is_trace(logging, 'rpc') # unchanged self.log.info("Test -logips formatting in net logs") self.restart_node(0, ['-debug=net', '-logips=1'])</details>
in src/logging.cpp:306 in 6eee953bb8 outdated
304 | - ret.push_back(LogCategory{.category = category, .active = WillLogCategory(flag)}); 305 | + BCLog::Level level = base; 306 | + if (!WillLogCategory(flag)) { 307 | + level = BCLog::Level::Info; 308 | + } else { 309 | + const auto it{m_category_log_levels.find(flag)};
stickies-v commented at 4:22 PM on April 2, 2026:Review note: my understanding is that we could just as well only use this map for
Traceoverrides, but I think you're cleaning up this map logic in a follow-up so there's not much point overhauling things more than necessary?
ajtowns commented at 6:05 PM on April 2, 2026:Yeah
in doc/release-notes-34038.md:13 in 6eee953bb8 outdated
8 | +- The `logging` RPC command is now able to switch trace level logs on 9 | + and off, via its third parameter. In order to distinguish categories 10 | + enabled for debug logging only, enabled for trace logging as well, 11 | + or not enabled for debug logging at all, it uses a different output 12 | + format. The `-deprecatedrpc=logging` startup option can currently be 13 | + used to revert to the old format. (PR#34038)
stickies-v commented at 4:31 PM on April 2, 2026:nit: took me a few reads to properly parse the second sentence. Attempted simplification:
- The `logging` RPC command is now able to switch trace level logs on and off, via its third parameter. Since a boolean can no longer represent a category's state, the output format is changed to three arrays (excluded, debug, trace) showing each category's logging level.. The `-deprecatedrpc=logging` startup option can currently be used to revert to the old format. (PR#34038)stickies-v approvedstickies-v commented at 4:35 PM on April 2, 2026: contributorACK 6eee953bb8ee6fe465838fa7ce6cb4ff64ade543
The new
-debugand-tracestartup options are a lot easier to use, and although I think the RPC interface is getting increasingly unwieldy, this seems to be the best approach minimizing breaking changes to expose the trace level there too. There's quite a few places where internal logging logic could be cleaned up (left various comments in previous reviews), but my understanding is many of these will be addressed in future work that is better done in separate PRs, which I agree with.DrahtBot requested review from ryanofsky on Apr 2, 20267e4b8887f8logging: replace -loglevel arg with -trace
Previously, trace logging for a category was enabled by saying "-debug=net -loglevel=net:trace" and saying "-loglevel=net:trace" (without "-debug" or "-debug=net") would silently do nothing. Simplify this so that "-debug=net" enables debug logging for net and "-trace=net" enables both debug and trace logging for net. Note that "-nodebug" and "-notrace" only disable previous "-debug=xxx" and "-trace=yyy" arguments, so "-debug=mempool -trace=net -nodebug" will cancel "-debug=mempool", but will still include both debug and trace logs for net.
976c1c07e3rpc: Update logging RPC to support trace level debugging
Also changes the output format, to distinguish debug/trace levels. Provides `-deprecatedrpc=logging` if old output format is needed. Co-Authored-By: stickies-v <stickies-v@protonmail.com>
[doc] Release notes for user-visible logging changes 286ef780fdajtowns force-pushed on Apr 2, 2026ajtowns commented at 6:21 PM on April 2, 2026: contributorBumped to address review comments.
stickies-v commented at 2:35 AM on April 3, 2026: contributorre-ACK 286ef780fd77f3f30d56e69a8a7f83c1736ea87f
ContributorsLabelsMilestone
32.0
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-26 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
