wallet: Correct dir iteration error handling #32736

hodlinator · 2025-06-12T13:42:34Z

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 (review)

DrahtBot · 2025-06-12T13:42:37Z

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
ACK	achow101, maflcko, rkrux

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.

fanquake · 2025-06-12T13:59:52Z

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

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

test/functional/wallet_multiwallet.py

maflcko

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

review ACK 6ed17c7dca35ec820dc1c0500ef32a132c641fa5 🤜

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 6ed17c7dca35ec820dc1c0500ef32a132c641fa5 🤜
8C1jo8YtfwmG7YDw/SkVQcm33apByQ744hTZUTZRr1uCttPMiBfHDSd3YQ0UxkMDHOvAMpU1SIwUm5b4tAV8AQ==

src/wallet/db.cpp

test/functional/wallet_multiwallet.py

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

achow101 · 2025-06-17T01:04:14Z

ACK 272cd09

maflcko · 2025-06-17T11:31:52Z

re-ACK 272cd09 🍽

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 272cd09b796a36596b325277bb43cb47b19c8e12 🍽
AEazdGZf0r70TCVJIeQ80JUBY7kWpOCzK+1Ox7cr7sGS7jMMX/Gob+wUeP8QofmQ6VtgMMTCg9yg/xM5jyhyBg==

rkrux

tACK 272cd09

rkrux · 2025-06-20T11:47:02Z

test/functional/wallet_multiwallet.py

+                assert_equal(result, {'wallets': []})
+            self.stop_node(0)
+            # Restore permissions
+            os.chmod(data_dir('wallets'), stat.S_IRUSR | stat.S_IWUSR | stat.S_IXUSR)


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.

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

rkrux · 2025-06-20T11:48:52Z

test/functional/wallet_multiwallet.py

+        elif os.geteuid() == 0:
+            self.log.warning('Skipping test involving chmod as it requires a non-root user.')


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

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)

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)")

DrahtBot added the Wallet label Jun 12, 2025

maflcko reviewed Jun 12, 2025

View reviewed changes

test/functional/wallet_multiwallet.py Outdated Show resolved Hide resolved

hodlinator marked this pull request as draft June 12, 2025 15:59

DrahtBot added the CI failed label Jun 12, 2025

hodlinator force-pushed the 2025/06/wallet_dir_iter branch from ab65c62 to 6ed17c7 Compare June 12, 2025 21:38

hodlinator marked this pull request as ready for review June 12, 2025 21:41

maflcko reviewed Jun 13, 2025

View reviewed changes

src/wallet/db.cpp Outdated Show resolved Hide resolved

src/wallet/db.cpp Outdated Show resolved Hide resolved

test/functional/wallet_multiwallet.py Outdated Show resolved Hide resolved

DrahtBot removed the CI failed label Jun 13, 2025

hodlinator and others added 3 commits June 13, 2025 13:41

wallet: Correct dir iteration error handling

893e51f

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

qa, wallet: Verify warning when failing to scan

1777644

log: Use warning level while scanning wallet dir

272cd09

hodlinator force-pushed the 2025/06/wallet_dir_iter branch from 6ed17c7 to 272cd09 Compare June 13, 2025 12:01

hodlinator changed the title ~~wallet: Warn upon failing to scan directory~~ wallet: Correct dir iteration error handling Jun 13, 2025

This was referenced Jun 14, 2025

qa: Fix wallet_multiwallet.py #31410

Closed

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

DrahtBot requested a review from maflcko June 17, 2025 01:04

maflcko requested a review from hebasto June 20, 2025 07:54

rkrux reviewed Jun 20, 2025

View reviewed changes

achow101 merged commit 482d255 into bitcoin:master Jun 20, 2025
19 checks passed

hodlinator deleted the 2025/06/wallet_dir_iter branch June 22, 2025 19:31

luke-jr mentioned this pull request Jun 25, 2025

Bugfix: Wallet: Skip inaccessible directories rather than abort the wallet list entirely #32812

Closed

		elif os.geteuid() == 0:
		self.log.warning('Skipping test involving chmod as it requires a non-root user.')

	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)")

wallet: Correct dir iteration error handling #32736

wallet: Correct dir iteration error handling #32736

Uh oh!

Conversation

hodlinator commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

fanquake commented Jun 12, 2025

Uh oh!

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jun 17, 2025

Uh oh!

maflcko commented Jun 17, 2025

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

hodlinator Jun 22, 2025

Choose a reason for hiding this comment

Uh oh!

rkrux Jun 20, 2025

Choose a reason for hiding this comment

Uh oh!

maflcko Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hodlinator Jun 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hodlinator commented Jun 12, 2025 •

edited

Loading

DrahtBot commented Jun 12, 2025 •

edited

Loading

maflcko Jun 20, 2025 •

edited

Loading