Skip to content

Conversation

fanquake
Copy link
Member

@fanquake fanquake commented Aug 20, 2025

Crypt-iQ and others added 17 commits August 20, 2025 11:37
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>

Github-Pull: bitcoin#32604
Rebased-From: df7972a
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>

Github-Pull: bitcoin#32604
Rebased-From: afb9e39
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>

Github-Pull: bitcoin#32604
Rebased-From: a6a35cc
…ogPrintLevel

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>

Github-Pull: bitcoin#32604
Rebased-From: d541409
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin#33011
Rebased-From: b8e92fb
Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

Github-Pull: bitcoin#33011
Rebased-From: 5f70bc8
Co-Authored-By: stickies-v <stickies-v@protonmail.com>

Github-Pull: bitcoin#33011
Rebased-From: 8319a13
Clean up the noisy LogLimitStats and remove references to the time
window.

Co-Authored-By: stickies-v <stickies-v@protonmail.com>

Github-Pull: bitcoin#33011
Rebased-From: 3c7cae4
… there are suppressions

In LogPrintStr_:
- remove an unnecessary BCLog since we are in the BCLog namespace.
- remove an unnecessary \n when rate limiting is triggered since
  FormatLogStrInPlace will add it.
- move the ratelimit bool into an else if block.
- prefix all log lines with [*] when suppressions exist. Previously this
  was only done if should_ratelimit was true.

In Reset:
- remove an unnecessary \n since FormatLogStrInPlace will add it.
- Change Level::Info to Level::Warning.

Github-Pull: bitcoin#33011
Rebased-From: e8f9c37
This allows us to safely and explicitly manage the dual dependency
on the limiter: one for the Logger, and one for the CScheduler.

Github-Pull: bitcoin#33011
Rebased-From: 3d630c2
Deduplicates repeated usage of the same functionality.

Github-Pull: bitcoin#33011
Rebased-From: 05d7c22
This ensures log tests behave consistently when other tests modify
the log category mask.

Github-Pull: bitcoin#33011
Rebased-From: 350193e
- Add helper functions and structs to improve readability and
  reusability of test code
- Make tests more specific by comparing all produced log lines with
  expected log lines instead of relying on approximations or proxies.

Github-Pull: bitcoin#33011
Rebased-From: 9f3b017
Use -nologratelimit by default in functional tests if the bitcoind
version supports it.

Co-Authored-By: stickies-v <stickies-v@protonmail.com>

Github-Pull: bitcoin#33011
Rebased-From: 5c74a0b
Change time_window from 20s to 1h so Reset is not accidentally called
if the test takes a while.

Change num_lines from 1024 to 10 since LogRateLimiter is parameterized
and does not require logging 1MiB of data.

Co-Authored-By: stickies-v <stickies-v@protonmail.com>

Github-Pull: bitcoin#33211
Rebased-From: 5dda364
@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 20, 2025

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/33225.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK dergoegge, instagibbs, stickies-v

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

@@ -137,6 +137,8 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor,
self.args.append("-logsourcelocations")
if self.version_is_at_least(239000):
self.args.append("-loglevel=trace")
if self.version_is_at_least(299900):
self.args.append("-nologratelimit")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjust?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could make sense to adjust for 29.x if users are running the functional tests, look at the functional test logs, and are confused why some tests have logs rate limited.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, small change that might turn out helpful during debugging

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, we can adjust this pre-final.

Copy link
Member

@dergoegge dergoegge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0022e25

Cherry-picked the commits myself locally and got the same result (except for the release note which was moved to doc/release-notes.md).

@fanquake fanquake added this to the 29.1 milestone Aug 20, 2025
@fanquake
Copy link
Member Author

Guix Build (aarch64):

