The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34222.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | bensig |
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Possible typos and grammar issues:
2026-01-09
416+ assert not (self.nodes[0].wallets_path / "wallet.dat").exists()
417+
418+ self.log.info('Checking createfromdump with an unnamed wallet')
419+ self.do_tool_createfromdump("", "wallet.dump")
420+ if not self.options.descriptors:
421+ os.rename(self.nodes[0].wallets_path / "default.wallet.dat", self.nodes[0].wallets_path / "wallet.dat")
os.rename isn’t throwing an exception on Windows? the wallet.dat should exist in the top-level dir after the do_tool_createfromdump
Github-Pull: bitcoin/bitcoin#31423
Rebased-From: d04f6a97ba9a55aa9455e1a805feeed4d630f59a
@davidgumberg discovered that a form of the deletion bug is also reachable if the user migrates a wallet at a relative path, resulting the entire directory at that relative path being deleted on a migration failure. This is problematic when that relative path contains more than just a wallet file. For example, with a wallet.dat in the datadir, migrating the wallet named “../” would result in deleting the entire datadir.
To resolve this, I’ve backported the entirety of #34156 and #34226 which adds tests for this case.
These backports are largely unclean as well.
4630@@ -4570,6 +4631,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4631 // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir
4632 fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
4633 fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
4634+ wallet_files_to_remove.insert(backup_path);
If the wallet originally was in the wallet dir directly, we’ll copy it over itself and then delete it?
Why not just move the file here (and never delete the backup)?
Actually, it looks like copy_file is throwing an exception: “filesystem error: cannot copy file: File exists”
So it’s not deleting the backup, but neither is it doing the intended cleanup at all. Probably want to extend the test to verify the cleanup happens, and then fix this.
(to be clear, for test “Test failure during migration of wallet named: unnamed (default)”)
The tests already check that this error is being hit and that the cleanup does not occur.
However this was needed for the other tests where cleanup is occurring. It is not universally true that the cleanup does not occur. It should only be for the unnamed wallet and for wallets by absolute path.
627+
628+ # Verify the /wallets/ path exists.
629+ assert self.master_node.wallets_path.exists()
630+
631+ # Verify both wallet paths exist.
632+ assert Path(old_path / "wallet.dat").exists()
137+ std::vector<fs::path> files;
138+ files.emplace_back(env->Directory() / m_filename);
139+ files.emplace_back(env->Directory() / "db.log");
140+ files.emplace_back(env->Directory() / ".walletlock");
141+ files.emplace_back(env->Directory() / "database" / "log.0000000001");
142+ files.emplace_back(env->Directory() / "database");
Good point. Added a check that env->m_databases.size() == 1 to return these.
It’s only the unnamed wallet and direct files in the walletsdir that can possibly run into this issue. For wallets in a subdirectory, it will be one wallet.dat per environment.
131@@ -132,6 +132,19 @@ class BerkeleyDatabase : public WalletDatabase
132 /** Return path to main database filename */
133 std::string Filename() override { return fs::PathToString(env->Directory() / m_filename); }
134
135+ std::vector<fs::path> Files() override
createfromdump?
Github-Pull: bitcoin/bitcoin#31423
Rebased-From: 1de423e0a08bbc63eed36c8772e9ef8b48e80fb8
Github-Pull: bitcoin/bitcoin#34215
Rebased-From: f78f6f1dc8e16d5a8a23749e77bc3bf17c91ae42
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
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
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
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
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
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
Github-Pull: bitcoin/bitcoin#34156
Rebased-From: b7c34d08dd9549a95cffc6ec1ffa4bb4f81e35eb
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