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

ryanofsky commented at 5:57 pm on June 26, 2024: contributor

Make new logging macros LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() compatible with wallet code and drop custom WalletLogPrintf() function. 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. Fix this by introducing a new WalletLogSource class inheriting from BCLog::Source which can include the wallet name in log messages.

This is based on #34778 + #29256. The non-base commits are:

DrahtBot commented at 5:57 pm on June 26, 2024: 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/30343.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	adyshimony

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34730 (util/log: Combine the warning/error log levels into a single alert level by ajtowns)
#34729 (Reduce log noise by ajtowns)
#34681 (wallet: refactor ScanForWalletTransactions by Eunovo)
#34639 (iwyu: Document or remove some pragma: export and other improvements by hebasto)
#34617 (fees: wallet: remove block policy fee estimator internals from wallet by ismaelsadeeq)
#34514 (refactor: remove unnecessary std::move for trivially copyable types by l0rinc)
#34502 (wallet: remove most asserts of WALLET_FLAG_DESCRIPTORS flag by rkrux)
#34400 (wallet: parallel fast rescan (approx 5x speed up with 16 threads) by Eunovo)
#34389 (net/log: standardize peer+addr log formatting via LogPeer by l0rinc)
#34193 (wallet: make migration more robust against failures by furszy)
#34075 (fees: Introduce Mempool Based Fee Estimation to reduce overestimation by ismaelsadeeq)
#34038 (logging: replace -loglevel with -trace, various API improvements by ajtowns)
#33727 (zmq: Log bind error at Error level, abort startup on init error by isrod)
#28333 (wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively by achow101)

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

LLM Linter (✨ experimental)

Possible typos and grammar issues:

testring -> testing [typo in test comment: “for easy manual testring to confirm these calls produce readble compile errors.”]
readble -> readable [typo in test comment: “produce readble compile errors.”]

2026-03-09 15:47:53

in src/wallet/wallet.h:448 in 5b30f17c22 outdated

444@@ -444,6 +445,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
445      */
446     mutable RecursiveMutex cs_wallet;
447 
448+    WalletLogSource m_log;

hodlinator commented at 7:16 pm on June 26, 2024:

Why not rename GetLogSource() -> Log() here and in WalletStorage and keep m_log private? I tried and it compiles as of 5b30f17c22ade44d301aed5051a398fa38251ef7.

ryanofsky commented at 3:46 am on June 27, 2024:

re: #30343 (review)

Why not rename GetLogSource() -> Log() here and in WalletStorage and keep m_log private? I tried and it compiles as of 5b30f17.

Nice suggestion, implemented. This is much cleaner looking.

in src/wallet/walletdb.cpp:553 in 5b30f17c22 outdated

