logging: API improvements #34038

pull ajtowns wants to merge 5 commits into bitcoin:master from ajtowns:202512-log-niceties changing 17 files +336 −201
  1. ajtowns commented at 5:56 am on December 10, 2025: contributor

    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
  2. threadsafety: Add STDLOCK() and improve annotation checking for StdMutex
    StdLockGuard did not ensure that the lock it was taking was not already held.
    Add a macro and an annotated StdCheckNotHeld() 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.
    0cac5364e6
  3. logging: Protect ShrinkDebugFile by m_cs
    We should not be logging while shrinking the debug file, so make sure
    that's true by using our mutex.
    3e22b772fa
  4. logging: Provide BCLog::NO_RATE_LIMIT to avoid rate limits 0aacb71627
  5. DrahtBot commented at 5:56 am on December 10, 2025: contributor

    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
    Concept ACK stickies-v

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #34051 (log: Remove brittle and confusing LogPrintLevel by maflcko)
    • #33646 (log: check fclose() results and report safely in logging.cpp by cedwies)
    • #33215 (Fix compatibility with -debuglogfile command-line option by hebasto)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    LLM Linter (✨ experimental)

    Possible typos and grammar issues:

    • eg -> e.g. [standard abbreviation punctuation; improves clarity in documentation example]
    • tagging not to rate limit -> tagging to not rate limit [word order makes the intent slightly unclear; this reads more naturally and is easier to parse]

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • self.nodes[0].logging([“mempool”], [“net”], [“http”]) in test/functional/feature_logging.py

    2025-12-11

  6. ajtowns force-pushed on Dec 10, 2025
  7. DrahtBot added the label CI failed on Dec 10, 2025
  8. DrahtBot commented at 6:01 am on December 10, 2025: contributor

    🚧 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.

    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.

  9. ajtowns commented at 6:04 am on December 10, 2025: contributor
    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.
  10. ajtowns force-pushed on Dec 10, 2025
  11. 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?

     0diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
     1index 0693b60557..25c313c552 100644
     2--- a/src/rpc/client.cpp
     3+++ b/src/rpc/client.cpp
     4@@ -325,7 +325,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     5     { "psbtbumpfee", 1, "replaceable"},
     6     { "psbtbumpfee", 1, "outputs"},
     7     { "psbtbumpfee", 1, "original_change_index"},
     8-    { "logging", 0, "include" },
     9+    { "logging", 0, "debug" },
    10     { "logging", 1, "exclude" },
    11     { "logging", 2, "trace" },
    12     { "disconnectnode", 1, "nodeid" },
    13diff --git a/src/rpc/node.cpp b/src/rpc/node.cpp
    14index abca5e69bd..06714bde37 100644
    15--- a/src/rpc/node.cpp
    16+++ b/src/rpc/node.cpp
    17@@ -225,20 +225,22 @@ static void EnableOrDisableLogCategories(UniValue cats, BCLog::Level new_level)
    18 
    19 static RPCHelpMan logging()
    20 {
    21+    const bool use_deprecated = IsDeprecatedRPCEnabled("logging");
    22+    const std::string debug_category_name{use_deprecated ? "include" : "debug"};
    23     return RPCHelpMan{"logging",
    24             "Gets and sets the logging configuration.\n"
    25             "When called without an argument, returns the the status of the supported logging categories.\n"
    26             "When called with arguments, adds or removes categories from debug or trace logging and return the lists above.\n"
    27-            "The arguments are evaluated in order \"include\", \"trace\", \"exclude\".\n"
    28-            "If an item is both included/traced and excluded, it will thus end up being excluded.\n"
    29+            "The arguments are evaluated in order \"" + debug_category_name + "\", \"trace\", \"exclude\".\n"
    30+            "If an item is debug and/or trace but also excluded, it will thus end up being excluded.\n"
    31             "The valid logging categories are: " + LogInstance().LogCategoriesString() + "\n"
    32             "In addition, the following are available as category names with special meanings:\n"
    33             "  - \"all\",  \"1\" : represent all logging categories.\n"
    34             ,
    35                 {
    36-                    {"include", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to debug logging",
    37+                    {debug_category_name, RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to add to debug logging",
    38                         {
    39-                            {"include_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the valid logging category"},
    40+                            {"debug_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the debug logging category"},
    41                         }},
    42                     {"exclude", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The categories to remove from debug logging",
    43                         {
    44@@ -249,14 +251,14 @@ static RPCHelpMan logging()
    45                             {"trace_category", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "the valid logging category"},
    46                         }},
    47                 },
    48-                {
    49-                    RPCResult{"If -deprecatedrpc=logging is set",
    50+                {   use_deprecated ?
    51+                    RPCResult{
    52                         RPCResult::Type::OBJ_DYN, "", "keys are the logging categories, and values indicates its status",
    53                         {
    54                             {RPCResult::Type::BOOL, "category", "if being debug logged or not. false:inactive, true:active"},
    55                         }
    56-                    },
    57-                    RPCResult{"Otherwise",
    58+                    } :
    59+                    RPCResult{
    60                         RPCResult::Type::OBJ, "", "",
    61                         {{
    62                             {RPCResult::Type::ARR, "excluded", "excluded categories", {{RPCResult::Type::STR, "", ""}}},
    63@@ -289,7 +291,6 @@ static RPCHelpMan logging()
    64         UpdateHTTPServerLogging(LogInstance().WillLogCategory(BCLog::LIBEVENT));
    65     }
    66 
    67-    bool use_deprecated = IsDeprecatedRPCEnabled("logging");
    68     UniValue result(UniValue::VOBJ);
    69     if (use_deprecated) {
    70         for (const auto& info : LogInstance().LogCategoriesInfo()) {
    71diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py
    72index fa1de774b7..4dfe13ec99 100755
    73--- a/test/functional/rpc_misc.py
    74+++ b/test/functional/rpc_misc.py
    75@@ -80,10 +80,12 @@ class RpcMiscTest(BitcoinTestFramework):
    76         assert('qt' in node.logging()['trace'])
    77         node.logging(exclude=['qt'])
    78         assert('qt' in node.logging()['excluded'])
    79-        node.logging(include=['qt'])
    80+        node.logging(debug=['qt'])
    81         assert('qt' in node.logging()['debug'])
    82         node.logging(trace=['qt'])
    83         assert('qt' in node.logging()['trace'])
    84+        # "include" is now deprecated in favour of "debug"
    85+        assert_raises_rpc_error(-8, "Unknown named parameter include", node.logging, include="net")
    86 
    87         # Test logging RPC returns the logging categories in alphabetical order.
    88         logging = node.logging()
    89@@ -125,6 +127,10 @@ class RpcMiscTest(BitcoinTestFramework):
    90         # Specifying an unknown index name returns an empty result
    91         assert_equal(node.getindexinfo("foo"), {})
    92 
    93+        # Test that deprecatedrpc="logging" maintains behaviour
    94+        self.restart_node(0, ["-deprecatedrpc=logging"])
    95+        node.logging(include=["net"])
    96+        assert_raises_rpc_error(-8, "Unknown named parameter debug", node.logging, debug="net")
    97 
    98 if __name__ == '__main__':
    99     RpcMiscTest(__file__).main()
    

    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 include in 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 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).


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

  12. fanquake added the label Needs release note on Dec 10, 2025
  13. stickies-v commented at 11:37 am on December 10, 2025: contributor
    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.
  14. ajtowns force-pushed on Dec 10, 2025
  15. logging: 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.
    4dc5c72b12
  16. ajtowns force-pushed on Dec 10, 2025
  17. DrahtBot removed the label CI failed on Dec 11, 2025
  18. ajtowns force-pushed on Dec 11, 2025
  19. ajtowns force-pushed on Dec 11, 2025
  20. DrahtBot added the label CI failed on Dec 11, 2025
  21. ajtowns force-pushed on Dec 11, 2025
  22. ajtowns force-pushed on Dec 11, 2025
  23. rpc: Update logging RPC to support trace level debugging
    Also changes the output format, to distinguish debug/trace
    levels. Provides `-deprecatedrpc=logging` if old ouptut format is needed.
    d18459500b
  24. ajtowns force-pushed on Dec 11, 2025
  25. DrahtBot removed the label CI failed on Dec 11, 2025
  26. DrahtBot added the label Needs rebase on Dec 14, 2025
  27. DrahtBot commented at 2:08 pm on December 14, 2025: contributor
    🐙 This pull request conflicts with the target branch and needs rebase.

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: 2025-12-15 00:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me