logging: API improvements #34038

ajtowns · 2025-12-10T05:56:20Z

A few changes:

Provide STDLOCK(m_cs) instead of StdLockGuard g(m_cs) for the logging mutexes, so that we can get better warnings from clang if annotations have been missed
Allow writing LogInfo(BCLog::NO_RATE_LIMIT, "log message") to avoid rate limiting, rather than having to supply a false argument to LogPrintLevel_(...)
Replace the -loglevel parameter with a new -trace parameter that works the same as -debug
Change the logging RPC to allow enabling trace debugging, and to differentiate trace vs debug levels

DrahtBot · 2025-12-10T05:56:28Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34038.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Approach ACK	stickies-v

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33646 (log: check fclose() results and report safely in logging.cpp by cedwies)
#33215 (Fix compatibility with -debuglogfile command-line option by hebasto)
#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.

DrahtBot · 2025-12-10T06:01:42Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20088899137/job/57632110650}
_{LLM reason (✨ experimental): Lint failure: the RPC code uses a fatal assert (rpc_assert), triggering the linter to fail.}

Hints

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.

ajtowns · 2025-12-10T06:04:15Z

The reason for the locking change for ShrinkDebugFile is that the following commit is enough for clang to figure out that the LogWarning("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.

stickies-v

Concept ACK. Both the new BCLog::NO_RATE_LIMIT and -loglevel changes 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.

stickies-v · 2025-12-10T11:19:23Z

src/rpc/node.cpp

@@ -235,12 +244,26 @@ static RPCHelpMan logging()
                        {
                            {"exclude_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the valid logging category"},
                        }},
+                    {"trace", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to trace logging",


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.

Seems like a fine idea, but not sure how you'd provide compatibility for include=["net"] with deprecaterpc?

I think this should work?

git diff on 77f272c094

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()

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 include in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds running deprecatedrpc=logging.

Also left include in the client side so that bitcoin-cli still works with old bitcoinds or bitcoinds

I'm not sure that's feasible without also adding a -deprecatedrpc option to cli (unless that already exists), which I don't think is worth it? Afaik we don't support disjoint bitcoind and bitcoin-cli versions, and I think bitcoin-cli is 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).

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, and debug=["net"] won't work because you're running with deprecatedrpc=logging)

StdLockGuard did not ensure that the lock it was taking was not already held. Add a macro and an annotated StdMutex::CheckNotHeld() function to correct that. Renames StdLockGuard to StdMutex::Guard, though that should only be used through the STDLOCK macro in order to get better thread safety checking.

We should not be logging while shrinking the debug file, so make sure that's true by using our mutex.

After the previous commit, LogPrintLevel_ is only used to implement other macros.

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.

Also changes the output format, to distinguish debug/trace levels. Provides `-deprecatedrpc=logging` if old ouptut format is needed.

ajtowns · 2025-12-21T03:06:43Z

Rebased on top of other logging changes, added a release note

stickies-v · 2026-01-02T08:05:21Z

src/threadsafety.h

-};

-// StdLockGuard provides an annotated version of std::lock_guard for us,
-// and should only be used when sync.h Mutex/LOCK/etc are not usable.


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?

stickies-v · 2026-01-02T08:36:40Z

src/logging.h

-    };

+        template <bool should_ratelimit=true, typename... Args>
+        inline void LogPrintFormatInternal(SourceLocation&& source_loc, BCLog::LogFlags flag, BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)


nit: can we multi-line these long signatures?

stickies-v · 2026-01-02T08:54:06Z

src/logging.h

        }
+
+        /** Return true if str parses as a log category and set the flag */
+        static bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);


in commit 08a6247: would be nice to get rid of the out-parameter

git diff on 08a6247

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;

stickies-v · 2026-01-02T09:07:02Z

src/threadsafety.h

+        ~Guard() UNLOCK_FUNCTION() = default;
+    };
+
+    static inline StdMutex& CheckNotHeld(StdMutex& cs) EXCLUSIVE_LOCKS_REQUIRED(!cs) LOCK_RETURNED(cs) { return cs; }


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?

stickies-v · 2026-01-02T10:16:12Z

src/logging.cpp

    StartLogging();
 }

 void BCLog::Logger::EnableCategory(BCLog::LogFlags flag)


Should we rename this to DebugCategory now for consistency?

stickies-v · 2026-01-02T10:19:28Z

doc/release-notes-34038.md

+
+- A new `-trace` option has been added. The only trace logs currently
+  supported are detailed logs about wallet sql statements, which can
+  be accessed via `-trace=wallet`. The `-loglevel` configuration option


