wallettool: fix unnamed createfromdump failure walletsdir deletion #34215

achow101 · 2026-01-07T00:15:45Z

As pointed out in #34156 (comment), it is possible for createfromdump to also accidentally delete the entire wallets directory if the wallet name is the empty string and the dumpfile contains a checksum error.

This is also fixed by removing the files created by only removing the directory for named wallets, and avoiding the use of fs::remove_all.

DrahtBot · 2026-01-07T00:15:58Z

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/34215.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	waketraindev, polespinasa, rkrux, willcl-ark, pablomartin4btc
Approach ACK	w0xlt

If your review is incorrectly listed, please copy-paste  into the comment that the bot should ignore.

w0xlt

Approach ACK

waketraindev · 2026-01-07T01:14:38Z

lgtm ACK f78f6f1

tested manually, datadir no longer removed.

$ ./bitcoin-wallet -datadir=./local-datadir -dumpfile=test.dump createfromdump; ls local-datadir
Error: Got key that was not hex: malformedData13370776657273696f6e
test-wallet

polespinasa

code review and tACK f78f6f1

Unlike #34156 this one should be backported as v29.x is also affected by this bug.
It can easily be tested with the commands shared here #34156 (comment)

rkrux

Code review and tACK f78f6f1

I can see the following error in the latest functional test if the cpp changes are reverted.

    assert self.nodes[0].wallets_path.exists()
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
AssertionError

rkrux · 2026-01-07T13:31:18Z

src/wallet/dump.cpp

+
    wallet.reset(); // The pointer deleter will close the wallet for us.

    // Remove the wallet dir if we have a failure


- // Remove the wallet dir if we have a failure + // If we have a failure, remove the wallet files and optionally the wallet dir if a named wallet

Or perhaps add the if a named wallet bit in the comment above about the gathering...

willcl-ark · 2026-01-07T14:19:40Z

src/wallet/dump.cpp

    if (!ret) {
-        fs::remove_all(wallet_path);
+        for (const auto& p : paths_to_remove) {
+            fs::remove(p);


One remaining unhandled case is that fs::remove() of the wallet_path fails (silently, as currently implemented) as the directory contains another file.

I can't think how that would be problematic, nor do I think it should be a blocker for this fix.

willcl-ark

ACK f78f6f1

We now safely gather paths to remove, and by dropping fs::remove_all() usage only remove files and non-empty directories.

pablomartin4btc

ACK f78f6f1

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

fanquake · 2026-01-07T14:35:05Z

Backported to 30.x in #34209.

janb84

(Post merge)
code review ACK f78f6f1

on failure a specific list of files and folders is removed not just the wallet_path, which was a bit harsh. Agree with the remarks of rkrux and willcl-ark

483d158 doc: update manual pages for v30.2rc1 (fanquake) 747a863 build: bump version to v30.2rc1 (fanquake) cc3cdbe doc: update release notes for v30.2rc1 (fanquake) c4082a4 wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow) 185ca0e test: coverage for migration failure when last sync is beyond prune height (furszy) bc71372 wallet: migration, fix watch-only and solvables wallets names (furszy) bef4b1f wallet: improve post-migration logging (furszy) ac940ac test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy) 8e5c02a test: add coverage for unnamed wallet migration failure (furszy) ac4d095 wallet: fix unnamed wallet migration failure (furszy) 454ac8e wallet: RestoreWallet failure, erase only what was created (furszy) Pull request description: Backports: * #34156 * #34215 ACKs for top commit: darosior: ACK 483d158 willcl-ark: tACK 483d158 janb84: ACK 483d158 w0xlt: ACK 483d158 marcofleon: lgtm ACK 483d158 Tree-SHA512: b43485a8745a663d3a031113a6a266af699b3e70b77f72a0e69ea1986adcaa0ee0d4ce51aacd0fde91611cb4bdba9b6be52e5dca59ff8b12135715b8b1ed5ef2

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

polespinasa · 2026-01-08T09:11:50Z

Just checked and 28.x is also affected by this issue.
Should it be backported? The release is in "Maintenance End" untill v31.0.

fanquake · 2026-01-08T10:27:34Z

It is being backported to 28.x in #34223.

fanquake · 2026-01-08T10:27:55Z

Being backported to 29.x in #34222.

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