ccc8209475974ddef8da13c2b4d09c3798ed26f1f8ca64bfad5cbaca858af1c0  guix-build-0022e25333a8/output/aarch64-linux-gnu/SHA256SUMS.part
4f1cc05dcf87206cd2e6dd601098c0df5518bbc347b9b42a6dbadbd45a24dbde  guix-build-0022e25333a8/output/aarch64-linux-gnu/bitcoin-0022e25333a8-aarch64-linux-gnu-debug.tar.gz
053fba6c3981b92e9c7af91149d49cae6a1f10d1e8c431e90cdde47eb1d0bab8  guix-build-0022e25333a8/output/aarch64-linux-gnu/bitcoin-0022e25333a8-aarch64-linux-gnu.tar.gz
8dfde460a7873ee28f5aa378992466a3f45822539b84ed7012e53397bf8326a4  guix-build-0022e25333a8/output/arm-linux-gnueabihf/SHA256SUMS.part
4f9e9bdb869228a83459316ede8944b1713c7b889c7eff63fbd243919fa5996f  guix-build-0022e25333a8/output/arm-linux-gnueabihf/bitcoin-0022e25333a8-arm-linux-gnueabihf-debug.tar.gz
2af0aa287c721d7f79b212c07404015daaf240af5fdaf4d85b54161434eae1f4  guix-build-0022e25333a8/output/arm-linux-gnueabihf/bitcoin-0022e25333a8-arm-linux-gnueabihf.tar.gz
3cabc17b3d94eace80340c242563fb51982f0255c2ee71bd919d6e0e57c2c46d  guix-build-0022e25333a8/output/arm64-apple-darwin/SHA256SUMS.part
7ce540197e7e472a6d7076dd313c5af96d0a7b34d8e63d88feeda72f66782133  guix-build-0022e25333a8/output/arm64-apple-darwin/bitcoin-0022e25333a8-arm64-apple-darwin-codesigning.tar.gz
78723c53f3582fc26b637d60afa048d1571ac95968fb065df59fec20459107f8  guix-build-0022e25333a8/output/arm64-apple-darwin/bitcoin-0022e25333a8-arm64-apple-darwin-unsigned.tar.gz
747ef90413cc3f3a4fe1214daed21af5d60c04315f46d09f58ca7b6fe81fc09a  guix-build-0022e25333a8/output/arm64-apple-darwin/bitcoin-0022e25333a8-arm64-apple-darwin-unsigned.zip
c02d931c215ac5284c0b6e7a5b74a162a86049a27f2c8c30c3a843c3ec705aad  guix-build-0022e25333a8/output/dist-archive/bitcoin-0022e25333a8.tar.gz
dff1110a83855d52b12c18f87bb196bb31f0ba4ef076bd57b404156ed054dfba  guix-build-0022e25333a8/output/powerpc64-linux-gnu/SHA256SUMS.part
d80b8504cef4d6e533094670ace2d9f1ec9774f68a21a79ae973c0b982330839  guix-build-0022e25333a8/output/powerpc64-linux-gnu/bitcoin-0022e25333a8-powerpc64-linux-gnu-debug.tar.gz
2e30996346ca24adab8ef8061e2feaa40dbb2afd9cad012a39f016ea1b011fe0  guix-build-0022e25333a8/output/powerpc64-linux-gnu/bitcoin-0022e25333a8-powerpc64-linux-gnu.tar.gz
017e723a8f94f5c49eed869cfa9c9b414ab40828502b49d50eee8c94c2a29cd4  guix-build-0022e25333a8/output/riscv64-linux-gnu/SHA256SUMS.part
fc50b6ee102fa86c7491e3f2dcb4adb3c43060ae143bb11b7ffb54ef7e2ac5e3  guix-build-0022e25333a8/output/riscv64-linux-gnu/bitcoin-0022e25333a8-riscv64-linux-gnu-debug.tar.gz
79f2ffe053a75a3a598cb8749a9563e494e6ce412908d37f560a43946c4bc779  guix-build-0022e25333a8/output/riscv64-linux-gnu/bitcoin-0022e25333a8-riscv64-linux-gnu.tar.gz
e566a7ab830b63dbd57a8d8d428cc8881de9d50d33ced754b19022ef5269cb72  guix-build-0022e25333a8/output/x86_64-apple-darwin/SHA256SUMS.part
f7a1c58360c7b767d3468a38641c662474dfb6168a13a2b35dd6d0999e4d77ee  guix-build-0022e25333a8/output/x86_64-apple-darwin/bitcoin-0022e25333a8-x86_64-apple-darwin-codesigning.tar.gz
78c9be4cd1c3bbb0cb539cabae56757c28a6ad9d9d2ea315edac3a1e224d6ff8  guix-build-0022e25333a8/output/x86_64-apple-darwin/bitcoin-0022e25333a8-x86_64-apple-darwin-unsigned.tar.gz
9f9b5403bb532739d3db2fba6a1634bba6a1305b89c088b33230dbdb97fe5f89  guix-build-0022e25333a8/output/x86_64-apple-darwin/bitcoin-0022e25333a8-x86_64-apple-darwin-unsigned.zip
ff2cddaba303fe3a9bcbb265cdfebe73079f1c4cfbcac8e35d7d12935c14e996  guix-build-0022e25333a8/output/x86_64-linux-gnu/SHA256SUMS.part
519aec959f78bca880d56f10b8115b0d4766229b4035fd043f091ee18110896a  guix-build-0022e25333a8/output/x86_64-linux-gnu/bitcoin-0022e25333a8-x86_64-linux-gnu-debug.tar.gz
c8a6b53be41da8863dff8521f9c4e33839e165070932ef291bb48a60db9359b2  guix-build-0022e25333a8/output/x86_64-linux-gnu/bitcoin-0022e25333a8-x86_64-linux-gnu.tar.gz
a1d301672c6ace71b4f5b2e737f2e975069563310d5d2bddf6784bb38fdc8e29  guix-build-0022e25333a8/output/x86_64-w64-mingw32/SHA256SUMS.part
049bdc4df1517a32d09a8b1df3ecbf75c6942a9b61ba1cd2671843f591c98403  guix-build-0022e25333a8/output/x86_64-w64-mingw32/bitcoin-0022e25333a8-win64-codesigning.tar.gz
7831b3903480cdf6656fe719f94c7f8ce111b7b44305cfe9184c370ed5b16b5f  guix-build-0022e25333a8/output/x86_64-w64-mingw32/bitcoin-0022e25333a8-win64-debug.zip
50941ff4c7711bdb71fe64c68573ebfab9e5b297441078a9c79d00ffcf8da01a  guix-build-0022e25333a8/output/x86_64-w64-mingw32/bitcoin-0022e25333a8-win64-setup-unsigned.exe
9207e0f6ab36764c7e53d27e69c6a55e4e2e7ce9fb8463a9da7f41e920c3317b  guix-build-0022e25333a8/output/x86_64-w64-mingw32/bitcoin-0022e25333a8-win64-unsigned.zip

@instagibbs
Copy link
Member

utACK 0022e25

re-applied all commits and only difference is release notes change location

Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0022e25 - all backports clean except the release notes one, as indicated.

Can't see any weird interactions with the 29.x branch.

@@ -137,6 +137,8 @@ def __init__(self, i, datadir_path, *, chain, rpchost, timewait, timeout_factor,
self.args.append("-logsourcelocations")
if self.version_is_at_least(239000):
self.args.append("-loglevel=trace")
if self.version_is_at_least(299900):
self.args.append("-nologratelimit")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, small change that might turn out helpful during debugging

@glozow glozow merged commit c5196bc into bitcoin:29.x Aug 20, 2025
21 of 24 checks passed
@fanquake fanquake deleted the backport_logging_ratelimiting branch August 20, 2025 13:56
@Crypt-iQ
Copy link
Contributor

Post-merge ACK 0022e25

Only change is release-notes. Tested that rate-limiting works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants