From 893e51ffeb0543e1c8d33e83b20c56f02d2b793c Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Fri, 13 Jun 2025 13:41:59 +0200 Subject: [PATCH 1/3] 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. --- src/wallet/db.cpp | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 9de0572947f..40692997d20 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -26,16 +26,7 @@ std::vector> ListDatabases(const fs::path& wall std::error_code ec; for (auto it = fs::recursive_directory_iterator(wallet_dir, ec); it != fs::recursive_directory_iterator(); it.increment(ec)) { - if (ec) { - if (fs::is_directory(*it)) { - it.disable_recursion_pending(); - LogPrintf("%s: %s %s -- skipping.\n", __func__, ec.message(), fs::PathToString(it->path())); - } else { - LogPrintf("%s: %s %s\n", __func__, ec.message(), fs::PathToString(it->path())); - } - continue; - } - + assert(!ec); // Loop should exit on error. try { const fs::path path{it->path().lexically_relative(wallet_dir)}; @@ -69,6 +60,14 @@ std::vector> ListDatabases(const fs::path& wall it.disable_recursion_pending(); } } + if (ec) { + // Loop could have exited with an error due to one of: + // * wallet_dir itself not being scannable. + // * increment() failure. (Observed on Windows native builds when + // removing the ACL read permissions of a wallet directory after the + // process started). + LogWarning("Error scanning directory entries under %s: %s", fs::PathToString(wallet_dir), ec.message()); + } return paths; } From 17776443675ddf804f92042883ad36ed040438c3 Mon Sep 17 00:00:00 2001 From: Hodlinator <172445034+hodlinator@users.noreply.github.com> Date: Fri, 13 Jun 2025 13:55:54 +0200 Subject: [PATCH 2/3] qa, wallet: Verify warning when failing to scan --- test/functional/wallet_multiwallet.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 87c8f627552..2c87fd5711d 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -78,6 +78,24 @@ class MultiWalletTest(BitcoinTestFramework): self.stop_nodes() assert_equal(os.path.isfile(wallet_dir(self.default_wallet_name, self.wallet_data_filename)), True) + self.log.info("Verify warning is emitted when failing to scan the wallets directory") + if platform.system() == 'Windows': + self.log.warning('Skipping test involving chmod as Windows does not support it.') + elif os.geteuid() == 0: + self.log.warning('Skipping test involving chmod as it requires a non-root user.') + else: + self.start_node(0) + with self.nodes[0].assert_debug_log(unexpected_msgs=['Error scanning directory entries under'], expected_msgs=[]): + result = self.nodes[0].listwalletdir() + assert_equal(result, {'wallets': [{'name': 'default_wallet', 'warnings': []}]}) + os.chmod(data_dir('wallets'), 0) + with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning directory entries under']): + result = self.nodes[0].listwalletdir() + assert_equal(result, {'wallets': []}) + self.stop_node(0) + # Restore permissions + os.chmod(data_dir('wallets'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR) + # create symlink to verify wallet directory path can be referenced # through symlink os.mkdir(wallet_dir('w7')) From 272cd09b796a36596b325277bb43cb47b19c8e12 Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 10 Jun 2025 15:57:33 +0200 Subject: [PATCH 3/3] log: Use warning level while scanning wallet dir --- src/wallet/db.cpp | 6 +++--- test/functional/wallet_multiwallet.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/wallet/db.cpp b/src/wallet/db.cpp index 40692997d20..781b6fa461d 100644 --- a/src/wallet/db.cpp +++ b/src/wallet/db.cpp @@ -56,7 +56,7 @@ std::vector> ListDatabases(const fs::path& wall } } } catch (const std::exception& e) { - LogPrintf("%s: Error scanning %s: %s\n", __func__, fs::PathToString(it->path()), e.what()); + LogWarning("Error while scanning wallet dir item: %s [%s].", e.what(), fs::PathToString(it->path())); it.disable_recursion_pending(); } } @@ -99,7 +99,7 @@ bool IsBDBFile(const fs::path& path) // This check also prevents opening lock files. std::error_code ec; auto size = fs::file_size(path, ec); - if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), fs::PathToString(path)); + if (ec) LogWarning("Error reading file_size: %s [%s]", ec.message(), fs::PathToString(path)); if (size < 4096) return false; std::ifstream file{path, std::ios::binary}; @@ -123,7 +123,7 @@ bool IsSQLiteFile(const fs::path& path) // A SQLite Database file is at least 512 bytes. std::error_code ec; auto size = fs::file_size(path, ec); - if (ec) LogPrintf("%s: %s %s\n", __func__, ec.message(), fs::PathToString(path)); + if (ec) LogWarning("Error reading file_size: %s [%s]", ec.message(), fs::PathToString(path)); if (size < 512) return false; std::ifstream file{path, std::ios::binary}; diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index 2c87fd5711d..a7b61e93280 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -147,7 +147,7 @@ class MultiWalletTest(BitcoinTestFramework): os.mkdir(wallet_dir('no_access')) os.chmod(wallet_dir('no_access'), 0) try: - with self.nodes[0].assert_debug_log(expected_msgs=['Error scanning']): + with self.nodes[0].assert_debug_log(expected_msgs=["Error while scanning wallet dir"]): walletlist = self.nodes[0].listwalletdir()['wallets'] finally: # Need to ensure access is restored for cleanup