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

achow101 · 2026-01-07T22:03:05Z

Backports #34222 to 28.x

DrahtBot · 2026-01-07T22:03:12Z

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  into the comment that the bot should ignore.

bensig · 2026-01-07T22:16:20Z

ACK 67f8bfa

achow101 · 2026-01-08T03:05:02Z

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

Github-Pull: bitcoin#31423 Rebased-From: d04f6a9

Github-Pull: bitcoin#31423 Rebased-From: 1de423e

Github-Pull: bitcoin#34215 Rebased-From: f78f6f1

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#34156 Rebased-From: 4ed0693

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: bitcoin@f6ee59b 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#34156 Rebased-From: f4c7e28

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#34156 Rebased-From: 36093bd

…rune 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#34156 Rebased-From: f011e0f

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#34156 Rebased-From: d70b159

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#34156 Rebased-From: 82caa81

…eight Github-Pull: bitcoin#34156 Rebased-From: b7c34d0

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: eeaf28d

achow101 · 2026-01-12T21:37:35Z

test/functional/wallet_migration.py


+    def unsynced_wallet_on_pruned_node_fails(self):
+        self.log.info("Test migration of an unsynced wallet on a pruned node fails gracefully")
+        shutil.rmtree(self.nodes[0].wallets_path / "database", ignore_errors=True)


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.

davidgumberg · 2026-01-14T18:43:53Z

test/functional/wallet_migration.py

+        # Make a file in the wallets dir that must still exist after migration
+        survive_path = self.nodes[0].wallets_path / "survive"
+        open(survive_path, "wb").close()
+        assert survive_path.exists()


question: Why add this? Doesn't:

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

below ensure that the wallets_path exists?

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

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

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.

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: 76cdeb7

Not blocking/important you can resolve this.

davidgumberg · 2026-01-14T18:48:24Z

test/functional/wallet_migration.py

+        self.nodes[0].setmocktime(mocked_time)
+        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="")
+        self.nodes[0].setmocktime(0)
+


probably only if retouching:

Suggested change

# Verify the /wallets/ path exists, the wallet is still BDB and the backup file is there.

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

with open(self.nodes[0].wallets_path / "wallet.dat", "rb") as f:

data = f.read(16)

_, _, magic = struct.unpack("QII", data)

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 · 2026-01-15T04:03:07Z

crACK 4d21972

DrahtBot added the Backport label Jan 7, 2026

achow101 force-pushed the 28.x-createfromdump-deletion branch from 67f8bfa to 36ce240 Compare January 7, 2026 23:42

achow101 changed the title ~~[28.x] wallettool: fix unnamed createfromdump failure walletsdir deletion~~ [28.x] Backport wallets directory deletion fixes Jan 8, 2026

achow101 marked this pull request as draft January 8, 2026 03:04

fanquake mentioned this pull request Jan 8, 2026

wallettool: fix unnamed createfromdump failure walletsdir deletion #34215

Merged

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

0a944e6

Github-Pull: bitcoin#31423 Rebased-From: d04f6a9

achow101 force-pushed the 28.x-createfromdump-deletion branch from 36ce240 to 9a888e6 Compare January 8, 2026 21:46

achow101 marked this pull request as ready for review January 8, 2026 21:47

achow101 force-pushed the 28.x-createfromdump-deletion branch from 9a888e6 to 4ade4cf Compare January 9, 2026 00:01

furszy and others added 3 commits January 8, 2026 18:12

wallet: introduce method to return all db created files

5f07b93

Github-Pull: bitcoin#31423 Rebased-From: 1de423e

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

b54cdfc

Github-Pull: bitcoin#34215 Rebased-From: f78f6f1

achow101 force-pushed the 28.x-createfromdump-deletion branch from 4ade4cf to f48270b Compare January 9, 2026 02:14

fanquake added this to the 28.4 milestone Jan 9, 2026

furszy added 5 commits January 9, 2026 11:26

test: add coverage for unnamed wallet migration failure

1d46624

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#34156 Rebased-From: 36093bd

achow101 force-pushed the 28.x-createfromdump-deletion branch from f48270b to a96a91c Compare January 10, 2026 01:15

bitcoin deleted a comment from FabriBanda Jan 11, 2026

furszy and others added 2 commits January 12, 2026 13:21

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

77622e0

…eight Github-Pull: bitcoin#34156 Rebased-From: b7c34d0

wallet: test: Failed migration cleanup

4d21972

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: eeaf28d

achow101 force-pushed the 28.x-createfromdump-deletion branch from a96a91c to 4d21972 Compare January 12, 2026 21:31

achow101 commented Jan 12, 2026

View reviewed changes

davidgumberg reviewed Jan 14, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

achow101 commented Jan 7, 2026

Uh oh!

DrahtBot commented Jan 7, 2026 •

edited

Loading

Uh oh!

bensig commented Jan 7, 2026

Uh oh!

achow101 commented Jan 8, 2026

Uh oh!

achow101 Jan 12, 2026

Uh oh!

davidgumberg Jan 14, 2026

Uh oh!

achow101 Jan 14, 2026

Uh oh!

davidgumberg Jan 14, 2026

Uh oh!

achow101 Jan 14, 2026

Uh oh!

davidgumberg Jan 15, 2026

Uh oh!

davidgumberg Jan 14, 2026 •

edited

Loading

Uh oh!

davidgumberg commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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

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

Are you sure you want to change the base?

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

Conversation

achow101 commented Jan 7, 2026

Uh oh!

DrahtBot commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Uh oh!

bensig commented Jan 7, 2026

Uh oh!

achow101 commented Jan 8, 2026

Uh oh!

achow101 Jan 12, 2026

Choose a reason for hiding this comment

Uh oh!

davidgumberg Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

davidgumberg Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

achow101 Jan 14, 2026

Choose a reason for hiding this comment

Uh oh!

davidgumberg Jan 15, 2026

Choose a reason for hiding this comment

Uh oh!

davidgumberg Jan 14, 2026 • 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!

davidgumberg commented Jan 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jan 7, 2026 •

edited

Loading

davidgumberg Jan 14, 2026 •

edited

Loading