#30343 wallet, logging: Replace WalletLogPrintf() with LogInfo()

ReviewabilityReviewsAgreementSize
#30343 ryanofsky wallet, logging: Replace WalletLogPrintf() with LogInfo()Review #29256 first0 (+1)Strong338 + 207 tests
  • Allow wallet logging to use proper severity levels instead of forcing every message to info level
  • Ensure source-location logging reports actual call sites while preserving wallet-name prefixes
  • Substantive support after concerns were resolved (adyshimony)
  • Objected to using fatal error level for non-fatal wallet issues, addressed by dropping commit (maflcko)
  • Interface suggestions and lifetime safety concerns were addressed (hodlinator, adyshimony)
  • 338 lines added or modified outside tests
  • 207 lines added or modified in tests
  • 210 lines removed in total
  • 18 files, 13 commits
Wallet#73 of 92
P3 · cleanup
  • P3 because it standardizes wallet logging on modern severity levels while preserving wallet prefixes
  • Fixes source location reporting in wallet logs without altering core wallet behavior
Ranking notes
Reviewability: ReadyStalePaused
Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets
Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs.
Size: lines added or modified outside tests, then in tests; greener = smaller.

Goal

Replaces the custom `WalletLogPrintf()` helper across the wallet subsystem with standard level-based logging macros (`LogInfo()`, `LogWarning()`, `LogError()`). To retain wallet-name prefixes without forcing all messages to info level, it introduces `WalletLogContext` inheriting from `util::log::Context`. It also fixes `-logsourcelocations` in wallet code so log lines record their true call site instead of the `WalletLogPrintf()` wrapper definition.

Problem: Wallet log messages previously had to use `WalletLogPrintf()` to prepend wallet names for multi-wallet disambiguation. This forced all wallet logs into info level regardless of severity and broke `-logsourcelocations` by making every wallet log entry point to the wrapper function.

Evidence of importance:

  • PR description: 'The new logging macros introduced in #28318 weren't previously useful in wallet code because wallet code prefixes most log messages with the wallet names to be be able to distinguish log output from multiple wallets... This change makes -logsourcelocations usable with wallet code because it logs actual source locations instead of the WalletLogPrintf() location.'
  • Commit 1bdbe05a22 incorporates review suggestions to differentiate warnings and errors rather than logging everything at info level.

Scope: This PR is stacked on top of #34778 and #29256. Only the last two commits in the branch are specific to this PR; the preceding commits belong to the underlying logging macro rewrite and context-argument infrastructure.

Risk of a mistake: low (wallet). Mistakes are confined to logging output and format strings, which are evaluated with compile-time format string checks.

