[28.x] Backport wallets directory deletion fixes

achow101 commented at 10:03 pm on January 7, 2026: member

Backports #34222 to 28.x

DrahtBot added the label Backport on Jan 7, 2026

DrahtBot commented at 10:03 pm on January 7, 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/34223.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg
Stale ACK	bensig

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

bensig commented at 10:16 pm on January 7, 2026: contributor

ACK 67f8bfa5287be3243f189844529fe3815f6e235f

achow101 force-pushed on Jan 7, 2026

achow101 renamed this:
~~[28.x] wallettool: fix unnamed createfromdump failure walletsdir deletion~~
[28.x] Backport wallets directory deletion fixes
on Jan 8, 2026

achow101 marked this as a draft on Jan 8, 2026

achow101 commented at 3:05 am on January 8, 2026: member

Going to backport the new commits from #34222 but it’s unclean and will take a little while.

refactor: remove sqlite dir path back-and-forth conversion

Github-Pull: bitcoin/bitcoin#31423
Rebased-From: d04f6a97ba9a55aa9455e1a805feeed4d630f59a

0a944e62cb

achow101 force-pushed on Jan 8, 2026

achow101 marked this as ready for review on Jan 8, 2026

achow101 force-pushed on Jan 9, 2026

wallet: introduce method to return all db created files

Github-Pull: bitcoin/bitcoin#31423
Rebased-From: 1de423e0a08bbc63eed36c8772e9ef8b48e80fb8

5f07b93d7f

wallettool: do not use fs::remove_all in createfromdump cleanup

Github-Pull: bitcoin/bitcoin#34215
Rebased-From: f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42

b54cdfc617

wallet: RestoreWallet failure, erase only what was created

Track what RestoreWallet creates so only those files and directories
are removed during a failure and nothing else. Preexisting paths
must be left untouched.

Note:
Using fs::remove_all() instead of fs::remove() in RestoreWallet does
not cause any problems currently, but the change is necessary for the
next commit which extends RestoreWallet to work with existing directories,
which may contain files that must not be deleted.

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: 4ed0693a3f2a427ef9e7ad016930ec29fa244995

e1e9d71da9

achow101 force-pushed on Jan 9, 2026

fanquake added this to the milestone 28.4 on Jan 9, 2026

wallet: fix unnamed wallet migration failure

When migrating any legacy unnamed wallet, a failed migration would
cause the cleanup logic to remove its parent directory. Since this
type of legacy wallet lives directly in the main '/wallets/' folder,
this resulted in unintentionally erasing all wallets, including the
backup file.

To be fully safe, we will no longer call `fs::remove_all`. Instead,
we only erase the individual db files we have created, leaving
everything else intact. The created wallets parent directories are
erased only if they are empty.
As part of this last change, `RestoreWallet` was modified to allow
an existing directory as the destination, since we no longer remove
the original wallet directory (we only remove the files we created
inside it). This also fixes the restore of top-level default wallets
during failures, which were failing due to the directory existence
check that always returns true for the /wallets/ directory.

This bug started after:
https://github.com/bitcoin/bitcoin/commit/f6ee59b6e2995a3916fb4f0d4cbe15ece2054494
Previously, the `fs::copy_file` call was failing for top-level wallets,
which prevented the `fs::remove_all` call from being reached.

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: f4c7e28e80bf9af50b03a770b641fd309a801589

e47af69222

test: add coverage for unnamed wallet migration failure

Verifies that a failed migration of the unnamed (default) wallet
does not erase the main /wallets/ directory, and also that the
backup file exists.

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: 36093bde63286e19821a9e62cdff1712b6245dc7

1d4662441f

test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure

The first test verifies that restoring into an existing empty directory
or a directory with no .dat db files succeeds, while restoring into a
dir with a .dat file fails.

The second test covers restoring into the default unnamed wallet
(wallet.dat), which also implicitly exercises the recovery path used
after a failed migration.

The third test covers failure during restore on a prune node. When
the wallet last sync was beyond the pruning height.

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: f011e0f0680a8c39988ae57dae57eb86e92dd449

75b59e5aba

wallet: improve post-migration logging

Right now, after migration the last message users see is "migration completed",
but the migration isn't actually finished yet. We still need to load the new wallets
to ensure consistency, and if that fails, the migration will be rolled back. This
can be confusing for users.

This change logs the post-migration loading step and if a wallet fails to load and
the migration will be rolled back.

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: d70b159c42008ac3b63d1c43d99d4f1316d2f1ef

fb4406e63a

wallet: migration, fix watch-only and solvables wallets names

Because the default wallet has no name, the watch-only and solvables
wallets created during migration end up having no name either.