Suggested change

be accessed via `-trace=wallet`. The `-loglevel` configuration option

be accessed via `-trace=walletdb`. The `-loglevel` configuration option

stickies-v · 2026-01-02T10:39:34Z

src/logging.cpp

 void BCLog::Logger::EnableCategory(BCLog::LogFlags flag)
 {
    m_categories |= flag;
+    SetCategoryLogLevel(flag, Level::Debug);


Shouldn't DisableCategory unset this, too?

stickies-v · 2026-01-02T12:40:40Z

src/logging.h

+        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);


I'm a bit unsure about the merit of these functions. I think just using SetCategoryLogLevel is a bit more straightforward, which can also be simplified now that we don't have -loglevel anymore. I think the string overloads could also be removed, and move input validation/parsing to the edge rather than in logging.

git diff on f565bbc

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};

Not implemented in my diff, but I think just using SetCategoryLevel would also make it quite straightforward to deduplicate the debug/trace parsing logic in SetLoggingCategories.

stickies-v

Approach ACK, reviewed code f565bbc

stickies-v · 2026-01-03T08:07:42Z

test/functional/feature_logging.py

+            assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace'])
+            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 'rpc' not in (logging['excluded'] + logging['trace'])


Suggested change

assert 'net' in logging['debug'] and 'rpc' not in (logging['excluded'] + logging['trace'])

assert 'net' in logging['debug'] and 'net' not in (logging['excluded'] + logging['trace'])

stickies-v · 2026-01-03T08:08:07Z

test/functional/feature_logging.py

+            assert 'http' in logging['excluded'] and 'http' not in (logging['debug'] + logging['trace'])
+            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 'rpc' not in (logging['excluded'] + logging['debug'])


Suggested change

assert 'net' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug'])

assert 'net' in logging['trace'] and 'net' not in (logging['excluded'] + logging['debug'])

stickies-v · 2026-01-03T08:15:01Z

src/init/common.cpp

+    argsman.AddArg("-trace=<category>", "Output trace logging (default: -notrace, supplying <category> is optional). "
+        "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.",
+        ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
+    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);


nit

Suggested change

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);

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);

stickies-v · 2026-01-03T08:27:24Z

src/init/common.cpp

+    }

-        const auto categories_to_process = (last_negated == categories.rend()) ? categories : std::ranges::subrange(last_negated.base(), categories.end());
+    // Special-case: Disregard any debugging categories appearing before -trace=0/none


Suggested change

// Special-case: Disregard any debugging categories appearing before -trace=0/none

// Special-case: Disregard any trace categories appearing before -trace=0/none

stickies-v · 2026-01-03T08:34:12Z

src/logging.h

+        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);


Not implemented in my diff, but I think just using SetCategoryLevel would also make it quite straightforward to deduplicate the debug/trace parsing logic in SetLoggingCategories.

stickies-v · 2026-01-03T08:46:00Z

src/test/logging_tests.cpp

        ArgsManager args;
-        args.AddArg("-loglevel", "...", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
-        const char* argv_test[] = {"bitcoind", "-loglevel=debug"};
+        args.AddArg("-trace", "...", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);


nit: this line can just be removed, we're not passing any trace args

stickies-v · 2026-01-03T08:53:44Z

src/test/logging_tests.cpp

+
+        const auto mempool_it{category_levels.find(BCLog::LogFlags::MEMPOOL)};
+        BOOST_CHECK(mempool_it != category_levels.end());
+        BOOST_CHECK_EQUAL(mempool_it->second, BCLog::Level::Debug);


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));

stickies-v · 2026-01-03T09:05:29Z

src/rpc/node.cpp

-            "The arguments are evaluated in order \"include\", \"exclude\".\n"
-            "If an item is both included and excluded, it will thus end up being excluded.\n"
+            "When called without an argument, returns the status of the supported logging categories.\n"
+            "When called with arguments, adds or removes categories from debug or trace logging and returns the lists above.\n"


nit: "the lists above" is not really clear, would just duplicate from the line above to highlight that the result is the same:

Suggested change

"When called with arguments, adds or removes categories from debug or trace logging and returns the lists above.\n"

"When called with arguments, adds or removes categories from debug or trace logging and returns the status of the supported logging categories.\n"

stickies-v · 2026-01-03T09:11:03Z

test/functional/feature_logging.py

+            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'])


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'])

stickies-v · 2026-01-03T09:18:21Z

src/rpc/node.cpp