550             }
551 
552             DatabaseCursor::Status status = cursor->Next(key, value);
553             if (status != DatabaseCursor::Status::DONE) {
554-                pwallet->WalletLogPrintf("Error: Unexpected legacy entry found in descriptor wallet %s. The wallet might have been tampered with or created with malicious intent.\n", pwallet->GetName());
555+                LogInfo(log, "Error: Unexpected legacy entry found in descriptor wallet %s. The wallet might have been tampered with or created with malicious intent.\n", pwallet->GetName());

hodlinator commented at 7:19 pm on June 26, 2024:

Shouldn’t this be a LogError/LogWarning with removed Error: -prefix?

ryanofsky commented at 3:47 am on June 27, 2024:

re: #30343 (review)

Shouldn’t this be a LogError/LogWarning with removed Error: -prefix?

Agree that makes sense. I implemented it in a followup commit so the main commit is still a refactoring that does not change behavior.

For now I did not remove “Error:” and “Warning:” prefixes because it would be a more significant, possibly unwelcome change, since the prefixes could help messages stand out, and might be relied upon by tests. I’m also not sure if the decision to include warning and error prefixes has been discussed before somewhere else. But I’m happy to change this if there is more feedback in support of it.

in src/wallet/walletdb.cpp:987 in 5b30f17c22 outdated

984         std::string purpose_str;
985         value >> purpose_str;
986         std::optional<AddressPurpose> purpose{PurposeFromString(purpose_str)};
987         if (!purpose) {
988-            pwallet->WalletLogPrintf("Warning: nonstandard purpose string '%s' for address '%s'\n", purpose_str, strAddress);
989+            LogInfo(log, "Warning: nonstandard purpose string '%s' for address '%s'\n", purpose_str, strAddress);

hodlinator commented at 7:20 pm on June 26, 2024:

LogWarning?

ryanofsky commented at 3:47 am on June 27, 2024:

re: #30343 (review)

LogWarning?

Thanks, done in a new commit (see earlier comment)

in src/wallet/scriptpubkeyman.h:36 in 5b30f17c22 outdated

30@@ -31,6 +31,7 @@
31 enum class OutputType;
32 
33 namespace wallet {
34+class WalletLogSource;
35 struct MigrationData;
36 class ScriptPubKeyMan;

hodlinator commented at 7:25 pm on June 26, 2024:

nit: Should strive for alphabetical ordering?

ryanofsky commented at 3:46 am on June 27, 2024:

re: #30343 (review)

nit: Should strive for alphabetical ordering?

Yes, now piped these through sort

in src/wallet/wallet.cpp:3871 in 5b30f17c22 outdated

3867@@ -3866,13 +3868,13 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
3868     AssertLockHeld(cs_wallet);
3869 
3870     if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
3871-        WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n");
3872+        LogInfo(m_log, "Cannot add WalletDescriptor to a non-descriptor wallet\n");

hodlinator commented at 7:48 pm on June 26, 2024:

Should maybe have been a warning all along?

ryanofsky commented at 3:46 am on June 27, 2024:

re: #30343 (review)

Should maybe have been a warning all along?

Good catch. It actually looks to me like all of the log prints in this function should be errors. I changed them to use LogError in a new commit. It’s a separate commit because the level change is a minor change in behavior, and is less obvious than other level changes.

maflcko commented at 5:41 am on June 27, 2024:

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. This seems like a warning here, as suggested above.

Some wallet code can probably use Result and pass errors up, possibly here as well. And the caller can decide whether to log as a warning or return over RPC (or something else).

ryanofsky commented at 6:51 am on June 27, 2024:

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. This seems like a warning here, as suggested above.

Oh, that is interesting. The descriptions of those log levels in https://github.com/bitcoin/bitcoin/blob/b27afb7fb774809c9c075d56e44657e0b607a00c/doc/developer-notes.md#logging are much more severe than I would have expected. Based on those descriptions I would expect LogError to be called LogFatal and LogWarning to be called LogCritical. I would have expected LogError to be used for normal errors, and LogWarning to be used for normal warnings.

Maybe the naming would be worth following up on, but I don’t think it would be good to do in this PR. It actually seems like LogInfo is the right log level to use here, not LogWarning. Or do you disagree? Should I just drop the last two commits in this PR?

in src/wallet/wallet.cpp:2491 in 5b30f17c22 outdated

2489         }
2490 
2491         // Delete purpose entry
2492         if (!batch.ErasePurpose(dest)) {
2493-            WalletLogPrintf("Error: cannot erase address book entry purpose\n");
2494+            LogInfo(m_log, "Error: cannot erase address book entry purpose\n");

hodlinator commented at 7:50 pm on June 26, 2024:

LogInfo(m_log, "Error: -> LogError( here and in other places?

ryanofsky commented at 3:46 am on June 27, 2024:

re: #30343 (review)

LogInfo(m_log, "Error: -> LogError( here and in other places?

Thanks, done in a new commit (see earlier comment)

hodlinator commented at 10:17 pm on June 26, 2024: contributor

Went through this and the parent PR together. It was helpful to see how that other PR’s changes were used in this one.

ryanofsky referenced this in commit 8de95b3b5d on Jun 27, 2024

ryanofsky referenced this in commit 5a629a60c4 on Jun 27, 2024

ryanofsky force-pushed on Jun 27, 2024

ryanofsky commented at 4:13 am on June 27, 2024: contributor

Thanks for the review! The suggestions are a big improvement and make the logging code more succinct and clean.

Rebased 5b30f17c22ade44d301aed5051a398fa38251ef7 -> 5a629a60c4739b94383596f9d040502b7b2c8b69 (pr/gwlog.1 -> pr/gwlog.2, compare) on top of the updated parent PR implementing review suggestions.

ryanofsky referenced this in commit 1ac3f01fe5 on Jun 27, 2024

ryanofsky referenced this in commit db8c6fc2ca on Jun 27, 2024

ryanofsky force-pushed on Jun 27, 2024

DrahtBot added the label CI failed on Jun 27, 2024

DrahtBot commented at 4:35 am on June 27, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is 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.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/26740126920

ryanofsky commented at 4:36 am on June 27, 2024: contributor

Updated 5a629a60c4739b94383596f9d040502b7b2c8b69 -> db8c6fc2ca5e1ad99aeda741e3b453f1f7bf7955 (pr/gwlog.2 -> pr/gwlog.3, compare) fixing initialization order bug in last push

DrahtBot removed the label CI failed on Jun 27, 2024

ryanofsky referenced this in commit 6b5b2354c6 on Jun 28, 2024

ryanofsky referenced this in commit d486e49b54 on Jun 28, 2024

ryanofsky referenced this in commit b0c2f8203d on Jun 28, 2024

ryanofsky referenced this in commit 29eaec25af on Jun 28, 2024

ryanofsky force-pushed on Jun 28, 2024

ryanofsky commented at 2:27 pm on June 28, 2024: contributor

Updated and rebased db8c6fc2ca5e1ad99aeda741e3b453f1f7bf7955 -> 29eaec25af32e543df3cff091938c3cb8f7cce17 (pr/gwlog.3 -> pr/gwlog.4, compare) with suggestion to use constexpr Rebased 29eaec25af32e543df3cff091938c3cb8f7cce17 -> 0260cd5ccc3120102c001b3ab6c5f29dcf184b58 (pr/gwlog.4 -> pr/gwlog.5, compare) due to conflict with #30406 Rebased 0260cd5ccc3120102c001b3ab6c5f29dcf184b58 -> 0d49ada2b42e5b48b21bb7cb854851f09641abbb (pr/gwlog.5 -> pr/gwlog.6, compare) on top of updated base pr/bclog.20 Rebased 0d49ada2b42e5b48b21bb7cb854851f09641abbb -> f6b93a976b0127b55705dc33caf986a3344e12a4 (pr/gwlog.6 -> pr/gwlog.7, compare) due to conflict with #30485 Rebased f6b93a976b0127b55705dc33caf986a3344e12a4 -> 4c8fe044d3241664151b3727e31ac2eaa741b717 (pr/gwlog.7 -> pr/gwlog.8, compare) due to conflicts with #26619, #30659, and other PRs Rebased 4c8fe044d3241664151b3727e31ac2eaa741b717 -> e8c6095c75dead138e79a18146ce865e7fe6529a (pr/gwlog.8 -> pr/gwlog.9, compare) due to conflicts with #31061

Rebased e8c6095c75dead138e79a18146ce865e7fe6529a -> af6f854df0d0dd72b05cda4d2e1ed0d1c2c00738 (pr/gwlog.9 -> pr/gwlog.10, compare)

Rebased af6f854df0d0dd72b05cda4d2e1ed0d1c2c00738 -> 2e4d0c3b51e2e8fbf913f28361d1a0a8d1f3e166 (pr/gwlog.10 -> pr/gwlog.11, compare)

Rebased 2e4d0c3b51e2e8fbf913f28361d1a0a8d1f3e166 -> 186b721e6107a0606be6c55edbdd8ec5d5fabd2b (pr/gwlog.11 -> pr/gwlog.12, compare)

Rebased 186b721e6107a0606be6c55edbdd8ec5d5fabd2b -> df507121593d4a1cd58c80e5e63735c115c728c5 (pr/gwlog.12 -> pr/gwlog.13, compare) on updated base PR to fix CI errors (https://github.com/bitcoin/bitcoin/actions/runs/18514132530?pr=30343), also fixing wallet name formatting causing wallet_tests/CreateWallet to fail

Rebased df507121593d4a1cd58c80e5e63735c115c728c5 -> e84bd31c04751e968a0936b93d6cc7e639ac4409 (pr/gwlog.13 -> pr/gwlog.14, compare)

Rebased e84bd31c04751e968a0936b93d6cc7e639ac4409 -> 75042af2053485badea67b1ff9b38bc820f3f24c (pr/gwlog.14 -> pr/gwlog.15, compare)

Rebased 75042af2053485badea67b1ff9b38bc820f3f24c -> 74bb399dfa1f0823f26fc3bfa7917f5f4fa36668 (pr/gwlog.15 -> pr/gwlog.16, compare)

Rebased 74bb399dfa1f0823f26fc3bfa7917f5f4fa36668 -> a08c9bc808ef19accc90335fb115f791a66e0158 (pr/gwlog.16 -> pr/gwlog.17, compare) on top of #29256 pr/bclog.34

Rebased a08c9bc808ef19accc90335fb115f791a66e0158 -> aac55f94938aa7ac727b840a9766adb848e998ab (pr/gwlog.17 -> pr/gwlog.18, compare) on top of #29256 pr/bclog.36

Rebased aac55f94938aa7ac727b840a9766adb848e998ab -> 7fc95e7cbb3ee3cae14df02602ac51a3d910945c (pr/gwlog.18 -> pr/gwlog.19, compare) on top of #29256 pr/bclog.37 with review suggestions

Rebased 7fc95e7cbb3ee3cae14df02602ac51a3d910945c -> 524c5b8ef547700e4b5a234bf6bebd4d775d98e5 (pr/gwlog.19 -> pr/gwlog.20, compare) on top of #29256 pr/bclog.39

glozow added the label Wallet on Jul 2, 2024

DrahtBot added the label Needs rebase on Jul 11, 2024

ryanofsky referenced this in commit 5cec69a56e on Jul 17, 2024

ryanofsky referenced this in commit 57346e0dea on Jul 17, 2024

ryanofsky referenced this in commit b9a3715858 on Jul 17, 2024

ryanofsky referenced this in commit 0260cd5ccc on Jul 17, 2024

ryanofsky force-pushed on Jul 17, 2024

DrahtBot removed the label Needs rebase on Jul 17, 2024

DrahtBot commented at 10:41 am on July 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/27589923083

Make sure to run all tests locally, according to the documentation.

The failure may 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.

DrahtBot added the label CI failed on Jul 22, 2024

DrahtBot removed the label CI failed on Jul 22, 2024

DrahtBot added the label Needs rebase on Jul 26, 2024

ryanofsky referenced this in commit c75687736a on Aug 7, 2024

ryanofsky referenced this in commit 0d51d583aa on Aug 7, 2024

ryanofsky force-pushed on Aug 7, 2024

ryanofsky referenced this in commit f16a846d79 on Aug 7, 2024

ryanofsky referenced this in commit 0d49ada2b4 on Aug 7, 2024

ryanofsky referenced this in commit 989f2ea404 on Aug 7, 2024

ryanofsky referenced this in commit f6b93a976b on Aug 7, 2024

ryanofsky referenced this in commit 5f6750a543 on Aug 7, 2024

ryanofsky referenced this in commit 517a4f1c0f on Aug 7, 2024

ryanofsky force-pushed on Aug 7, 2024

DrahtBot removed the label Needs rebase on Aug 7, 2024

DrahtBot added the label Needs rebase on Aug 15, 2024

ryanofsky referenced this in commit 8e9e7b90cd on Dec 9, 2024

ryanofsky referenced this in commit 4c8fe044d3 on Dec 9, 2024

ryanofsky referenced this in commit 980a494f6c on Dec 9, 2024

ryanofsky referenced this in commit 09fcfffd48 on Dec 9, 2024

ryanofsky force-pushed on Dec 9, 2024

DrahtBot removed the label Needs rebase on Dec 9, 2024

DrahtBot added the label Needs rebase on Jan 15, 2025

ryanofsky referenced this in commit b7e37883da on Mar 12, 2025

ryanofsky force-pushed on Mar 12, 2025

ryanofsky referenced this in commit e8c6095c75 on Mar 12, 2025

ryanofsky referenced this in commit 278ff86ca2 on Mar 12, 2025

ryanofsky referenced this in commit 6784d2baae on Mar 12, 2025

DrahtBot removed the label Needs rebase on Mar 12, 2025

ryanofsky referenced this in commit 6d0745d22b on Apr 3, 2025

ryanofsky referenced this in commit 1218acc259 on Apr 3, 2025

ryanofsky referenced this in commit af6f854df0 on Apr 3, 2025

ryanofsky force-pushed on Apr 3, 2025

ryanofsky referenced this in commit c3161fcac9 on Apr 3, 2025

ryanofsky referenced this in commit 4a10cb6ed9 on Apr 3, 2025

ryanofsky referenced this in commit 2be41ef85f on Apr 3, 2025

ryanofsky referenced this in commit eaac991552 on Apr 4, 2025

ryanofsky referenced this in commit c96e91eced on Apr 4, 2025

ryanofsky referenced this in commit a7266cfc9d on Apr 4, 2025

ryanofsky referenced this in commit e6f21513ed on Apr 4, 2025

ryanofsky force-pushed on Apr 4, 2025

ryanofsky referenced this in commit 2e4d0c3b51 on Apr 4, 2025

DrahtBot added the label CI failed on Apr 4, 2025

DrahtBot commented at 3:37 am on April 4, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/39937553048

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.

DrahtBot removed the label CI failed on Apr 4, 2025

DrahtBot added the label Needs rebase on Apr 10, 2025

ryanofsky referenced this in commit 8e3ee9d7e7 on Oct 15, 2025

ryanofsky referenced this in commit b4c6b16c37 on Oct 15, 2025

ryanofsky referenced this in commit 9e7dabc35d on Oct 15, 2025

ryanofsky referenced this in commit 9e250970d2 on Oct 15, 2025

ryanofsky referenced this in commit 351ebaebf5 on Oct 15, 2025

ryanofsky referenced this in commit 158f6a6562 on Oct 15, 2025

ryanofsky referenced this in commit 186b721e61 on Oct 15, 2025

ryanofsky force-pushed on Oct 15, 2025

DrahtBot removed the label Needs rebase on Oct 15, 2025

ryanofsky referenced this in commit a5700c7911 on Oct 16, 2025

ryanofsky referenced this in commit 9a4310c644 on Oct 16, 2025

ryanofsky referenced this in commit e4ca362858 on Oct 16, 2025

ryanofsky referenced this in commit 98e2cef781 on Oct 16, 2025

ryanofsky referenced this in commit ffa668313a on Oct 16, 2025

ryanofsky referenced this in commit df50712159 on Oct 16, 2025

ryanofsky force-pushed on Oct 16, 2025

ryanofsky referenced this in commit 47332ca19b on Oct 16, 2025

ryanofsky referenced this in commit 8a4587f0c3 on Oct 16, 2025

DrahtBot added the label Needs rebase on Oct 31, 2025

ryanofsky referenced this in commit 9883750ab0 on Nov 10, 2025

ryanofsky referenced this in commit 09517b75a9 on Nov 10, 2025

ryanofsky referenced this in commit 6bbf29c5f8 on Nov 10, 2025

ryanofsky referenced this in commit 702a1d8257 on Nov 10, 2025

ryanofsky force-pushed on Nov 10, 2025

ryanofsky referenced this in commit 0f3c2338d8 on Nov 10, 2025

ryanofsky referenced this in commit e84bd31c04 on Nov 10, 2025

ryanofsky referenced this in commit 3babe4a45b on Nov 10, 2025

ryanofsky referenced this in commit a992199e44 on Nov 10, 2025

DrahtBot removed the label Needs rebase on Nov 11, 2025

DrahtBot added the label Needs rebase on Dec 6, 2025

ryanofsky referenced this in commit 7c49fcfc21 on Dec 12, 2025

ryanofsky referenced this in commit e4918d23b0 on Dec 12, 2025

ryanofsky referenced this in commit 2d0bc18b9f on Dec 12, 2025

ryanofsky referenced this in commit 526272c1f0 on Dec 12, 2025

ryanofsky referenced this in commit 161a936b4a on Dec 12, 2025

ryanofsky referenced this in commit a5704ff85c on Dec 12, 2025

ryanofsky force-pushed on Dec 12, 2025

ryanofsky referenced this in commit d3422488aa on Dec 12, 2025

ryanofsky referenced this in commit 75042af205 on Dec 12, 2025

DrahtBot removed the label Needs rebase on Dec 12, 2025

DrahtBot added the label Needs rebase on Dec 14, 2025

ryanofsky referenced this in commit fdcf06d4bb on Dec 16, 2025

ryanofsky referenced this in commit 049b0f6fca on Dec 16, 2025

ryanofsky referenced this in commit 90476d0bb5 on Dec 16, 2025

ryanofsky referenced this in commit e393af85aa on Dec 16, 2025

ryanofsky referenced this in commit b7e59129cf on Dec 16, 2025

ryanofsky force-pushed on Dec 16, 2025

ryanofsky referenced this in commit 74bb399dfa on Dec 16, 2025

ryanofsky referenced this in commit 97dce72d90 on Dec 16, 2025

ryanofsky referenced this in commit 1c80687bc2 on Dec 16, 2025

DrahtBot removed the label Needs rebase on Dec 16, 2025

DrahtBot added the label Needs rebase on Dec 18, 2025

ryanofsky referenced this in commit 5d43d4a39c on Jan 18, 2026

ryanofsky referenced this in commit cb2538a9b7 on Jan 18, 2026

ryanofsky referenced this in commit ac3938a2f4 on Jan 18, 2026

ryanofsky referenced this in commit 1634b44feb on Jan 18, 2026

ryanofsky referenced this in commit 05d22deffd on Jan 18, 2026

ryanofsky referenced this in commit 38b0c982b5 on Jan 18, 2026

ryanofsky referenced this in commit bb05a1a220 on Jan 18, 2026

ryanofsky referenced this in commit bb3f964dba on Jan 18, 2026

ryanofsky force-pushed on Jan 18, 2026

ryanofsky referenced this in commit 4fb99c22c1 on Jan 18, 2026

ryanofsky referenced this in commit a08c9bc808 on Jan 18, 2026

DrahtBot removed the label Needs rebase on Jan 18, 2026

l0rinc commented at 4:51 pm on January 23, 2026: contributor

I wanted to push a simple scripted diff to rename WalletLogPrintf, to LogInfo, but noticed this pR already does that (in a more elaborate way). Is it a draft because it’s based on #29256 which should be reviewed first? That one has two nacks, can you make a version of this that simply renames the leftover LogPrintf reference or is that just a demonstration for the usefulness of log contexts?

ryanofsky commented at 5:48 pm on January 23, 2026: contributor

Would be curious about your scripted-diff. I don’t think we want to only replace WalletLogPrintf calls with LogInfo calls, because the point of WalletLogPrintf is to include wallet names in log messages, and it would bad to lose these, and awkward to have to specify them in every log call. I think this PR takes a nice approach of allowing Loginfo and other logging macros to work in wallet code including wallet names as part of the log context.

This PR is ready to review and has had some review, but is a draft because is based on another PR and should not be merged before that one. The other PR has nacks because originally it allowed LogInfo calls to accept category constants, and there was a debate about whether it was harmful to log category information, but I eventually gave in and removed it, so I don’t think the nacks are still relevant.

DrahtBot added the label Needs rebase on Feb 4, 2026

ryanofsky referenced this in commit 3a73b718e4 on Feb 9, 2026

ryanofsky referenced this in commit c76a2be4eb on Feb 9, 2026

ryanofsky referenced this in commit 2819b6d067 on Feb 9, 2026

ryanofsky referenced this in commit daeeb2042f on Feb 9, 2026

ryanofsky referenced this in commit 8714e10178 on Feb 9, 2026

ryanofsky referenced this in commit fd8d0a1039 on Feb 9, 2026

ryanofsky referenced this in commit 20f4088260 on Feb 9, 2026

ryanofsky referenced this in commit ac12a998ca on Feb 9, 2026

ryanofsky referenced this in commit f503957457 on Feb 9, 2026

ryanofsky referenced this in commit aac55f9493 on Feb 9, 2026

ryanofsky force-pushed on Feb 9, 2026

DrahtBot removed the label Needs rebase on Feb 9, 2026

in src/util/log.h:17 in aac55f9493

adyshimony commented at 2:57 am on March 1, 2026:

#include <type_traits>

ryanofsky commented at 5:31 pm on March 3, 2026:

re: #30343 (review)

#include <type_traits>

Thanks, added this!

in src/util/log.h:100 in aac55f9493 outdated

102+namespace detail {
103+//! Internal helper to get log source object from the first macro argument.
104+template <bool take_category, typename Source>
105+requires (Source::log_source)
106+const Source& GetSource(const Source& source LIFETIMEBOUND) { return source; }
107

adyshimony commented at 3:00 am on March 1, 2026:

Block rvalue Source in compile time by deleting the overload

0template <bool take_category, typename Source>
1requires (std::remove_cvref_t<Source>::log_source && !std::is_lvalue_reference_v<Source>)
2const std::remove_cvref_t<Source>& GetSource(Source&&) = delete;

ryanofsky commented at 5:43 pm on March 3, 2026:

re: #30343 (review)

Block rvalue Source in compile time by deleting the overload

Assuming this change is intended to address the temporary lifetime issue pointed out in your other comment, I think this could make sense. Replied to that comment directly, though here

adyshimony commented at 4:03 am on March 1, 2026: none

Reviewed and tested aac55f94938a.

I found an issue, at log.h LogPrint_ , 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 by deleting the overload. I failed to find any rvalue usage that exists today. Solution suggested in inline comments.

I used the following test to confirm the lifetime issue (for demo of the issue only; we don’t need it if we block rvalues at compile time):

 0struct LifetimeCheckedSource : public util::log::Source {
 1    LifetimeCheckedSource(int& counter, BCLog::Logger& logger, bool& alive)
 2        : Source{BCLog::VALIDATION, &logger}, m_counter{counter}, m_alive{alive}
 3    {
 4        m_alive = true;
 5    }
 6    ~LifetimeCheckedSource()
 7    {
 8        m_alive = false;
 9    }
10    template <typename... Args>
11    std::string Format(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args) const
12    {
13        CHECK_NONFATAL(m_alive);
14        return tfm::format("Custom #%d %s", ++m_counter, tfm::format(fmt, args...));
15    }
16    int& m_counter;
17    bool& m_alive;
18};
19
20BOOST_AUTO_TEST_CASE(logging_temporary_source_lifetime)
21{
22    BCLog::Logger logger;
23    logger.m_log_timestamps = false;
24    logger.EnableCategory(BCLog::LogFlags::ALL);
25    logger.SetLogLevel(BCLog::Level::Debug);
26    logger.StartLogging();
27
28    std::vector<std::string> messages;
29    logger.PushBackCallback([&](const std::string& s) { messages.push_back(s); });
30
31    int counter{0};
32    bool alive{false};
33    auto make_source = [&counter, &logger, &alive]() {
34        return LifetimeCheckedSource{counter, logger, alive};
35    };
36    LogDebug(make_source(), "temporary %s", "source");
37
38    BOOST_REQUIRE_EQUAL(messages.size(), 1U);
39    BOOST_CHECK_EQUAL(messages[0], "[validation] Custom [#1](/bitcoin-bitcoin/1/) temporary source\n");
40    BOOST_CHECK_EQUAL(counter, 1);
41}

Run build/bin/test_bitcoin –run_test=logging_tests/logging_temporary_source_lifetime to see it fail.

in src/wallet/wallet.cpp:3788 in aac55f9493

3784@@ -3782,7 +3785,7 @@ util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> CWallet::AddWall
3785 
3786     auto spk_man = GetDescriptorScriptPubKeyMan(desc);
3787     if (spk_man) {
3788-        WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString());
3789+        LogError(m_log, "Update existing descriptor: %s\n", desc.descriptor->ToString());

adyshimony commented at 4:07 am on March 1, 2026:

Isn’t that a happy flow? Maybe we can use LogInfo here?

ryanofsky commented at 5:34 pm on March 3, 2026:

re: #30343 (review)

Isn’t that a happy flow? Maybe we can use LogInfo here?

Makes sense, and now dropped this commit since the code has changed since it was written. (It looks like the original version of this commit 5a629a60c4739b94383596f9d040502b7b2c8b69 did have the same mistake.)

ryanofsky referenced this in commit 5f5ac95167 on Mar 2, 2026

ryanofsky referenced this in commit fab63e17b7 on Mar 2, 2026

ryanofsky referenced this in commit ee3fb8cf39 on Mar 2, 2026

ryanofsky referenced this in commit a2a2bd7898 on Mar 2, 2026

ryanofsky referenced this in commit 89223d52db on Mar 2, 2026

ryanofsky referenced this in commit ba41e5e0fd on Mar 2, 2026

ryanofsky referenced this in commit 7fc95e7cbb on Mar 2, 2026

ryanofsky force-pushed on Mar 2, 2026

adyshimony commented at 0:10 am on March 3, 2026: none

Reviewe and test 7fc95e7cbb3e after the rebase.

Concept ACK except this issue

ryanofsky commented at 6:00 pm on March 3, 2026: contributor

Thanks for the reviews @adyshimony! Note that most of your comments apply to #29256 which this is based on (#29256 is subset of this PR), so you may be interested in reviewing that directly. Some of your comments are now addressed in recent updates to both PRs.

re: #30343#pullrequestreview-3871171023

I found an issue, at log.h LogPrint_ , rvalue source arguments can be bound by reference and used after their lifetime ends.

I think this issue is already handled by the LIFETIMEBOUND annotation in GetSource().

When I build your test, the compiler shows warning: temporary bound to local reference '_source' will be destroyed at the end of the full-expression [-Wdangling] on line LogDebug(make_source(), "temporary %s", "source");.

So I think any code written in that unsafe style should be caught locally or by CI, and not be an issue. Your suggestion #30343 (review) to restrict GetSource() overloading so it can’t bind to lvalues could also make sense if LIFETIMEBOUND warning isn’t sufficient, but I feel like just using LIFETIMEBOUND avoids unnecessary complexity and provides a better diagnostic.

adyshimony commented at 11:54 am on March 4, 2026: none

Ohhh, I didn’t catch this earlier because I was building with GCC!

After switching to Clang, I can see it. This is Clang only flag, so reviewers using only GCC may miss it…

I still think the cleaner approach is to reject temporary sources directly at compile time by deleting the rvalue overload, so it clearly says “this usage is not allowed”.

Given the existing lvalue overload, the additional deleted overload can be simple:

0template <bool take_category, typename Source>
1requires (Source::log_source)
2const Source& GetSource(const Source&&) = delete;

In any case, with either approach, ACK on https://github.com/bitcoin/bitcoin/commit/7fc95e7cbb3ee3cae14df02602ac51a3d910945c.

ryanofsky referenced this in commit 8517c14b77 on Mar 4, 2026

ryanofsky referenced this in commit 6ebac258c5 on Mar 4, 2026

log test: verify log argument evaluation semantics

Test that LogInfo/LogWarning/LogError always evaluate their arguments
even when logging is disabled.

ajtowns pointed out this behavior was important and could affect non-logging
code if changed in
https://github.com/bitcoin/bitcoin/pull/34374#discussion_r2734793117

Co-authored-by: l0rinc <pap.lorinc@gmail.com>

449fc7ce3b

log test: Add test for all accepted logging arguments

Add new logging test to call macros with all allowed combinations of macro
arguments.

The new test replaces a less comprehensive test that doesn't cover log
statements without format arguments. It's also moved to the top of the test
suite because it's a good illustration of what typical log prints look like,
what logging calls are allowed and disallowed, and what the resulting log
output looks like.

1ee5ae0c17

log refactor: Ensure categories are not logged at info and higher levels

Previously this used to be possible through the LogPrintLevel call but now that
call is removed, this change is just an internal refactoring and has no visible
effect except in tests.

8f3e279bb9

log refactor: log macro rewrite

Rewrite log macros to fix a number of issues: lack of control over rate
limiting, unnecessary strprintf calls during fuzzing, confusing error messages
when macros are called with the wrong arguments.

More specifically, benefits are:

- Control over rate limiting: `LogPrintLevel_` hack used to bypass rate limiting
  is dropped. A `LogPrint` macro is added that can control rate limiting (as
  well as other options if they are added in the future).

- Unnecessary `strprintf` calls are now skipped in bitcoind when
  `-noprinttoconsole -nodebuglogfile` options are used, and skipped in tests and
  kernel applications when logging is disabled. This change should not affect
  bitcoind noticeably, but it could speed up fuzz tests calling functions that
  log.

- Clearer error messages: Differences between macros accepting and not accepting
  category arguments has been a source of confusion. Now if you pass a category
  to a macro which does not accept it, or forget to pass a category to a macro
  which requires it, you will see a clear error message telling you to add or
  remove the category instead of more confusing macro errors.

- Previously you could call `LogPrintLevel_` to bypass restrictions on passing
  category arguments. Now `LogPrint` enforces all requirements.

- Previously "always evaluate arguments" behavior at Info and higher levels
  looked accidental and was undocumented
  (https://github.com/bitcoin/bitcoin/pull/34374#discussion_r2734793117). Now
  the behavior is documented and explicit.

- There is now a single macro, `LogPrint` implementing all logging functionality
  and enforcing all restrictions, instead of different sets of macros with
  branching code paths and unexplained differences.

Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>

e983d97a91

Merge branch 'pr/relog' into pr/bclog 7055a641bc

log refactor: Allow log macros to accept context arguments

Allow LogDebug(), LogTrace(), LogInfo(), LogWarning(), and LogError() macros to
accept context arguments to provide more information in log messages and more
control over logging to callers.

This functionality is used in followup PRs:

- https://github.com/bitcoin/bitcoin/pull/30342 - To let libbitcoinkernel send
  output to specfic `BCLog::Logger` instances instead of a global instance, so
  output can be disambiguated and applications can have more control over
  logging.

- https://github.com/bitcoin/bitcoin/pull/30343 - To replace custom
  `WalletLogPrintf` calls with standard logging calls that automatically include
  wallet names and don't log everything at info level.

This commit does not change behavior of current log prints or require them to
be updated. It includes tests and documentation covering the new functionality.

Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>
Co-Authored-By: stickies-v <stickies-v@protonmail.com>

502820b5a5

doc: Add documentation about log levels and macros c410293dfa

log refactor: Add support for custom log contexts

Custom log contexts allow overridding log formatting and adding metadata, such
as request ids or wallet names to log messages, while still using standard
macros for logging. This is used to replace WalletLogPrintf() functions with
LogInfo() calls in followup PR #30343.

724b5aed21

Merge branch 'pr/bclog' into pr/gwlog 093df3b3bc

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

Make new logging macros LogDebug(), LogTrace(), LogInfo(), LogWarning(), and
LogError() compatible with wallet code and drop custom WalletLogPrintf()
function. 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. Fix this
by introducing a new WalletLogContext class inheriting from BCLog::Context
which can include the wallet name in log messages

6a572cd178

wallet, logging: Switch LogInfo to LogWarning/LogError

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors.

Suggested by hodlinator in
https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655406670
https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655407301
https://github.com/bitcoin/bitcoin/pull/30343#discussion_r1655436010

524c5b8ef5

ryanofsky referenced this in commit a71c30c23d on Mar 9, 2026

ryanofsky referenced this in commit c2d97a2313 on Mar 9, 2026

ryanofsky force-pushed on Mar 9, 2026

ryanofsky referenced this in commit 16de9cdc05 on Mar 9, 2026

DrahtBot added the label Needs rebase on Mar 11, 2026

DrahtBot commented at 2:00 pm on March 11, 2026: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)