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

ryanofsky · 2024-06-26T17:57:15Z

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 #29256. The non-base commits are:

DrahtBot · 2024-06-26T17:57:18Z

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.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32149 (wallet, migration: Fix empty wallet crash by pablomartin4btc)
#32023 (wallet: removed duplicate call to GetDescriptorScriptPubKeyMan by saikiran57)
#31423 (wallet: migration, don't create spendable wallet from a watch-only legacy wallet by furszy)
#31398 (wallet: refactor: various master key encryption cleanups by theStack)
#31296 (wallet: Translate [default wallet] string in progress messages by ryanofsky)
#30221 (wallet: Ensure best block matches wallet scan state by achow101)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)
#29136 (wallet: addhdkey RPC to add just keys to wallets via new void(KEY) descriptor by achow101)
#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.

hodlinator

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

src/wallet/wallet.h

src/wallet/walletdb.cpp

src/wallet/scriptpubkeyman.h

hodlinator · 2024-06-26T19:48:56Z

src/wallet/wallet.cpp

@@ -3866,13 +3868,13 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
    AssertLockHeld(cs_wallet);

    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
-        WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n");
+        LogInfo(m_log, "Cannot add WalletDescriptor to a non-descriptor wallet\n");


Should maybe have been a warning all along?

re: #30343 (comment)

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.

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

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?

src/wallet/wallet.cpp

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

ryanofsky

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

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

src/wallet/scriptpubkeyman.h

src/wallet/wallet.cpp

ryanofsky · 2024-06-27T03:46:52Z

src/wallet/wallet.cpp

@@ -3866,13 +3868,13 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
    AssertLockHeld(cs_wallet);

    if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
-        WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n");
+        LogInfo(m_log, "Cannot add WalletDescriptor to a non-descriptor wallet\n");


re: #30343 (comment)

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.

src/wallet/wallet.h

src/wallet/walletdb.cpp

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

DrahtBot · 2024-06-27T04:35:00Z

🚧 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 · 2024-06-27T04:36:55Z

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

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

ryanofsky · 2024-06-28T14:27:13Z

Updated and rebased db8c6fc -> 29eaec2 (pr/gwlog.3 -> pr/gwlog.4, compare) with suggestion to use constexpr
Rebased 29eaec2 -> 0260cd5 (pr/gwlog.4 -> pr/gwlog.5, compare) due to conflict with #30406
Rebased 0260cd5 -> 0d49ada (pr/gwlog.5 -> pr/gwlog.6, compare) on top of updated base pr/bclog.20
Rebased 0d49ada -> f6b93a9 (pr/gwlog.6 -> pr/gwlog.7, compare) due to conflict with #30485
Rebased f6b93a9 -> 4c8fe04 (pr/gwlog.7 -> pr/gwlog.8, compare) due to conflicts with #26619, #30659, and other PRs
Rebased 4c8fe04 -> e8c6095 (pr/gwlog.8 -> pr/gwlog.9, compare) due to conflicts with #31061

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

Test will be extended in next commit to cover support for context objects.

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: - bitcoin#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. - bitcoin#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>

Needed to fix errors like: const Context &GetContext(const Context &)': expects 1 arguments - 3 provided due to a compiler bug: https://stackoverflow.com/questions/5134523/msvc-doesnt-expand-va-args-correctly/5134656#5134656 Example CI failure: https://github.com/bitcoin/bitcoin/actions/runs/8072891468/job/22055528830?pr=29256

Disallow passing BCLog category constants to LogInfo(), LogWarning(), and LogError() macros, and disallow omitting BCLog categories when calling LogDebug() and LogTrace() macros. These restrictions have existed since the logging macros were added in bitcoin#28318 and not technically neccessary, but are believed to be useful to prevent log spam and prevent users from filtering out important messages based on category. Co-Authored-By: Hodlinator <172445034+hodlinator@users.noreply.github.com>

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 bitcoin#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

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

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: - bitcoin#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. - bitcoin#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.

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

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: - bitcoin#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. - bitcoin#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.

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

DrahtBot · 2025-04-04T03:37:05Z

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

Hints

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 · 2025-04-10T01:25:56Z

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

DrahtBot · 2025-07-08T01:44:43Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2025-07-08T04:09:04Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

ryanofsky mentioned this pull request Jun 26, 2024

log, refactor: Allow log macros to accept context arguments #29256

Open

hodlinator reviewed Jun 26, 2024

View reviewed changes

DrahtBot mentioned this pull request Jun 27, 2024

wallet: Ensure best block matches wallet scan state #30221

Merged

ryanofsky force-pushed the pr/gwlog branch from 5b30f17 to 5a629a6 Compare June 27, 2024 03:49

ryanofsky commented Jun 27, 2024

View reviewed changes

ryanofsky force-pushed the pr/gwlog branch from 5a629a6 to db8c6fc Compare June 27, 2024 04:34

DrahtBot added the CI failed label Jun 27, 2024

DrahtBot removed the CI failed label Jun 27, 2024

This was referenced Jun 28, 2024

logging: Use LogFatal instead LogError for fatal errors #30347

Closed

Remove the legacy wallet and BDB dependency #28710

Merged

ryanofsky force-pushed the pr/gwlog branch from db8c6fc to 29eaec2 Compare June 28, 2024 14:26

DrahtBot removed the Needs rebase label Mar 12, 2025

This was referenced Mar 20, 2025

wallet: removed duplicate call to GetDescriptorScriptPubKeyMan #32023

Merged

wallet, migration: Fix empty wallet crash #32149

Merged

ryanofsky and others added 8 commits April 3, 2025 05:54

log, test: Add test for currently accepted logging arguments

f62272d

Test will be extended in next commit to cover support for context objects.

Merge remote-tracking branch 'origin/pull/29256/head'

e249f9d

wallet, logging: Switch LogInfo to LogWarning/LogError

e6f2151

Switch to LogWarning / LogError in obvious places where wallet code is clearly outputting warnings and errors. Suggested by hodlinator in bitcoin#30343 (comment) bitcoin#30343 (comment) bitcoin#30343 (comment)

wallet, logging: Switch LogInfo to LogError in AddWalletDescriptor

2e4d0c3

Seems like all of these cases are treated as errors, so it makes sense to use the corresponding log level. Change suggested by hodlinator in bitcoin#30343 (comment)

ryanofsky force-pushed the pr/gwlog branch from e8c6095 to af6f854 Compare April 3, 2025 17:31

ryanofsky force-pushed the pr/gwlog branch from af6f854 to 2e4d0c3 Compare April 4, 2025 03:37

DrahtBot added the CI failed label Apr 4, 2025

DrahtBot removed the CI failed label Apr 4, 2025

DrahtBot added the Needs rebase label Apr 10, 2025

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

ryanofsky commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

hodlinator left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hodlinator Jun 26, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky Jun 27, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jun 27, 2024

Uh oh!

ryanofsky commented Jun 27, 2024

Uh oh!

ryanofsky commented Jun 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 4, 2025

Uh oh!

DrahtBot commented Apr 10, 2025

Uh oh!

DrahtBot commented Jul 8, 2025

Uh oh!

DrahtBot commented Jul 8, 2025

Uh oh!

Uh oh!

ryanofsky commented Jun 26, 2024 •

edited

Loading

DrahtBot commented Jun 26, 2024 •

edited

Loading

ryanofsky Jun 27, 2024 •

edited

Loading

ryanofsky commented Jun 28, 2024 •

edited

Loading