This fixes it by applying the same prefix name we use for the backup
file for an unnamed default wallet.

Before: watch-only wallet named "_watchonly"
After:  watch-only wallet named "default_wallet_watchonly"

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: 82caa8193a3e36f248dcc949e0cd41def191efac

86eaf71e60

achow101 force-pushed on Jan 10, 2026

bitcoin deleted a comment on Jan 11, 2026

test: coverage for migration failure when last sync is beyond prune height

Github-Pull: bitcoin/bitcoin#34156
Rebased-From: b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb

77622e000b

wallet: test: Failed migration cleanup

Refactor a common way to perform the failed migration test that exists
for default wallets, and add relative-path wallets and absolute-path
wallets.

Github-Pull: 34226
Rebased-From: eeaf28dbe0e09819ab0e95bb7762b29536bdeef6

4d219725a7

achow101 force-pushed on Jan 12, 2026

in test/functional/wallet_migration.py:1123 in 4d219725a7

1117@@ -1013,6 +1118,29 @@ def check_comments():
1118         wallet.unloadwallet()
1119 
1120 
1121+    def unsynced_wallet_on_pruned_node_fails(self):
1122+        self.log.info("Test migration of an unsynced wallet on a pruned node fails gracefully")
1123+        shutil.rmtree(self.nodes[0].wallets_path / "database", ignore_errors=True)

achow101 commented at 9:37 pm on January 12, 2026:

Note for reviewers: This line is necessary because we are creating another database of the same name as one that was already open in this environment previously, so the db log files are referring to the wrong database. This would not be an issue if unloadwallet also shutdown the BDB environment, but that only happens when bitcoind is stopped.

However, there is no issue with data corruption as all of the data is compacted into the wallet.dat files. It’s just that the environment files aren’t being cleaned up.

in test/functional/wallet_migration.py:631 in 4d219725a7

633+        shutil.copyfile(wallet_path / "wallet.dat", watch_only_dir / "wallet.dat")
634+
635+        # Make a file in the wallets dir that must still exist after migration
636+        survive_path = self.nodes[0].wallets_path / "survive"
637+        open(survive_path, "wb").close()
638+        assert survive_path.exists()

davidgumberg commented at 6:43 pm on January 14, 2026:

question: Why add this? Doesn’t:

0        assert self.nodes[0].wallets_path.exists()

below ensure that the wallets_path exists?

achow101 commented at 6:53 pm on January 14, 2026:

It could have been that the directory was destroyed and recreated.

davidgumberg commented at 7:00 pm on January 14, 2026:

Wouldn’t that case be caught by the backup_path.exists() check?

achow101 commented at 7:37 pm on January 14, 2026:

The purpose of this case is to make sure a file created before migration exists after migration. If we treat migration as a black box that may do things to the filesystem, checking that files produced during migration are there after migration does not preclude migration having deleted the directory, recreated it, then created the file there.

davidgumberg commented at 4:02 am on January 15, 2026:

Sounds fair enough, but if that’s the case shouldn’t such a check be on master, I’m only asking because this was added in the backport, I see that you also added that check to the 29.x backport: https://github.com/bitcoin/bitcoin/pull/34222/changes/76cdeb7b06232050c7d20ffa1395697cc4e53295

Not blocking/important you can resolve this.

in test/functional/wallet_migration.py:1109 in 77622e000b

1104+        # Check migration failure
1105+        mocked_time = int(time.time())
1106+        self.nodes[0].setmocktime(mocked_time)
1107+        assert_raises_rpc_error(-4, "last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)\nUnable to restore backup of wallet.", self.nodes[0].migratewallet, wallet_name="")
1108+        self.nodes[0].setmocktime(0)
1109+

davidgumberg commented at 6:48 pm on January 14, 2026:

probably only if retouching:

0        # Verify the /wallets/ path exists, the wallet is still BDB and the backup file is there.
1        assert self.nodes[0].wallets_path.exists()
2        with open(self.nodes[0].wallets_path / "wallet.dat", "rb") as f:
3            data = f.read(16)
4            _, _, magic = struct.unpack("QII", data)
5            assert_equal(magic, BTREE_MAGIC)

assert self.nodes[0].wallets_path.exists() is redundant with the backup path check below, but would make the reason for test failure more obvious and makes the rebase cleaner

davidgumberg commented at 4:03 am on January 15, 2026: contributor

crACK 4d219725a76672929fc405c39c869a4ab208efa9

fanquake requested review from furszy on Jan 29, 2026

[28.x] Backport wallets directory deletion fixes #34223

Code Coverage & Benchmarks

Reviews