Merge bitcoin/bitcoin#32736: wallet: Correct dir iteration error handling

272cd09 log: Use warning level while scanning wallet dir (MarcoFalke)
1777644 qa, wallet: Verify warning when failing to scan (Hodlinator)
893e51f wallet: Correct dir iteration error handling (Hodlinator)

Pull request description:

  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: bitcoin/bitcoin#31410 (review)

ACKs for top commit:
  achow101:
    ACK 272cd09
  maflcko:
    re-ACK 272cd09 🍽
  rkrux:
    tACK 272cd09

Tree-SHA512: 969afde2e37f885ed0c823dc36d2dbeaa0378639849c6a26f8ac67b4f1997eea95bbcae6d58aef5b716807210f37eb166c0cda7ba1d6caffd34249970833af3a

Author: achow101

src/wallet/db.cpp

@@ -26,16 +26,7 @@ std::vector<std::pair<fs::path, std::string>> 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)};
 
@@ -65,10 +56,18 @@ std::vector<std::pair<fs::path, std::string>> 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();
         }
     }
+    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;
 }
@@ -100,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};
@@ -124,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};

test/functional/wallet_multiwallet.py

@@ -78,6 +78,24 @@ def wallet_file(name):
         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'))
@@ -129,7 +147,7 @@ def wallet_file(name):
         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