log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError #32604

pull Crypt-iQ wants to merge 5 commits into bitcoin:master from Crypt-iQ:log_ratelimiting_05192025 changing 8 files +466 −62
  1. Crypt-iQ commented at 8:48 pm on May 23, 2025: contributor

    This revives the work done by dergoegge in #21603. The approach is similar — this PR uses std::source_location under the hood now that we can use c++20 features. It also resets the rate limiting statistics via the CScheduler. The logging functions have also changed slightly since that PR was opened, so work has been done to preserve the intent of the original rate limiting change. I have tried to give commit attribution where possible.

    Approach: Each source code location is given an hourly logging quota of 1MiB of logging per hour. Logging is only dropped from source locations that exceed the quota.

    • Only logging to disk is rate limited. Logging to console is not rate limited.
    • Printing with the category argument is not rate limited.
    • UpdateTip: new best=[…] is logged without rate limiting. High log volume is expected for that source location during IBD.
    • When logging is restarted a tally of how many bytes were dropped is printed.
    • All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed.

    I’ve repurposed the old logging rpc mentioned in #21603 in another branch for testing here. This can be used to log from source locations and test out the new changes in logging behavior. Note that the setmocktime RPC needs to be used to set the mock time past the current clock time to reset the logging messages.

    Example usage:

    0bitcoin-cli -regtest excessivelog 1 1048500 # log just under 1MiB
    1bitcoin-cli -regtest excessivelog 1 100 # this should get the total amount logged above 1MiB
    2                                        # and the rate limiting logic should kick in
    3bitcoin-cli -regtest excessivelog 2 1048500
    4bitcoin-cli -regtest excessivelog 2 100 # trigger rate limiting from another location
    5bitcoin-cli -regtest mockscheduler 3600 # fast-forward the scheduler
    6bitcoin-cli -regtest excessivelog 1 100 # this should trigger logging to resume and will log the source locations that were reset
    

    Example output:

    02025-07-02T22:03:56Z [warning] Excessive logging detected from rpc/node.cpp:142 (RPCHelpMan excessivelog()): >1048576 bytes logged during the last time window of 3600s. Suppressing logging to disk from this source location until time window resets. Console logging unaffected. Last log entry.
    1[*] 2025-07-02T22:03:56Z aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    22025-07-02T22:04:58Z (mocktime: 2262-04-11T23:47:15Z) Restarting logging from rpc/node.cpp:142 (RPCHelpMan excessivelog()): 121 bytes were dropped during the last 3600s.
    32025-07-02T22:04:58Z (mocktime: 2262-04-11T23:47:15Z) Restarting logging from rpc/node.cpp:139 (RPCHelpMan excessivelog()): 121 bytes were dropped during the last 3600s.
    
  2. DrahtBot commented at 8:48 pm on May 23, 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/32604.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko, stickies-v, glozow
    Concept ACK davidgumberg, brunoerg
    Stale ACK janb84, l0rinc, dergoegge

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
    • #29256 (log, refactor: Allow log macros to accept context arguments by ryanofsky)
    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

    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.

  3. DrahtBot added the label Utils/log/libs on May 23, 2025
  4. davidgumberg commented at 9:00 pm on May 23, 2025: contributor
    Concept ACK
  5. gmaxwell commented at 9:34 pm on May 23, 2025: contributor
    Any thoughts about adding some kind of dying gasp so that if a node crashes or hits some fatal error the most recent unfiltered log can be saved?
  6. DrahtBot added the label CI failed on May 23, 2025
  7. DrahtBot commented at 9:47 pm on May 23, 2025: contributor

    🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42810774045 LLM reason (✨ experimental): The CI failure is due to linting errors related to std::filesystem and locale dependence.

    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.

  8. Crypt-iQ commented at 8:49 pm on May 24, 2025: contributor

    if a node crashes or hits some fatal error the most recent unfiltered log can be saved?

    Does unfiltered here mean a log line that was rate limited or does it mean something else in this context? I think that we would need to know ahead of time if we were going to crash to save the most recent unfiltered log unless we somehow always cache it?

  9. dergoegge commented at 12:07 pm on May 26, 2025: member

    Concept ACK

    Thanks for picking this up!

  10. in src/logging.h:33 in 0bc6ed61cf outdated
    30@@ -27,10 +31,29 @@ static const bool DEFAULT_LOGTIMESTAMPS = true;
    31 static const bool DEFAULT_LOGTHREADNAMES = false;
    32 static const bool DEFAULT_LOGSOURCELOCATIONS = false;
    33 static constexpr bool DEFAULT_LOGLEVELALWAYS = false;
    


    janb84 commented at 1:22 pm on May 26, 2025:
    nit: would be nice to align the other initializations to the new C++ 11 {} style in a follow up PR, to keep the code base consistent.

    Crypt-iQ commented at 4:23 pm on May 29, 2025:
    I can do that in a follow-up

    Crypt-iQ commented at 1:16 pm on July 18, 2025:
    I have opted not to do this in the follow-up PR.
  11. janb84 commented at 1:41 pm on May 26, 2025: contributor

    ACK 0bc6ed61cfab6d97e74103efd41c46faf5941ff6

    Ratelimits loging to disk if loggin exceeds 1 MiB in 1 hour (WINDOW_MAX_BYTES const & WINDOW_SIZE const)

    • code review
    • build & tested
  12. DrahtBot requested review from davidgumberg on May 26, 2025
  13. DrahtBot requested review from dergoegge on May 26, 2025
  14. brunoerg commented at 7:47 pm on May 26, 2025: contributor
    Concept ACK
  15. Crypt-iQ force-pushed on May 28, 2025
  16. Crypt-iQ commented at 4:22 pm on May 29, 2025: contributor
    The tidy job is failing because it doesn’t like the logging macros being used in lambda functions. It seems like this is pre-existing so I’m not sure why it’s failing now. The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don’t have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up?
  17. in src/logging.h:98 in 6dece0553c outdated
    124+        BLOCKSTORAGE               = (CategoryMask{1} << 25),
    125+        TXRECONCILIATION           = (CategoryMask{1} << 26),
    126+        SCAN                       = (CategoryMask{1} << 27),
    127+        TXPACKAGES                 = (CategoryMask{1} << 28),
    128+        UNCONDITIONAL_RATE_LIMITED = (CategoryMask{1} << 29),
    129+        UNCONDITIONAL_ALWAYS       = (CategoryMask{1} << 30),
    


    stickies-v commented at 9:34 am on May 30, 2025:
    nit: I think a brief docstring on their suggested usage here would be helpful here

    l0rinc commented at 9:49 am on June 3, 2025:
    I’m not sure introducing two extra category flags is a good abstraction here - feels noisy. Wouldn’t it be cleaner to decide based on exposure to the attack surface instead? For example, during IBD (or any state with zero peer connections) we could leave all logs unrestricted, and always enable rate-limiting when node is reachable by peers.

    Crypt-iQ commented at 2:08 pm on June 3, 2025:
    See #32604 (review), will change. Imo, having to determine whether we are in IBD or have zero peer connections is heading into too much complexity. This PR shouldn’t cause IBD debug logs to be rate-limited, but if it does we should fix that.

    Crypt-iQ commented at 5:50 pm on June 4, 2025:
    Resolving as I’ve removed categories.
  18. in src/init/common.cpp:43 in 6dece0553c outdated
    39@@ -40,6 +40,7 @@ void AddLoggingArgs(ArgsManager& argsman)
    40     argsman.AddArg("-loglevelalways", strprintf("Always prepend a category and level (default: %u)", DEFAULT_LOGLEVELALWAYS), ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
    41     argsman.AddArg("-printtoconsole", "Send trace/debug info to console (default: 1 when no -daemon. To disable logging to file, set -nodebuglogfile)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
    42     argsman.AddArg("-shrinkdebugfile", "Shrink debug.log file on client startup (default: 1 when no -debug)", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);
    43+    argsman.AddArg("-ratelimitlogging", strprintf("Rate limit unconditional logging to disk (default: %u)", DEFAULT_RATELIMITLOGGING), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
    


    stickies-v commented at 9:40 am on May 30, 2025:
    Is there a good use case for disabling rate limiting? I can’t think of one. Letting the user define the amount of rate limiting is an alternative, but even then, I’m not sure. If an attacker or bug is able to reach the sane default maximum, then is the user really going to get any benefit from being to inspect the additional volume of un-ratelimited logs? I’m not sure. If so, I think it would make sense to not have this as a configurable option.

    Crypt-iQ commented at 11:33 am on May 30, 2025:
    That’s a good point, I can’t think of any concrete use case to disable rate limiting here.

    fanquake commented at 12:54 pm on May 30, 2025:
    @dergoegge might remember why this was added?

    stickies-v commented at 1:05 pm on May 30, 2025:
    fwiw: I can’t find any discussion about it in #21603, and the option was present from the first version of the PR (https://github.com/bitcoin/bitcoin/commit/4648b6d207139ec0ab2994f56c0a47f81cdf516a#diff-b1e19192258d83199d8adaa5ac31f067af98f63554bfdd679bd8e8073815e69d)

    l0rinc commented at 9:22 am on June 3, 2025:

    When I’m benchmarking IBD, I don’t want any artificial size or rate limits. Since the attack only matters when peers can influence us, could we simply disable rate-limiting for all logs during IBD/reindexes/assumeutxo?


    Should the docs explicitly state the 1 MiB-per-hour cap, or could we replace the boolean flag with a numeric “MiB per hour” parameter (retaining 0 and 1 as the current meanings)?


    fanquake commented at 10:14 am on June 3, 2025:

    Since the attack only matters when peers can influence us, could we simply disable rate-limiting for all logs during IBD

    ?


    l0rinc commented at 10:33 am on June 3, 2025:
    Can you be more specific :)?

    fanquake commented at 10:39 am on June 3, 2025:
    What do you mean by “peers not being able to influence us during IBD”?

    l0rinc commented at 10:59 am on June 3, 2025:

    I meant that during IBD we’re mostly replaying validated history, and my understanding is that rate-limiting is meant to protect logs caused by peer-supplied invalid data, not the deterministic IBD progress logs (e.g. -debug=bench or -debug=leveldb, which are very verbose (way over the 1MiB/h), but isn’t related to a user-sent serialization error or similar cheap attack). “Attacking” a node that is still in the original IBD seems less severe to me than skipped logs. I also understand if you disagree with this and I would appreciate an explanation, I want to understand.

    We can still get invalid data (thanks for triggering a clarification), but maybe the guard should added per LogFlags and we may want to blacklist instead of whitelisting. Or is this meant as a final catch-all for all the unknowns, e.g. maybe there’s a trick to enable LevelDB logs externally and this should still guard against that?


    Crypt-iQ commented at 1:32 pm on June 3, 2025:
    I believe that if somebody is using -debug=bench or -debug=leveldb, the fact that they are using a category should bypass the rate limit if LogDebug is used. I think the mitigation should be in place whether or not we’re in IBD – if we have any peers (inbound or outbound), we need to be cautious. This patch is meant to be the “final catch-all” like you said to fully eliminate this class of attack.

    l0rinc commented at 3:04 pm on June 3, 2025:
    Thanks for clarifying

    stickies-v commented at 9:33 am on June 5, 2025:
    It seems like this startup option is still in the latest force-push (5914a2ee6c915585b7213217047ad1644b873376), but from this discussion it doesn’t seem like there are good arguments to keep it? I’m not sure if @l0rinc still needs it for IBD bench reasons, but imo that’s way too much of a niche to keep it as a startup option?

    l0rinc commented at 9:41 am on June 24, 2025:

    I’m doing a reindex benchmark to check if the current implementation has any performance burden

    Edit: reindexing until 900k indicates there’s no slowdown caused by this change

     0COMMITS="e95bfc1d537e06d223dfcaec510181aa3d3f8386 f29b081b9277332523b7025394cad15b02cc53d2"; \
     1STOP=900000; DBCACHE=2500; \
     2CC=gcc; CXX=g++; \
     3BASE_DIR="/mnt/my_storage"; DATA_DIR="$BASE_DIR/BitcoinData"; LOG_DIR="$BASE_DIR/logs"; \
     4(echo ""; for c in $COMMITS; do git fetch -q origin $c && git log -1 --pretty='%h %s' $c || exit 1; done; echo "") && \
     5hyperfine \
     6  --sort command \
     7  --runs 1 \
     8  --export-json "$BASE_DIR/rdx-$(sed -E 's/(\w{8})\w+ ?/\1-/g;s/-$//'<<<"$COMMITS")-$STOP-$DBCACHE-$CC.json" \
     9  --parameter-list COMMIT ${COMMITS// /,} \
    10  --prepare "killall bitcoind; rm -f $DATA_DIR/debug.log; git checkout {COMMIT}; git clean -fxd; git reset --hard && \
    11    cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release && ninja -C build bitcoind && \
    12    ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=1000 -printtoconsole=0; sleep 10" \
    13  --cleanup "cp $DATA_DIR/debug.log $LOG_DIR/debug-{COMMIT}-$(date +%s).log" \
    14  "COMPILER=$CC ./build/bin/bitcoind -datadir=$DATA_DIR -stopatheight=$STOP -dbcache=$DBCACHE -reindex -blocksonly -connect=0 -printtoconsole=0"
    

    e95bfc1d53 Merge bitcoin/bitcoin#32797: doc: archive 28.2 release notes f29b081b92 doc: add release notes for new rate limiting logging behavior

    0Benchmark 1: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=2500 -reindex -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e95bfc1d537e06d223dfcaec510181aa3d3f8386)
    1  Time (abs ):        29033.291 s               [User: 43597.672 s, System: 1644.619 s]
    2 
    3Benchmark 2: COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=2500 -reindex -blocksonly -connect=0 -printtoconsole=0 (COMMIT = f29b081b9277332523b7025394cad15b02cc53d2)
    4  Time (abs ):        28875.472 s               [User: 43552.019 s, System: 1541.380 s]
    
    0Relative speed comparison
    1        1.01          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=2500 -reindex -blocksonly -connect=0 -printtoconsole=0 (COMMIT = e95bfc1d537e06d223dfcaec510181aa3d3f8386)
    2        1.00          COMPILER=gcc ./build/bin/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=900000 -dbcache=2500 -reindex -blocksonly -connect=0 -printtoconsole=0 (COMMIT = f29b081b9277332523b7025394cad15b02cc53d2)
    

    Crypt-iQ commented at 12:40 pm on June 26, 2025:
    Sweet, thanks for doing this! I was curious about any performance hits.
  19. in src/logging.h:184 in 6dece0553c outdated
    180@@ -113,17 +181,17 @@ namespace BCLog {
    181         std::atomic<Level> m_log_level{DEFAULT_LOG_LEVEL};
    182 
    183         /** Log categories bitfield. */
    184-        std::atomic<CategoryMask> m_categories{BCLog::NONE};
    185+        std::atomic<CategoryMask> m_categories{DEFAULT_LOG_FLAGS};
    


    stickies-v commented at 10:03 am on May 30, 2025:

    Hmm, I’m feeling uneasy about using m_categories, when rate limiting flags fundamentally don’t represent a category. I can see how it reduces the diff, but I’m not sure that’s worth it. I’ll think about an alternative approach - have you considered any already?

    Since the user doesn’t have to be able to configure any of this, and we only have a single UNCONDITIONAL_ALWAYS use case (and likely won’t have (many) more in the future), I think adding an is_conditional=True arg to LogPrintLevel and a LogInfoAlways macro might be an alternative?


    Crypt-iQ commented at 11:44 am on May 30, 2025:
    I haven’t considered alternative approaches, but I think adding a boolean is better than adding special-cased categories.

    stickies-v commented at 12:43 pm on May 30, 2025:

    Or perhaps even better than a boolean is a uint64_t rate_limit=DEFAULT_RATE_LIMIT so we can do away with the distinction between ratelimited and non-ratelimited logging entirely? We could use rate_limit=0 as a synonym for non-ratelimited logging, but I think even UpdateTipLog() doesn’t have to be exempt from rate limiting, we could still set a sane value that allows doing entire IBD in one hour, for example.

    Then UpdateTipLog() could just use:

    0    LogInfo(/*rate_limit=*/100, "%s%s: new best=%s height=%d version=0x%08x log2_work=%f tx=%lu date='%s' progress=%f cache=%.1fMiB(%utxo)%s\n",
    

    Crypt-iQ commented at 4:21 pm on May 30, 2025:
    I think callers just need to be aware that they cannot set the rate limit very high in certain cases. Maybe this change and a comment will suffice?

    Crypt-iQ commented at 7:56 pm on June 3, 2025:

    While implementing this, I came to the conclusion that because macros don’t have default arguments, skipping the rate limiting for UpdateTipLog means that we need to: 1) introduce a new macro for IBD specifically for UpdateTipLog, or 2) modify all call-sites of whichever macro UpdateTipLog uses to accept a rate_limit argument.

    I am partial to introducing a new macro for UpdateTipLog since it gets rid of the noise in the categories logic. I think that option 2) to modify all call-sites to pass in a rate-limiting argument is cumbersome when most callers don’t need to be aware of rate limiting and it should instead be done under-the-hood (i.e. UpdateTipLog is the exception to the rule). I think using a boolean for rate-limiting vs no-rate-limiting is better than a uint64_t because then the proposed SourceLocationCounter needs to also keep track of this window-scaling value (i.e. multiple of WINDOW_MAX_BYTES).

    I might be wrong here, curious to hear thoughts.


    stickies-v commented at 2:07 pm on June 4, 2025:
    1. to modify all call-sites to pass in a rate-limiting argument is cumbersome

    Absolutely agree that would not be an improvement.

    I think using a boolean for rate-limiting vs no-rate-limiting is better than a uint64_t

    I think either will work fine, the uint64_t generalizes a bit nicer but we probably won’t be needing the generalization in the future, so whichever ends up being the most elegant implementation now is what I think we should go for.

    I am partial to introducing a new macro for UpdateTipLog

    That seems sensible to me, yes.


    Crypt-iQ commented at 5:45 pm on June 4, 2025:
    I’ve changed this so that the logging macros eventually pass a should_ratelimit bool to LogPrintStr. Instead of introducing a new macro for UpdateTipLog, it now uses LogPrintLevel which can bypass the rate-limiting. I’m not sure how I feel about using LogPrintLevel as I don’t want to encourage more usage of it in the codebase as it could be a potential footgun.
  20. stickies-v commented at 10:21 am on May 30, 2025: contributor
    Concept ACK on locking down this attack vector, and the code changes required seem acceptable. Also nice to start using std::source_location here.
  21. fanquake commented at 10:57 am on May 30, 2025: member

    The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don’t have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up? @hebasto can you take a look here? I ran test_bitcoin.exe from this branch (rebased) under wine and didn’t see any failures.

  22. in src/logging.cpp:374 in 6dece0553c outdated
    367@@ -364,33 +368,71 @@ std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level l
    368 
    369 static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
    370 {
    371-    return buflog.str.size() + buflog.logging_function.size() + buflog.source_file.size() + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    372+    return buflog.str.size() + buflog.logging_function.size() + strlen(buflog.source_loc.file_name()) + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    373 }
    374 
    375-void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
    376+bool BCLog::Logger::FormatLogStrAndRateLimit(std::string& str, BCLog::LogFlags category, BCLog::Level level, const std::source_location& source_loc, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime)
    


    stickies-v commented at 12:19 pm on May 30, 2025:

    I think it’s unintuitive that this function returns true when the operation failed. It seems like the rate limiting can be carved out pretty much as-is into a separate bool NeedsRateLimiting() function? There are only 2 callsites of FormatLogStrAndRateLimit, and the first one (StartLogging) doesn’t look like it needs rate limiting because we already have a max buffer size?

    edit: this would pair fairly well with my suggestion here


    Crypt-iQ commented at 4:20 pm on May 30, 2025:
    Yup, this makes sense as well. Can change.

    Crypt-iQ commented at 5:48 pm on June 4, 2025:
    I’ve carved this out into a NeedsRateLimiting function and have kept the logic of FormatLogStrInPlace mostly unchanged. The reason I had the original change is because FormatLogStrInPlace inserts a timestamp at the beginning of str and I wanted the prepended rate-limiting logs to occur after the timestamp. With the new change, the rate-limiting logs are first in str followed by the time-stamp. I think now the logs look a little wonky, but I like the code change better. I could also extract the time-stamp prepend logic so it’s called after NeedsRateLimiting.
  23. hebasto commented at 12:20 pm on May 30, 2025: member

    The windows cross-built job is failing on the rate_limiting test at every file-size comparison. I don’t have a windows machine to debug this, but I think maybe fs::file_size is failing or some other quirk is showing up?

    @hebasto can you take a look here? I ran test_bitcoin.exe from this branch (rebased) under wine and didn’t see any failures.

    It seems to be related to MSVCRT behaviour, as binaries linked to UCRT pass all unit tests.

  24. in src/logging.h:144 in 6dece0553c outdated
    112+    //! Fixed window rate limiter for logging.
    113+    class LogRateLimiter
    114+    {
    115+    private:
    116+        //! Timestamp of the last window reset.
    117+        std::chrono::time_point<NodeClock> m_last_reset;
    


    stickies-v commented at 12:30 pm on May 30, 2025:

    Not a big issue, but since we have quite a few log source locations, this class will be instantiated a fair amount of (N) times. An alternative approach would be to have a single LogRateLimiter instance that encapsulates most of the logic in 6b54362c19ca9baf9dfa9be9281f6faeda169420, and has a single m_last_reset member, spawning a minimal SourceLocationCounter struct for each source location, containing just the m_available_bytes and m_dropped_bytes members.

    This:

    • reduces N std::chrono::time_point<NodeClock> instances to just 1
    • allows LogRateLimiter to completely clean up memory for log locations that haven’t been triggered in the last hour
    • improves code encapsulation

    Overall, this shouldn’t be a big change from what you have now, mostly just moving code around into a new class?


    Crypt-iQ commented at 4:18 pm on May 30, 2025:
    Yup I had thought about this and wasn’t sure how to evaluate the overhead. I’ll change it.

    Crypt-iQ commented at 5:40 pm on June 4, 2025:
    Implemented this change, lmk what you think. I think this encapsulates code a bit better, but it probably could still be improved.

    stickies-v commented at 9:44 am on June 5, 2025:
    I’ve left some comments for further encapsulation, but yeah I think this a step in the right direction, thanks!
  25. in src/logging.cpp:404 in 6dece0553c outdated
    404+
    405+            str.insert(0, strprintf("Restarting logging from %s:%d (%s): "
    406+                                    "(%d MiB) were dropped during the last hour.\n",
    407+                                    source_loc.file_name(), source_loc.line(), logging_function,
    408+                                    dropped_bytes / (1024 * 1024)));
    409+        } else if (is_ratelimited && !was_ratelimited) {
    


    stickies-v commented at 12:49 pm on May 30, 2025:

    nit: It would be nice if we could catch this in debug builds (ideally even at a fraction, say 20% of the limit), to help inform if our default limit needs to change. Assume() is an option, but that would probably break unit tests in debug builds.

    nit, because 1MB is small enough that it should prevent issues even on devices with little disk size (unless an attacker finds multiple log vulnerabilities), and large enough that no single unconditional logging statement should ever hit that in the foreseeable future, so I think “driving blind” is acceptable here.


    Crypt-iQ commented at 5:39 pm on June 4, 2025:
    I want to address this because I think it would be useful to catch in debug builds via Assume but also don’t want to break tests. Do you think this should be addressed?

    l0rinc commented at 7:30 pm on July 6, 2025:
    Not sure we would catch that with our test suite. But if it’s meant to draw the users’ attention early for false positives, maybe we could log a warning at least. (theoretically the tests could also fail for unregistered extra log warnings)
  26. dergoegge commented at 8:47 am on June 3, 2025: member
    I vaguely remember having trouble with the unit tests on windows too. Iirc it had something to do with windows line endings being different (i.e. \r\n on windows vs \n on linux), which throws off the size accounting.
  27. in src/logging.h:172 in 6dece0553c outdated
    167@@ -105,6 +168,11 @@ namespace BCLog {
    168         size_t m_cur_buffer_memusage GUARDED_BY(m_cs){0};
    169         size_t m_buffer_lines_discarded GUARDED_BY(m_cs){0};
    170 
    171+        //! Fixed window rate limiters for each source location that has attempted to log something.
    172+        std::unordered_map<std::source_location, LogRateLimiter, SourceLocationHasher, SourceLocationEqual> m_ratelimiters GUARDED_BY(m_cs);
    


    l0rinc commented at 9:57 am on June 3, 2025:
    Do we ever clear m_ratelimiters, or does that map only grow over the lifetime of the process?

    Crypt-iQ commented at 2:09 pm on June 3, 2025:
    This never gets cleared. I haven’t calculated the overhead of the map, but it could make sense to clear it periodically. Do you think it should just be on a timer or something?

    stickies-v commented at 2:13 pm on June 3, 2025:
    note: this is part of my suggestion in #32604 (review)

    l0rinc commented at 3:03 pm on June 3, 2025:
    @stickies-v, my mistake, I quickly wanted to make sure this doesn’t affect my IBD benchmarks, didn’t have time to review it in detail yet

    Crypt-iQ commented at 5:51 pm on June 4, 2025:
    Both the map and set are now cleared if LogRateLimiter determines that the logging window should be reset.
  28. maflcko commented at 10:03 am on June 3, 2025: member

    The tidy job is failing because it doesn’t like the logging macros being used in lambda functions. It seems like this is pre-existing so I’m not sure why it’s failing now.

    It happens after commit dfbc3e46b8661b112a91c5f00a4dce439f4f5914 and looks unrelated. Maybe just use the workaround from ./src/util/check.h?

  29. l0rinc commented at 10:09 am on June 3, 2025: contributor

    Concept ACK, but I’m not yet sure about the approach. Let me know if I misunderstood something important here.

    Edit:

    All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed

    Will that appear on the console as well, which didn’t have any messages dropped?

  30. l0rinc changes_requested
  31. Crypt-iQ force-pushed on Jun 4, 2025
  32. Crypt-iQ commented at 5:58 pm on June 4, 2025: contributor

    The latest change uses a single BCLog::LogRateLimiter instance and uses a slimmer way of tracking the amount of logging bytes left per std::source_location. I like the approach better as m_categories is no longer being used. I would like to point out that the rate-limiting logic can be bypassed if LogPrintLevel is used. I think LogPrintLevel callers need to be cautious and I kind of think this macro shouldn’t be used if level >= BCLog::Level::Info.

    Will that appear on the console as well, which didn’t have any messages dropped?

    Yes this appears in the console as well. I actually was unsure if console-logging should be rate-limited or not. Hopefully users have sensible limits on their terminal history?

    I will work on fixing the CI, I think review can still happen in the meantime while I debug the windows failure and fix the tidy job.

  33. Crypt-iQ force-pushed on Jun 4, 2025
  34. DrahtBot removed the label CI failed on Jun 4, 2025
  35. in src/logging.h:40 in bec7462686 outdated
    33@@ -31,6 +34,24 @@ extern const char * const DEFAULT_DEBUGLOGFILE;
    34 
    35 extern bool fLogIPs;
    36 
    37+struct SourceLocationEqual {
    38+    bool operator()(const std::source_location& lhs, const std::source_location& rhs) const noexcept
    39+    {
    40+        return strcmp(lhs.file_name(), rhs.file_name()) == 0 && lhs.line() == rhs.line();
    


    stickies-v commented at 8:38 am on June 5, 2025:

    in bec74626867abefbfc55da5aaaf8a0c030663f3a:

    nit: comparing ints is more performant than comparing strings, and is more differentiating than the filename, so would do the line comparison first

    0        return lhs.line() == rhs.line() && strcmp(lhs.file_name(), rhs.file_name()) == 0 ;
    
  36. in src/logging.cpp:379 in 5535df69a2 outdated
    372@@ -364,17 +373,63 @@ std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level l
    373 
    374 static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
    375 {
    376-    return buflog.str.size() + buflog.logging_function.size() + buflog.source_file.size() + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    377+    return buflog.str.size() + buflog.logging_function.size() + strlen(buflog.source_loc.file_name()) + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    378 }
    379 
    380-void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags category, BCLog::Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const
    381+bool BCLog::Logger::NeedsRateLimiting(bool should_ratelimit, const std::source_location& source_loc, std::string& str, std::string_view logging_function)
    


    stickies-v commented at 9:27 am on June 5, 2025:

    NeedsRateLimiting as well as the underlying maps imo belong to LogRateLimiter rather than Logger. I also don’t think should_ratelimit is a meaningful parameter, the function should just not be called if that’s false. I also don’t understand why logging_function is a separate parameter - can’t we just get that from source_loc?

    Suggested diff that incorporates all of those suggestions:

      0diff --git a/src/logging.cpp b/src/logging.cpp
      1index 1e20c10368..c4ae881ffd 100644
      2--- a/src/logging.cpp
      3+++ b/src/logging.cpp
      4@@ -5,6 +5,7 @@
      5 
      6 #include <logging.h>
      7 #include <memusage.h>
      8+#include <sync.h>
      9 #include <util/check.h>
     10 #include <util/fs.h>
     11 #include <util/string.h>
     12@@ -376,41 +377,40 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
     13     return buflog.str.size() + buflog.logging_function.size() + strlen(buflog.source_loc.file_name()) + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
     14 }
     15 
     16-bool BCLog::Logger::NeedsRateLimiting(bool should_ratelimit, const std::source_location& source_loc, std::string& str, std::string_view logging_function)
     17+bool BCLog::LogRateLimiter::NeedsRateLimiting(const std::source_location& source_loc, std::string& str)
     18 {
     19+    StdLockGuard scoped_lock(m_cs);
     20     // Whether or not logging to disk was/is ratelimited for this source location.
     21     bool was_ratelimited{false};
     22     bool is_ratelimited{false};
     23 
     24-    if (m_ratelimit && should_ratelimit) {
     25-        // Check to see if we were rate limited before calling ResetWindow.
     26-        was_ratelimited = m_suppressed_locations.find(source_loc) != m_suppressed_locations.end();
     27+    // Check to see if we were rate limited before calling ResetWindow.
     28+    was_ratelimited = m_suppressed_locations.find(source_loc) != m_suppressed_locations.end();
     29 
     30-        // If the m_limiter window has elapsed, then we need to clear the unordered map and set.
     31-        if (m_limiter.ResetWindow()) {
     32-            m_source_locations.clear();
     33-            m_suppressed_locations.clear();
     34-        }
     35+    // If the m_limiter window has elapsed, then we need to clear the unordered map and set.
     36+    if (ResetWindow()) {
     37+        m_source_locations.clear();
     38+        m_suppressed_locations.clear();
     39+    }
     40 
     41-        is_ratelimited = !m_source_locations[source_loc].Consume(str.size());
     42+    is_ratelimited = !m_source_locations[source_loc].Consume(str.size());
     43 
     44-        if (!is_ratelimited && was_ratelimited) {
     45-            uint64_t dropped_bytes = m_source_locations[source_loc].GetDroppedBytes();
     46+    if (!is_ratelimited && was_ratelimited) {
     47+        uint64_t dropped_bytes{m_source_locations[source_loc].GetDroppedBytes()};
     48 
     49-            str.insert(0, strprintf("Restarting logging from %s:%d (%s): "
     50-                                    "(%d MiB) were dropped during the last hour.\n",
     51-                                    source_loc.file_name(), source_loc.line(), logging_function,
     52-                                    dropped_bytes / (1024 * 1024)));
     53-        } else if (is_ratelimited && !was_ratelimited) {
     54-            // Logging from this source location will be suppressed until the current window resets.
     55-            m_suppressed_locations.insert(source_loc);
     56+        str.insert(0, strprintf("Restarting logging from %s:%d (%s): "
     57+                                "(%d MiB) were dropped during the last hour.\n",
     58+                                source_loc.file_name(), source_loc.line(), source_loc.function_name(),
     59+                                dropped_bytes / (1024 * 1024)));
     60+    } else if (is_ratelimited && !was_ratelimited) {
     61+        // Logging from this source location will be suppressed until the current window resets.
     62+        m_suppressed_locations.insert(source_loc);
     63 
     64-            str.insert(0, strprintf("Excessive logging detected from %s:%d (%s): >%d MiB logged during the last hour."
     65-                                    "Suppressing logging to disk from this source location for up to one hour. "
     66-                                    "Console logging unaffected. Last log entry.",
     67-                                    source_loc.file_name(), source_loc.line(), logging_function,
     68-                                    LogRateLimiter::WINDOW_MAX_BYTES / (1024 * 1024)));
     69-        }
     70+        str.insert(0, strprintf("Excessive logging detected from %s:%d (%s): >%d MiB logged during the last hour."
     71+                                "Suppressing logging to disk from this source location for up to one hour. "
     72+                                "Console logging unaffected. Last log entry.",
     73+                                source_loc.file_name(), source_loc.line(), source_loc.function_name(),
     74+                                LogRateLimiter::WINDOW_MAX_BYTES / (1024 * 1024)));
     75     }
     76 
     77     // To avoid confusion caused by dropped log messages when debugging an issue,
     78@@ -479,7 +479,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_
     79     }
     80 
     81     FormatLogStrInPlace(str_prefixed, category, level, source_loc, logging_function, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime());
     82-    bool ratelimit = NeedsRateLimiting(should_ratelimit, source_loc, str_prefixed, logging_function);
     83+    bool ratelimit = m_ratelimit && should_ratelimit && m_limiter.NeedsRateLimiting(source_loc, str_prefixed);
     84 
     85     if (m_print_to_console) {
     86         // print to console
     87diff --git a/src/logging.h b/src/logging.h
     88index b94b9cb495..f9441f6913 100644
     89--- a/src/logging.h
     90+++ b/src/logging.h
     91@@ -48,7 +48,7 @@ struct SourceLocationHasher {
     92     {
     93         // Use CSipHasher(0, 0) as a simple way to get uniform distribution.
     94         return static_cast<size_t>(CSipHasher(0, 0)
     95-                                       .Write(std::hash<std::string>{}(s.file_name()))
     96+                                       .Write(std::hash<std::string_view>{}(s.file_name()))
     97                                        .Write(std::hash<int>{}(s.line()))
     98                                        .Finalize());
     99     }
    100@@ -106,13 +106,18 @@ namespace BCLog {
    101     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    102     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    103 
    104+    class SourceLocationCounter;
    105     //! Fixed window rate limiter for logging.
    106     class LogRateLimiter
    107     {
    108     private:
    109         //! Timestamp of the last window reset.
    110         std::chrono::time_point<NodeClock> m_last_reset;
    111-
    112+        //! Counters for each source location that has attempted to log something.
    113+        std::unordered_map<std::source_location, SourceLocationCounter, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_cs);
    114+        //! Set of source file locations that were dropped on the last log attempt.
    115+        std::unordered_set<std::source_location, SourceLocationHasher, SourceLocationEqual> m_suppressed_locations GUARDED_BY(m_cs);
    116+        mutable StdMutex m_cs; // TODO: does LogRateLimiter need to use Logger:m_cs instead? Didn't investigate.
    117     public:
    118         //! Interval after which the window is reset.
    119         static constexpr std::chrono::hours WINDOW_SIZE{1};
    120@@ -120,6 +125,8 @@ namespace BCLog {
    121         static constexpr uint64_t WINDOW_MAX_BYTES{1024 * 1024};
    122         //! Attempts to reset the window if the window interval has passed. Returns true if the window was reset.
    123         bool ResetWindow();
    124+        //! Check if we need to rate limit a source_location.
    125+        bool NeedsRateLimiting(const std::source_location& source_loc, std::string& str) EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
    126 
    127         LogRateLimiter() : m_last_reset{NodeClock::now()} {}
    128 
    129@@ -176,10 +183,6 @@ namespace BCLog {
    130 
    131         //! Keeps track of the last time we've reset the logging window.
    132         LogRateLimiter m_limiter GUARDED_BY(m_cs);
    133-        //! Counters for each source location that has attempted to log something.
    134-        std::unordered_map<std::source_location, SourceLocationCounter, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_cs);
    135-        //! Set of source file locations that were dropped on the last log attempt.
    136-        std::unordered_set<std::source_location, SourceLocationHasher, SourceLocationEqual> m_suppressed_locations GUARDED_BY(m_cs);
    137 
    138         //! Category-specific log level. Overrides `m_log_level`.
    139         std::unordered_map<LogFlags, Level> m_category_log_levels GUARDED_BY(m_cs);
    140@@ -193,9 +196,6 @@ namespace BCLog {
    141 
    142         void FormatLogStrInPlace(std::string& str, LogFlags category, Level level, const std::source_location& source_loc, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const;
    143 
    144-        /** Check if we need to rate limit a source_location. */
    145-        bool NeedsRateLimiting(bool should_ratelimit, const std::source_location& source_loc, std::string& str, std::string_view logging_function) EXCLUSIVE_LOCKS_REQUIRED(m_cs);
    146-
    147         std::string LogTimestampStr(SystemClock::time_point now, std::chrono::seconds mocktime) const;
    148 
    149         /** Slots that connect to the print signal */
    

    Note: I didn’t think about thread safety much yet, so I just threw in another m_cs to make it compile and pass tests, might be wrong/unsafe as-is.


    Crypt-iQ commented at 8:11 pm on June 5, 2025:
    I’ve removed logging_function and __func__ usage. I think it makes the function names a little longer in the map, but I’m ok with it. I think m_cs is unnecessary if m_ratelimiter is guarded by m_cs?

    stickies-v commented at 9:24 am on June 6, 2025:

    I think it makes the function names a little longer in the map, but I’m ok with it.

    Oh interesting, I didn’t realize that. On my machine, __func__ contains just the function name, whereas function_name() returns the entire signature, e.g.:

    0__func__: AppInitParameterInteraction
    1source_location: bool AppInitParameterInteraction(const ArgsManager &)
    

    I don’t see how this affects any maps, though? I think it just affects the messages when we stop and restart logging?

    I think m_cs is unnecessary if m_ratelimiter is guarded by m_cs?

    Yeah nice, that seems correct. Probably good to add “This class is not thread-safe.” to the LogRateLimiter docstring, given that it is likely to be used in a multithreaded context.


    Crypt-iQ commented at 4:00 pm on June 6, 2025:

    I don’t see how this affects any maps, though? I think it just affects the messages when we stop and restart logging?

    Oh, I think I mistakenly thought that function_name was included in the map somehow or used when hashing std::source_location, but I see that’s not actually the case.

  37. in src/logging.h:51 in 5535df69a2 outdated
    46+struct SourceLocationHasher {
    47+    size_t operator()(const std::source_location& s) const noexcept
    48+    {
    49+        // Use CSipHasher(0, 0) as a simple way to get uniform distribution.
    50+        return static_cast<size_t>(CSipHasher(0, 0)
    51+                                       .Write(std::hash<std::string>{}(s.file_name()))
    


    stickies-v commented at 9:27 am on June 5, 2025:

    nit: string_view to avoid unnecessary string construction

    0                                       .Write(std::hash<std::string_view>{}(s.file_name()))
    
  38. in src/logging.h:358 in 5535df69a2 outdated
    359-            LogPrintLevel_(category, level, __VA_ARGS__); \
    360-        }                                                 \
    361+// Note that conditional logging is performed WITHOUT rate limiting. Users specifying
    362+// -debug are assumed to be developers or power users who are aware that -debug may cause
    363+// excessive disk usage due to logging.
    364+#define LogPrintLevel(category, level, ...)                                           \
    


    stickies-v commented at 9:44 am on June 5, 2025:

    The previous docstring is misleading, LogPrintLevel() can also be used for unconditional logging when a Level::Info is passed as level, as is e.g. done in mapport.cpp and in a few other places. I don’t think this is a robust way of deciding on rate limiting.

    I think I would prefer either:

    1. using LogPrintLevel_ directly in UpdateTipLog(), with a comment on why we’re using a private function and not enabling rate limiting here. A LogInfoAlways macro could be an alternative.
    2. refactoring logging.h replace a lot of the current macros to become regular functions using std::source_location, allowing easier overloads for ratelimiting. This probably makes more sense in a separate pull, so imo going with 1. in this PR and then improving with 2. separately would be nice?

    Crypt-iQ commented at 8:10 pm on June 5, 2025:
    I’ve changed the misleading docstring and I’ve implemented suggestion 1. and I will pick up suggestion 2. as well. I think this might conflict a bit with the work in #29256.

    stickies-v commented at 10:04 am on June 6, 2025:
    Nice. Since af86b55f996cf06e8e9179d5c1fe7ce3d94fa3c6 now uses LogPrintLevel_ directly, I think the changes to LogPrintLevel in ed3c0a592b60a565ec5dade7c3af62e7f4cf75a1 can now be reverted and rate limiting applied consistently to all “public” macros? (would also require an update to the UpdateTipLog docstring)

    Crypt-iQ commented at 4:35 pm on June 6, 2025:
    I’m a little unsure about adding rate-limiting to LogPrintLevel as then if a user runs with -debug, they could potentially hit the rate-limit via LogDebug or by some of the existing LogPrintLevel calls. I think it is in the user’s best interest to have rate-limiting applied to all public macros like you suggested, but I also don’t want to potentially confuse users as to why their debug.log is incomplete due to rate-limiting when they are just running with -debug.

    stickies-v commented at 11:18 am on June 9, 2025:

    Oh no I agree, debug logs should be exempt from ratelimiting. My point is just that I think it’s bad to have different logging behaviour depending on whether LogInfo() or LogPrintLevel(..., BCLog::Info) is used, so I think something like the below would work well?

     0diff --git a/src/logging.h b/src/logging.h
     1index d588ef86dc..5f298405e7 100644
     2--- a/src/logging.h
     3+++ b/src/logging.h
     4@@ -349,17 +349,13 @@ inline void LogPrintFormatInternal(const std::source_location& source_loc, const
     5 // Use a macro instead of a function for conditional logging to prevent
     6 // evaluating arguments when logging for the category is not enabled.
     7 
     8-// Log by prefixing the output with the passed category name and severity level. This can either
     9-// log conditionally if the category is allowed or unconditionally if level >= BCLog::Level::Info
    10-// is passed. Note that this function bypasses rate limiting. Callers need to take care to ensure
    11-// that an attacker cannot trigger a disk-fill vulnerability if level >= Info is used. Additionally,
    12-// users specifying -debug are assumed to be developers or power users who are aware that -debug
    13-// may cause excessive disk usage due to logging.
    14-#define LogPrintLevel(category, level, ...)                                           \
    15-    do {                                                                              \
    16-        if (LogAcceptCategory((category), (level))) {                                 \
    17-            LogPrintLevel_(category, level, /*should_ratelimit=*/false, __VA_ARGS__); \
    18-        }                                                                             \
    19+// Log conditionally, prefixing the output with the passed category name and severity level.
    20+#define LogPrintLevel(category, level, ...)                             \
    21+    do {                                                                \
    22+        if (LogAcceptCategory((category), (level))) {                   \
    23+            bool rate_limit{level >= BCLog::Info};                      \
    24+            LogPrintLevel_(category, level, rate_limit, __VA_ARGS__);   \
    25+        }                                                               \
    26     } while (0)
    27 
    28 // Log conditionally, prefixing the output with the passed category name.
    

    Crypt-iQ commented at 1:04 pm on June 9, 2025:
    Oh I completely misunderstood! I like this suggestion a lot and I think it plugs the hole. I am not sure the future plans for LogPrintLevel but I wonder if eventually all instances of LogPrintLevel with level >= BCLog::Info could instead be converted to LogInfo or the equivalent?

    stickies-v commented at 2:03 pm on June 9, 2025:

    In a general sense, having a generalized logging function is helpful when the loglevel isn’t known in advance, as is e.g. the case here: https://github.com/bitcoin/bitcoin/blob/f3bbc746647d1fd23bf5cfe357e32f38c5f6319c/src/httpserver.cpp#L440

    It seems like we currently don’t really need it in our codebase, but I would keep that discussion for a separate PR because it would lead us astray a bit much here?


    Crypt-iQ commented at 3:27 pm on June 11, 2025:
    Resolving as it’s now addressed.
  39. in src/logging.cpp:393 in 5535df69a2 outdated
    390+
    391+        // If the m_limiter window has elapsed, then we need to clear the unordered map and set.
    392+        if (m_limiter.ResetWindow()) {
    393+            m_source_locations.clear();
    394+            m_suppressed_locations.clear();
    395+        }
    


    stickies-v commented at 10:15 am on June 5, 2025:

    This logic should be encapsulated in ResetWindow imo, so probably this statement could just be:

    0        m_limiter.MaybeResetWindow();
    
  40. Crypt-iQ force-pushed on Jun 5, 2025
  41. Crypt-iQ force-pushed on Jun 5, 2025
  42. DrahtBot added the label CI failed on Jun 5, 2025
  43. DrahtBot commented at 6:57 pm on June 5, 2025: contributor

    🚧 At least one of the CI tasks failed. Task previous releases, depends DEBUG: https://github.com/bitcoin/bitcoin/runs/43565644662 LLM reason (✨ experimental): The build failed due to a compilation error caused by incomplete type declaration of BCLog::SourceLocationCounter.

    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.

  44. Crypt-iQ force-pushed on Jun 5, 2025
  45. Crypt-iQ commented at 8:12 pm on June 5, 2025: contributor

    The latest push encapsulates things a bit more. I think there’s still room for improvement and am open to suggestions – I don’t like how WINDOW_MAX_BYTES is not a LogRateLimiter member and I also dislike the friend class Logger usage.

    I think the windows cross-built job is failing because a static BCLog::Logger* is shared across tests and is triggering the rate-limiting logic before the logging tests are run. The CI logs have an asterisk: Mismatch at position 0: [*] foo5: bar5 != foo5: bar5

  46. Crypt-iQ force-pushed on Jun 5, 2025
  47. Crypt-iQ force-pushed on Jun 5, 2025
  48. DrahtBot removed the label CI failed on Jun 5, 2025
  49. in src/logging.h:107 in 911ee520c8 outdated
    103@@ -82,15 +104,68 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    106     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    107+    constexpr uint64_t WINDOW_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
    


    stickies-v commented at 7:53 am on June 6, 2025:
    nit: moving it into the BCLog namespace seems fine, but BCLog::WINDOW_MAX_BYTES is not a very helpful name. RATELIMIT_MAX_BYTES probably better?
  50. in src/logging.h:147 in 911ee520c8 outdated
    142+        //! Counters for each source location that has attempted to log something.
    143+        std::unordered_map<std::source_location, SourceLocationCounter, SourceLocationHasher, SourceLocationEqual> m_source_locations;
    144+        //! Set of source file locations that were dropped on the last log attempt.
    145+        std::unordered_set<std::source_location, SourceLocationHasher, SourceLocationEqual> m_suppressed_locations;
    146+
    147+        //! Attempts to reset the logging window if the window interval has passed. This will clear m_source_locations and m_suppressed_locations if a reset occurs.
    


    stickies-v commented at 8:04 am on June 6, 2025:

    nit: line length

    0        //! Attempts to reset the logging window if the window interval has passed. This will clear
    1        //! m_source_locations and m_suppressed_locations if a reset occurs.
    
  51. in src/logging.h:153 in 911ee520c8 outdated
    148+        void MaybeResetWindow();
    149+
    150+    public:
    151+        //! Interval after which the window is reset.
    152+        static constexpr std::chrono::hours WINDOW_SIZE{1};
    153+        //! Check if we need to rate limit a source location.
    


    stickies-v commented at 8:06 am on June 6, 2025:

    Docstring could be beefed up a bit to highlight that this is not a pure check but has side effects. E.g.:

    0        //! Consumes `source_loc`'s available bytes corresponding to the size of the (formatted)
    1        //! `str` and returns true if it exceeds the allowance in the current time window.
    
  52. in src/logging.cpp:399 in 911ee520c8 outdated
    395+        uint64_t dropped_bytes = m_source_locations[source_loc].GetDroppedBytes();
    396+
    397+        str.insert(0, strprintf("Restarting logging from %s:%d (%s): "
    398+                                "(%d MiB) were dropped during the last hour.\n",
    399+                                source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    400+                                dropped_bytes / (1024 * 1024)));
    


    stickies-v commented at 9:28 am on June 6, 2025:
    This will now only be shown for a single source location per time window. I think this should be moved to MaybeResetWindow(), where we iterate over m_suppressed_locations before clearing it?

    Crypt-iQ commented at 3:31 pm on June 11, 2025:
    I’ve made this change, lmk what you think. MaybeResetWindow() now uses str.insert(0, ...) per m_suppressed_locations.

    stickies-v commented at 12:24 pm on June 19, 2025:
    Yeah, that looks good, thanks. It’s unfortunate that these message are counted in is_ratelimited = !m_source_locations[source_loc].Consume(str.size());, but suppressions should be extremely rare, so triggering enough of them to cause a meaningful difference here seems impractical. A solution with not too much change could be to change to MaybeResetWindow(const std::string& str, std::string& resume_msg), so we can keep separate accounting, but yeah not sure that’s worth it/important.

    Crypt-iQ commented at 8:50 pm on June 23, 2025:
    Deciding to keep as-is
  53. in src/logging.cpp:405 in 911ee520c8 outdated
    425 
    426     str.insert(0, GetLogPrefix(category, level));
    427 
    428     if (m_log_sourcelocations) {
    429-        str.insert(0, strprintf("[%s:%d] [%s] ", RemovePrefixView(source_file, "./"), source_line, logging_function));
    430+        str.insert(0, strprintf("[%s:%d] [%s] ", RemovePrefixView(source_loc.file_name(), "./"), source_loc.line(), source_loc.function_name()));
    


    stickies-v commented at 9:43 am on June 6, 2025:
    As you pointed out, this changes the logging output by now including the full function signature instead of just the function name. I think that’s generally an improvement, but should probably be mentioned in the release notes (which would be good to have for the new rate limiting behaviour in general anyway) because it might break downstream log parsing. Would suggest adding commit at the top with release notes?

    Crypt-iQ commented at 8:50 pm on June 23, 2025:
    Done
  54. in src/logging.cpp:454 in 911ee520c8 outdated
    449@@ -400,10 +450,8 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_
    450                 .now=SystemClock::now(),
    451                 .mocktime=GetMockTime(),
    452                 .str=str_prefixed,
    453-                .logging_function=std::string(logging_function),
    454-                .source_file=std::string(source_file),
    455                 .threadname=util::ThreadGetInternalName(),
    456-                .source_line=source_line,
    457+                .source_loc=source_loc,
    


    stickies-v commented at 9:46 am on June 6, 2025:

    Since source_loc is fairly large and this function is frequently called, I think it might make sense to optimize this a bit further and have LogPrintStr_ (and its callsites) take a std::source_location&& so we can move it here and avoid the copy?

      0diff --git a/src/logging.cpp b/src/logging.cpp
      1index e31e346549..006bac6a98 100644
      2--- a/src/logging.cpp
      3+++ b/src/logging.cpp
      4@@ -74,8 +74,7 @@ bool BCLog::Logger::StartLogging()
      5     // dump buffered messages from before we opened the log
      6     m_buffering = false;
      7     if (m_buffer_lines_discarded > 0) {
      8-        const auto source_loc{std::source_location::current()};
      9-        LogPrintStr_(strprintf("Early logging buffer overflowed, %d log lines discarded.\n", m_buffer_lines_discarded), source_loc, BCLog::ALL, Level::Info, /*should_ratelimit=*/false);
     10+        LogPrintStr_(strprintf("Early logging buffer overflowed, %d log lines discarded.\n", m_buffer_lines_discarded), std::source_location::current(), BCLog::ALL, Level::Info, /*should_ratelimit=*/false);
     11     }
     12     while (!m_msgs_before_open.empty()) {
     13         const auto& buflog = m_msgs_before_open.front();
     14@@ -434,13 +433,13 @@ void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags catego
     15     str.insert(0, LogTimestampStr(now, mocktime));
     16 }
     17 
     18-void BCLog::Logger::LogPrintStr(std::string_view str, const std::source_location& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     19+void BCLog::Logger::LogPrintStr(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     20 {
     21     StdLockGuard scoped_lock(m_cs);
     22-    return LogPrintStr_(str, source_loc, category, level, should_ratelimit);
     23+    return LogPrintStr_(str, std::move(source_loc), category, level, should_ratelimit);
     24 }
     25 
     26-void BCLog::Logger::LogPrintStr_(std::string_view str, const std::source_location& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     27+void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     28 {
     29     std::string str_prefixed = LogEscapeMessage(str);
     30 
     31@@ -451,7 +450,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, const std::source_locatio
     32                 .mocktime=GetMockTime(),
     33                 .str=str_prefixed,
     34                 .threadname=util::ThreadGetInternalName(),
     35-                .source_loc=source_loc,
     36+                .source_loc=std::move(source_loc),
     37                 .category=category,
     38                 .level=level,
     39             };
     40diff --git a/src/logging.h b/src/logging.h
     41index d588ef86dc..edef5c074f 100644
     42--- a/src/logging.h
     43+++ b/src/logging.h
     44@@ -201,7 +201,7 @@ namespace BCLog {
     45         std::list<std::function<void(const std::string&)>> m_print_callbacks GUARDED_BY(m_cs) {};
     46 
     47         /** Send a string to the log output (internal) */
     48-        void LogPrintStr_(std::string_view str, const std::source_location& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     49+        void LogPrintStr_(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     50             EXCLUSIVE_LOCKS_REQUIRED(m_cs);
     51 
     52         std::string GetLogPrefix(LogFlags category, Level level) const;
     53@@ -220,7 +220,7 @@ namespace BCLog {
     54         std::atomic<bool> m_reopen_file{false};
     55 
     56         /** Send a string to the log output */
     57-        void LogPrintStr(std::string_view str, const std::source_location& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     58+        void LogPrintStr(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
     59             EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
     60 
     61         /** Returns whether logs will be written to any output */
     62@@ -320,7 +320,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
     63 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
     64 
     65 template <typename... Args>
     66-inline void LogPrintFormatInternal(const std::source_location& source_loc, const BCLog::LogFlags flag, const BCLog::Level level, const bool should_ratelimit, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
     67+inline void LogPrintFormatInternal(std::source_location&& source_loc, const BCLog::LogFlags flag, const BCLog::Level level, const bool should_ratelimit, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
     68 {
     69     if (LogInstance().Enabled()) {
     70         std::string log_msg;
     71@@ -329,7 +329,7 @@ inline void LogPrintFormatInternal(const std::source_location& source_loc, const
     72         } catch (tinyformat::format_error& fmterr) {
     73             log_msg = "Error \"" + std::string{fmterr.what()} + "\" while formatting log message: " + fmt.fmt;
     74         }
     75-        LogInstance().LogPrintStr(log_msg, source_loc, flag, level, should_ratelimit);
     76+        LogInstance().LogPrintStr(log_msg, std::move(source_loc), flag, level, should_ratelimit);
     77     }
     78 }
     79 
     80diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
     81index 0043df182e..f5ce17aa23 100644
     82--- a/src/test/logging_tests.cpp
     83+++ b/src/test/logging_tests.cpp
     84@@ -97,12 +97,12 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintStr, LogSetup)
     85         std::source_location::current(),
     86         std::source_location::current(),
     87     };
     88-    LogInstance().LogPrintStr("foo1: bar1", source_locs[0], BCLog::LogFlags::NET, BCLog::Level::Debug, /*should_ratelimit=*/false);
     89-    LogInstance().LogPrintStr("foo2: bar2", source_locs[1], BCLog::LogFlags::NET, BCLog::Level::Info, /*should_ratelimit=*/false);
     90-    LogInstance().LogPrintStr("foo3: bar3", source_locs[2], BCLog::LogFlags::ALL, BCLog::Level::Debug, /*should_ratelimit=*/false);
     91-    LogInstance().LogPrintStr("foo4: bar4", source_locs[3], BCLog::LogFlags::ALL, BCLog::Level::Info, /*should_ratelimit=*/false);
     92-    LogInstance().LogPrintStr("foo5: bar5", source_locs[4], BCLog::LogFlags::NONE, BCLog::Level::Debug, /*should_ratelimit=*/false);
     93-    LogInstance().LogPrintStr("foo6: bar6", source_locs[5], BCLog::LogFlags::NONE, BCLog::Level::Info, /*should_ratelimit=*/false);
     94+    LogInstance().LogPrintStr("foo1: bar1", std::move(source_locs[0]), BCLog::LogFlags::NET, BCLog::Level::Debug, /*should_ratelimit=*/false);
     95+    LogInstance().LogPrintStr("foo2: bar2", std::move(source_locs[1]), BCLog::LogFlags::NET, BCLog::Level::Info, /*should_ratelimit=*/false);
     96+    LogInstance().LogPrintStr("foo3: bar3", std::move(source_locs[2]), BCLog::LogFlags::ALL, BCLog::Level::Debug, /*should_ratelimit=*/false);
     97+    LogInstance().LogPrintStr("foo4: bar4", std::move(source_locs[3]), BCLog::LogFlags::ALL, BCLog::Level::Info, /*should_ratelimit=*/false);
     98+    LogInstance().LogPrintStr("foo5: bar5", std::move(source_locs[4]), BCLog::LogFlags::NONE, BCLog::Level::Debug, /*should_ratelimit=*/false);
     99+    LogInstance().LogPrintStr("foo6: bar6", std::move(source_locs[5]), BCLog::LogFlags::NONE, BCLog::Level::Info, /*should_ratelimit=*/false);
    100     std::ifstream file{tmp_log_path};
    101     std::vector<std::string> log_lines;
    102     for (std::string log; std::getline(file, log);) {
    

    Crypt-iQ commented at 3:37 pm on June 11, 2025:
    I’ve sort of implemented this. I wasn’t able to use std::source_location&& in LogPrintStr_ because it’s passed to two functions (FormatLogStrInPlace and NeedsRateLimiting) and implicit invalidation might occur. Instead, I pass std::source_location& and call std::move only when needed to invoke the move constructor.

    Crypt-iQ commented at 5:33 pm on June 12, 2025:
    I am not sure how I feel about my approach here because the caller of LogPrintStr_ now needs to be aware that std::source_location& can be moved-from?

    Crypt-iQ commented at 11:18 pm on June 12, 2025:
    I misunderstood the original suggestion and have now implemented it in ca0c1a7a9ce9198c950e123d36c9e4bb13db13da
  55. stickies-v commented at 10:07 am on June 6, 2025: contributor
    Approach ACK. I think this PR needs release notes because of the behaviour change.
  56. Crypt-iQ force-pushed on Jun 11, 2025
  57. Crypt-iQ force-pushed on Jun 11, 2025
  58. DrahtBot added the label CI failed on Jun 11, 2025
  59. DrahtBot commented at 3:46 pm on June 11, 2025: contributor

    🚧 At least one of the CI tasks failed. Task ARM, unit tests, no functional tests: https://github.com/bitcoin/bitcoin/runs/43903011789 LLM reason (✨ experimental): The CI failure is caused by a compilation error in logging_tests.cpp due to a narrowing type conversion resulting in the build stopping.

    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.

  60. Crypt-iQ commented at 4:56 pm on June 11, 2025: contributor

    The latest push a7bd7c9a69de72739326026b7ab660181c8d02fb includes:

    • rate limiting for callers of LogPrintLevel with level >= BCLog::Level::Info.
    • fix for the windows cross-built CI by using GetFileSize instead of fs::file_size in logging_tests/rate_limiting

    I’ve repurposed the old logging rpc from #21603 in another branch for testing here. This can be used to log from source locations and test out the new changes in logging behavior. Note that the setmocktime RPC needs to be used to set the mock time past the current clock time to reset the logging messages.

    Example usage:

    0bitcoin-cli -regtest excessivelog 1 1048500 # log just under 1MiB
    1bitcoin-cli -regtest excessivelog 1 100 # this should get the total amount logged above 1MiB
    2                                        # and the rate limiting logic should kick in
    3bitcoin-cli -regtest excessivelog 2 1048500
    4bitcoin-cli -regtest excessivelog 2 100 # trigger rate limiting from another location
    5bitcoin-cli -regtest setmocktime <N> # set this past the current clock time because of how `LogRateLimiter` is initialized
    6bitcoin-cli -regtest excessivelog 1 100 # this should trigger logging to resume and will log the source locations that were reset
    

    Example output:

    0[*] Excessive logging detected from rpc/node.cpp:139 (RPCHelpMan excessivelog()): >1 MiB logged during the last hour.Suppressing logging to disk from this source location for up to one hour. Console logging unaffected. Last log entry.2025-06-11T16:32:41Z aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    1[*] Excessive logging detected from rpc/node.cpp:142 (RPCHelpMan excessivelog()): >1 MiB logged during the last hour.Suppressing logging to disk from this source location for up to one hour. Console logging unaffected. Last log entry.2025-06-11T16:33:04Z aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
    2Restarting logging from rpc/node.cpp:139 (RPCHelpMan excessivelog()): (1 MiB) were dropped during the last hour.
    3Restarting logging from rpc/node.cpp:142 (RPCHelpMan excessivelog()): (0 MiB) were dropped during the last hour.
    
  61. DrahtBot removed the label CI failed on Jun 11, 2025
  62. Crypt-iQ force-pushed on Jun 12, 2025
  63. in src/logging.cpp:398 in b7d9c669ca outdated
    396+    if (is_ratelimited && !was_ratelimited) {
    397+        // Logging from this source location will be suppressed until the current window resets.
    398+        m_suppressed_locations.insert(source_loc);
    399+
    400+        str.insert(0, strprintf("Excessive logging detected from %s:%d (%s): >%d MiB logged during the last hour."
    401+                                "Suppressing logging to disk from this source location for up to one hour. "
    


    DrahtBot commented at 9:40 am on June 13, 2025:
    In the two-part string literal in NeedsRateLimiting: “…during the last hour.”“Suppressing logging…” -> “…during the last hour. Suppressing logging…” [missing space between sentences due to C++ literal concatenation]
  64. in src/logging.h:51 in b7d9c669ca outdated
    46+    size_t operator()(const std::source_location& s) const noexcept
    47+    {
    48+        // Use CSipHasher(0, 0) as a simple way to get uniform distribution.
    49+        return static_cast<size_t>(CSipHasher(0, 0)
    50+                                       .Write(std::hash<std::string_view>{}(s.file_name()))
    51+                                       .Write(std::hash<int>{}(s.line()))
    


    stickies-v commented at 10:47 am on June 19, 2025:

    nit: I don’t think hashing the line is necessary?

    0                                       .Write(s.line())
    

    Crypt-iQ commented at 8:51 pm on June 23, 2025:
    Yup good catch
  65. in src/logging.cpp:392 in b7d9c669ca outdated
    389+    was_ratelimited = m_suppressed_locations.find(source_loc) != m_suppressed_locations.end();
    390+
    391+    // If the window has elapsed, then we need to clear the unordered map and set.
    392+    MaybeResetWindow(str);
    393+
    394+    is_ratelimited = !m_source_locations[source_loc].Consume(str.size());
    


    stickies-v commented at 11:07 am on June 19, 2025:

    nit: I think this can be simplified a bit:

     0diff --git a/src/logging.cpp b/src/logging.cpp
     1index 827d1bad10..fec8e5ec58 100644
     2--- a/src/logging.cpp
     3+++ b/src/logging.cpp
     4@@ -379,17 +379,13 @@ static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
     5 
     6 bool BCLog::LogRateLimiter::NeedsRateLimiting(const std::source_location& source_loc, std::string& str)
     7 {
     8-    // Whether or not logging to disk was/is ratelimited for this source location.
     9-    bool was_ratelimited{false};
    10-    bool is_ratelimited{false};
    11-
    12     // Check to see if we were rate limited before calling MaybeResetWindow.
    13-    was_ratelimited = m_suppressed_locations.find(source_loc) != m_suppressed_locations.end();
    14+    bool was_ratelimited{m_suppressed_locations.contains(source_loc)};
    15 
    16     // If the window has elapsed, then we need to clear the unordered map and set.
    17     MaybeResetWindow(str);
    18 
    19-    is_ratelimited = !m_source_locations[source_loc].Consume(str.size());
    20+    bool is_ratelimited{!m_source_locations[source_loc].Consume(str.size())};
    21 
    22     if (is_ratelimited && !was_ratelimited) {
    23         // Logging from this source location will be suppressed until the current window resets.
    
  66. in src/logging.h:216 in ca0c1a7a9c outdated
    195@@ -196,15 +196,15 @@ namespace BCLog {
    196         /** Log categories bitfield. */
    197         std::atomic<CategoryMask> m_categories{BCLog::NONE};
    198 
    199-        void FormatLogStrInPlace(std::string& str, LogFlags category, Level level, std::string_view source_file, int source_line, std::string_view logging_function, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const;
    200+        void FormatLogStrInPlace(std::string& str, LogFlags category, Level level, const std::source_location& source_loc, std::string_view threadname, SystemClock::time_point now, std::chrono::seconds mocktime) const;
    


    stickies-v commented at 11:10 am on June 19, 2025:
    nit: updating the ca0c1a7a9ce9198c950e123d36c9e4bb13db13da commit message with a brief mention of the behaviour change for -logsourcelocations would be helpful for both review and historical documentation purposes.
  67. in src/logging.cpp:368 in ca0c1a7a9c outdated
    364@@ -364,7 +365,7 @@ std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level l
    365 
    366 static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
    367 {
    368-    return buflog.str.size() + buflog.logging_function.size() + buflog.source_file.size() + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    369+    return buflog.str.size() + strlen(buflog.source_loc.function_name()) + strlen(buflog.source_loc.file_name()) + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    


    stickies-v commented at 11:30 am on June 19, 2025:
    nit: I don’t think it’s crucial to be super precise here, since it’s only used briefly during startup, but I think this underestimating the std::source_location size a bit. It’s not accounting the 2*4 bytes for line and column, nor the pointers (3 on gcc) used to implement std::source_location. If people think it’s crucial to get this (more) right, probably adding a separate MemUsage(const std::source_location&) could make sense.

    Crypt-iQ commented at 12:49 pm on June 19, 2025:
    I also noticed this and wasn’t sure if it was worth bringing up.

    stickies-v commented at 3:46 pm on June 19, 2025:

    I was long overdue for my biannual confusion about (dynamic) memory usage, but luckily here it is. It seems like I got it entirely backwards in my previous comment. The memory pointed to by std::source_location is static, so I don’t think we need to account for any of it beyond what the memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>)) statement is already doing.

    Orthogonal, but since we’re touching the line anyway: dynamic memory usage of std::string should be measured with .capacity() instead of .size(). Or even better, use the DynamicUsage(const std::string&) function we already have to also account for small string optimization, which, of course, is totally a thing.

    So the function could become:

    0static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
    1{
    2    return memusage::DynamicUsage(buflog.str) +
    3           memusage::DynamicUsage(buflog.threadname) +
    4           memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    5}
    

    All in all, this should mean that thanks to ca0c1a7a9ce9198c950e123d36c9e4bb13db13da we can now buffer quite a bit more messages within our 1MB limit. Obviously not going to change anything meaningful, but wouldn’t hurt to add a line to commit msg to highlight that.


    Crypt-iQ commented at 8:51 pm on June 23, 2025:
    Nice catch, I missed that std::source_location was part of BufferedLog and therefore included in the MallocUsage call.

    l0rinc commented at 9:47 am on June 24, 2025:
    👍
  68. in src/logging.cpp:425 in ca0c1a7a9c outdated
    418@@ -418,13 +419,13 @@ void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags catego
    419     str.insert(0, LogTimestampStr(now, mocktime));
    420 }
    421 
    422-void BCLog::Logger::LogPrintStr(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
    423+void BCLog::Logger::LogPrintStr(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level)
    424 {
    425     StdLockGuard scoped_lock(m_cs);
    426-    return LogPrintStr_(str, logging_function, source_file, source_line, category, level);
    427+    return LogPrintStr_(str, std::move(source_loc), category, level);
    


    stickies-v commented at 11:32 am on June 19, 2025:
    nit: requires #include <utility>
  69. in src/test/logging_tests.cpp:125 in b7d9c669ca outdated
    132+        "[" + file_name + ":" + util::ToString(source_locs[0].line()) + "] [" + function_name + "] [net] foo1: bar1",
    133+        "[" + file_name + ":" + util::ToString(source_locs[1].line()) + "] [" + function_name + "] [net:info] foo2: bar2",
    134+        "[" + file_name + ":" + util::ToString(source_locs[2].line()) + "] [" + function_name + "] [debug] foo3: bar3",
    135+        "[" + file_name + ":" + util::ToString(source_locs[3].line()) + "] [" + function_name + "] foo4: bar4",
    136+        "[" + file_name + ":" + util::ToString(source_locs[4].line()) + "] [" + function_name + "] [debug] foo5: bar5",
    137+        "[" + file_name + ":" + util::ToString(source_locs[5].line()) + "] [" + function_name + "] foo6: bar6",
    


    stickies-v commented at 11:50 am on June 19, 2025:

    nit: can be cleaned up a bit:

     0diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
     1index c9b7913ee4..d673792ab6 100644
     2--- a/src/test/logging_tests.cpp
     3+++ b/src/test/logging_tests.cpp
     4@@ -114,15 +114,16 @@ BOOST_FIXTURE_TEST_CASE(logging_LogPrintStr, LogSetup)
     5     for (std::string log; std::getline(file, log);) {
     6         log_lines.push_back(log);
     7     }
     8-    std::string file_name = util::RemovePrefix(source_locs[0].file_name(), "./");
     9-    std::string function_name{source_locs[0].function_name()};
    10+    auto format_source_location = [](const std::source_location& loc) {
    11+        return tfm::format("[%s:%s] [%s]", util::RemovePrefixView(loc.file_name(), "./"), loc.line(), loc.function_name());
    12+    };
    13     std::vector<std::string> expected = {
    14-        "[" + file_name + ":" + util::ToString(source_locs[0].line()) + "] [" + function_name + "] [net] foo1: bar1",
    15-        "[" + file_name + ":" + util::ToString(source_locs[1].line()) + "] [" + function_name + "] [net:info] foo2: bar2",
    16-        "[" + file_name + ":" + util::ToString(source_locs[2].line()) + "] [" + function_name + "] [debug] foo3: bar3",
    17-        "[" + file_name + ":" + util::ToString(source_locs[3].line()) + "] [" + function_name + "] foo4: bar4",
    18-        "[" + file_name + ":" + util::ToString(source_locs[4].line()) + "] [" + function_name + "] [debug] foo5: bar5",
    19-        "[" + file_name + ":" + util::ToString(source_locs[5].line()) + "] [" + function_name + "] foo6: bar6",
    20+        format_source_location(source_locs[0]) + " [net] foo1: bar1",
    21+        format_source_location(source_locs[1]) + " [net:info] foo2: bar2",
    22+        format_source_location(source_locs[2]) + " [debug] foo3: bar3",
    23+        format_source_location(source_locs[3]) + " foo4: bar4",
    24+        format_source_location(source_locs[4]) + " [debug] foo5: bar5",
    25+        format_source_location(source_locs[5]) + " foo6: bar6",
    26     };
    27     BOOST_CHECK_EQUAL_COLLECTIONS(log_lines.begin(), log_lines.end(), expected.begin(), expected.end());
    28 }
    

    Crypt-iQ commented at 8:51 pm on June 23, 2025:
    done
  70. in src/validation.cpp:2981 in b7d9c669ca outdated
    2985-        coins_tip.DynamicMemoryUsage() * (1.0 / (1 << 20)),
    2986-        coins_tip.GetCacheSize(),
    2987-        !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages) : "");
    2988+
    2989+    // We use the private LogPrintLevel_ macro here because we want to disable rate limiting from this location. This code
    2990+    // could use LogPrintLevel with the Info level to bypass rate-limiting, but it is more opaque. Using LogPrintLevel_
    


    stickies-v commented at 12:02 pm on June 19, 2025:

    This code could use LogPrintLevel with the Info level to bypass rate-limiting

    I don’t think that’s correct? I’m not sure a long docstring is required here anyway, we already have decent documentation for the macros and functions involved. A brief “don’t rate limit because this can be hit frequently during IBD” seems sufficient to me without overloading the reader with information.


    Crypt-iQ commented at 12:48 pm on June 19, 2025:
    I think this is an old comment, will change.
  71. in src/test/util/logging.h:38 in b7d9c669ca outdated
    32@@ -33,7 +33,7 @@ class DebugLogHelper
    33 
    34 public:
    35     explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });
    36-    ~DebugLogHelper() { check_found(); }
    37+    ~DebugLogHelper() noexcept(false) { check_found(); }
    


    stickies-v commented at 12:05 pm on June 19, 2025:
    nit: (a brief version of) the commit message in 32615bffa6424625078e5bd9f6d79c7e99c9a322 would probably be helpful here as a docstring too?

    l0rinc commented at 7:36 pm on July 6, 2025:

    The resulting comment just repeats what the code already does:

    //! Mark as noexcept(false) to catch any thrown exceptions.

    We don’t comment every noexcept(false) since it’s self-explanatory. What else could noexcept(false) mean?

    is just repeating what the doc states:

    expression is specified to be non-throwing(since C++17), and false otherwise.

  72. in src/logging.cpp:400 in b7d9c669ca outdated
    397+        // Logging from this source location will be suppressed until the current window resets.
    398+        m_suppressed_locations.insert(source_loc);
    399+
    400+        str.insert(0, strprintf("Excessive logging detected from %s:%d (%s): >%d MiB logged during the last hour."
    401+                                "Suppressing logging to disk from this source location for up to one hour. "
    402+                                "Console logging unaffected. Last log entry.",
    


    stickies-v commented at 12:18 pm on June 19, 2025:
    Should probably terminate with \n?
  73. stickies-v approved
  74. stickies-v commented at 12:28 pm on June 19, 2025: contributor

    utACK b7d9c669cadf666f595a3e20ee3726a9ede88acd modulo missing release notes

    Thanks for quickly addressing my copious amounts of comments, it’s been fun reviewing this. I’ve left a few (final?) nits, but none are blocking. Would prefer to squash the last 2 test commits into the 2 commits where the relevant functionality was introduced, but either is fine.

  75. DrahtBot requested review from brunoerg on Jun 19, 2025
  76. DrahtBot requested review from janb84 on Jun 19, 2025
  77. DrahtBot requested review from l0rinc on Jun 19, 2025
  78. Crypt-iQ force-pushed on Jun 23, 2025
  79. in src/logging.h:41 in 2ac8696b53 outdated
    34@@ -31,6 +35,24 @@ extern const char * const DEFAULT_DEBUGLOGFILE;
    35 
    36 extern bool fLogIPs;
    37 
    38+struct SourceLocationEqual {
    39+    bool operator()(const std::source_location& lhs, const std::source_location& rhs) const noexcept
    40+    {
    41+        return lhs.line() == rhs.line() && strcmp(lhs.file_name(), rhs.file_name()) == 0;
    


    l0rinc commented at 7:32 am on June 24, 2025:

    nit: I find mixing C++20 code with C API a bit clunky, if you touch again, consider either comparing their std::string_view wrappers or maybe even adding such a helper to string.h for const char* values.

    edit: also note that equality check may be easier to optimize internally than a comparison

  80. in src/logging.cpp:115 in 2ac8696b53 outdated
    107@@ -106,6 +108,15 @@ void BCLog::Logger::DisconnectTestLogger()
    108     m_msgs_before_open.clear();
    109 }
    110 
    111+void BCLog::Logger::ResetLimiter()
    112+{
    113+    StdLockGuard scoped_lock(m_cs);
    114+    const auto now{NodeClock::now()};
    115+    m_limiter.m_last_reset = now;
    


    l0rinc commented at 7:34 am on June 24, 2025:

    I understand when the time is split off from the measured code to make sure it’s accurate - but I don’t see how that’s the case here, consider inlining it:

    0    m_limiter.m_last_reset = NodeClock::now();
    
  81. in src/logging.h:361 in 2ac8696b53 outdated
    342 
    343-#define LogPrintLevel_(category, level, ...) LogPrintFormatInternal(__func__, __FILE__, __LINE__, category, level, __VA_ARGS__)
    344+#define LogPrintLevel_(category, level, should_ratelimit, ...) LogPrintFormatInternal(std::source_location::current(), category, level, should_ratelimit, __VA_ARGS__)
    345 
    346-// Log unconditionally.
    347+// Log unconditionally. Uses basic rate limiting to mitigate disk filling attacks.
    


    l0rinc commented at 7:44 am on June 24, 2025:
    Seems to me the first two sentences contradict each other now: “log unconditionally, unless it doesn’t fulfill the rate limiting condition”

    stickies-v commented at 12:15 pm on June 24, 2025:

    Semantically, I agree. But practically, I think it’s fine as is. Conditional vs unconditional are at this point fairly established concepts in our logging code in that they indicate whether or not logging categories need to be enabled or not. Rate limiting should in practice never occur, so for all intents and purposes, it is indeed unconditional.

    How would you suggest to improve this? I can’t think of an improvement.


    l0rinc commented at 12:20 pm on June 24, 2025:
    I don’t consider “established” a relevant argument in this case - it just prevents new contributors from understanding the codebase. But if you think this isn’t confusing, I don’t mind resolving the comment, I did eventually understand it, just got sidetracked by the contradiction and wanted to simplify it to the next person. If we do correct it, we could be more specific instead of overly generalizing it (ie specify the restrictions instead of claiming there are none).

    Crypt-iQ commented at 7:22 pm on June 26, 2025:

    I don’t think this is confusing because of prior context. I can understand the point about newer contributors. I’ve come across other places in the codebase where I’ve had to learn the context by combing through IRC, PRs, commit messages, blogs, etc. I think it’s similar to adding the information in the commit message rather than including a comment since a user must search for the commit message info.

    EDIT: That said, I’m open to suggestions, but I thought the context was pretty clear for me because I went through the code and the original PR.

  82. in src/logging.h:380 in 2ac8696b53 outdated
    370+// Log by prefixing the output with the passed category name and severity level. This can either
    371+// log conditionally if the category is allowed or unconditionally if level >= BCLog::Level::Info
    372+// is passed. If this function logs unconditionally, logging to disk is rate-limited. This is
    373+// important so that callers don't need to worry about accidentally introducing a disk-fill
    374+// vulnerability if level >= Info is used. Additionally, users specifying -debug are assumed to be
    375+// developers or power users who are aware that -debug may cause excessive disk usage due to logging.
    


    l0rinc commented at 7:49 am on June 24, 2025:
    That’s a reasonable assumption - if a bug could trigger debug logging, we’d likely have bigger problems I guess
  83. in src/test/logging_tests.cpp:71 in 2ac8696b53 outdated
    66@@ -60,6 +67,9 @@ struct LogSetup : public BasicTestingSetup {
    67 
    68         LogInstance().SetLogLevel(BCLog::Level::Debug);
    69         LogInstance().SetCategoryLogLevel({});
    70+
    71+        // Prevent tests from getting rate-limited.
    


    l0rinc commented at 7:50 am on June 24, 2025:
    Does this “prevent” rate limiting or just allows one more megabyte?

    stickies-v commented at 12:10 pm on June 24, 2025:
    The latter. “Ensure tests don’t use each other’s rate limiting budget” could be an alternative?
  84. in src/test/logging_tests.cpp:105 in 2ac8696b53 outdated
    107+        std::source_location::current(),
    108+        std::source_location::current(),
    109+        std::source_location::current(),
    110+        std::source_location::current(),
    111+        std::source_location::current(),
    112+    };
    


    l0rinc commented at 7:59 am on June 24, 2025:

    this seems a bit hacky to me - this is basically a parameterized test where we implicitly verify the inputs produce the desired outputs, line-by-line. It could make sense to store it in a way that indicates more clearly which outputs we’re expecting for which inputs.

    We do need separate ::current() calls to capture that the lines are assigned properly, we could add those to the parameterized test values as well.

    I also find it a bit awkward that we’re concatenating the result of a tfm::format with strings instead of having a final formatter.

    Lastly, do we really need the RemovePrefix here, given that we know the exact file structiure?

    Please consider something like this for clarity:

     0BOOST_FIXTURE_TEST_CASE(logging_LogPrintStr, LogSetup)
     1{
     2    LogInstance().m_log_sourcelocations = true;
     3
     4    struct Case {
     5        std::string msg;
     6        BCLog::LogFlags cat;
     7        BCLog::Level level;
     8        std::string prefix;
     9        std::source_location loc;
    10    };
    11
    12    std::vector<Case> cases = {
    13        {"foo1: bar1", BCLog::NET,  BCLog::Level::Debug, "[net] ",      std::source_location::current()},
    14        {"foo2: bar2", BCLog::NET,  BCLog::Level::Info,  "[net:info] ", std::source_location::current()},
    15        {"foo3: bar3", BCLog::ALL,  BCLog::Level::Debug, "[debug] ",    std::source_location::current()},
    16        {"foo4: bar4", BCLog::ALL,  BCLog::Level::Info,  "",            std::source_location::current()},
    17        {"foo5: bar5", BCLog::NONE, BCLog::Level::Debug, "[debug] ",    std::source_location::current()},
    18        {"foo6: bar6", BCLog::NONE, BCLog::Level::Info,  "",            std::source_location::current()},
    19    };
    20
    21    std::vector<std::string> expected;
    22    for (auto& [msg, cat, level, prefix, loc] : cases) {
    23        expected.push_back(tfm::format("[%s:%s] [%s] %s%s", loc.file_name(), loc.line(), loc.function_name(), prefix, msg));
    24        LogInstance().LogPrintStr(msg, std::move(loc), cat, level, /*should_ratelimit=*/false);
    25    }
    26    std::ifstream file{tmp_log_path};
    27    std::string line;
    28    for (const auto& exp : expected) {
    29        BOOST_REQUIRE(std::getline(file, line));
    30        BOOST_CHECK_EQUAL(line, exp);
    31    }
    32    BOOST_CHECK(!std::getline(file, line));
    33}
    

    Crypt-iQ commented at 9:33 pm on July 2, 2025:

    Lastly, do we really need the RemovePrefix here, given that we know the exact file structiure?

    Yup, the file name may differ across platforms and the test will fail on some platforms otherwise. Necessary because it’s done here: https://github.com/bitcoin/bitcoin/blob/68ca13e1f96a9dae997558a1d7a873b10056091b/src/logging.cpp#L377

    I have implemented your suggestion other than that.

  85. in src/test/logging_tests.cpp:300 in 2ac8696b53 outdated
    296@@ -276,4 +297,126 @@ BOOST_FIXTURE_TEST_CASE(logging_Conf, LogSetup)
    297     }
    298 }
    299 
    300+BOOST_AUTO_TEST_CASE(logging_sourcelocation_counter)
    


    l0rinc commented at 8:26 am on June 24, 2025:
    👍 nice and clear test
  86. in src/logging.h:48 in 2ac8696b53 outdated
    43+};
    44+
    45+struct SourceLocationHasher {
    46+    size_t operator()(const std::source_location& s) const noexcept
    47+    {
    48+        // Use CSipHasher(0, 0) as a simple way to get uniform distribution.
    


    l0rinc commented at 8:35 am on June 24, 2025:

    I’m not a fan of dead comments, could we rather extract it to a named variable, something like CSipHasher uniform(0, 0) instead of explaining code with text?


    nit: Maybe my formatter is off, but do we really need to push the formatted lines so far out?


    stickies-v commented at 10:49 am on June 24, 2025:
    I don’t think this is a dead comment, the choice for using siphash over e.g. std::hash is not obvious (to me), and just naming the variable uniform would not have been clear to me. Would prefer keeping this as is.

    l0rinc commented at 10:51 am on June 24, 2025:
    Can we name the variable such that the choice is obvious instead of adding extra comments?

    l0rinc commented at 10:58 am on June 24, 2025:
    By “dead” I meant that it doesn’t bite back (like code does), it can state absolutely anything and the compiler won’t help us. Comments usually seem to me like we’ve given up on trying to express our design within the boundaries of the language. In a few cases here I think the code can be slightly improved to eliminate the need for extra comments.

    stickies-v commented at 12:03 pm on June 24, 2025:
    The choice of a hashing algorithm has many dimensions (distribution, performance, collision resistance, …). Capturing that rationale in a variable name seems an unrealistic expectation. I’m all for removing dead comments but this seems not the place to me.

    l0rinc commented at 12:06 pm on June 24, 2025:
    It’s possible that this isn’t the place, but it doesn’t currently explain performance or collision resistance either - but if those are importance, maybe we could explain them in the commit message instead.

    Crypt-iQ commented at 9:35 pm on July 2, 2025:
    I have kept the comment – I made a note in the commit message about the change.
  87. in src/logging.h:105 in 2ac8696b53 outdated
    103@@ -82,15 +104,74 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    


    l0rinc commented at 8:37 am on June 24, 2025:
    I find this confusing (either the name or the value), by default debug logs aren’t displayed…

    stickies-v commented at 12:08 pm on June 24, 2025:
    This variable is not used or changed in this PR.

    l0rinc commented at 12:14 pm on June 24, 2025:
    This argument may not be, but the test is validating the changed code, dead parameter names don’t increase my confidence in the correctness of the PR. It may have an obvious explanation, but it seemed related to me, given that we’re touching surrounding code

    stickies-v commented at 12:22 pm on June 24, 2025:
    What do you mean with “dead parameter names”? This variable defines the default log level, which indeed is debug level. If you run bitcoind -debug=net, you’ll start seeing net debug logs, you don’t have to set -loglevel=debug.

    l0rinc commented at 12:30 pm on June 24, 2025:
    I replied to the wrong thread here, thanks for the explanation, please resolve it.
  88. in src/logging.h:110 in 2ac8696b53 outdated
    103@@ -82,15 +104,74 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    106     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    107+    constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
    108+
    109+    //! Keeps track of an individual source location and how many available bytes are left for logging from it.
    110+    class SourceLocationCounter
    


    l0rinc commented at 8:43 am on June 24, 2025:
    naming nit again: we’re not counting the source locations, rather assigning the statistics we collect here to a std::source_location later - we’re mixing abstraction levels this way, we’re naming something based on how we’re planning on using it later (instead of what it is or what it does), such as LogLimitStats or something similar.
  89. in src/test/logging_tests.cpp:380 in 2ac8696b53 outdated
    326+    case 0:
    327+        LogInfo("%s\n", message);
    328+        break;
    329+    case 1:
    330+        LogInfo("%s\n", message);
    331+        break;
    


    l0rinc commented at 8:50 am on June 24, 2025:
    These two cases are basically the same and they depend on a magic int that I will have to tie to some other integers somewhere else to understand what it means. Could we make this a parameterized test as well, maybe with a std::function and more type safety instead of an int with only 4 allowed values

    Crypt-iQ commented at 9:40 pm on July 2, 2025:
    I haven’t implemented this, could this be left for a follow-up?

    l0rinc commented at 9:45 pm on July 2, 2025:
    I doubt anyone will review it if we push it separately - but I think we could make these tests less awkward. Not a blocker from my side, just a preference

    Crypt-iQ commented at 10:01 pm on July 2, 2025:
    I agree with you that the tests are awkward, but I do want to limit churn as this PR has been through a few iterations. I would definitely review a follow-up if you wanted to make the PR. This could also be a good candidate for https://www.bitcoin-followups.info/

    l0rinc commented at 12:58 pm on July 6, 2025:

    I share the eagerness to get this merged, but I don’t think this PR is ready yet. It still feels like a draft, not polished yet, contains multiple styles because we don’t want to touch a previous author’s work. As far as I can tell there’s no rush, let’s do it properly and minimize having to touch the same code in multiple PRs.


    Anf if you insist on keeping this logic here, at least merge the two identical cases:

    0    case 0:
    1    case 1:
    2        LogInfo("%s\n", message);
    3        break;
    

    stickies-v commented at 12:16 pm on July 7, 2025:

    contains multiple styles

    I don’t think that’s a good reason to hold up a useful and important PR that already has gone several iterations (in this and a previous PR) when nitting about documentation styles can block progress entirely as eventually reviewers lose interest and dilligence in re-reviewing. Follow-ups are entirely appropriate places for this to happen.

  90. in src/test/logging_tests.cpp:420 in 2ac8696b53 outdated
    365+    const char* log_path{utf8_path.c_str()};
    366+
    367+    // Use GetFileSize because fs::file_size may require a flush to be accurate.
    368+    std::streamsize log_file_size{static_cast<std::streamsize>(GetFileSize(log_path))};
    369+
    370+    // Logging 1 MiB should be allowed.
    


    l0rinc commented at 8:53 am on June 24, 2025:
    can we make this value parameterizable instead of referencing a value in a comment? It’s not trivial that the 1024 (or 512 + 512 in other places) has to coincide with the 1_MiB in another place.

    Crypt-iQ commented at 9:39 pm on July 2, 2025:
    Now uses a parameter, but I have kept the comment. I think it is clear as is? I think a follow-up could address this?
  91. in src/validation.cpp:2980 in 2ac8696b53 outdated
    2984-        chainman.GuessVerificationProgress(tip),
    2985-        coins_tip.DynamicMemoryUsage() * (1.0 / (1 << 20)),
    2986-        coins_tip.GetCacheSize(),
    2987-        !warning_messages.empty() ? strprintf(" warning='%s'", warning_messages) : "");
    2988+
    2989+    // Disable rate limiting in LogPrintLevel_ so that this location is not rate limited during IBD.
    


    l0rinc commented at 8:55 am on June 24, 2025:
    the code already states /*should_ratelimit=*/false, there’s no point in repeating that a few more times in a comment

    stickies-v commented at 12:26 pm on June 24, 2025:
    The point of the docstring is to explain why we don’t ratelimit, which is because we don’t want IBD tip logs to be suppressed. Documenting that absolutely seems helpful to me.

    l0rinc commented at 12:28 pm on June 24, 2025:
    Yes, that part does seem helpful and we can’t express it with code, we should keep that part!

    Crypt-iQ commented at 9:40 pm on July 2, 2025:
    I’ve modified the comment so it doesn’t repeat the phrase “rate limit” twice.

    l0rinc commented at 7:50 pm on July 6, 2025:
    It’s still a bit noisy, but adding IBD as context could be helpful for some.
  92. in src/logging.h:121 in 40298a86d2 outdated
    116+        uint64_t m_dropped_bytes{0};
    117+
    118+    public:
    119+        //! Consume bytes from the window if enough bytes are available.
    120+        //!
    121+        //! Returns whether or not enough bytes were available.
    


    l0rinc commented at 8:59 am on June 24, 2025:
    0        //! Returns whether enough bytes were available.
    
  93. in src/logging.cpp:532 in 40298a86d2 outdated
    527+    const auto now{NodeClock::now()};
    528+    if ((now - m_last_reset) >= WINDOW_SIZE) {
    529+        m_last_reset = now;
    530+
    531+        // Iterate through m_suppressed_locations and log that we're resetting the window for each
    532+        // suppressed location.
    


    l0rinc commented at 9:03 am on June 24, 2025:
    The comment duplicates the code, this is already clear from the method name and from what follows.
  94. in src/logging.h:107 in 40298a86d2 outdated
    103@@ -82,6 +104,65 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    106     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    107+    constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
    


    l0rinc commented at 9:10 am on June 24, 2025:
    do we need to mix the two system here, base 1000 with base 1024?

    stickies-v commented at 12:06 pm on June 24, 2025:
    Both are frequently used throughout the codebase. RATELIMIT_MAX_BYTES and DEFAULT_MAX_LOG_BUFFER are not related to eachother beyond both being used in logging. I don’t see any footgun potential between using one or the other, so this seems like an either-is-fine option that can be left to the author to decide, and very quickly becomes bikeshedding otherwise.
  95. in src/logging.cpp:369 in d41a39e43c outdated
    365@@ -364,7 +366,9 @@ std::string BCLog::Logger::GetLogPrefix(BCLog::LogFlags category, BCLog::Level l
    366 
    367 static size_t MemUsage(const BCLog::Logger::BufferedLog& buflog)
    368 {
    369-    return buflog.str.size() + buflog.logging_function.size() + buflog.source_file.size() + buflog.threadname.size() + memusage::MallocUsage(sizeof(memusage::list_node<BCLog::Logger::BufferedLog>));
    370+    return memusage::DynamicUsage(buflog.str) +
    


    l0rinc commented at 9:16 am on June 24, 2025:

    I think this memory usage change needs a commit message explanation in d41a39e43c219eb0b06b8df2c2c5d24baaf015c4 (it explains why logging_function counting was removed, but not why source_file was as far as I can tell)


    Did you validate in any way that this is indeed representative of the memory consumption? Or it’s just a guess anyway, so it’s not important?


    Crypt-iQ commented at 12:33 pm on June 26, 2025:
    I didn’t validate it, but I can. I think it is important that the new and old versions of MemUsage are roughly the same.

    Crypt-iQ commented at 9:41 pm on July 2, 2025:
    Modified the commit message and have checked that MemUsage returns an accurate accounting of the memory used. Thanks for this comment, it encouraged me to verify this.

    l0rinc commented at 9:47 pm on July 2, 2025:
    Can you show us the raw measurements?

    Crypt-iQ commented at 3:55 pm on July 7, 2025:
    IIRC, logging a 1024-byte line in my test had a memory overhead of ~1140 bytes. Prior to the change it was ~1200 bytes or so.
  96. in src/logging.cpp:412 in d41a39e43c outdated
    417@@ -414,13 +418,13 @@ void BCLog::Logger::FormatLogStrInPlace(std::string& str, BCLog::LogFlags catego
    418     str.insert(0, LogTimestampStr(now, mocktime));
    


    l0rinc commented at 9:22 am on June 24, 2025:
    isn’t it expensive to insert so often in front of the string?

    Crypt-iQ commented at 9:43 pm on July 2, 2025:
    I have not run benchmarks, though this insert is pre-existing so will resolve. It would be nicer if there was a better way to format the string other than the insert-at-beginning logic in FormatLogStrInPlace.
  97. in test/functional/feature_config_args.py:85 in d41a39e43c outdated
    81@@ -82,7 +82,7 @@ def test_negated_config(self):
    82 
    83         self.log.debug('Verifying that disabling of the config file means garbage inside of it does ' \
    84             'not prevent the node from starting, and message about existing config file is logged')
    85-        ignored_file_message = [f'[InitConfig] Data directory "{self.nodes[0].datadir_path}" contains a "bitcoin.conf" file which is explicitly ignored using -noconf.']
    86+        ignored_file_message = [f'Data directory "{self.nodes[0].datadir_path}" contains a "bitcoin.conf" file which is explicitly ignored using -noconf.']
    


    l0rinc commented at 9:26 am on June 24, 2025:
    Can you please explain in the commit message which change caused the removal of [InitConfig] here?

    Crypt-iQ commented at 9:43 pm on July 2, 2025:
    Modified the commit message 👍
  98. in src/logging.cpp:1 in b44d31455a outdated


    l0rinc commented at 9:32 am on June 24, 2025:
    Commit message claims a source location can log up to 1MiB of data, but code states constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging - I’m confused why we have this discrepancy

    stickies-v commented at 10:53 am on June 24, 2025:
    They could be the same value, but they’re unrelated things, and I don’t think it’s relevant in this PR. DEFAULT_MAX_LOG_BUFFER represents the max size of the buffer that holds log messages (across all locations) that are produced before we start writing them to file.

    l0rinc commented at 10:55 am on June 24, 2025:
    Misunderstood that part, thanks. The different base part still applies (though I know this isn’t uncommon in the code base)

    Crypt-iQ commented at 9:44 pm on July 2, 2025:
    I agree this is confusing and threw me. The different base part is pre-existing so will resolve. I think another PR could eliminate the different bases unless there’s a compelling “historical” reason to keep both?
  99. in src/logging.cpp:469 in b44d31455a outdated
    465@@ -457,6 +466,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& so
    466     }
    467 
    468     FormatLogStrInPlace(str_prefixed, category, level, source_loc, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime());
    469+    bool ratelimit = should_ratelimit && m_limiter.NeedsRateLimiting(source_loc, str_prefixed);
    


    l0rinc commented at 9:33 am on June 24, 2025:
    can we move this closer to its usage?

    stickies-v commented at 12:17 pm on June 24, 2025:
    That would mean the rate limiting warnings are no longer logged to console and callbacks (NeedsRateLimiting has side effects), so I think keeping this as-is is better.

    l0rinc commented at 12:22 pm on June 24, 2025:

    Ah, so there’s a side-effect here that wasn’t obvious to me, thanks for clarifying.

    Edit: this seems like a code smell, doesn’t this indicate that NeedsRateLimiting does too many things and should be split out to smaller, more focused methods? If we can’t give it a proper name that reflects what it does, that’s usually a good sign that it’s not as simple as it could be


    stickies-v commented at 9:58 pm on June 24, 2025:
    I agree NeedsRateLimiting has too many responsibilities. I think it’s okay as-is, but your comment nerdsniped me into revisiting this. I’m working on a branch that removes the side effects from NeedsRateLimiting and a couple of other improvements. I’m not sure yet if it’ll be too much of a detour for this PR, but at least it’ll be interesting to be able to compare. Should have it ready tomorrow.

    stickies-v commented at 4:23 pm on June 27, 2025:

    Sorry for the wait, branch is ready: https://github.com/stickies-v/bitcoin/commits/2025-06/schedule-ratelimit-reset/ . I added the “fixup” comments as commits that should be squashable into the current branch as-is (requiring rebase for the commits above, though). Hopefully the isolation of the changes makes this easier to review and potentially adopt into this PR.

    The main change is that I’ve carved out scheduling from LogRateLimiter (leaving that to CScheduler), which allows quite a bit of subsequent improvements. Specifically for this thread, I’ve refactored NeedsRateLimiting into Consume that does not have any side-effects (except update internal accounting, as expected by the Consume name).

    The other changes (see diff) I’ve implemented in this branch include:

    • change Logger::m_limiter to a pointer so it can more easily be (re)set, allowing for easier testing.
    • add unit tests specifically for the LogRateLimiter interface that I find quite a bit easier to understand than the existing rate_limit tests, but there may be merit in keeping both (I’ve updated the latter so they don’t break, but they should probably be deduplicated at least somewhat)
    • made LogRateLimiter thread-safe since it is now used in multiple threads
    • various cleanups, including removing the unnecessary m_suppressed_locations (now a m_suppression_active boolean), parameterizing the max_bytes and reset_window, …
    • removes the need for the test-only BCLog::Logger::ResetLimiter() function

    I think the PR is in a good enough shape as it is, and I don’t want to block progress. I like the new interface of my suggestion quite a bit better than the old one, and I think it is worth the time and effort to overhaul this (I spent a lot of time carving everything out in separate commits to make rebasing easy), but I understand this PR has had a lot of back-and-forth already. I’ll leave it up to the author if he wants to change the approach here, of if I’ll do it in a follow-up (which unfortunately would be a lot of churn). In either case, I’ll shortly do a full re-review.


    Crypt-iQ commented at 3:19 pm on June 30, 2025:
    Leaving a note that I am going to implement these changes and plan to push up either later today or tomorrow.

    l0rinc commented at 4:09 pm on June 30, 2025:
    Sweet, I’m planning on reviewing it this week!

    Crypt-iQ commented at 9:44 pm on July 2, 2025:
    Latest change addresses this.
  100. l0rinc approved
  101. l0rinc commented at 9:49 am on June 24, 2025: contributor

    Can you please update the PR descripton now that there’s no UNCONDITIONAL_ALWAYS category?

    I’m mostly ok with the change, I’m testing if it introduces any slowdown and would prefer making the tests a bit more typesafe and compact (and the failure seems related to the change) and some of the new classes could use a more descriptive naming in my opinion.

  102. dergoegge changes_requested
  103. dergoegge commented at 11:10 am on June 25, 2025: member

    Code review ACK 2ac8696b53e455dd27c8341828404a23b5cb68a9

    I think the commit order needs to change, so that 3c30ee9107fd0e916f9784b091a4d02f3a73ce46 comes before the tests that make use of the change?

    nit: I’d also suggest to squash b44d31455ad46ca3ed95690dfc0a913445b9c1c9 and 5fe08e01384c4a4c8525060d365fa38c312758f0 into one commit, so there is no intermediate state between the commits where the validation logs are rate limited.

  104. DrahtBot requested review from stickies-v on Jun 25, 2025
  105. DrahtBot requested review from l0rinc on Jun 25, 2025
  106. stickies-v commented at 4:33 pm on June 27, 2025: contributor

    This thread nerdsniped me into reimplementing this PR with a different approach, leaving the scheduled resetting to CScheduler. This feels like a much more natural fit to me, and allows a couple of other improvements. See branch here, or diff here.

    Either way, I’ll do a full re-review soon. I think the PR achieves its (important) stated goals, and while I think my suggested improvement is worthwhile, it isn’t necessary or blocking. Making progress is more important than getting it perfectly right here.

  107. DrahtBot requested review from stickies-v on Jun 27, 2025
  108. in src/logging.cpp:551 in 2ac8696b53 outdated
    546+            uint64_t dropped_bytes = m_source_locations[source_loc].GetDroppedBytes();
    547+
    548+            str.insert(0, strprintf("Restarting logging from %s:%d (%s): "
    549+                                    "(%d MiB) were dropped during the last hour.\n",
    550+                                    source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    551+                                    dropped_bytes / (1024 * 1024)));
    


    stickies-v commented at 1:48 pm on June 30, 2025:

    nit:

    log strings currently look like:

    Restarting logging from ../../src/rpc/request.cpp:140 (GenerateAuthCookieResult GenerateAuthCookie(const std::optionalfs::perms &, std::string &, std::string &)): (0 MiB) were dropped during the last hour.

    Would suggest printing this as bytes and removing the parentheses.

    0                                    "%d bytes were dropped during the last hour.\n",
    1                                    source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    2                                    dropped_bytes));
    

    Crypt-iQ commented at 9:45 pm on July 2, 2025:
    Changed, I think tracking bytes is better.
  109. in src/logging.h:187 in 2ac8696b53 outdated
    170         struct BufferedLog {
    171             SystemClock::time_point now;
    172             std::chrono::seconds mocktime;
    173-            std::string str, logging_function, source_file, threadname;
    174-            int source_line;
    175+            std::string str, threadname;
    


    stickies-v commented at 2:02 pm on June 30, 2025:

    nit

    0            std::string str threadname;
    

    Crypt-iQ commented at 9:45 pm on July 2, 2025:
    Did not implement as this gave me a compilation error?

    stickies-v commented at 11:30 am on July 3, 2025:
    Sorry, please disregard. I think I misread this as std::string, threadname and probably didn’t even try compiling the suggestion.
  110. in doc/release-notes-32604.md:6 in 2ac8696b53 outdated
    0@@ -0,0 +1,6 @@
    1+Logging
    2+-------
    3+Unconditional logging to disk via LogPrintf, LogInfo, LogWarning, LogError, and
    4+LogPrintLevel is now rate limited by giving each source location a logging quota of
    5+1MiB per hour. This affects -logsourcelocation as the entire function signature is
    6+logged rather than only the name.
    


    stickies-v commented at 2:27 pm on June 30, 2025:

    nit: the -logsourcelocation behaviour change is unrelated to the rate limiting, so I think this phrasing is a bit confusing. Would put it in a separate paragraph:

    0Unconditional logging to disk via LogPrintf, LogInfo, LogWarning, LogError, and
    1LogPrintLevel is now rate limited by giving each source location a logging quota of
    21MiB per hour. (#32604)
    3
    4When `-logsourcelocation` is enabled, the log output now contains the entire function signature
    5instead of just the function name. (#32604)
    

    Crypt-iQ commented at 9:46 pm on July 2, 2025:
    Agree phrasing was confusing, have changed.
  111. stickies-v approved
  112. stickies-v commented at 2:36 pm on June 30, 2025: contributor

    ACK 2ac8696b53e455dd27c8341828404a23b5cb68a9

    I still would prefer the approach I suggested here (and am happy to re-review), but I think the PR is fine as is now too.

  113. fanquake added this to the milestone 30.0 on Jul 2, 2025
  114. fanquake commented at 1:47 pm on July 2, 2025: member
    Added this to 30.0.
  115. Crypt-iQ force-pushed on Jul 2, 2025
  116. Crypt-iQ commented at 10:15 pm on July 2, 2025: contributor

    The latest push 34fc54d5476af464407c8de64c58897a35dce5cb should address the most pressing comments. IMO there is still a bit of cleanup to be done in the tests, but I would like to leave that for a follow-up. I’m going to test this branch more, so I’ve updated the branch mentioned in OP that can trigger the rate limiting logic: https://github.com/Crypt-iQ/bitcoin/tree/log_ratelimiting_05192025_rpc

    Windows CI failure is unrelated and is addressed by https://github.com/bitcoin/bitcoin/pull/32859

  117. DrahtBot added the label CI failed on Jul 3, 2025
  118. DrahtBot commented at 0:09 am on July 3, 2025: contributor

    🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/45248600923 LLM reason (✨ experimental): Clang-tidy detected a recursive call chain in logging.cpp, leading to a function within a recursive call being treated as an error.

    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.

  119. in src/logging.h:204 in 34fc54d547 outdated
    199@@ -105,6 +200,9 @@ namespace BCLog {
    200         size_t m_cur_buffer_memusage GUARDED_BY(m_cs){0};
    201         size_t m_buffer_lines_discarded GUARDED_BY(m_cs){0};
    202 
    203+        //! Manages the rate limiting of each log location.
    204+        std::unique_ptr<LogRateLimiter> m_limiter;
    


    stickies-v commented at 10:41 am on July 3, 2025:

    This is causing a TSan issue:

     0[18:53:46.472] WARNING: ThreadSanitizer: data race (pid=9715)
     1[18:53:46.472]   Read of size 8 at 0x723c00000068 by thread T1 (mutexes: write M0):
     2[18:53:46.472]     [#0](/bitcoin-bitcoin/0/) std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>::operator bool[abi:ne200100]() const /usr/lib/llvm-20/bin/../include/c++/v1/__memory/unique_ptr.h:287:12 (bitcoind+0xc87f7a) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
     3[18:53:46.472]     [#1](/bitcoin-bitcoin/1/) BCLog::Logger::LogPrintStr_(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::source_location&&, BCLog::LogFlags, BCLog::Level, bool) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/util/./logging.cpp:455:29 (bitcoind+0xc87f7a)
     4[18:53:46.472]     [#2](/bitcoin-bitcoin/2/) BCLog::Logger::LogPrintStr(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::source_location&&, BCLog::LogFlags, BCLog::Level, bool) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/util/./logging.cpp:418:12 (bitcoind+0xc8bc32) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
     5[18:53:46.472]     [#3](/bitcoin-bitcoin/3/) void LogPrintFormatInternal<std::__1::basic_string_view<char, std::__1::char_traits<char>>>(std::__1::source_location&&, BCLog::LogFlags, BCLog::Level, bool, util::ConstevalFormatString<sizeof...(std::__1::basic_string_view<char, std::__1::char_traits<char>>)>, std::__1::basic_string_view<char, std::__1::char_traits<char>> const&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./logging.h:354:23 (bitcoind+0x6e776d) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
     6[18:53:46.472]     [#4](/bitcoin-bitcoin/4/) util::TraceThread(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/util/./util/thread.cpp:20:9 (bitcoind+0xc84c3b) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
     7[18:53:46.472]     [#5](/bitcoin-bitcoin/5/) decltype(std::declval<void (*)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>)>()(std::declval<char const*>(), std::declval<AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>())) std::__1::__invoke[abi:ne200100]<void (*)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>(void (*&&)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const*&&, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-20/bin/../include/c++/v1/__type_traits/invoke.h:179:25 (bitcoind+0x1c6a5c) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
     8[18:53:46.472]     [#6](/bitcoin-bitcoin/6/) void std::__1::__thread_execute[abi:ne200100]<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 2ul, 3ul>(std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>&, std::__1::__tuple_indices<2ul, 3ul>) /usr/lib/llvm-20/bin/../include/c++/v1/__thread/thread.h:199:3 (bitcoind+0x1c6a5c)
     9[18:53:46.472]     [#7](/bitcoin-bitcoin/7/) void* std::__1::__thread_proxy[abi:ne200100]<std::__1::tuple<std::__1::unique_ptr<std::__1::__thread_struct, std::__1::default_delete<std::__1::__thread_struct>>, void (*)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const*, AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7>>(void*) /usr/lib/llvm-20/bin/../include/c++/v1/__thread/thread.h:208:3 (bitcoind+0x1c6a5c)
    10[18:53:46.472] 
    11[18:53:46.472]   Previous write of size 8 at 0x723c00000068 by main thread:
    12[18:53:46.472]     [#0](/bitcoin-bitcoin/0/) std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>::reset[abi:ne200100](BCLog::LogRateLimiter*) /usr/lib/llvm-20/bin/../include/c++/v1/__memory/unique_ptr.h:298:19 (bitcoind+0x1b779a) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    13[18:53:46.472]     [#1](/bitcoin-bitcoin/1/) std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>::operator=[abi:ne200100](std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>&&) /usr/lib/llvm-20/bin/../include/c++/v1/__memory/unique_ptr.h:240:5 (bitcoind+0x1b779a)
    14[18:53:46.472]     [#2](/bitcoin-bitcoin/2/) BCLog::Logger::SetRateLimiting(std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>&&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./logging.h:275:23 (bitcoind+0x1b779a)
    15[18:53:46.472]     [#3](/bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:1381:19 (bitcoind+0x1b779a)
    16[18:53:46.472]     [#4](/bitcoin-bitcoin/4/) AppInit(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:237:43 (bitcoind+0x199892) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    17[18:53:46.472]     [#5](/bitcoin-bitcoin/5/) main /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:283:10 (bitcoind+0x199892)
    18[18:53:46.472] 
    19[18:53:46.472]   Location is heap block of size 232 at 0x723c00000000 allocated by main thread:
    20[18:53:46.472]     [#0](/bitcoin-bitcoin/0/) operator new(unsigned long) <null> (bitcoind+0x1977f6) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    21[18:53:46.472]     [#1](/bitcoin-bitcoin/1/) LogInstance() /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/util/./logging.cpp:43:36 (bitcoind+0xc8756b) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    22[18:53:46.472]     [#2](/bitcoin-bitcoin/2/) init::AddLoggingArgs(ArgsManager&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init/common.cpp:31:209 (bitcoind+0xb0a2c7) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    23[18:53:46.472]     [#3](/bitcoin-bitcoin/3/) SetupServerArgs(ArgsManager&, bool) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:446:5 (bitcoind+0x1a0e07) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    24[18:53:46.472]     [#4](/bitcoin-bitcoin/4/) ParseArgs(node::NodeContext&, int, char**) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:116:5 (bitcoind+0x198e34) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    25[18:53:46.472]     [#5](/bitcoin-bitcoin/5/) main /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:278:10 (bitcoind+0x198e34)
    26[18:53:46.472] 
    27[18:53:46.472]   Mutex M0 (0x723c00000000) created at:
    28[18:53:46.472]     [#0](/bitcoin-bitcoin/0/) pthread_mutex_lock <null> (bitcoind+0x11559b) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    29[18:53:46.472]     [#1](/bitcoin-bitcoin/1/) std::__1::mutex::lock() <null> (libc++.so.1.0.20+0x713cc) (BuildId: 30ef7da36db6fb0c014ee96603f7649f755cb793)
    30[18:53:46.472]     [#2](/bitcoin-bitcoin/2/) init::LogPackageVersion() /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init/common.cpp:151:5 (bitcoind+0xb0dc4e) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    31[18:53:46.472]     [#3](/bitcoin-bitcoin/3/) InitLogging(ArgsManager const&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:818:5 (bitcoind+0x1b0390) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    32[18:53:46.475]     [#4](/bitcoin-bitcoin/4/) AppInit(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:181:9 (bitcoind+0x199572) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    33[18:53:46.475]     [#5](/bitcoin-bitcoin/5/) main /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:283:10 (bitcoind+0x199572)
    34[18:53:46.475] 
    35[18:53:46.475]   Thread T1 'b-scheduler' (tid=9717, running) created by main thread at:
    36[18:53:46.475]     [#0](/bitcoin-bitcoin/0/) pthread_create <null> (bitcoind+0x11388e) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    37[18:53:46.475]     [#1](/bitcoin-bitcoin/1/) std::__1::__libcpp_thread_create[abi:ne200100](unsigned long*, void* (*)(void*), void*) /usr/lib/llvm-20/bin/../include/c++/v1/__thread/support/pthread.h:182:10 (bitcoind+0x1b75c1) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    38[18:53:46.475]     [#2](/bitcoin-bitcoin/2/) std::__1::thread::thread<void (&)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const (&) [10], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7, 0>(void (&)(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::function<void ()>), char const (&) [10], AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*)::$_7&&) /usr/lib/llvm-20/bin/../include/c++/v1/__thread/thread.h:218:14 (bitcoind+0x1b75c1)
    39[18:53:46.475]     [#3](/bitcoin-bitcoin/3/) AppInitMain(node::NodeContext&, interfaces::BlockAndHeaderTipInfo*) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./init.cpp:1363:34 (bitcoind+0x1b75c1)
    40[18:53:46.475]     [#4](/bitcoin-bitcoin/4/) AppInit(node::NodeContext&) /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:237:43 (bitcoind+0x199892) (BuildId: 2b13e1fe2bba2fe47e258c908d3d80d2947d2486)
    41[18:53:46.475]     [#5](/bitcoin-bitcoin/5/) main /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/./bitcoind.cpp:283:10 (bitcoind+0x199892)
    42[18:53:46.475] 
    43[18:53:46.475] SUMMARY: ThreadSanitizer: data race /ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/util/./logging.cpp:455:29 in BCLog::Logger::LogPrintStr_(std::__1::basic_string_view<char, std::__1::char_traits<char>>, std::__1::source_location&&, BCLog::LogFlags, BCLog::Level, bool)
    44[18:53:46.475] ==================
    

    In my suggestion, I removed the mutex guard for m_limiter because the class is now thread-safe, but of course that doesn’t prevent race conditions on the setting of m_limiter, e.g. when one thread calls SetRateLimiting() and another inspects if m_limiter is set, which is what TSan caught here.

    Can be fixed by re-introducing the m_cs guard:

     0diff --git a/src/logging.h b/src/logging.h
     1index f81f4947af..c801e94e28 100644
     2--- a/src/logging.h
     3+++ b/src/logging.h
     4@@ -201,7 +201,7 @@ namespace BCLog {
     5         size_t m_buffer_lines_discarded GUARDED_BY(m_cs){0};
     6 
     7         //! Manages the rate limiting of each log location.
     8-        std::unique_ptr<LogRateLimiter> m_limiter;
     9+        std::unique_ptr<LogRateLimiter> m_limiter GUARDED_BY(m_cs);
    10 
    11         //! Category-specific log level. Overrides `m_log_level`.
    12         std::unordered_map<LogFlags, Level> m_category_log_levels GUARDED_BY(m_cs);
    13@@ -270,8 +270,9 @@ namespace BCLog {
    14         /** Only for testing */
    15         void DisconnectTestLogger() EXCLUSIVE_LOCKS_REQUIRED(!m_cs);
    16 
    17-        void SetRateLimiting(std::unique_ptr<LogRateLimiter>&& limiter)
    18+        void SetRateLimiting(std::unique_ptr<LogRateLimiter>&& limiter) EXCLUSIVE_LOCKS_REQUIRED(!m_cs)
    19         {
    20+            StdLockGuard scoped_lock(m_cs);
    21             m_limiter = std::move(limiter);
    22         }
    23 
    

    Sorry about the oversight.


    Crypt-iQ commented at 11:45 am on July 3, 2025:
    Will implement. I should have caught this as well – I am glad we have TSAN in CI. I will test the branch locally under TSAN.
  120. in src/logging.cpp:460 in 34fc54d547 outdated
    454+    FormatLogStrInPlace(str_prefixed, category, level, source_loc, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime());
    455+    bool ratelimit{false};
    456+    if (should_ratelimit && m_limiter) {
    457+        auto status{m_limiter->Consume(source_loc, str_prefixed)};
    458+        if (status == BCLog::LogRateLimiter::Status::NEWLY_SUPPRESSED) {
    459+            LogPrintStr_(strprintf(
    


    stickies-v commented at 11:27 am on July 3, 2025:

    I added this recursive LogPrintStr_ because I think it’s better to have separate log statements than inserting into the existing string, and added the should_ratelimit=false which makes it safe for infinite recursion. I forgot to add the clang-tidy suppression though, causing this CI failure. It seems like we can only suppress this by marking all of LogPrintStr_ as NOLINT, instead of just this single source, which is not ideal.

     0diff --git a/src/logging.cpp b/src/logging.cpp
     1index 5c7d7fc57d..1fdbceff5f 100644
     2--- a/src/logging.cpp
     3+++ b/src/logging.cpp
     4@@ -418,6 +418,7 @@ void BCLog::Logger::LogPrintStr(std::string_view str, std::source_location&& sou
     5     return LogPrintStr_(str, std::move(source_loc), category, level, should_ratelimit);
     6 }
     7 
     8+// NOLINTNEXTLINE(misc-no-recursion)
     9 void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
    10 {
    11     std::string str_prefixed = LogEscapeMessage(str);
    12@@ -455,6 +456,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& so
    13     if (should_ratelimit && m_limiter) {
    14         auto status{m_limiter->Consume(source_loc, str_prefixed)};
    15         if (status == BCLog::LogRateLimiter::Status::NEWLY_SUPPRESSED) {
    16+            // NOLINTNEXTLINE(misc-no-recursion)
    17             LogPrintStr_(strprintf(
    18                              "Excessive logging detected from %s:%d (%s): >%d bytes logged during "
    19                              "the last time window of %is. Suppressing logging to disk from this "
    20@@ -463,7 +465,7 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& so
    21                              source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    22                              m_limiter->m_max_bytes,
    23                              Ticks<std::chrono::seconds>(m_limiter->m_reset_window)),
    24-                         std::source_location::current(), LogFlags::ALL, Level::Warning, /*should_ratelimit=*/false);
    25+                         std::source_location::current(), LogFlags::ALL, Level::Warning, /*should_ratelimit=*/false);  // with should_ratelimit=false, this cannot lead to infinite recursion
    26         }
    27         ratelimit = status == BCLog::LogRateLimiter::Status::STILL_SUPPRESSED;
    28         // To avoid confusion caused by dropped log messages when debugging an issue,
    

    Crypt-iQ commented at 1:00 pm on July 3, 2025:
    Unfortunate that it can only be resolved by marking the function as NOLINT. Addressed.

    l0rinc commented at 12:39 pm on July 6, 2025:
    As mentioned before, I’m not comfortable with this, can we find a safer way?

    Crypt-iQ commented at 3:59 pm on July 7, 2025:
    I think this is safe. I understand the concern, but the NOLINT and the comment next to the recursion should be a bright red flag to anybody modifying the code in the future.
  121. Crypt-iQ force-pushed on Jul 3, 2025
  122. in src/logging.h:110 in 6a7147358c outdated
    103@@ -82,15 +104,88 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    106     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    107+    constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
    108+
    109+    //! Keeps track of an individual source location and how many available bytes are left for logging from it.
    110+    class LogLimitStats
    


    stickies-v commented at 2:19 pm on July 3, 2025:

    nit: making this a simple struct LogRateLimiter::Stats would clean things up a bit more.

    Diff that does that + updates naming to reflect the phasing out of LogLocationCounter naming and updates documentation to not mention concepts it’s not aware of (e.g. Stats does not know anything about time windows).

      0diff --git a/src/logging.cpp b/src/logging.cpp
      1index a090803652..7dc81928f2 100644
      2--- a/src/logging.cpp
      3+++ b/src/logging.cpp
      4@@ -384,10 +384,10 @@ BCLog::LogRateLimiter::Status BCLog::LogRateLimiter::Consume(
      5     const std::string& str)
      6 {
      7     StdLockGuard scoped_lock(m_mutex);
      8-    auto& counter{m_source_locations.try_emplace(source_loc, m_max_bytes).first->second};
      9-    Status status{counter.GetDroppedBytes() > 0 ? Status::STILL_SUPPRESSED : Status::UNSUPPRESSED};
     10+    auto& stats{m_source_locations.try_emplace(source_loc, m_max_bytes).first->second};
     11+    Status status{stats.dropped_bytes > 0 ? Status::STILL_SUPPRESSED : Status::UNSUPPRESSED};
     12 
     13-    if (!counter.Consume(str.size()) && status == Status::UNSUPPRESSED) {
     14+    if (!stats.Consume(str.size()) && status == Status::UNSUPPRESSED) {
     15         status = Status::NEWLY_SUPPRESSED;
     16         m_suppression_active = true;
     17     }
     18@@ -549,8 +549,8 @@ void BCLog::LogRateLimiter::Reset()
     19         source_locations.swap(m_source_locations);
     20         m_suppression_active = false;
     21     }
     22-    for (const auto& [source_loc, counter] : source_locations) {
     23-        uint64_t dropped_bytes{counter.GetDroppedBytes()};
     24+    for (const auto& [source_loc, stats] : source_locations) {
     25+        uint64_t dropped_bytes{stats.dropped_bytes};
     26         if (dropped_bytes == 0) continue;
     27         LogPrintLevel_(
     28             LogFlags::ALL, Level::Info, /*should_ratelimit=*/false,
     29@@ -560,15 +560,15 @@ void BCLog::LogRateLimiter::Reset()
     30     }
     31 }
     32 
     33-bool BCLog::LogLimitStats::Consume(uint64_t bytes)
     34+bool BCLog::LogRateLimiter::Stats::Consume(uint64_t bytes)
     35 {
     36-    if (bytes > m_available_bytes) {
     37-        m_dropped_bytes += bytes;
     38-        m_available_bytes = 0;
     39+    if (bytes > available_bytes) {
     40+        dropped_bytes += bytes;
     41+        available_bytes = 0;
     42         return false;
     43     }
     44 
     45-    m_available_bytes -= bytes;
     46+    available_bytes -= bytes;
     47     return true;
     48 }
     49 
     50diff --git a/src/logging.h b/src/logging.h
     51index c801e94e28..3253bdf910 100644
     52--- a/src/logging.h
     53+++ b/src/logging.h
     54@@ -106,43 +106,18 @@ namespace BCLog {
     55     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
     56     constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
     57 
     58-    //! Keeps track of an individual source location and how many available bytes are left for logging from it.
     59-    class LogLimitStats
     60-    {
     61-    private:
     62-        //! Remaining bytes in the current window interval.
     63-        uint64_t m_available_bytes;
     64-        //! Number of bytes that were not consumed within the current window.
     65-        uint64_t m_dropped_bytes{0};
     66-
     67-    public:
     68-        LogLimitStats(uint64_t max_bytes) : m_available_bytes{max_bytes} {}
     69-        //! Consume bytes from the window if enough bytes are available.
     70-        //!
     71-        //! Returns whether enough bytes were available.
     72-        bool Consume(uint64_t bytes);
     73-
     74-        uint64_t GetAvailableBytes() const
     75-        {
     76-            return m_available_bytes;
     77-        }
     78-
     79-        uint64_t GetDroppedBytes() const
     80-        {
     81-            return m_dropped_bytes;
     82-        }
     83-    };
     84-
     85     /**
     86      * Fixed window rate limiter for logging.
     87      */
     88     class LogRateLimiter
     89     {
     90+    public:
     91+        struct Stats;
     92     private:
     93         mutable StdMutex m_mutex;
     94 
     95-        //! Counters for each source location that has attempted to log something.
     96-        std::unordered_map<std::source_location, LogLimitStats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
     97+        //! Stats for each source location that has attempted to log something.
     98+        std::unordered_map<std::source_location, Stats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
     99         //! True if at least one log location is suppressed. Cached view on m_source_locations for performance reasons.
    100         std::atomic<bool> m_suppression_active{false};
    101 
    102@@ -154,7 +129,7 @@ namespace BCLog {
    103          *                          reset_window interval.
    104          * [@param](/bitcoin-bitcoin/contributor/param/) max_bytes         Maximum number of bytes that can be logged for each source
    105          *                          location.
    106-         * [@param](/bitcoin-bitcoin/contributor/param/) reset_window      Time window after which the byte counters are reset.
    107+         * [@param](/bitcoin-bitcoin/contributor/param/) reset_window      Time window after which the stats are reset.
    108          */
    109         LogRateLimiter(SchedulerFunction scheduler_func, uint64_t max_bytes, std::chrono::seconds reset_window);
    110         //! Maximum number of bytes logged per location per window.
    111@@ -178,6 +153,19 @@ namespace BCLog {
    112         bool SuppressionsActive() const { return m_suppression_active; }
    113     };
    114 
    115+    //! Keeps track of an individual source location and how many available bytes are left for logging from it.
    116+    struct LogRateLimiter::Stats
    117+    {
    118+        //! Remaining bytes
    119+        uint64_t available_bytes;
    120+        //! Number of bytes that were consumed but didn't fit in the available bytes
    121+        uint64_t dropped_bytes{0};
    122+
    123+            Stats(uint64_t max_bytes) : available_bytes{max_bytes} {}
    124+            //! Updates internal accounting and returns true if enough available_bytes were remaining
    125+            bool Consume(uint64_t bytes);
    126+        };
    127+
    128     class Logger
    129     {
    130     public:
    131diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
    132index 00fb44eaa3..0b21aed588 100644
    133--- a/src/test/logging_tests.cpp
    134+++ b/src/test/logging_tests.cpp
    135@@ -348,25 +348,25 @@ BOOST_AUTO_TEST_CASE(logging_log_rate_limiter)
    136 
    137 BOOST_AUTO_TEST_CASE(logging_log_limit_stats)
    138 {
    139-    BCLog::LogLimitStats counter{BCLog::RATELIMIT_MAX_BYTES};
    140+    BCLog::LogRateLimiter::Stats stats{BCLog::RATELIMIT_MAX_BYTES};
    141 
    142-    // Check that counter gets initialized correctly.
    143-    BOOST_CHECK_EQUAL(counter.GetAvailableBytes(), BCLog::RATELIMIT_MAX_BYTES);
    144-    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 0ull);
    145+    // Check that stats gets initialized correctly.
    146+    BOOST_CHECK_EQUAL(stats.available_bytes, BCLog::RATELIMIT_MAX_BYTES);
    147+    BOOST_CHECK_EQUAL(stats.dropped_bytes, 0ull);
    148 
    149     const uint64_t MESSAGE_SIZE{512 * 1024};
    150-    BOOST_CHECK(counter.Consume(MESSAGE_SIZE));
    151-    BOOST_CHECK_EQUAL(counter.GetAvailableBytes(), BCLog::RATELIMIT_MAX_BYTES - MESSAGE_SIZE);
    152-    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 0ull);
    153+    BOOST_CHECK(stats.Consume(MESSAGE_SIZE));
    154+    BOOST_CHECK_EQUAL(stats.available_bytes, BCLog::RATELIMIT_MAX_BYTES - MESSAGE_SIZE);
    155+    BOOST_CHECK_EQUAL(stats.dropped_bytes, 0ull);
    156 
    157-    BOOST_CHECK(counter.Consume(MESSAGE_SIZE));
    158-    BOOST_CHECK_EQUAL(counter.GetAvailableBytes(), BCLog::RATELIMIT_MAX_BYTES - MESSAGE_SIZE * 2);
    159-    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 0ull);
    160+    BOOST_CHECK(stats.Consume(MESSAGE_SIZE));
    161+    BOOST_CHECK_EQUAL(stats.available_bytes, BCLog::RATELIMIT_MAX_BYTES - MESSAGE_SIZE * 2);
    162+    BOOST_CHECK_EQUAL(stats.dropped_bytes, 0ull);
    163 
    164     // Consuming more bytes after already having consumed 1MB should fail.
    165-    BOOST_CHECK(!counter.Consume(500));
    166-    BOOST_CHECK_EQUAL(counter.GetAvailableBytes(), 0ull);
    167-    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 500ull);
    168+    BOOST_CHECK(!stats.Consume(500));
    169+    BOOST_CHECK_EQUAL(stats.available_bytes, 0ull);
    170+    BOOST_CHECK_EQUAL(stats.dropped_bytes, 500ull);
    171 }
    172 
    173 void LogFromLocation(int location, std::string message)
    

    Crypt-iQ commented at 3:10 pm on July 4, 2025:
    Stats is a better name, LogRateLimiter::LogLimitStats is a bit verbose / stutter-y. Will address in a follow-up, I am not so good with naming.

    l0rinc commented at 12:49 pm on July 6, 2025:
    I don’t think we’re at the “follow-up” stage yet, let’s fix whatever we can in this PR, we’re not in any hurry as far as I can tell. The nits left by @stickies-v are reasonable, he’s willing to re-review, same for me, let’s spend a bit more time wit this, it’s not ready just yet…
  123. DrahtBot removed the label CI failed on Jul 3, 2025
  124. dergoegge approved
  125. dergoegge commented at 9:13 am on July 4, 2025: member
    Code review ACK 6a7147358c9d6e3883dcdbbee9fb2c1cb4baf5ff
  126. DrahtBot requested review from stickies-v on Jul 4, 2025
  127. in src/logging.h:41 in 6a7147358c outdated
    34@@ -31,6 +35,24 @@ extern const char * const DEFAULT_DEBUGLOGFILE;
    35 
    36 extern bool fLogIPs;
    37 
    38+struct SourceLocationEqual {
    39+    bool operator()(const std::source_location& lhs, const std::source_location& rhs) const noexcept
    40+    {
    41+        return lhs.line() == rhs.line() && std::string_view(lhs.file_name()) == std::string_view(rhs.file_name());
    


    stickies-v commented at 12:18 pm on July 4, 2025:

    review note: if a single line contains two log statements, they will be sharing the same stats. I can’t find any current such instances in the code, and the only realistic scenario in which I could see something like that happen is:

    0warn ? LogWarning(msg) : LogInfo(msg)
    

    which would logically be equivalent to the same log location (e.g. using LogPrintLevel) anyway.

    This behaviour can be verified with:

     0diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
     1index 00fb44eaa3..0632dcd438 100644
     2--- a/src/test/logging_tests.cpp
     3+++ b/src/test/logging_tests.cpp
     4@@ -369,7 +369,7 @@ BOOST_AUTO_TEST_CASE(logging_log_limit_stats)
     5     BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 500ull);
     6 }
     7 
     8-void LogFromLocation(int location, std::string message)
     9+void LogFromLocation(int location, std::string message, bool warn = true)
    10 {
    11     switch (location) {
    12     case 0:
    13@@ -384,13 +384,16 @@ void LogFromLocation(int location, std::string message)
    14     case 3:
    15         LogPrintLevel(BCLog::LogFlags::ALL, BCLog::Level::Info, "%s\n", message);
    16         break;
    17+    case 4:
    18+        warn ? LogInfo("%s\n", message) : LogInfo("%s\n", message);
    19+        break;
    20     }
    21 }
    22 
    23-void LogFromLocationAndExpect(int location, std::string message, std::string expect)
    24+void LogFromLocationAndExpect(int location, std::string message, std::string expect, bool warn = true)
    25 {
    26     ASSERT_DEBUG_LOG(expect);
    27-    LogFromLocation(location, message);
    28+    LogFromLocation(location, message, warn);
    29 }
    30 
    31 BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
    32@@ -465,6 +468,13 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
    33 
    34     BOOST_CHECK_MESSAGE(log_file_size < GetFileSize(log_path), "location 3 should be exempt from rate limiting");
    35 
    36+    // log from warn "sublocation" until limit is reached
    37+    for (int i = 0; i < 1024; ++i) {
    38+        BOOST_CHECK_THROW(LogFromLocationAndExpect(4, log_message, "Excessive logging detected", /*warn=*/true), std::runtime_error);
    39+    }
    40+    // we haven't yet logged from the non-warn "sublocation", yet it is ratelimited right away
    41+    LogFromLocationAndExpect(4, log_message, "Excessive logging detected", /*warn=*/false);
    42+
    43     LogInstance().m_log_timestamps = prev_log_timestamps;
    44     LogInstance().m_log_sourcelocations = prev_log_sourcelocations;
    45     LogInstance().m_log_threadnames = prev_log_threadnames;
    

    Crypt-iQ commented at 3:09 pm on July 4, 2025:
    I had thought about that as well. I looked for patterns like this, but I did not look very hard. Do you think this deserves a comment in a follow-up to warn against behavior like this?

    l0rinc commented at 12:46 pm on July 6, 2025:

    I was also worried about that, especially when the logging is called via some dedicated method which hijacks the location, even though the call sites should probably not be bundled together. Or if macros are messing up the lines in any way.


    On the other hand, is it really necessary to do the accounting per line, aren’t we over-complicating a hypothetical scenario?

    The original PR stated:

    The main point against global rate limiting is that it opens another attack vector where an attacker could trigger rate limiting and then execute a 2. attack which would then not be document in the logs at all

    Maybe, but this could apply currently as well, you just have to make sure you exhaust the target logger before the second attack. And as far as I can tell currently any hack which would enable all logs to be written would allow log-locations * 1MiB additional lines to be added. That kinda’ seems worse to me, maybe we should investigate a simpler solution first.

    But if we stay with the per-line logging-quota, I think this should at least be explained in the PR description.


    stickies-v commented at 12:35 pm on July 7, 2025:

    when the logging is called via some dedicated method which hijacks the location

    I don’t think we can reasonably protect against malicious modifications to the binary?

    Or if macros are messing up the lines in any way.

    That seems both unlikely and just something that should be, and is, tested.

    you just have to make sure you exhaust the target logger

    “just” seems inappropriate, unconditional logging should always take care that it can never be used to exhaust resources, but with this PR we add another layer of defence in case of an oversight.


    Crypt-iQ commented at 4:09 pm on July 7, 2025:

    On the other hand, is it really necessary to do the accounting per line, aren’t we over-complicating a hypothetical scenario?

    I don’t think this is hypothetical – security vulnerabilities these days are increasingly multi-layered and complex and it is not unheard of for attackers to chain together exploits. I have personally had to use logs to piece together what happened after-the-fact when somebody’s machine was compromised. The concern brought up in the earlier PRs was about an attacker finding two vulnerabilities if there is a global limit:

    • a logging vulnerability that triggers the global rate limit
    • a separate vulnerability which goes undetected because all logs to disk are now suppressed

    Maybe, but this could apply currently as well, you just have to make sure you exhaust the target logger before the second attack.

    I don’t think this applies currently – because the rate limiting is done per source-code location, an attacker will leave a trail of evidence.

    And as far as I can tell currently any hack which would enable all logs to be written would allow log-locations * 1MiB additional lines to be added.

    I can’t find a bug in the current version of the code that allows this to happen. If there is some way to bypass the rate limiting logic, that would be severe. However, the same issue would also exist in the global rate limiting case.

    But if we stay with the per-line logging-quota, I think this should at least be explained in the PR description.

    This is explained in the PR description under the “Approach” section – it mentions source code locations.


    l0rinc commented at 6:48 pm on July 7, 2025:

    The PR description still states:

    Both the quota and time window are configurable

    As far as I can tell that’s not the case anymore.

    When logging is restarted a tally of how many messages/bytes were dropped is printed

    Do we also print the message count?


    But if we stay with the per-line logging-quota, I think this should at least be explained in the PR description

    I meant the reason for choosing to restrict based on source lines and not globally. I’m still not fully convinced that it’s more dangerous to restrict all logging, but I can accept if others think it is - I’m probably just not up-to-date with these attacks.


    Crypt-iQ commented at 7:22 pm on July 7, 2025:

    As far as I can tell that’s not the case anymore.

    Ah, this was confusing. I meant to say that LogRateLimiter is parameterized, but it reads like a node operator is able to configure the quote and time window without modifying the source code. I’ve removed this line since it’s confusing.

    Do we also print the message count?

    Removed mention of “messages”.

  128. in src/test/logging_tests.cpp:465 in 6a7147358c outdated
    461+    log_file_size = GetFileSize(log_path);
    462+    for (int i = 0; i < 1024; ++i) {
    463+        BOOST_CHECK_THROW(LogFromLocationAndExpect(3, log_message, "Excessive logging detected"), std::runtime_error);
    464+    }
    465+
    466+    BOOST_CHECK_MESSAGE(log_file_size < GetFileSize(log_path), "location 3 should be exempt from rate limiting");
    


    stickies-v commented at 12:59 pm on July 4, 2025:

    I think there are a number of issues with this part of the test:

    • the docstring states locations 2 and 3 are exempt, but I don’t see why they would be
    • it tests that the location can log up to the ratelimit amount, which is true in any case
    • it tests that the filesize increases, which would also happen if ratelimiting were to have happened, so it doesn’t really catch anything

    I think the logging_filesize_rate_limit test can be improved and cleaned up in a follow-up (to e.g. address the very confusing BOOST_CHECK_THROW(LogFromLocationAndExpect()) pattern), so perhaps this can all be done together, but with a minimal diff I would suggest to change this to:

     0diff --git a/src/test/logging_tests.cpp b/src/test/logging_tests.cpp
     1index 00fb44eaa3..847279ad4d 100644
     2--- a/src/test/logging_tests.cpp
     3+++ b/src/test/logging_tests.cpp
     4@@ -379,10 +379,10 @@ void LogFromLocation(int location, std::string message)
     5         LogInfo("%s\n", message);
     6         break;
     7     case 2:
     8-        LogPrintLevel(BCLog::LogFlags::NONE, BCLog::Level::Info, "%s\n", message);
     9+        LogDebug(BCLog::LogFlags::HTTP, "%s\n", message);
    10         break;
    11     case 3:
    12-        LogPrintLevel(BCLog::LogFlags::ALL, BCLog::Level::Info, "%s\n", message);
    13+        LogPrintLevel_(BCLog::LogFlags::ALL, BCLog::Level::Info, /*should_ratelimit=*/false, "%s\n", message);
    14         break;
    15     }
    16 }
    17@@ -449,20 +449,21 @@ BOOST_FIXTURE_TEST_CASE(logging_filesize_rate_limit, LogSetup)
    18 
    19     BOOST_CHECK_THROW(LogFromLocationAndExpect(1, log_message, "Restarting logging"), std::runtime_error);
    20 
    21-    // Attempt to log 1MiB from location 2 and 1MiB from location 3. These exempt locations should be allowed to log
    22-    // without limit.
    23-    log_file_size = GetFileSize(log_path);
    24-    for (int i = 0; i < 1024; ++i) {
    25-        BOOST_CHECK_THROW(LogFromLocationAndExpect(2, log_message, "Excessive logging detected"), std::runtime_error);
    26+    // Fill up locations 2 and 3 to the ratelimit amount
    27+    for (size_t loc : {2, 3}) {
    28+        for (int i = 0; i < 1024; ++i) {
    29+            LogFromLocation(loc, log_message);
    30+        }
    31     }
    32 
    33+    // Ensure location 2 (debug logging) can go beyond the ratelimiting, it is exempt.
    34+    log_file_size = GetFileSize(log_path);
    35+    BOOST_CHECK_THROW(LogFromLocationAndExpect(2, log_message, "Excessive logging detected"), std::runtime_error);
    36     BOOST_CHECK_MESSAGE(log_file_size < GetFileSize(log_path), "location 2 should be exempt from rate limiting");
    37 
    38+    // Ensure location 3 (with explicit should_ratelimit=false) can go beyond the ratelimiting, it is exempt.
    39     log_file_size = GetFileSize(log_path);
    40-    for (int i = 0; i < 1024; ++i) {
    41-        BOOST_CHECK_THROW(LogFromLocationAndExpect(3, log_message, "Excessive logging detected"), std::runtime_error);
    42-    }
    43-
    44+    BOOST_CHECK_THROW(LogFromLocationAndExpect(3, log_message, "Excessive logging detected"), std::runtime_error);
    45     BOOST_CHECK_MESSAGE(log_file_size < GetFileSize(log_path), "location 3 should be exempt from rate limiting");
    46 
    47     LogInstance().m_log_timestamps = prev_log_timestamps;
    

    Crypt-iQ commented at 3:10 pm on July 4, 2025:
    Will address in a follow-up, I think this is from an older version of the PR where the functions used in location 2/3 were not caught by the rate limit.

    l0rinc commented at 8:18 pm on July 6, 2025:
    Let’s do these here, I don’t think we’re in a rush.

    stickies-v commented at 12:36 pm on July 7, 2025:
    We’re trying to get this in for 30.0. We shouldn’t rush it in if we have concerns about the stability or effectiveness of this code, but blocking it for stylistic or test improvement purposes seems counter-productive when they can be done in a follow-up, ensuring progress on the important bits.

    l0rinc commented at 6:49 pm on July 7, 2025:
    Ok, if you’re fine with that, I’m fine with that, let’s do the remaining ones in follow-ups.

    maflcko commented at 8:21 am on July 10, 2025:

    I think there are a number of issues with this part of the test:

    I also noticed those in my review, but also noticed you mentioned it already.

  129. stickies-v approved
  130. stickies-v commented at 1:12 pm on July 4, 2025: contributor

    ACK 6a7147358c9d6e3883dcdbbee9fb2c1cb4baf5ff

    Keeping nits to a minimum because I’ve already been quite loud on this PR and I want to make sure we get the improvements merged soon. Left a few nits/test comments that can be done in a follow-up (although I’d be happy to quickly re-ACK here).

  131. Crypt-iQ commented at 3:06 pm on July 4, 2025: contributor

    Left a few nits/test comments that can be done in a follow-up

    I will make a follow-up PR post-merge to address the comments (which are all reasonable, just want to limit re-review)

  132. in src/logging.h:138 in 6a7147358c outdated
    133+        }
    134+    };
    135+
    136+    /**
    137+     * Fixed window rate limiter for logging.
    138+     */
    


    l0rinc commented at 10:27 am on July 6, 2025:

    nit: so many comment styles used here, most in new code. Now that this is a one-liner, we can unify it with the rest of the comments here:

    0    //! Fixed window rate limiter for logging.
    

    If you apply this, please see other similar ones as well.

    The problem with these inconsistencies is not just cosmetic, these differences are distracting us from being able to treat inconsistencies as code smells.


    l0rinc commented at 10:52 am on July 6, 2025:

    we seem to have a lot of (comment) styles here, now that it’s this short, let’s simplify and unify this:

    0    //! Fixed window rate limiter for logging.
    
  133. in src/logging.h:146 in 6a7147358c outdated
    141+    private:
    142+        mutable StdMutex m_mutex;
    143+
    144+        //! Counters for each source location that has attempted to log something.
    145+        std::unordered_map<std::source_location, LogLimitStats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
    146+        //! True if at least one log location is suppressed. Cached view on m_source_locations for performance reasons.
    


    l0rinc commented at 10:29 am on July 6, 2025:

    nit:

    0        //! Whether any log locations are suppressed. Cached view on m_source_locations for performance reasons.
    

    l0rinc commented at 1:17 pm on July 6, 2025:

    Cached view on m_source_locations for performance reasons

    Can you explain these performance reasons? Is it to avoid iterating the values of the map to find a 0? But shouldn’t we only prefix the dropped lines by * anyway (why prefix the lines that aren’t dropped in the first place?), so why do we care about a global suppression state, i.e.

    0// To avoid confusion caused by dropped log messages when debugging an issue,
    1// we prefix log lines with "[*]" when there are any suppressed source locations.
    2if (m_limiter->SuppressionsActive()) {
    3    str_prefixed.insert(0, "[*] ");
    4}
    

    I think this is more confusing than only prefixing the dropped lines, i.e. something like:

    0if (m_print_to_console && ratelimit) {
    1    str_prefixed.insert(0, "[*] ");
    2}
    

    Which would eliminate the need for m_suppression_active as far as I can tell.


    stickies-v commented at 12:07 pm on July 7, 2025:

    Can you explain these performance reasons?

    Not having to acquire a lock and iterate over an entire map.

    But shouldn’t we only prefix the dropped lines by * anyway

    No, the point is to continue making it visible to users that log lines are being suppressed, because they might have missed the initial announcement.


    Crypt-iQ commented at 4:12 pm on July 7, 2025:

    But shouldn’t we only prefix the dropped lines by * anyway (why prefix the lines that aren’t dropped in the first place?), so why do we care about a global suppression state, i.e.

    To build on @stickies-v comment, terminals may have a history limit and so if the prefix was only for dropped lines, an attack (which should now be thwarted!) could go undetected.


    l0rinc commented at 6:50 pm on July 7, 2025:
    Fair points, maybe we could hint at that in the first message which announces that some logs are being suppressed (explaining what the new * prefix indicates).
  134. in src/test/logging_tests.cpp:447 in 6a7147358c outdated
    442+    {
    443+        ASSERT_DEBUG_LOG("Restarting logging");
    444+        MockForwardAndSync(scheduler, 1min);
    445+    }
    446+
    447+    // BOOST_CHECK_NO_THROW(LogFromLocationAndExpect(0, log_message, "Restarting logging"));
    


    l0rinc commented at 10:37 am on July 6, 2025:
    what’s the reason for commented out code? Is this PR still in draft mode or is it ready for review?

    stickies-v commented at 12:18 pm on July 7, 2025:
    This line should have been removed indeed, as it was replaced by ASSERT_DEBUG_LOG("Restarting logging");.

    Crypt-iQ commented at 4:13 pm on July 7, 2025:
    My mistake, addressed. This might have been a rebase artifact, I was pretty sure that I removed it.
  135. in src/test/logging_tests.cpp:436 in 6a7147358c outdated
    431+    log_file_size = GetFileSize(log_path);
    432+    for (int i = 0; i < 1024; ++i) {
    433+        LogFromLocation(0, log_message);
    434+    }
    435+
    436+    BOOST_CHECK_MESSAGE(log_file_size == GetFileSize(log_path), "all further logs from location 0 should be dropped");
    


    l0rinc commented at 10:38 am on July 6, 2025:
    this way we’re not seeing the unequal values, just the message. Why not BOOST_CHECK_EQUAL?
  136. in src/test/logging_tests.cpp:411 in 6a7147358c outdated
    406+    scheduler.m_service_thread = std::thread([&] { scheduler.serviceQueue(); });
    407+    auto sched_func = [&scheduler](auto func, auto window) { scheduler.scheduleEvery(std::move(func), window); };
    408+    auto limiter = std::make_unique<BCLog::LogRateLimiter>(sched_func, 1024 * 1024, 20s);
    409+    LogInstance().SetRateLimiting(std::move(limiter));
    410+
    411+    // Log 1024-character lines (1023 plus newline) to make the math simple.
    


    l0rinc commented at 10:40 am on July 6, 2025:
    Given that we had problems with Windows so far, it might not be immediately obvious if this newline is a single char on Windows as well - can we generalize the test so that it doesn’t matter?
  137. in src/test/logging_tests.cpp:372 in 6a7147358c outdated
    367+    BOOST_CHECK(!counter.Consume(500));
    368+    BOOST_CHECK_EQUAL(counter.GetAvailableBytes(), 0ull);
    369+    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 500ull);
    370+}
    371+
    372+void LogFromLocation(int location, std::string message)
    


    l0rinc commented at 10:42 am on July 6, 2025:

    Is there a reason to copy the large message on each invocation?

    0void LogFromLocation(int location, const std::string& message)
    
  138. in src/logging.h:169 in 6a7147358c outdated
    164+        //! Suppression status of a source log location.
    165+        enum class Status {
    166+            UNSUPPRESSED,     // string fits within the limit
    167+            NEWLY_SUPPRESSED, // suppression has started since this string
    168+            STILL_SUPPRESSED, // suppression is still ongoing
    169+        };
    


    l0rinc commented at 10:48 am on July 6, 2025:

    I haven’t spent time trying to come up with an alternative yet, so I might be completely wrong, but I’m not sure I fully understand yet why we need these states. If the role of the extra NEWLY_SUPPRESSED state is just to signal that we need to log a warning message now, maybe we can simplify a bit since it seems to me some work might be duplicated here (i.e. that we should be able to get rid of this enum completely). Let me know if you need me to create a prototype to (dis)prove my hunch.


    Also, this change is meant to avoid an attack, we don’t have to be that precise with the calculations, just a rough accounting should suffice as far as I can tell - especially if it makes the code simpler.

    It’s what we’re doing in https://github.com/bitcoin/bitcoin/blob/e419b0e17f8acfe577c35c62a8a71a19aad249f3/src/txdb.cpp#L132 as well - we just call it an approximate size and keep adding values to it until the limit is exceeded. Would that not suffice here?


    stickies-v commented at 12:13 pm on July 7, 2025:

    I’m not sure I fully understand yet why we need these states

    It’s because the callsites behave differently whether a log line is un, newly or still suppressed. Previously, we encapsulated all that logic inside the Consume() function (then called NeedsRateLimiting(), but that gave the function too many responsibilities (as I believe you also pointed out), so this cleans that up. It think perhaps working out the code you’d prefer would be helpful.


    Crypt-iQ commented at 4:31 pm on July 7, 2025:

    Also, this change is meant to avoid an attack, we don’t have to be that precise with the calculations, just a rough accounting should suffice as far as I can tell - especially if it makes the code simpler.

    The accounting could be less precise, I had not considered that. I like that this PR is precise and I can account for the bytes.

  139. in src/logging.h:152 in 6a7147358c outdated
    147+        std::atomic<bool> m_suppression_active{false};
    148+
    149+    public:
    150+        using SchedulerFunction = std::function<void(std::function<void()>, std::chrono::milliseconds)>;
    151+        /**
    152+         * @param scheduler_func    Callable object used to schedule resetting the window. The first
    


    l0rinc commented at 10:51 am on July 6, 2025:

    I passionately dislike these huge comments, they’re usually distracting from the actual code (the ultimate source of truth) and over-explain something that - if indeed hard to understand - should be refactored instead.

    • why is the outer function called a Callable object, but the internal one a function to be executed
    • why are we even explaining SchedulerFunction here - the code already clearly states the same, the comment just duplicates exactly the same information in a place that should explain LogRateLimiter. IfSchedulerFunction is complicated, we could extract it to a dedicated type and explain it there if it’s hard to understand.

    stickies-v commented at 12:10 pm on July 7, 2025:
    This is a public class, explaining the parameters and interface seems very desirable to me. scheduler_func could mean many things, when I review code I like to be able to quickly see what something’s intent is. This seems like a pretty obvious best practice?

    Crypt-iQ commented at 4:35 pm on July 7, 2025:
    I like comments because it gives you an overview of the code to come. I can understand the aversion to comments, but I personally find them pretty helpful especially here. Sure, the code is the real source of truth, but comments have their use and I think usage here is reasonable.
  140. in src/logging.h:147 in 6a7147358c outdated
    142+        mutable StdMutex m_mutex;
    143+
    144+        //! Counters for each source location that has attempted to log something.
    145+        std::unordered_map<std::source_location, LogLimitStats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
    146+        //! True if at least one log location is suppressed. Cached view on m_source_locations for performance reasons.
    147+        std::atomic<bool> m_suppression_active{false};
    


    l0rinc commented at 11:58 am on July 6, 2025:
    Can we document somewhere the expected behavior for when the node is restarted?

    Crypt-iQ commented at 4:37 pm on July 7, 2025:
    Have documented this in the commit message of 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6
  141. in doc/release-notes-32604.md:7 in 6a7147358c outdated
    0@@ -0,0 +1,8 @@
    1+Logging
    2+-------
    3+Unconditional logging to disk via LogPrintf, LogInfo, LogWarning, LogError, and
    4+LogPrintLevel is now rate limited by giving each source location a logging quota of
    5+1MiB per hour. (#32604)
    6+
    7+When `-logsourcelocation` is enabled, the log output now contains the entire
    


    l0rinc commented at 12:05 pm on July 6, 2025:

    Should be plural:

    0When `-logsourcelocations` is enabled, the log output now contains the entire
    

    Crypt-iQ commented at 4:37 pm on July 7, 2025:
    Ah, my bad. Fixed.
  142. in doc/release-notes-32604.md:5 in 6a7147358c outdated
    0@@ -0,0 +1,8 @@
    1+Logging
    2+-------
    3+Unconditional logging to disk via LogPrintf, LogInfo, LogWarning, LogError, and
    4+LogPrintLevel is now rate limited by giving each source location a logging quota of
    5+1MiB per hour. (#32604)
    


    l0rinc commented at 12:10 pm on July 6, 2025:

    LogPrintLevel can be debug as well which won’t be rate limited - so this generalization seems inaccurate to me. And the important part of our message (i.e. that we’re hiding some logs now) is mentioned too late:

    0Unconditional logging to disk is now rate limited via `LogPrintf`, `LogInfo`, `LogWarning`, `LogError`,
    1and the corresponding `LogPrintLevel` calls by giving each source location a quota of
    21MiB per hour. (#32604)
    

    Crypt-iQ commented at 4:38 pm on July 7, 2025:
    This is better – I’ve implemented this to make it clear. I was attempting to walk a line between being verbose and being succinct.
  143. in src/init.cpp:1381 in 6a7147358c outdated
    1377@@ -1378,6 +1378,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
    1378         }
    1379     }, std::chrono::minutes{5});
    1380 
    1381+    LogInstance().SetRateLimiting(std::make_unique<BCLog::LogRateLimiter>(
    


    l0rinc commented at 12:11 pm on July 6, 2025:
    👍 for removing the config option, this is simpler
  144. in src/logging.cpp:463 in 6a7147358c outdated
    459+        if (status == BCLog::LogRateLimiter::Status::NEWLY_SUPPRESSED) {
    460+            // NOLINTNEXTLINE(misc-no-recursion)
    461+            LogPrintStr_(strprintf(
    462+                             "Excessive logging detected from %s:%d (%s): >%d bytes logged during "
    463+                             "the last time window of %is. Suppressing logging to disk from this "
    464+                             "source location until time window resets. Console logging "
    


    l0rinc commented at 12:15 pm on July 6, 2025:
    but console logging is affected, we’ll prefix them with * (which we’re not explaining anywhere as far as I can tell)

    Crypt-iQ commented at 3:53 pm on July 7, 2025:
    This is mentioned in the PR description and also in the commit message of b5d0cc29ac378f360424472f637f6d6af74660a5.
  145. in src/logging.cpp:468 in 6a7147358c outdated
    464+                             "source location until time window resets. Console logging "
    465+                             "unaffected. Last log entry.\n",
    466+                             source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    467+                             m_limiter->m_max_bytes,
    468+                             Ticks<std::chrono::seconds>(m_limiter->m_reset_window)),
    469+                         std::source_location::current(), LogFlags::ALL, Level::Warning, /*should_ratelimit=*/false); // with should_ratelimit=false, this cannot lead to infinite recursion
    


    l0rinc commented at 12:16 pm on July 6, 2025:
    I find this concerning, it’s very unintuitive that logging announcement would even pose such a threat

    stickies-v commented at 12:24 pm on July 7, 2025:
    Can you elaborate? Recursion is not inherently bad. It can lead to bugs, which is why we need to mindful about when we use it, but to me this looks like the best alternative. What do you suggest?

    l0rinc commented at 6:54 pm on July 7, 2025:

    Recursion is not inherently bad

    When the stopping condition isn’t rock solit, it kinda’ is. Maybe it is solid in this case - but given that we had to add a comment to reassure the readers, it may not be. I’d sleep better knowing that we haven’t just introduced a new attack vector…

    What do you suggest?

    In other cases we’ve switched recursive methods to iterative ones, e.g. #32351. I haven’t investigated what that would imply here, got already tired at the end of reviewing the rest of the code.

  146. in src/logging.cpp:473 in 6a7147358c outdated
    469+                         std::source_location::current(), LogFlags::ALL, Level::Warning, /*should_ratelimit=*/false); // with should_ratelimit=false, this cannot lead to infinite recursion
    470+        }
    471+        ratelimit = status == BCLog::LogRateLimiter::Status::STILL_SUPPRESSED;
    472+        // To avoid confusion caused by dropped log messages when debugging an issue,
    473+        // we prefix log lines with "[*]" when there are any suppressed source locations.
    474+        if (m_limiter->SuppressionsActive()) {
    


    l0rinc commented at 12:17 pm on July 6, 2025:
    should we guard this by m_print_to_console? And as explained before, I think it would make more sense to only prefix dropped lines.

    stickies-v commented at 12:25 pm on July 7, 2025:

    should we guard this by m_print_to_console?

    Why? It seems useful to also indicate on disk when logs are suppressed. debug.log is an important source of information for many.

    I think it would make more sense to only prefix dropped lines.

    I disagree, having a visual cue on debug.log seems very valuable.

  147. in src/logging.h:107 in 6a7147358c outdated
    103@@ -82,15 +104,88 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    106     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    107+    constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
    


    l0rinc commented at 12:25 pm on July 6, 2025:

    we may want to mention here whether this is the total (or per line) and that it’s only counting a subset of logged bytes (reading the comment somebody would rightfully wonder “but I’m sure we’re logging more during IBD”)

    0// maximum number of bytes that can be logged from a given source location within the specified time-window
    

    Crypt-iQ commented at 4:44 pm on July 7, 2025:
    I think this is clear because there is no global rate limit in the code and only a source location rate limit.
  148. in src/logging.cpp:422 in 6a7147358c outdated
    420+    return LogPrintStr_(str, std::move(source_loc), category, level, should_ratelimit);
    421 }
    422 
    423-void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_function, std::string_view source_file, int source_line, BCLog::LogFlags category, BCLog::Level level)
    424+// NOLINTNEXTLINE(misc-no-recursion)
    425+void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool should_ratelimit)
    


    l0rinc commented at 12:38 pm on July 6, 2025:

    given that we will have to quote the boolean’s name in every usage, consider a shorter parameter name than should_ratelimit:

    0void BCLog::Logger::LogPrintStr_(std::string_view str, std::source_location&& source_loc, BCLog::LogFlags category, BCLog::Level level, bool rate_limited)
    

    (note that ratelimit doesn’t seem to be an accepted single word, in other cases we’re delimiting the two, e.g. bool rate_limit{level >= BCLog::Level::Info}; a few lines below or https://github.com/bitcoin/bitcoin/blob/46f79dde67e46cd0f86db9406c43cf602394c049/src/qt/rpcconsole.cpp#L1260)

  149. in src/logging.cpp:479 in 6a7147358c outdated
    475+            str_prefixed.insert(0, "[*] ");
    476+        }
    477+    }
    478 
    479     if (m_print_to_console) {
    480         // print to console
    


    l0rinc commented at 12:52 pm on July 6, 2025:
    nit: redundant comment

    Crypt-iQ commented at 4:47 pm on July 7, 2025:
    This is pre-existing so this can be left as-is IMO.
  150. in src/logging.h:172 in 6a7147358c outdated
    167+            NEWLY_SUPPRESSED, // suppression has started since this string
    168+            STILL_SUPPRESSED, // suppression is still ongoing
    169+        };
    170+        //! Consumes `source_loc`'s available bytes corresponding to the size of the (formatted)
    171+        //! `str` and returns its status.
    172+        [[nodiscard]] Status Consume(
    


    l0rinc commented at 12:55 pm on July 6, 2025:
    maybe it’s a code smell that we’re afraid that people will forget to check the return value…

    stickies-v commented at 12:13 pm on July 7, 2025:

    maybe it’s a code smell

    Perhaps you can implement an alternative and share it here?

  151. in src/test/logging_tests.cpp:414 in 6a7147358c outdated
    409+    LogInstance().SetRateLimiting(std::move(limiter));
    410+
    411+    // Log 1024-character lines (1023 plus newline) to make the math simple.
    412+    std::string log_message(1023, 'a');
    413+
    414+    std::string utf8_path{LogInstance().m_file_path.utf8string()};
    


    l0rinc commented at 1:02 pm on July 6, 2025:
    there’s a lot of inconsistency in these tests, can you please unify the styles so that it doesn’t look like it was written by multiple people? (auto vs types, brace init vs =, 20s vs 1024 * 1024, BOOST_CHECK_MESSAGE(=) vs BOOST_CHECK_EQUAL, etc)
  152. in src/logging.cpp:390 in f608adf241 outdated
    381+{
    382+    StdLockGuard scoped_lock(m_mutex);
    383+    auto& counter{m_source_locations.try_emplace(source_loc, m_max_bytes).first->second};
    384+    Status status{counter.GetDroppedBytes() > 0 ? Status::STILL_SUPPRESSED : Status::UNSUPPRESSED};
    385+
    386+    if (!counter.Consume(str.size()) && status == Status::UNSUPPRESSED) {
    


    l0rinc commented at 1:10 pm on July 6, 2025:

    if the only production usage is inverted, we should likely invert the method’s return value to make it more intuitive.

    does not have any side-effects (except update internal accounting, as expected by the Consume name).

    Or even better, having both a side-effect and returning a value is quite confusing (e.g. we can’t invert the && here to if (status == Status::UNSUPPRESSED && !counter.Consume(str.size())), so maybe the Consume could change the state of counter instead and we could query that state here instead.


    stickies-v commented at 12:39 pm on July 7, 2025:

    if the only production usage is inverted, we should likely invert the method’s return value to make it more intuitive.

    I disagree. Returning true on successful operation is intuitive to me.

  153. in src/logging.h:116 in f608adf241 outdated
    111+    {
    112+    private:
    113+        //! Remaining bytes in the current window interval.
    114+        uint64_t m_available_bytes;
    115+        //! Number of bytes that were not consumed within the current window.
    116+        uint64_t m_dropped_bytes{0};
    


    l0rinc commented at 1:16 pm on July 6, 2025:
    Is that really useful to the user? I’d be more interested in the number of log lines that were dropped instead of the bytes. “1234 bytes dropped” -> “5 log lines dropped”

    Crypt-iQ commented at 4:52 pm on July 7, 2025:
    This is a fair point, I could see this going either way. In the presence of disk fill attacks, my personal preference would be to know how many bytes did not make it to disk rather than log lines (which does not convey the number of bytes).
  154. in src/logging.h:346 in 6a7147358c outdated
    342@@ -239,7 +343,7 @@ static inline bool LogAcceptCategory(BCLog::LogFlags category, BCLog::Level leve
    343 bool GetLogCategory(BCLog::LogFlags& flag, std::string_view str);
    344 
    345 template <typename... Args>
    346-inline void LogPrintFormatInternal(std::string_view logging_function, std::string_view source_file, const int source_line, const BCLog::LogFlags flag, const BCLog::Level level, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
    347+inline void LogPrintFormatInternal(std::source_location&& source_loc, const BCLog::LogFlags flag, const BCLog::Level level, const bool should_ratelimit, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
    


    l0rinc commented at 1:36 pm on July 6, 2025:

    nit: we probably don’t have to keep the consts here:

    0inline void LogPrintFormatInternal(std::source_location&& source_loc, BCLog::LogFlags flag, BCLog::Level level, bool should_ratelimit, util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)
    
  155. in src/logging.cpp:458 in 6a7147358c outdated
    450@@ -424,7 +451,29 @@ void BCLog::Logger::LogPrintStr_(std::string_view str, std::string_view logging_
    451         return;
    452     }
    453 
    454-    FormatLogStrInPlace(str_prefixed, category, level, source_file, source_line, logging_function, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime());
    455+    FormatLogStrInPlace(str_prefixed, category, level, source_loc, util::ThreadGetInternalName(), SystemClock::now(), GetMockTime());
    456+    bool ratelimit{false};
    457+    if (should_ratelimit && m_limiter) {
    458+        auto status{m_limiter->Consume(source_loc, str_prefixed)};
    459+        if (status == BCLog::LogRateLimiter::Status::NEWLY_SUPPRESSED) {
    


    l0rinc commented at 7:53 pm on July 6, 2025:

    nit: We’re already inside BCLog:

    0        if (status == LogRateLimiter::Status::NEWLY_SUPPRESSED) {
    
  156. in src/logging.cpp:470 in 6a7147358c outdated
    466+                             source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    467+                             m_limiter->m_max_bytes,
    468+                             Ticks<std::chrono::seconds>(m_limiter->m_reset_window)),
    469+                         std::source_location::current(), LogFlags::ALL, Level::Warning, /*should_ratelimit=*/false); // with should_ratelimit=false, this cannot lead to infinite recursion
    470+        }
    471+        ratelimit = status == BCLog::LogRateLimiter::Status::STILL_SUPPRESSED;
    


    l0rinc commented at 7:58 pm on July 6, 2025:

    seems we just checked the value of this a few lines before, we could probably put this in an else (or even switch-case):

    0        } else if (status == LogRateLimiter::Status::STILL_SUPPRESSED) {
    1            ratelimit = true;
    2        }
    
  157. in src/logging.cpp:78 in 6a7147358c outdated
    74@@ -73,12 +75,12 @@ bool BCLog::Logger::StartLogging()
    75     // dump buffered messages from before we opened the log
    76     m_buffering = false;
    77     if (m_buffer_lines_discarded > 0) {
    78-        LogPrintStr_(strprintf("Early logging buffer overflowed, %d log lines discarded.\n", m_buffer_lines_discarded), __func__, __FILE__, __LINE__, BCLog::ALL, Level::Info);
    79+        LogPrintStr_(strprintf("Early logging buffer overflowed, %d log lines discarded.\n", m_buffer_lines_discarded), std::source_location::current(), BCLog::ALL, Level::Info, /*should_ratelimit=*/false);
    


    l0rinc commented at 9:13 pm on July 6, 2025:
    This is indeed a lot nicer. 👍
  158. in src/logging.h:51 in 6a7147358c outdated
    46+    size_t operator()(const std::source_location& s) const noexcept
    47+    {
    48+        // Use CSipHasher(0, 0) as a simple way to get uniform distribution.
    49+        return static_cast<size_t>(CSipHasher(0, 0)
    50+                                       .Write(std::hash<std::string_view>{}(s.file_name()))
    51+                                       .Write(s.line())
    


    l0rinc commented at 9:35 pm on July 6, 2025:

    maybe we could add the column here as well for the mentioned cases where we have multiple on the same line

    0.Write(s.column())
    1.Write(s.line())
    

    Crypt-iQ commented at 4:56 pm on July 7, 2025:
    Holding off on implementing in this PR. I have thought about using column(), but I wanted to keep behavior as close as possible to the original PR. This can be implemented in a future PR to not rate limit a source location accidentally if they are on the same line. For this to matter for practical purposes, there would need to be a log line that can hit the rate limit and also exists on the same line as another log line that should not be rate limited.
  159. in src/logging.h:50 in 6a7147358c outdated
    45+struct SourceLocationHasher {
    46+    size_t operator()(const std::source_location& s) const noexcept
    47+    {
    48+        // Use CSipHasher(0, 0) as a simple way to get uniform distribution.
    49+        return static_cast<size_t>(CSipHasher(0, 0)
    50+                                       .Write(std::hash<std::string_view>{}(s.file_name()))
    


    l0rinc commented at 9:35 pm on July 6, 2025:

    Do we need to double-hash the filename or can we just feed the chars directly to siphash?

    0.Write(MakeUCharSpan(std::string_view{s.file_name()}))
    

    Crypt-iQ commented at 5:00 pm on July 7, 2025:
    Hmm, I did not think about that. The std::hash might not be necessary then.

    maflcko commented at 8:27 am on July 10, 2025:
    yeah, i think the std::hash can be dropped here

    l0rinc commented at 12:47 pm on July 10, 2025:
  160. in src/logging.cpp:465 in 6a7147358c outdated
    461+            LogPrintStr_(strprintf(
    462+                             "Excessive logging detected from %s:%d (%s): >%d bytes logged during "
    463+                             "the last time window of %is. Suppressing logging to disk from this "
    464+                             "source location until time window resets. Console logging "
    465+                             "unaffected. Last log entry.\n",
    466+                             source_loc.file_name(), source_loc.line(), source_loc.function_name(),
    


    l0rinc commented at 9:39 pm on July 6, 2025:
    we could log the .column() as well, now that we have it for free
  161. in src/logging.cpp:405 in 6a7147358c outdated
    403 
    404     str.insert(0, GetLogPrefix(category, level));
    405 
    406     if (m_log_sourcelocations) {
    407-        str.insert(0, strprintf("[%s:%d] [%s] ", RemovePrefixView(source_file, "./"), source_line, logging_function));
    408+        str.insert(0, strprintf("[%s:%d] [%s] ", RemovePrefixView(source_loc.file_name(), "./"), source_loc.line(), source_loc.function_name()));
    


    l0rinc commented at 9:40 pm on July 6, 2025:
    I understand that this is outside the scope of the change, but let the record show that we have the column available here as well, now that we’ve switched to std::source_location
  162. in src/logging.h:145 in 6a7147358c outdated
    140+    {
    141+    private:
    142+        mutable StdMutex m_mutex;
    143+
    144+        //! Counters for each source location that has attempted to log something.
    145+        std::unordered_map<std::source_location, LogLimitStats, SourceLocationHasher, SourceLocationEqual> m_source_locations GUARDED_BY(m_mutex);
    


    l0rinc commented at 9:47 pm on July 6, 2025:

    not necessarily recommending it, just thought I’ll mention that adding an std::map here instead could theoretically be simpler and faster. We wouldn’t need expensive (double) hashing or a separate SourceLocationEqual, just a friendly:

    0struct SourceLocationLess
    1{
    2    constexpr bool operator()(const std::source_location& a, const std::source_location& b) const noexcept
    3    {
    4        return std::tuple{std::string_view{a.file_name()}, a.line(), a.column()} < std::tuple{std::string_view{b.file_name()}, b.line(), b.column()};
    5    }
    6};
    

    Crypt-iQ commented at 5:05 pm on July 7, 2025:
    I had considered it, but did not implement or bench the difference. Replacing SourceLocationEqual & SourceLocationHasher with SourceLocationLess could be a good future improvement.
  163. in src/logging.h:38 in 6a7147358c outdated
    34@@ -31,6 +35,24 @@ extern const char * const DEFAULT_DEBUGLOGFILE;
    35 
    36 extern bool fLogIPs;
    37 
    38+struct SourceLocationEqual {
    


    l0rinc commented at 9:48 pm on July 6, 2025:

    can be constexpr:

    0struct SourceLocationEqual
    1{
    2    constexpr bool operator()(const std::source_location& lhs, const std::source_location& rhs) const noexcept
    3    {
    4        return lhs.line() == rhs.line()
    5               && std::string_view(lhs.file_name()) == std::string_view(rhs.file_name());
    6    }
    7};
    
  164. l0rinc changes_requested
  165. l0rinc commented at 10:10 pm on July 6, 2025: contributor

    We’re getting closer, but the PR is not ready yet. I went over the code multiple times, left nits and comments and questions all over, regardless of the severity. I hope you can untangle the different directions, I know that so many comments can be overwhelming, I hope you take it the way that I meant them.

    My main concerns are that the code doesn’t seem finished yet, it still contains commented out code, typos, redundant comments, dangerous recursion, prefixes for non-dropped log lines, duplicate state management - and I’m not even entirely sure we should be restricting logs per line in the first place. Also, having so many remaining inconsistencies and mixed styles and undocumented decisions makes it harder to find code smells that would help us catch mistakes (e.g. like the ones I mentioned above).

  166. DrahtBot requested review from l0rinc on Jul 6, 2025
  167. stickies-v commented at 12:50 pm on July 7, 2025: contributor

    My main concerns are that the code doesn’t seem finished yet, it still contains commented out code, typos, redundant comments, dangerous recursion, prefixes for non-dropped log lines, duplicate state management - and I’m not even entirely sure we should be restricting logs per line in the first place.

    I’ve responded to most of your concerns in-line. I think you’ve left some excellent suggestions for a follow-up, but I’ve not seen anything that imo warrants not merging this PR in this current shape. I understand that you’d like to see improvements, and I’d be happy to review the follow-up (as well as re-review here if the author decides to address anything), but without blockers I think it’s absolutely fair for the author to decide that these can be done in a follow-up. I’d prefer not having a third version of this PR.

  168. Crypt-iQ force-pushed on Jul 7, 2025
  169. stickies-v approved
  170. stickies-v commented at 3:49 pm on July 7, 2025: contributor

    re-ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9

    No changes since 6a7147358c9d6e3883dcdbbee9fb2c1cb4baf5ff except commit message and release notes improvements, and removing a commented-out test line.

  171. DrahtBot requested review from dergoegge on Jul 7, 2025
  172. Crypt-iQ commented at 5:19 pm on July 7, 2025: contributor

    The latest push f47e2ea9137c3a832e07d6dd845c55d35d533fa9 has a few (minor) changes:

    • removing the comment in logging_filesize_rate_limit
    • documents restart behavior in the commit message of 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6
    • fixes the typo of -logsourcelocations in the release notes
    • makes the release notes more clear wrt. LogPrintLevel

    We’re getting closer, but the PR is not ready yet. I went over the code multiple times, left nits and comments and questions all over, regardless of the severity.

    I have responded to some of the comments that were about behavior changes. I hope this isn’t taken as lip-service to get the PR merged, but I will make a follow-up PR post-merge to address the style and test comments (I have a list of the comments that need to be addressed). I think the most pressing comments are about behavior changes and I have attempted to address those. I think this behavior change is overdue for the codebase and it has went through iterations across several previous PRs of striking a balance between getting this mitigation shipped into production and some other less-pressing issues.

    I know that so many comments can be overwhelming, I hope you take it the way that I meant them.

    I don’t take it personally, review is a scarce resource and more review is better than less review. As a newcomer, I’m realizing the (appropriate) high bar for review in this project. At this point, though, I think this PR is finished as far as behavior goes and I think it is reasonable to leave the non-behavior changes to a follow-up PR.

  173. l0rinc approved
  174. l0rinc commented at 7:07 pm on July 7, 2025: contributor

    While there are still more nits left than I’m comfortable with, I understand wanting to have some progress - and the current PR is indeed an improvement.

    Lightly tested code review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9

    I’m looking forward to ironing out the wrinkles in follow-up PRs.

  175. dergoegge approved
  176. dergoegge commented at 12:46 pm on July 8, 2025: member
    Code review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9
  177. in doc/release-notes-32604.md:5 in f47e2ea913 outdated
    0@@ -0,0 +1,8 @@
    1+Logging
    2+-------
    3+Unconditional logging to disk is now rate limited via `LogPrintf`, `LogInfo`,
    4+`LogWarning, `LogError`, and the corresponding `LogPrintLevel` calls by giving
    5+each source location a quota of 1MiB per hour. (#32604)
    


    glozow commented at 8:40 pm on July 8, 2025:
    Maybe worth adding that “All logs will be prefixed with [*] if there is at least one source location that is currently being suppressed.” Somebody who sees a [*] may wonder what that means, and hopefully find that in the release notes.

    Crypt-iQ commented at 1:23 pm on July 9, 2025:
    Added in 4c772cbd83e502a1339e8993d192ea6416ecd45c

    glozow commented at 6:39 pm on July 9, 2025:
    fwiw I didn’t mean for this to hold up the PR, but thanks for taking the suggestion!
  178. glozow commented at 8:40 pm on July 8, 2025: member
    lightly reviewed ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9
  179. in doc/release-notes-32604.md:4 in f47e2ea913 outdated
    0@@ -0,0 +1,8 @@
    1+Logging
    2+-------
    3+Unconditional logging to disk is now rate limited via `LogPrintf`, `LogInfo`,
    4+`LogWarning, `LogError`, and the corresponding `LogPrintLevel` calls by giving
    


    maflcko commented at 6:43 am on July 9, 2025:

    from the llm linter:

    • +LogWarning, LogError, → LogWarning, LogError` [missing backtick before comma makes the inline code span invalid]

    However, all of those are implementation details anyway, and it could make sense to remove those. End-users probably wonder what those mean for them, so it could make sense to instead mention the name of the setting that affects them:

    “Unconditional logging to disk is now rate limited by giving each source location a quota of 1MiB per hour. Unconditional logging is any logging with a log level higher than debug, that is info, warning and error.”


    Crypt-iQ commented at 1:23 pm on July 9, 2025:
    Changed in 4c772cbd83e502a1339e8993d192ea6416ecd45c
  180. in src/test/util/logging.h:37 in 2ce8a4f59a outdated
    32@@ -33,7 +33,9 @@ class DebugLogHelper
    33 
    34 public:
    35     explicit DebugLogHelper(std::string message, MatchFn match = [](const std::string*){ return true; });
    36-    ~DebugLogHelper() { check_found(); }
    37+
    38+    //! Mark as noexcept(false) to catch any thrown exceptions.
    


    maflcko commented at 7:15 am on July 9, 2025:
    nit: Probably wanted to say “… to be able to catch …” or “to throw any exceptions.” Otherwise this reads a bit as if noexcept(false) were to catch any thrown exceptions. Could also just remove the comment, but just a nit.

    Crypt-iQ commented at 1:47 pm on July 9, 2025:
    Will address in follow-up
  181. in src/logging.cpp:557 in b5d0cc29ac outdated
    527+    for (const auto& [source_loc, counter] : source_locations) {
    528+        uint64_t dropped_bytes{counter.GetDroppedBytes()};
    529+        if (dropped_bytes == 0) continue;
    530+        LogPrintLevel_(
    531+            LogFlags::ALL, Level::Info,
    532+            "Restarting logging from %s:%d (%s): %d bytes were dropped during the last %ss.\n",
    


    maflcko commented at 7:54 am on July 9, 2025:

    nit in b5d0cc29ac378f360424472f637f6d6af74660a5: Can remove the trailing \n (it is added by logging).

    Also, could use the Warning log level instead, because rate-limiting should never happen in practise and if it does, it seems like something that should be fixed by adjusting the logging at that location?


    Crypt-iQ commented at 1:47 pm on July 9, 2025:
    Will address in follow-up to remove \n and use Warning
  182. in src/logging.h:107 in b5d0cc29ac outdated
    103@@ -82,6 +104,79 @@ namespace BCLog {
    104     };
    105     constexpr auto DEFAULT_LOG_LEVEL{Level::Debug};
    106     constexpr size_t DEFAULT_MAX_LOG_BUFFER{1'000'000}; // buffer up to 1MB of log data prior to StartLogging
    107+    constexpr uint64_t RATELIMIT_MAX_BYTES{1024 * 1024}; // maximum number of bytes that can be logged within one window
    


    maflcko commented at 8:07 am on July 9, 2025:
    nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: Could clarify in the comment “… of bytes per source location …”, but just a nit.

    Crypt-iQ commented at 1:48 pm on July 9, 2025:
    Will address in follow-up, the comment is confusing as pointed out by @l0rinc
  183. in src/test/logging_tests.cpp:360 in b5d0cc29ac outdated
    339+    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 0ull);
    340+
    341+    const uint64_t MESSAGE_SIZE{512 * 1024};
    342+    BOOST_CHECK(counter.Consume(MESSAGE_SIZE));
    343+    BOOST_CHECK_EQUAL(counter.GetAvailableBytes(), BCLog::RATELIMIT_MAX_BYTES - MESSAGE_SIZE);
    344+    BOOST_CHECK_EQUAL(counter.GetDroppedBytes(), 0ull);
    


    maflcko commented at 8:13 am on July 9, 2025:
    nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: I don’t think the ll is needed to silence the w-sign-compare warning. I’d go with just 0u, or if you want to fully specify the type uint64_t{0}.

    Crypt-iQ commented at 1:48 pm on July 9, 2025:
    Will address in follow-up
  184. in src/test/logging_tests.cpp:462 in 6e47dad25c outdated
    457+
    458+    BOOST_CHECK_MESSAGE(log_file_size < GetFileSize(log_path), "location 2 should be exempt from rate limiting");
    459+
    460+    log_file_size = GetFileSize(log_path);
    461+    for (int i = 0; i < 1024; ++i) {
    462+        BOOST_CHECK_THROW(LogFromLocationAndExpect(3, log_message, "Excessive logging detected"), std::runtime_error);
    


    maflcko commented at 10:10 am on July 9, 2025:
    nit in 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6: Could use the HasReason helper, but up to you.
  185. maflcko commented at 10:11 am on July 9, 2025: member

    left some style nits, which are trivial to ignore, or follow-up with. Nothing blocking.

    review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9 🌬

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9 🌬
    3+pM3Ddrk7WibLtn6aVjOQrHL+AGn6QroR8oMDjJ+OI61/kG1FDIzwCo6dTnDvuZcMfHPBic+N1tQI2C4dPYwCg==
    
  186. test: Mark ~DebugLogHelper as noexcept(false)
    We mark ~DebugLogHelper as noexcept(false) to be able to catch the
    exception it throws. This lets us use it in test in combination with
    BOOST_CHECK_THROW and BOOST_CHECK_NO_THROW to check that certain log
    messages are (not) logged.
    
    Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
    df7972a6cf
  187. log: introduce LogRateLimiter, LogLimitStats, Status
    LogRateLimiter will be used to keep track of source locations and our
    current time-based logging window. It contains an unordered_map and a
    m_suppressions_active bool to track source locations. The map is keyed
    by std::source_location, so a custom Hash function (SourceLocationHasher)
    and custom KeyEqual function (SourceLocationEqual) is provided.
    SourceLocationHasher uses CSipHasher(0,0) under the hood to get a
    uniform distribution.
    
    A public Reset method is provided so that a scheduler (e.g. the
    "b-scheduler" thread) can periodically reset LogRateLimiter's state when
    the time window has elapsed.
    
    The LogRateLimiter::Consume method checks if we have enough available
    bytes in our rate limiting budget to log an additional string. It
    returns a Status enum that denotes the rate limiting status and can
    be used by the caller to emit a warning, skip logging, etc.
    
    The Status enum has three states:
    - UNSUPPRESSED     (logging was successful)
    - NEWLY_SUPPRESSED (logging was succcesful, next log will be suppressed)
    - STILL_SUPPRESSED (logging was unsuccessful)
    
    LogLimitStats counts the available bytes left for logging per source
    location for the current logging window. It does not track actual source
    locations; it is used as a value in m_source_locations.
    
    Also exposes a SuppressionsActive() method so the logger can use
    that in a later commit to prefix [*] to logs whenenever suppressions
    are active.
    
    Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
    Co-Authored-By: stickies-v <stickies-v@protonmail.com>
    afb9e39ec5
  188. log: use std::source_location in place of __func__, __FILE__, __LINE__
    The std::source_location conveniently stores the file name, line number,
    and function name of a source code location. We switch to using it instead
    of the __func__ identifier and the __FILE__ and __LINE__ macros.
    
    BufferedLog is changed to have a std::source_location member, replacing the
    source_file, source_line, and logging_function members. As a result,
    MemUsage no longer explicitly counts source_file or logging_function as the
    std::source_location memory usage is included in the MallocUsage call.
    
    This also changes the behavior of -logsourcelocations as std::source_location
    includes the entire function signature. Because of this, the functional test
    feature_config_args.py must be changed to no longer include the function
    signature as the function signature can differ across platforms.
    
    Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
    Co-Authored-By: stickies-v <stickies-v@protonmail.com>
    a6a35cc0c2
  189. log: Add rate limiting to LogPrintf, LogInfo, LogWarning, LogError, LogPrintLevel
    To mitigate disk-filling attacks caused by unsafe usages of LogPrintf and
    friends, we rate-limit them by passing a should_ratelimit bool that
    eventually makes its way to LogPrintStr which may call
    LogRateLimiter::Consume. The rate limiting is accomplished by
    adding a LogRateLimiter member to BCLog::Logger which tracks source
    code locations for the given logging window.
    
    Every hour, a source location can log up to 1MiB of data. Source
    locations that exceed the limit will have their logs suppressed for the
    rest of the window determined by m_limiter.
    
    This change affects the public LogPrintLevel function if called with
    a level >= BCLog::Level::Info.
    
    The UpdateTipLog function has been changed to use the private LogPrintLevel_
    macro with should_ratelimit set to false. This allows UpdateTipLog to log
    during IBD without hitting the rate limit.
    
    Note that on restart, a source location that was rate limited before the
    restart will be able to log until it hits the rate limit again.
    
    Co-Authored-By: Niklas Gogge <n.goeggi@gmail.com>
    Co-Authored-By: stickies-v <stickies-v@protonmail.com>
    d541409a64
  190. doc: add release notes for new rate limiting logging behavior 4c772cbd83
  191. Crypt-iQ force-pushed on Jul 9, 2025
  192. Crypt-iQ commented at 1:24 pm on July 9, 2025: contributor
    4c772cbd83e502a1339e8993d192ea6416ecd45c changes the release notes for clarity.
  193. maflcko commented at 1:27 pm on July 9, 2025: member

    Looks like the release notes were changed a bit. No other changes.

    re-ACK 4c772cbd83e502a1339e8993d192ea6416ecd45c 🕚

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: re-ACK 4c772cbd83e502a1339e8993d192ea6416ecd45c 🕚
    3aX1k/VIO05nW5TdjiVUsNZRpBgwjspZr8wge8cxoZ0AX7CEH3A11ewaSn3WYsL7j8+H4P6G+OvfHD1xFYdGtDg==
    
  194. DrahtBot requested review from glozow on Jul 9, 2025
  195. DrahtBot requested review from dergoegge on Jul 9, 2025
  196. DrahtBot requested review from stickies-v on Jul 9, 2025
  197. DrahtBot requested review from l0rinc on Jul 9, 2025
  198. in doc/release-notes-32604.md:5 in 4c772cbd83
    0@@ -0,0 +1,10 @@
    1+Logging
    2+-------
    3+Unconditional logging to disk is now rate limited by giving each source location
    4+a quota of 1MiB per hour. Unconditional logging is any logging with a log level
    5+higher than debug, that is `info`, `warning`, and `error`. All logs will be
    


    stickies-v commented at 6:25 pm on July 9, 2025:

    nit: Conditional logs (or rather, everything with should_ratelimit=false) don’t get the prefix. Perhaps the better/easier UX is to just apply it to all logs?

    0higher than debug, that is `info`, `warning`, and `error`. All unconditional logs will be
    
  199. stickies-v approved
  200. stickies-v commented at 6:26 pm on July 9, 2025: contributor
    re-ACK 4c772cbd83, no changes except release notes update
  201. glozow commented at 6:39 pm on July 9, 2025: member
    reACK 4c772cb
  202. glozow merged this on Jul 9, 2025
  203. glozow closed this on Jul 9, 2025

  204. davidgumberg commented at 9:29 pm on July 9, 2025: contributor

    Post-merge ACK https://github.com/bitcoin/bitcoin/commit/4c772cbd83e502a1339e8993d192ea6416ecd45c

    Lightly reviewed code, tested by running IBD with debug=all and -nodebug, no messages suppressed, ran logging related benchmarks and measured performance impact is minimal.

    One more really minor and feel-free-to-disregard nit for the follow-up pile:

    The LOG_TIME* macros incorrectly report source locations from logging/timer.h rather than where the macros are invoked. There are currently only two places where these log unconditionally, when flushing to anchors.dat and when writing UTXO snapshots so this is not a problem today AFAICT, but it is a bit of a footgun for future users of LOG_TIME*, one approach to solving it could look like: https://github.com/davidgumberg/bitcoin/commit/5fb07e80a422cf7b65da94dda2e1c0619c80eab3

    Benchmark results

    0$ ./build/bin/bench_bitcoin -filter="Log.*" -min-time=10000
    

    Branch (4c772cbd83e502a1339e8993d192ea6416ecd45c)

    ns/op op/s err% ins/op cyc/op IPC bra/op miss% total benchmark
    901.02 1,109,855.48 0.7% 14,673.87 3,198.96 4.587 3,030.13 0.1% 10.83 LogWithDebug
    903.56 1,106,735.32 0.4% 15,262.89 3,206.54 4.760 3,119.37 0.1% 10.81 LogWithThreadNames
    2.61 383,423,939.51 0.1% 70.00 11.18 6.263 16.00 0.0% 10.99 LogWithoutDebug
    817.98 1,222,522.02 0.6% 13,723.71 2,866.27 4.788 2,793.33 0.1% 10.77 LogWithoutThreadNames
    28.37 35,246,387.38 0.7% 440.00 121.69 3.616 107.00 0.0% 11.04 LogWithoutWriteToFile

    Pre-merge master (b7e9dc8e46)

    ns/op op/s err% ins/op cyc/op IPC bra/op miss% total benchmark
    883.86 1,131,404.74 1.1% 14,673.94 3,136.38 4.679 3,030.14 0.1% 10.83 LogWithDebug
    897.16 1,114,625.39 0.7% 15,262.84 3,189.17 4.786 3,119.36 0.1% 10.84 LogWithThreadNames
    2.60 384,716,694.70 0.3% 70.00 11.15 6.279 16.00 0.0% 11.01 LogWithoutDebug
    816.35 1,224,958.62 0.2% 13,723.80 2,852.35 4.811 2,793.35 0.1% 10.72 LogWithoutThreadNames
    28.28 35,357,376.60 0.4% 440.00 121.30 3.627 107.00 0.0% 11.01 LogWithoutWriteToFile
  205. l0rinc referenced this in commit 7c7f92e38d on Jul 10, 2025
  206. l0rinc referenced this in commit 8d2ba746d7 on Jul 10, 2025
  207. l0rinc referenced this in commit 8a618fba55 on Jul 10, 2025

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-08-03 06:13 UTC

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