wallet: Correct dir iteration error handling

hodlinator commented at 1:42 pm on June 12, 2025: contributor

Make wallet DB properly detect and report failure to scan wallet directory. Seems to have been broken since moving from Boost to std::filesystem.

Found while reviewing: #31410#pullrequestreview-2604068753

DrahtBot commented at 1:42 pm on June 12, 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/32736.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	maflcko

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31410 (qa: Fix wallet_multiwallet.py by hebasto)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)

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.

DrahtBot added the label Wallet on Jun 12, 2025

fanquake commented at 1:59 pm on June 12, 2025: member

https://cirrus-ci.com/task/6556243914915840?logs=ci#L4845:

0[09:50:58.842]                                    AssertionError: [node 0] Expected messages "['Error scanning directory entries under']" does not partially match log:

in test/functional/wallet_multiwallet.py:82 in ab65c62276 outdated

77@@ -78,6 +78,15 @@ def wallet_file(name):
78         self.stop_nodes()
79         assert_equal(os.path.isfile(wallet_dir(self.default_wallet_name, self.wallet_data_filename)), True)
80 
81+        self.log.info("Verify warning is emitted when failing to scan the wallets directory")
82+        os.chmod(data_dir('wallets'), 0)

maflcko commented at 2:13 pm on June 12, 2025:

I don’t think chmod works under root. So this test will likely fail in CI (or other places that use root) and pass locally (or other places that not use root).

hodlinator commented at 9:40 pm on June 12, 2025:

Fixed in latest push.

hodlinator marked this as a draft on Jun 12, 2025

DrahtBot added the label CI failed on Jun 12, 2025

hodlinator force-pushed on Jun 12, 2025

hodlinator marked this as ready for review on Jun 12, 2025

in src/wallet/db.cpp:36 in 6ed17c7dca outdated

32@@ -33,6 +33,7 @@ std::vector<std::pair<fs::path, std::string>> ListDatabases(const fs::path& wall
33             } else {
34                 LogPrintf("%s: %s %s\n", __func__, ec.message(), fs::PathToString(it->path()));
35             }
36+            ec.clear();

maflcko commented at 7:14 am on June 13, 2025:

is this code ever hit? At least corecheck shows it as dead code and according to the docs any error should always lead to the end iterator, which already breaks the loop, no?

https://en.cppreference.com/w/cpp/filesystem/recursive_directory_iterator.html says: “If the recursive_directory_iterator reports an error […], it becomes equal to the default-constructed iterator, also known as the end iterator.”

hodlinator commented at 12:19 pm on June 13, 2025:

Agreed, replaced block with assert(!ec). Seems to be leftovers from Boost filesystem. Updated PR title/description.

in src/wallet/db.cpp:79 in 6ed17c7dca outdated

74+        // Loop could have exited with an error due to one of:
75+        // * wallet_dir itself not being scannable.
76+        // * increment() failure. (Observed on Windows native builds when
77+        //   removing the ACL read permissions of a wallet directory after the
78+        //   process started).
79+        LogWarning("Error scanning directory entries under %s, loop probably aborted: %s", fs::PathToString(wallet_dir), ec.message());

maflcko commented at 7:15 am on June 13, 2025:

when changing the log here, might as well cherry-pick commit fa13b462ed6f5f7f1c5287bad70a443fd0e1f128 for all the other warning logs in this same file?

hodlinator commented at 12:19 pm on June 13, 2025:

Taken!

in test/functional/wallet_multiwallet.py:90 in 6ed17c7dca outdated

85+            self.log.warning('Skipping test involving chmod as it requires a non-root user.')
86+        else:
87+            os.chmod(data_dir('wallets'), 0)
88+            self.start_node(0)
89+            with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning directory entries under']):
90+                _ = self.nodes[0].listwalletdir()

maflcko commented at 7:19 am on June 13, 2025:

why discard the result. It should be expected to be empty, no?

nit: If you want to take an extra step, it could make sense to pass up the warnings into the result dict as "warnings": ["...",], instead of relying on the debug log

hodlinator commented at 12:21 pm on June 13, 2025:

Added negative case for before we call chmod, then added assertion of return values in both cases. Punting on “warnings” for now.

maflcko commented at 7:20 am on June 13, 2025: member

The code looks correct, matches the upstream docs, and passes the tests. I left some nits.

review ACK 6ed17c7dca35ec820dc1c0500ef32a132c641fa5 🤜

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK 6ed17c7dca35ec820dc1c0500ef32a132c641fa5 🤜
38C1jo8YtfwmG7YDw/SkVQcm33apByQ744hTZUTZRr1uCttPMiBfHDSd3YQ0UxkMDHOvAMpU1SIwUm5b4tAV8AQ==

DrahtBot removed the label CI failed on Jun 13, 2025

wallet: Correct dir iteration error handling

Seems to have been broken since conversion from Boost in #20744. The std::filesystem iteration aborts upon failure while Boost might have allowed skipping over faulty entries.

893e51ffeb

qa, wallet: Verify warning when failing to scan 1777644367

log: Use warning level while scanning wallet dir 272cd09b79

hodlinator force-pushed on Jun 13, 2025

hodlinator renamed this:
~~wallet: Warn upon failing to scan directory~~
wallet: Correct dir iteration error handling
on Jun 13, 2025

wallet: Correct dir iteration error handling #32736

Code Coverage & Benchmarks

Reviews

Conflicts