+                    RPCResult{"Otherwise",
+                        RPCResult::Type::OBJ, "", "",
+                        {{
+                            {RPCResult::Type::ARR, "excluded", "excluded categories", {{RPCResult::Type::STR, "", ""}}},


"exclude" makes sense when configuring categories, but wouldn't "disabled" or "inactive" be a more appropriate name for the result?

ajtowns force-pushed the 202512-log-niceties branch from 9320a64 to 90d6db5 Compare December 10, 2025 06:00

DrahtBot added the CI failed label Dec 10, 2025

ajtowns force-pushed the 202512-log-niceties branch from 90d6db5 to 1df5691 Compare December 10, 2025 06:08

fanquake added the Needs release note label Dec 10, 2025

stickies-v reviewed Dec 10, 2025

View reviewed changes

This was referenced Dec 10, 2025

log: check fclose() results and report safely in logging.cpp #33646

Open

Fix compatibility with -debuglogfile command-line option #33215

Draft

ajtowns force-pushed the 202512-log-niceties branch 2 times, most recently from 77f272c to bee6ec5 Compare December 10, 2025 23:26

DrahtBot removed the CI failed label Dec 11, 2025

ajtowns force-pushed the 202512-log-niceties branch 2 times, most recently from 46a8129 to 2f71e41 Compare December 11, 2025 03:48

DrahtBot added the CI failed label Dec 11, 2025

ajtowns force-pushed the 202512-log-niceties branch 3 times, most recently from e4d30ec to d184595 Compare December 11, 2025 14:10

DrahtBot removed the CI failed label Dec 11, 2025

DrahtBot mentioned this pull request Dec 12, 2025

log: Remove brittle and confusing LogPrintLevel #34051

Merged

DrahtBot added the Needs rebase label Dec 14, 2025

ajtowns force-pushed the 202512-log-niceties branch from d184595 to 2bdaf02 Compare December 15, 2025 03:57

DrahtBot removed the Needs rebase label Dec 15, 2025

This comment was marked as abuse.

Sign in to view

DrahtBot added the Needs rebase label Dec 18, 2025

ajtowns mentioned this pull request Dec 20, 2025

RFC: separate kernel logging infrastructure #34062

Open

ajtowns added 7 commits December 20, 2025 15:11

logging: Protect ShrinkDebugFile by m_cs

f3dbad4

We should not be logging while shrinking the debug file, so make sure that's true by using our mutex.

logging: Provide BCLog::NO_RATE_LIMIT to avoid rate limits

480c501

logging: Rename LogPrintLevel_ into detail_ namespace

543742d

After the previous commit, LogPrintLevel_ is only used to implement other macros.

logging: Move GetLogCategory into Logger class

08a6247

rpc: Update logging RPC to support trace level debugging

c663558

Also changes the output format, to distinguish debug/trace levels. Provides `-deprecatedrpc=logging` if old ouptut format is needed.

ajtowns force-pushed the 202512-log-niceties branch from 2bdaf02 to c663558 Compare December 20, 2025 05:19

[doc] Release notes for user-visible logging changes

f565bbc

DrahtBot added CI failed and removed Needs rebase CI failed labels Dec 20, 2025

stickies-v reviewed Jan 2, 2026

View reviewed changes

stickies-v reviewed Jan 3, 2026

View reviewed changes

	be accessed via `-trace=wallet`. The `-loglevel` configuration option
	be accessed via `-trace=walletdb`. The `-loglevel` configuration option

	assert 'net' 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 'net' in logging['trace'] and 'rpc' not in (logging['excluded'] + logging['debug'])
	assert 'net' in logging['trace'] and 'net' not in (logging['excluded'] + logging['debug'])

	// Special-case: Disregard any debugging categories appearing before -trace=0/none
	// Special-case: Disregard any trace categories appearing before -trace=0/none

	"When called with arguments, adds or removes categories from debug or trace logging and returns the lists above.\n"
	"When called with arguments, adds or removes categories from debug or trace logging and returns the status of the supported logging categories.\n"

logging: API improvements #34038

Are you sure you want to change the base?

logging: API improvements #34038

Uh oh!

Conversation

ajtowns commented Dec 10, 2025

Uh oh!

DrahtBot commented Dec 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Dec 10, 2025

Uh oh!

ajtowns commented Dec 10, 2025

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

ajtowns commented Dec 21, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

DrahtBot commented Dec 10, 2025 •

edited

Loading

stickies-v Dec 11, 2025 •

edited

Loading

stickies-v Jan 2, 2026 •

edited

Loading