Skip to content

wallet: Correct dir iteration error handling #32736

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
achow101 merged 3 commits into from
Jun 20, 2025
Merged

wallet: Correct dir iteration error handling #32736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
893e51f
wallet: Correct dir iteration error handling
hodlinator Jun 13, 2025
1777644
qa, wallet: Verify warning when failing to scan
hodlinator Jun 13, 2025
272cd09
log: Use warning level while scanning wallet dir
Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions src/wallet/db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)};

Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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};
Expand All @@ -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};
Expand Down
20 changes: 19 additions & 1 deletion test/functional/wallet_multiwallet.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.')
Comment on lines +84 to +85

@rkrux rkrux Jun 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice checking for the effective user ID, was the intention to add it as a safety check?

@maflcko maflcko Jun 20, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice checking for the effective user ID, was the intention to add it as a safety check?

no, the test would not pass when this check/skip is removed. See #32736 (comment)

@hodlinator hodlinator Jun 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To elaborate, we probably run tests under root on CI in some cases to enable tracing:

bitcoin/test/functional/test_framework/test_framework.py

Lines 971 to 975 in 272cd09

def skip_if_no_bpf_permissions(self):
"""Skip the running test if we don't have permissions to do BPF syscalls and load BPF maps."""
# check for 'root' permissions
if os.geteuid() != 0:
raise SkipTest("no permissions to use BPF (please review the tests carefully before running them with higher privileges)")

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)

@rkrux rkrux Jun 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit:

- os.chmod(data_dir('wallets'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)
+ os.chmod(data_dir('wallets'), stat.S_IRWXU)

Initially, I thought of fetching the stat modes from os.stat and using those to set them back in chmod here. But RWXU is the most common, so fine.

@hodlinator hodlinator Jun 22, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat! Maybe something that could be done in #31410 now this one merged?


# create symlink to verify wallet directory path can be referenced
# through symlink
os.mkdir(wallet_dir('w7'))
Expand Down Expand Up @@ -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
Expand Down
Loading