wallet: reduce unconditional logging during load #33299

pull mzumsande wants to merge 1 commits into bitcoin:master from mzumsande:2025_wallet_log_less changing 4 files +11 −3
  1. mzumsande commented at 8:09 pm on September 3, 2025: contributor

    Currently the unconditional log during init with a default wallet happens three times:

    02025-09-03T19:57:16Z init message: Verifying wallet(s)…
12025-09-03T19:57:16Z Using SQLite Version 3.45.1
22025-09-03T19:57:16Z Using wallet XXX/.bitcoin/regtest
32025-09-03T19:57:16Z Using SQLite Version 3.45.1
42025-09-03T19:57:16Z Using wallet XXX/.bitcoin/regtest
5(...)
62025-09-03T19:57:16Z Using SQLite Version 3.45.1
72025-09-03T19:57:16Z Using wallet XXX/.bitcoin/regtest
82025-09-03T19:57:16Z init message: Loading wallet…

    For non-default wallets it’s logged two times.

    That seems a bit too much, so just log the SQLite version just one, and remove the log for the full path of the wallet, since it’s already clear from other logs which wallet is being loaded.

  2. DrahtBot added the label Wallet on Sep 3, 2025
  3. DrahtBot commented at 8:09 pm on September 3, 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/33299.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, stickies-v, achow101
    Concept ACK pablomartin4btc, l0rinc

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

  4. stickies-v commented at 2:19 pm on September 5, 2025: contributor

    Concept ACK, but I think unconditional logging in the ctor is not the right approach, suggested alternative:

     0diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
 1index 79e2c9688d..11d7c9029d 100644
 2--- a/src/wallet/init.cpp
 3+++ b/src/wallet/init.cpp
 4@@ -19,6 +19,7 @@
 5 #include <util/moneystr.h>
 6 #include <util/translation.h>
 7 #include <wallet/coincontrol.h>
 8+#include <wallet/sqlite.h>
 9 #include <wallet/wallet.h>