Changed since the previous assessment: Rebased onto the updated base stack (#34778 and #29256), adapting to changes in the underlying `LOG_EMIT` macro and context hooks while keeping the wallet conversion intact.

Reviewability

Ready · Review #29256 first

Stacked on #29256; the base PR's changes are included in this diff.

Waiting on (from the thread read):

  • reviewer: review of the rebased commits 2024-06-28

Agreement

Strong

substantive support, no open objection (adyshimony)

Thread notes: The author updated the rebase log comment c:2197070499 detailing multiple rebases on top of #29256; no new review claims were raised.

Objections, suggestions, and questions:

ReviewerTypeHarm, request, or questionStatusMust beAuthor repliedQuote
hodlinator (member)suggestion, interfacerename GetLogSource() to Log() and keep m_log private in WalletStorage and Walletfixed · fix pushed · implemented in commit and pushansweredyes2024-06-26: 'Why not rename `GetLogSource()` -> `Log()` here and in `WalletStorage` and keep `m_log` private?'
Status from: 2024-06-27: ryanofsky: 'Nice suggestion, implemented. This is much cleaner looking.'
maflcko (member)objection, correctnessLogError is reserved for fatal errors requiring node shutdown, making its use for wallet errors inconsistent with guidelinesfixed · fix pushed · commit introducing LogError was droppedchangedyes2024-06-27: 'Are you sure that `LogError` is the right level? It should only be used for fatal errors where the node needs to shut down, according to the docs.'
Status from: 2026-03-03: ryanofsky: 'Makes sense, and now dropped this commit since the code has changed since it was written.'
l0rinc (member)question, approachasks whether PR is a draft due to dependency on #29256 and whether a simpler scripted-diff renaming WalletLogPrintf could be done insteadanswered · author explained draft status and why scripted diff would lose wallet namesansweredyes2026-01-23: 'Is it a draft because it's based on https://github.com/bitcoin/bitcoin/pull/29256 which should be reviewed first? That one has two nacks, can you make a version of this that simply renames the leftove'
Status from: 2026-01-23: ryanofsky: 'I don't think we want to only replace WalletLogPrintf calls with LogInfo calls, because the point of WalletLogPrintf is '
adyshimony (none)objection, safetyrvalue source arguments can be bound by reference and used after their lifetime endsaccepted · fix pushed · reviewer noted LIFETIMEBOUND is caught under Clang and gave ACK regardless of approachchangedyes2026-03-01: 'rvalue source arguments can be bound by reference and used after their lifetime ends. I tried to solve this in a few ways, but I think the most elegant simple one is to block rvalues at compile time'
Status from: 2026-03-04: adyshimony: 'In any case, with either approach, ACK on 7fc95e7cbb3ee3cae14df02602ac51a3d910945c.'
adyshimony (none)objection, correctnesslogging normal operation under an error severity levelfixed · fix pushed · commit containing the log level change was droppedchangedyes2026-03-01: 'Isn't that a happy flow? Maybe we can use LogInfo here?'
Status from: 2026-03-03: ryanofsky: 'Makes sense, and now dropped this commit since the code has changed since it was written.'

Support:

  • adyshimony (none) (ACK, 2026-03-04) [read]: In any case, with either approach, ACK on 7fc95e7cbb3ee3cae14df02602ac51a3d910945c.

Participants:

  • hodlinator (member, neutral, 7 statements 2024-06-26 to 2024-06-26): reviewed and suggested renaming GetLogSource to Log() and adjusting log levels
  • maflcko (member, objection, 1 statements 2024-06-27 to 2024-06-27): objected to using LogError for non-fatal errors
  • l0rinc (member, question, 1 statements 2026-01-23 to 2026-01-23): asked why the PR is draft and whether a simpler scripted-diff rename would suffice
  • adyshimony (none, support, 6 statements 2026-03-01 to 2026-03-04): tested and ACKed; raised lifetime safety and log level concerns which were addressed

Dependencies

Depends on: #34778, #29256

Based on (shares commits with): #29256, #34778

Files

228 lines under test/bench/ci.

  • src/util/log.h +185/-62
  • src/test/logging_tests.cpp +207/-21
  • src/wallet/wallet.cpp +34/-29
  • src/logging.cpp +18/-24
  • src/wallet/walletdb.cpp +24/-18
  • src/wallet/logging.h +29/-0
  • src/kernel/bitcoinkernel.cpp +7/-13
  • src/wallet/wallet.h +8/-10
  • src/wallet/scan.cpp +8/-8
  • src/wallet/scriptpubkeyman.h +5/-9
  • src/wallet/spend.cpp +5/-3
  • src/wallet/scriptpubkeyman.cpp +4/-3
  • src/logging.h +3/-3
  • src/validation.cpp +3/-2
  • doc/developer-notes.md +1/-2
  • src/wallet/receive.cpp +2/-1
  • src/wallet/dump.cpp +1/-1
  • src/wallet/wallettool.cpp +1/-1

Card

The short text the ranking pass compares across PRs.

This PR replaces the custom WalletLogPrintf() logging wrapper across wallet files with Bitcoin Core standard level-based logging macros (LogInfo, LogWarning, LogError). It introduces a WalletLogContext class so wallet-name prefixes remain intact while enabling proper severity level filtering. Additionally, it fixes -logsourcelocations for wallet code by attributing messages to actual call sites rather than the helper definition. The pull request is based on logging macro refactors in #34778 and #29256.

Data

dossier JSON · extract JSON · model ledger, generated 2026-11-01T01:16, confidence high, input hash 2df33f9eaf2ee2ff
thread read through 2026-03-04 (30 statements), head 1bdbe05a22
labels: Wallet · draft

Categories

Wallet

P3 · cleanup

Replaces legacy `WalletLogPrintf()` with standard logging macros (`LogInfo()`, `LogWarning()`, `LogError()`). This addresses the problem where wallet log messages were previously forced into info level regardless of severity and fixes `-logsourcelocations` to report actual call sites rather than the wrapper definition.

Membership: Carries the Wallet label, modifies wallet files across the subsystem, and updates wallet-specific logging infrastructure.

Factors: security/stability 0, bug 1, performance 0, user value 0, leverage 0