[29.x] Fix #34222 backport bugs

luke-jr commented at 3:47 pm on January 21, 2026: member

DrahtBot added the label Backport on Jan 21, 2026

DrahtBot commented at 3:47 pm on January 21, 2026: 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/34370.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

is expected -> are expected [Subject “Subdirectories” is plural, so the verb should be plural (“are expected”) to be grammatically correct.]

2026-01-23 13:19:29

achow101 commented at 6:08 pm on January 21, 2026: member

What, exactly, are the bugs that this is fixing? Can you add tests for them?

in src/wallet/bdb.h:151 in db549e7a7d

153+            };
154+            bool clean_dir{true};
155+            const auto& env_dir = env->Directory();
156+            for (const auto& entry : fs::recursive_directory_iterator(env_dir)) {
157+                const auto& path = entry.path().lexically_proximate(env_dir);
158+                if (allowed_paths.contains(path) || fs::PathToString(path).starts_with("database/log.")) {

achow101 commented at 6:46 pm on January 21, 2026:

This prevents cleanup of database/ if you createfromdump an unnamed wallet but have other wallets in directories only.

luke-jr commented at 8:17 pm on January 21, 2026:

Fixed

luke-jr commented at 8:18 pm on January 21, 2026: member

What, exactly, are the bugs that this is fixing?

Documented in each commit description.

Can you add tests for them?

I think it would conflict with #34372, so better to add them after that lands. (I will start working on them locally for now, and add here if it makes sense)

luke-jr force-pushed on Jan 21, 2026

luke-jr commented at 9:50 pm on January 21, 2026: member

Added a test for the first fix (leaving database files intact when db env is shared).

Second (correct wallet name in post-migration reload error message) is in theory untestable since any way to trigger it would be a bug (known cases would be caught on the empty non-watchonly wallet in 29.x first)

Third (avoiding fs::rename) would AFAIK require some kind of filesystem mount, so either root or FUSE. It doesn’t seem worth the effort.

Fourth (putting the backup in the wallet’s own dir) should be tested by #34372 (“QA: wallet_migration: Test migration of wallets without a dedicated directory”) already

luke-jr force-pushed on Jan 21, 2026

in src/wallet/bdb.cpp:355 in 6a3926aea3

350+
351+        const auto env_dir = env->Directory();
352+        const auto db_subdir = env_dir / "database";
353+        if (fs::exists(db_subdir)) {
354+            if (!fs::is_directory(db_subdir)) return false;
355+            for (const auto& entry : fs::directory_iterator(env_dir / "database")) {

achow101 commented at 11:24 pm on January 21, 2026:

In 6a3926aea371bf9217031ccbf79032ccba7f3cf8 “Wallet/bdb: Safely and correctly list files only used by the single wallet”

0            for (const auto& entry : fs::directory_iterator(db_subdir)) {

luke-jr commented at 1:19 pm on January 23, 2026:

done

in src/wallet/bdb.cpp:356 in 6a3926aea3

351+        const auto env_dir = env->Directory();
352+        const auto db_subdir = env_dir / "database";
353+        if (fs::exists(db_subdir)) {
354+            if (!fs::is_directory(db_subdir)) return false;
355+            for (const auto& entry : fs::directory_iterator(env_dir / "database")) {
356+                const auto& path = entry.path().lexically_proximate(db_subdir);

achow101 commented at 11:26 pm on January 21, 2026:

In 6a3926aea371bf9217031ccbf79032ccba7f3cf8 “Wallet/bdb: Safely and correctly list files only used by the single wallet”

Could use filename() as that’s what’s being retrieved here.

0                const auto& filename = entry.path().filename();

luke-jr commented at 1:19 pm on January 23, 2026:

done

in src/wallet/bdb.cpp:370 in 6a3926aea3

365+            "db.log",
366+            ".walletlock",
367+            "database"
368+        };
369+        for (const auto& entry : fs::directory_iterator(env_dir)) {
370+            const auto& path = entry.path().lexically_proximate(env_dir);

achow101 commented at 11:28 pm on January 21, 2026:

In 6a3926aea371bf9217031ccbf79032ccba7f3cf8 “Wallet/bdb: Safely and correctly list files only used by the single wallet”

Could use use filename() as that’s what’s being retrieved here.

0            const auto& filename = entry.path().filename();

luke-jr commented at 1:19 pm on January 23, 2026:

done

achow101 added the label Wallet on Jan 22, 2026

Wallet/bdb: Safely and correctly list files only used by the single wallet

If any other files exist in the directory, we cannot assume the sharable files are exclusively for this wallet.
But if they are, this also cleans up other log.* files

7475d134f6

Wallet/Migration: If loading the new watchonly or solvables wallet fails, log the correct wallet name in error message 60f529027c

Wallet/Migration: Skip moving the backup file back and forth for no reason

Since we no longer delete the wallet directory, there's no need to vacate it
The moving only served to risk errors by crossing filesystem boundaries (which fs::rename can't handle)

cef01d0be5

Bugfix: Wallet/Migration: Move backup into wallet directory when migrating from non-directory

While 30.x+ keep backup files in walletdir, 29.x places them in the migrated wallet directory

69a6b9b115

QA: tool_wallet: Check that db.log is deleted with a lone legacy wallet, but not with a shared db environment 65173944ed

luke-jr force-pushed on Jan 23, 2026

achow101 commented at 8:31 pm on January 26, 2026: member

ACK 65173944ed60df3b9cffca95932aed8720921478

[29.x] Fix #34222 backport bugs #34370

Code Coverage & Benchmarks

Reviews

LLM Linter (✨ experimental)