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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34215.
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 <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
lgtm ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
tested manually, datadir no longer removed.
0$ ./bitcoin-wallet -datadir=./local-datadir -dumpfile=test.dump createfromdump; ls local-datadir
1Error: Got key that was not hex: malformedData13370776657273696f6e
2test-wallet
code review and tACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
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)
280+ std::vector<fs::path> paths_to_remove = wallet->GetDatabase().Files();
281+ if (!name.empty()) paths_to_remove.push_back(wallet_path);
282+
283 wallet.reset(); // The pointer deleter will close the wallet for us.
284
285 // Remove the wallet dir if we have a failure
0- // Remove the wallet dir if we have a failure
1+ // If we have a failure, remove the wallet files and optionally the wallet dir if a named wallet
if a named wallet bit in the comment above about the gathering…
Code review and tACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
I can see the following error in the latest functional test if the cpp changes are reverted.
0 assert self.nodes[0].wallets_path.exists()
1 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
2AssertionError
284
285 // Remove the wallet dir if we have a failure
286 if (!ret) {
287- fs::remove_all(wallet_path);
288+ for (const auto& p : paths_to_remove) {
289+ 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.
ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
We now safely gather paths to remove, and by dropping fs::remove_all() usage only remove files and non-empty directories.
(Post merge) code review ACK f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
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
Milestone
30.2