76cdeb7 wallet: test: Failed migration cleanup (David Gumberg) 9405e91 test: coverage for migration failure when last sync is beyond prune height (furszy) 5e8ad98 wallet: migration, fix watch-only and solvables wallets names (furszy) a7e2d10 wallet: improve post-migration logging (furszy) 9ea84c0 test: restorewallet, coverage for existing dirs, unnamed wallet and prune failure (furszy) 833848e test: add coverage for unnamed wallet migration failure (furszy) a074d36 wallet: fix unnamed wallet migration failure (furszy) d91f56e wallet: RestoreWallet failure, erase only what was created (furszy) cc324aa wallettool: do not use fs::remove_all in createfromdump cleanup (Ava Chow) 01c04d3 wallet: introduce method to return all db created files (furszy) abaf1e3 refactor: remove sqlite dir path back-and-forth conversion (furszy) Pull request description: Backports: * #34215 * #34156 * #34226 * 2 required commits from #31423 Note that this backport is unclean and several changes have to be made to most commits to accommodate BDB and the differences in migration cleanup behavior. ACKs for top commit: furszy: Code review ACK 76cdeb7 brunoerg: light code review ACK 76cdeb7 + backported the functional tests without the fixes and all of them failed accordingly. glozow: light review ACK 76cdeb7. Tree-SHA512: 432268117783fc9a221d895a6f6601b6a2a5031c76d1443cf804cc1d486b40fcded982409d548acd1c01a13c7b378b840fcc3fbe823d6ba5ffc4ebe017d4e13c

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

f78f6f1

achow101 mentioned this pull request Jan 7, 2026

wallet: fix unnamed legacy wallet migration failure #34156

Merged

w0xlt reviewed Jan 7, 2026

View reviewed changes

DrahtBot requested a review from w0xlt January 7, 2026 01:14

polespinasa approved these changes Jan 7, 2026

View reviewed changes

fanquake added Needs backport (29.x) Needs backport (30.x) labels Jan 7, 2026

fanquake added this to the 30.2 milestone Jan 7, 2026

rkrux reviewed Jan 7, 2026

View reviewed changes

willcl-ark reviewed Jan 7, 2026

View reviewed changes

willcl-ark approved these changes Jan 7, 2026

View reviewed changes

pablomartin4btc approved these changes Jan 7, 2026

View reviewed changes

fanquake merged commit cd6e4c9 into bitcoin:master Jan 7, 2026
27 checks passed

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jan 7, 2026

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

c4082a4

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

fanquake mentioned this pull request Jan 7, 2026

[30.x]: Final backports & 30.2rc1 #34209

Merged

fanquake removed the Needs backport (30.x) label Jan 7, 2026

janb84 reviewed Jan 7, 2026

View reviewed changes

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 7, 2026

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

edc003a

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 7, 2026

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

5b8e09b

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

achow101 mentioned this pull request Jan 7, 2026

[29.x] Backport wallets directory deletion fixes #34222

Merged

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 7, 2026

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

67f8bfa

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 7, 2026

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

c6877e3

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 7, 2026

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

36ce240

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 8, 2026

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

df00a2e

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

fanquake removed the Needs backport (29.x) label Jan 8, 2026

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 8, 2026

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

17fac3a

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 9, 2026

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

cc324aa

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

achow101 added a commit to achow101/bitcoin that referenced this pull request Jan 9, 2026

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

b54cdfc

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

achow101 mentioned this pull request Jan 13, 2026

[29.x] backports + final changes for 29.3 #34268

Open


		wallet.reset(); // The pointer deleter will close the wallet for us.

		// Remove the wallet dir if we have a failure

wallettool: fix unnamed createfromdump failure walletsdir deletion #34215

wallettool: fix unnamed createfromdump failure walletsdir deletion #34215

Uh oh!

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!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

waketraindev commented Jan 7, 2026

Uh oh!

polespinasa left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

willcl-ark Jan 7, 2026

Choose a reason for hiding this comment

Uh oh!

willcl-ark left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fanquake commented Jan 7, 2026

Uh oh!

janb84 left a comment

Choose a reason for hiding this comment

Uh oh!

polespinasa commented Jan 8, 2026

Uh oh!

fanquake commented Jan 8, 2026

Uh oh!

fanquake commented Jan 8, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

DrahtBot commented Jan 7, 2026 •

edited

Loading