[28.x] Backport wallets directory deletion fixes #34223
Open
+384
−31
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34223. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste |
Contributor
|
ACK 67f8bfa |
achow101
force-pushed
the
28.x-createfromdump-deletion
branch
from
67f8bfa to
36ce240
Compare
achow101
changed the title
Member
Author
|
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
achow101
force-pushed
the
28.x-createfromdump-deletion
branch
from
36ce240 to
9a888e6
Compare
achow101
force-pushed
the
28.x-createfromdump-deletion
branch
from
9a888e6 to
4ade4cf
Compare
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
achow101
force-pushed
the
28.x-createfromdump-deletion
branch
from
4ade4cf to
f48270b
Compare
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
achow101
force-pushed
the
28.x-createfromdump-deletion
branch
from
f48270b to
a96a91c
Compare
…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
force-pushed
the
28.x-createfromdump-deletion
branch
from
a96a91c to
4d21972
Compare
achow101
commented
Jan 12, 2026
|
|
||
| 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) |
Member
Author
There was a problem hiding this comment.
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backports #34222 to 28.x