10 #include <walletinitinterface.h>
11 
12@@ -108,6 +109,7 @@ void WalletInit::Construct(NodeContext& node) const
13         LogInfo("Wallet disabled!");
14         return;
15     }
16+    LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
17     auto wallet_loader = node.init->makeWalletLoader(*node.chain);
18     node.wallet_loader = wallet_loader.get();
19     node.chain_clients.emplace_back(std::move(wallet_loader));
20diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
21index 60561b1572..c9c7780893 100644
22--- a/src/wallet/load.cpp
23+++ b/src/wallet/load.cpp
24@@ -85,6 +85,7 @@ bool VerifyWallets(WalletContext& context)
25         const auto& wallet_file = wallet.get_str();
26         const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet_file));
27 
28+        LogInfo("Verifying wallet %s", fs::PathToString(path));
29         if (!wallet_paths.insert(path).second) {
30             chain.initWarning(strprintf(_("Ignoring duplicate -wallet %s."), wallet_file));
31             continue;
32diff --git a/src/wallet/sqlite.cpp b/src/wallet/sqlite.cpp
33index 0940cbac3b..2e4b9abc1c 100644
34--- a/src/wallet/sqlite.cpp
35+++ b/src/wallet/sqlite.cpp
36@@ -116,8 +116,6 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
37 {
38     {
39         LOCK(g_sqlite_mutex);
40-        LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
41-        LogInfo("Using wallet %s", fs::PathToString(m_dir_path));
42 
43         if (++g_sqlite_count == 1) {
44             // Setup logging
  5. pablomartin4btc commented at 2:14 pm on September 6, 2025: member

    Concept ACK

    02025-09-06T04:01:48Z init message: Verifying wallet(s)…
12025-09-06T04:01:48Z Using SQLite Version 3.37.2
22025-09-06T04:01:48Z Using wallet /tmp/btc/regtest/wallets
32025-09-06T04:01:48Z Using /16 prefix for IP bucketing
4...
52025-09-06T04:01:48Z Using SQLite Version 3.37.2
62025-09-06T04:01:48Z Using wallet /tmp/btc/regtest/wallets
72025-09-06T04:01:48Z init message: Loading wallet…
82025-09-06T04:01:48Z [default wallet] Last client version = 299900

    I’d prefer the approach proposed by @stickies-v, which I’ve tested as I did with this PR’s fix, it seems a better place to move the loggings and as a result leaves the code cleaner.

    Another suggestion, I know it’s not part of the scope of this PR but since you are there in sqlite.cpp, if you want you could add a separate commit to replace the LogPrintf() calls with LogInfo(), something that has been done in different PRs (#32604, #29641) and it would make the code more consistent.

  6. mzumsande commented at 7:50 pm on September 9, 2025: contributor

    Concept ACK, but I think unconditional logging in the ctor is not the right approach, suggested alternative:

    I like the move of the Sqlite log - it seems suficient to log the sqlite version just once instead for each wallet load. But the suggested spot fails the linter (“Error: init.cpp.o depends on sqlite.cpp.o symbol”) so I kept in sqlite.cpp.

    In the latest push I just removed the second log:

    I doubt that logging the full path in the success case adds much, while creating anonymization efforts in case of sharing logs publicly. The wallet name is already being logged multiple times:

    02025-09-09T19:35:20Z init message: Loading wallet…
12025-09-09T19:35:20Z [w1] Last client version = 299900
22025-09-09T19:35:20Z [w1] Descriptors: 8, Descriptor Keys: 1 plaintext, 0 encrypted, 1 total.
32025-09-09T19:35:20Z [w1] Setting spkMan to active: id = b5d7ca2c5950616dc0ce4c21f1f1590da9858efe2a8ab00e2b10fa2786c3fe9f, type = legacy, internal = false
4(...)

    (but I only see the default wallet twice as any named wallets)

    I meant the case where -wallet is not specified and a default wallet exists, see https://github.com/bitcoin/bitcoin/blob/c0894a0a2be032cd9a5d5945643689230ab10255/src/wallet/load.cpp#L66C13-L66C31

    Another suggestion, I know it’s not part of the scope of this PR (…)

    I think I’d prefer to not do that here, maybe this can be done in one sweep for the entire wallet or so.

  7. mzumsande force-pushed on Sep 9, 2025
  8. mzumsande force-pushed on Sep 9, 2025
  9. mzumsande force-pushed on Sep 9, 2025
  10. mzumsande force-pushed on Sep 9, 2025
  11. in src/wallet/sqlite.cpp:135 in 862faf3fa7 
    131@@ -135,6 +132,9 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
132         if (ret != SQLITE_OK) {
133             throw std::runtime_error(strprintf("SQLiteDatabase: Failed to initialize SQLite: %s\n", sqlite3_errstr(ret)));
134         }
135+        if (g_sqlite_count == 1) {
    achow101 commented at 6:40 pm on September 17, 2025:
    There’s already a similar count == 1 check above for first time initialization that this log line can be moved into.
    mzumsande commented at 6:57 pm on September 17, 2025:
    I did that at first but changed it on purpose - in case the result of SQLiteDatabaseVersion() depended on sqlite3_initialize (which it doesn’t currently, but doesn’t seem completely unreasonable) we’d log the correct thing here too.
  12. achow101 commented at 7:54 pm on September 17, 2025: member
    ACK 862faf3fa7a42238017f8d673b0c656531c688dc
  13. DrahtBot requested review from pablomartin4btc on Sep 17, 2025
  14. DrahtBot requested review from stickies-v on Sep 17, 2025
  15. luke-jr referenced this in commit 76165e94c6 on Sep 19, 2025
  16. pablomartin4btc commented at 3:00 am on September 21, 2025: member

    tested 862faf3fa7a42238017f8d673b0c656531c688dc

     0...
 12025-09-19T21:11:13Z Using wallet directory /tmp/btc/regtest/wallets
 22025-09-19T21:11:13Z init message: Verifying wallet(s)…
 32025-09-19T21:11:13Z Using SQLite Version 3.37.2
 42025-09-19T21:11:13Z Using SQLite Version 3.37.2
 5...
 62025-09-19T21:11:13Z init message: Verifying blocks…
 72025-09-19T21:11:13Z Block index and chainstate loaded
 82025-09-19T21:11:13Z Using SQLite Version 3.37.2
 92025-09-19T21:11:13Z init message: Loading wallet…
102025-09-19T21:11:13Z [default wallet] Last client version = 309900
11...
  17. DrahtBot requested review from pablomartin4btc on Sep 21, 2025
  18. l0rinc commented at 3:05 am on September 21, 2025: contributor
    Concept ACK
  19. in src/wallet/sqlite.cpp:137 in 862faf3fa7 
    131@@ -135,6 +132,9 @@ SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_pa
132         if (ret != SQLITE_OK) {
133             throw std::runtime_error(strprintf("SQLiteDatabase: Failed to initialize SQLite: %s\n", sqlite3_errstr(ret)));
134         }
135+        if (g_sqlite_count == 1) {
136+            LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
137+        }
    pablomartin4btc commented at 3:15 am on September 21, 2025:

    what about (and no longer need to check g_sqlite_count)…

    0        static bool sqlite_version_logged = false;
1        if (!sqlite_version_logged) {
2            LogInfo("Using SQLite Version %s", SQLiteDatabaseVersion());
3            sqlite_version_logged = true;
4        }
    mzumsande commented at 5:22 pm on September 22, 2025:
    thanks - I took that.
  20. DrahtBot requested review from pablomartin4btc on Sep 21, 2025
  21. mzumsande force-pushed on Sep 22, 2025
  22. mzumsande commented at 5:23 pm on September 22, 2025: contributor

    This version doesn’t work properly and produces the same issue it’s trying to fix

    Good catch, must have missed this during testing! Changed to your suggestion with a static.

  23. mzumsande commented at 5:49 pm on September 22, 2025: contributor

    This version doesn’t work properly and produces the same issue it’s trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does –g_sqlite_count.

    Logging aside, I wonder if it’s useful and intended to do all the existing setup including sqlite3_initialize(), followed by sqlite3_shutdown() multiple times during init for each wallet checked during VerifyWallets()? Seems like the intention of g_sqlite_count is to do it only once?!

  24. furszy commented at 6:26 pm on September 22, 2025: member

    This version doesn’t work properly and produces the same issue it’s trying to fix as every call to MakeWalletDatabase() within conditionals in VerifyWallets() (src/wallet/load.cpp), ends up also calling the SQLiteDatabase::Cleanup() in the destructor which does –g_sqlite_count.

    Logging aside, I wonder if it’s useful and intended to do all the existing setup including sqlite3_initialize(), followed by sqlite3_shutdown() multiple times during init for each wallet checked during VerifyWallets()? Seems like the intention of g_sqlite_count is to do it only once?!

    Not entirely. In the absence of a DB manager, they were placed in the constructor so we can reallocate resources as needed, and we make sure everything is cleaned up when the last db object is destroyed. See the sqlite db destructor. This implementation did not account for the initial verification step; it only considered multiple wallets being loaded simultaneously and remaining active for the entire app lifecycle.

  25. pablomartin4btc commented at 6:31 pm on September 22, 2025: member

    Logging aside, I wonder if it’s useful and intended to do all the existing setup including sqlite3_initialize(), followed by sqlite3_shutdown() multiple times during init for each wallet checked during VerifyWallets()? Seems like the intention of g_sqlite_count is to do it only once?!

    So, we have one sqlite (/ walletdb/ count) obj instance per wallet (/ wallet file), so during init we check first if there’s a default wallet and then also check the wallets in the settings config (what verify does in verify wallets)… we could refactor verify (step 5 in init.cpp) and the load (step 9) together so if the db/ file is opened we keep it… but if there are any that can’t be opened that would destroy the instance and decrease the counting. I’d need to investigate if the init steps in between depends on anything that need to be separated (verify first but not keep the wallets opened - step 5 - and load them later - step 9).

    Also going back to the approach from @stickies-v I can see now that the dbversion method should be polymorphic with all formats (eg BerkeleyRO), even we only open descriptors at the moment during migration we open the old legacy for read-only purposes.

  26. mzumsande commented at 11:28 pm on September 22, 2025: contributor
    After having talking with @furszy about this, I’ll push a slightly different approach soon - will put it in draft until then.
  27. mzumsande marked this as a draft on Sep 22, 2025
  28. wallet, log: reduce unconditional logging during load 
    The removed statements were logged up to two or three times for each loaded
wallet. The SQLite version only needs to be logged once.
The full wallet path is dropped, since the existing unconditional
logging while loading wallets is sufficient (also reduces anonymization
efforts in case of sharing logs).

Co-authored-by: Martin Zumsande <mzumsande@gmail.com>
    fc861332b3
  29. mzumsande force-pushed on Sep 29, 2025
  30. mzumsande commented at 6:15 pm on September 29, 2025: contributor

    Changed to approach by @furszy which removes the logging from the sqlite ctor similar as suggested by stickies-v, and should also pass the linter.

    I’m only trying to remove the repeated logging here, no idea if the VerifyWallets() / g_sqlite_count interactions that the discussion here revealed are worth fixing - something for another PR or Issue perhaps.

  31. mzumsande marked this as ready for review on Sep 29, 2025
  32. furszy commented at 6:40 pm on September 29, 2025: member
    utACK fc861332b351c9390400054ff74193ce26eb0713
  33. DrahtBot requested review from l0rinc on Sep 29, 2025
  34. DrahtBot requested review from achow101 on Sep 29, 2025
  35. in src/wallet/load.cpp:1 in fc861332b3
    pablomartin4btc commented at 3:26 am on September 30, 2025:
    nit: if you re-touch, same as walletdb.cpp, copyright dates needs to be updated…
    pablomartin4btc commented at 3:27 am on September 30, 2025:
    nit: if you re-touch… please update the copyright (also to match its header walletdb.h) // Copyright (c) 2009-present The Bitcoin Core developers
  36. pablomartin4btc commented at 3:53 am on September 30, 2025: member

    Not sure about this, if there are no descriptor wallets during startup (no default, no one in settings), we would be logging the sqlite version but if the user had legacy wallets in settings, that would print a warning (FAILED_LEGACY_DISABLED -> initWarning) not showing the berkeley version of the DB instantiated (which code is not in master either but it doesn’t look consistent). If we have a diff DB format in the future we’d need to change this LogDBInfo() (once someone finds it).

    I’d prefer first the previous approach or as an alternative move the logic to MakeDatabase() in src/wallet/walletdb.cpp: https://github.com/bitcoin/bitcoin/blob/d8fe258cd6105704bf4427eda048dd7085ca516d/src/wallet/walletdb.cpp#L1367-L1373

  37. DrahtBot requested review from pablomartin4btc on Sep 30, 2025
  38. in src/wallet/load.cpp:54 in fc861332b3 
    50@@ -51,6 +51,8 @@ bool VerifyWallets(WalletContext& context)
51     }
52 
53     LogInfo("Using wallet directory %s", fs::PathToString(GetWalletDir()));
54+    // Print general DB information
    stickies-v commented at 10:58 am on September 30, 2025:
    nit: the function name is informative enough imo, no need for docstring here
  39. in src/wallet/walletdb.h:30 in fc861332b3 
    26@@ -27,6 +27,9 @@ class CWallet;
27 class CWalletTx;
28 struct WalletContext;
29 
30+// Logs information about the database, including available engines, features, and other capabilities
    stickies-v commented at 11:02 am on September 30, 2025:

    nit: docstring doesn’t correspond to what’s actually being printed. To avoid it going stale quickly, how about a simple:

    // Logs information about the database(s) used by the wallet.

  40. stickies-v approved
  41. stickies-v commented at 11:15 am on September 30, 2025: contributor

    ACK fc861332b351c9390400054ff74193ce26eb0713

    nit: Would still prefer doing the logging from WalletInit::Construct, VerifyWallets feels out of place to me, but no biggie.

     0diff --git a/src/wallet/init.cpp b/src/wallet/init.cpp
 1index 79e2c9688d..4013e12967 100644
 2--- a/src/wallet/init.cpp
 3+++ b/src/wallet/init.cpp
 4@@ -20,6 +20,7 @@
 5 #include <util/translation.h>
 6 #include <wallet/coincontrol.h>
 7 #include <wallet/wallet.h>
 8+#include <wallet/walletdb.h>
 9 #include <walletinitinterface.h>
10 
11 using node::NodeContext;
12@@ -108,6 +109,7 @@ void WalletInit::Construct(NodeContext& node) const
13         LogInfo("Wallet disabled!");
14         return;
15     }
16+    LogDBInfo();
17     auto wallet_loader = node.init->makeWalletLoader(*node.chain);
18     node.wallet_loader = wallet_loader.get();
19     node.chain_clients.emplace_back(std::move(wallet_loader));
20diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
21index f362064473..60561b1572 100644
22--- a/src/wallet/load.cpp
23+++ b/src/wallet/load.cpp
24@@ -51,8 +51,6 @@ bool VerifyWallets(WalletContext& context)
25     }
26 
27     LogInfo("Using wallet directory %s", fs::PathToString(GetWalletDir()));
28-    // Print general DB information
29-    LogDBInfo();
30 
31     chain.initMessage(_("Verifying wallet(s)…"));
32
  42. furszy commented at 1:59 pm on September 30, 2025: member

    If we have a diff DB format in the future we’d need to change this LogDBInfo() (once someone finds it).

    That’s the idea. Have a single place where we log all possible DB engines and configurations. The DB engine and its configuration are not specific to any wallet; they are part of the general setup.

    I’d prefer first the previous approach or as an alternative move the logic to MakeDatabase() in src/wallet/walletdb.cpp

    That would print the same message multiple times. The goal is to print it only once during init so we know the available DB engine version.

  43. pablomartin4btc commented at 2:31 pm on September 30, 2025: member

    That’s the idea. Have a single place where we log all possible DB engines and configurations.

    At the moment, we use Berkeley DB when we want to migrate a Legacy wallet, the above statement maybe should consider this case.

    That would print the same message multiple times. The goal is to print it only once during init so we know the available DB engine version.

    Nop, previous one was 5e130a1e999b8a15d709cb698814c26a85eb5447, but I’m ok to centralize the DB engines logging in one place, still not sure if would be VerifyWallets or WalletInit::Construct.

  44. furszy commented at 2:34 pm on September 30, 2025: member

    At the moment, we use Berkeley DB when we want to migrate a Legacy wallet, the above statement maybe should consider this case.

    You can’t initialize a BDB wallet during startup. The BDB engine is not part of the binary anymore. We just have a BDB reader that parses the db file on-demand during migration.

    Nop, previous one was https://github.com/bitcoin/bitcoin/commit/5e130a1e999b8a15d709cb698814c26a85eb5447, but I’m ok to centralize the DB engines logging in one place, still not sure if would be VerifyWallets or WalletInit::Construct.

    The goal is to not introduce a global static boolean.

  45. achow101 commented at 5:42 pm on September 30, 2025: member
    ACK fc861332b351c9390400054ff74193ce26eb0713
  46. achow101 merged this on Sep 30, 2025
  47. achow101 closed this on Sep 30, 2025
  48. mzumsande deleted the branch on Sep 30, 2025


mzumsande DrahtBot stickies-v pablomartin4btc achow101 l0rinc furszy


l0rinc achow101

Labels
Wallet

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-10-10 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me