Fixes #31447.
During migration failure, only load wallet back into memory when the wallet was loaded prior to migration. This fixes the case where BDB is not supported, which implies that no legacy wallet can be loaded into memory due to the lack of db writing functionality.
Link to error description #31447 (comment).
This PR also improves migration backup related comments to better document the current workflow.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31451.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux, achow101, pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/34150908075
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
Maybe also add a BDB only test that a when a loaded wallet is migrated, it is loaded after migration?
I assume you meant a BDB only test verifying that the legacy wallet remains loaded after a migration failure –> just pushed it.
During migration failure, only load wallet back into memory when the
wallet was loaded prior to migration. This fixes the case where BDB
is not supported, which implies that no legacy wallet can be loaded
into memory due to the lack of db writing functionality.
This commit also improves migration backup related comments to better
document the current workflow.
Co-authored-by: Ava Chow <github@achow101.com>
893@@ -894,9 +894,7 @@ def test_failed_migration_cleanup(self):
894 shutil.copytree(self.old_node.wallets_path / "failed", self.master_node.wallets_path / "failed")
895 assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
896
897- assert "failed" in self.master_node.listwallets()
898- assert "failed_watchonly" not in self.master_node.listwallets()
899- assert "failed_solvables" not in self.master_node.listwallets()
900+ assert all(wallet not in self.master_node.listwallets() for wallet in ["failed", "failed_watchonly", "failed_solvables"])
listwallets calls here!
4565@@ -4560,17 +4566,24 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4566 }
4567
4568 // Restore the backup
4569- DatabaseStatus status;
4570- std::vector<bilingual_str> warnings;
4571- if (!RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, error, warnings)) {
4572- error += _("\nUnable to restore backup of wallet.");
4573+ // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory.
Nit
0 // Convert the backup file in the top-level wallets dir to the wallet db file by renaming it and moving it into the wallet's directory.
4583+ // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir
4584 fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
4585 fs::remove(temp_backup_location);
4586
4587+ // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
4588+ // This check is performed after restoration to avoid an early error before saving the backup.
before saving the backup.
Isn’t the backup saved already at this* point?
before saving the backup.
Isn’t the backup saved already at the point?
Yes, the sentence basically means “this assertion was specifically placed here, and not before the fs::copy_file line that is one line above, because we want the backup to be stored in the correct location regardless of whether the wallet was invalidly loaded or not”
4585 fs::remove(temp_backup_location);
4586
4587+ // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
4588+ // This check is performed after restoration to avoid an early error before saving the backup.
4589+ bool wallet_reloaded = ptr_wallet != nullptr;
4590+ assert(was_loaded == wallet_reloaded);
907@@ -910,6 +908,22 @@ def test_failed_migration_cleanup(self):
908 _, _, magic = struct.unpack("QII", data)
909 assert_equal(magic, BTREE_MAGIC)
910
911+ ####################################################
912+ # Perform the same test with a loaded legacy wallet.
test_failed_migration_cleanup twice from run_test with boolean loaded_wallet as an argument to the test.
Should we also not add the checks from lines 899 to 909 in the loaded wallet test below? If it makes sense, then maybe we can consider calling
test_failed_migration_cleanuptwice fromrun_testwithboolean loaded_walletas an argument to the test.
Yeah, that’s possible, but I’m not sure it’s worth the refactoring since we’re planning to remove BDB soon (this extra test will be removed too).
Also, running it from run_test in two different ways would require creating a different wallet for each run (because create_legacy_wallet can’t be called with the same name twice), which would mean decoupling the wallet name into a variable, which requires a good number of lines changes. Another option is to create a test_failed_migration_cleanup inner function post-setup and call it twice, e.g. 393c506533a1183225d767d18dd54c3d961e1a28.
The approach shared in https://github.com/bitcoin/bitcoin/commit/393c506533a1183225d767d18dd54c3d961e1a28#diff-571cc226b2c9fcca7adef3916d9328f24948edf3bf0b5e66b8b45dcf75cbe502R896 looks good to me - increases assertions while getting rid of duplication as well!
Yes BDB will be removed but I’d assume the wallet_migration tests won’t be? That’s why these tests now use previous releases to create legacy wallets from what I understand: #31248
This extra test can also stick around imo, it’s quite a specific case that led to the bug.
Yes BDB will be removed but I’d assume the
wallet_migrationtests won’t be? That’s why these tests now use previous releases to create legacy wallets from what I understand: #31248This extra test can also stick around imo, it’s quite a specific case that led to the bug.
Yes, wallet_migration.py uses a previous release binary to create the legacy wallet and the latest version binary to perform migration. Since BDB support will be removed, the latest version binary will not allow loading a legacy wallet, a prerequisite for this test, thus making it impossible to run this test case.
Nice catching the bug, especially helpful for me because the migration test fails in my system as well! The diff looks quite different and shortened compared to what I was looking at day before yesterday. Mostly LGTM.
Attaching a failed wallet snapshot of the newly added part functional test in my system. I bumped the mock time by 10000 ms to make the file modified time difference apparent.
0master node, failed wallet, before migration:
1node0/regtest/wallets/failed/failed_1733991924.legacy.bak Thu, 12 Dec 2024 08:25:24 16384 bytes
2node0/regtest/wallets/failed/database Thu, 12 Dec 2024 08:25:24 96 bytes
3node0/regtest/wallets/failed/wallet.dat Thu, 12 Dec 2024 08:25:24 16384 bytes
4node0/regtest/wallets/failed/db.log Thu, 12 Dec 2024 08:25:24 0 bytes
5node0/regtest/wallets/failed/.walletlock Thu, 12 Dec 2024 08:25:24 0 bytes
6master node, failed wallet, after migration:
7node0/regtest/wallets/failed/database Thu, 12 Dec 2024 08:25:25 96 bytes
8node0/regtest/wallets/failed/wallet.dat Thu, 12 Dec 2024 08:25:25 16384 bytes
9node0/regtest/wallets/failed/failed_1734001939.legacy.bak Thu, 12 Dec 2024 08:25:25 16384 bytes
10node0/regtest/wallets/failed/db.log Thu, 12 Dec 2024 08:25:25 0 bytes
11node0/regtest/wallets/failed/.walletlock Thu, 12 Dec 2024 08:25:25 0 bytes
4573+ // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory.
4574+ // Reload it into memory if the wallet was previously loaded.
4575+ bilingual_str restore_error;
4576+ const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded);
4577+ if (!restore_error.empty()) {
4578+ error += restore_error + _("\nUnable to restore backup of wallet.");
Note for a follow-up (up-for-grabs):
In a previous version of this PR, as well as in #31250 (9beab081e977e), there was a bug here. We were passing the existing error field to RestoreWallet() and then checking error.empty() afterward. This was problematic because this is the error recovery path, meaning error was already set at this point.
This issue no longer exists because we now pass a new error object to RestoreWallet() and concatenate the outcomes. However, it would be good to verify that the message “Unable to restore backup of wallet” is not returned inside the test_failed_migration_cleanup test case. This will require modifying assert_raises_rpc_error to confirm that the specific message is not part of the error response.
I’ll let furszy confirm but I believe conceptually this^ is what he meant. You can open a PR for others to review and provide suggestions. Also, you can keep the PR in draft mode if you prefer so, which lets the reviewers know the PR is not ready for review yet.
But it’s easier and better to give suggestions in a PR than it is on a standalone commit - you will also get CI feedback while you’re waiting for a response.
ACK 589ed1a8eafe1daed379ebb383602c8f220aef19
on top of previous review
tACK 589ed1a8eafe1daed379ebb383602c8f220aef19
I’ve reproduced the issue described in #31447 (sqllite-only or BDB not compiled). This PR fixed the test issue and it temporarily adds (later will be removed with BDB) a validation when the legacy wallet is loaded (with BDB actually compiled; cmake -B build -DWITH_BDB=ON).
restorewallet RPC, before this PR the wallet was loaded and the “load_on_startup” argument was set, this is no longer the case unless we pass load_after_restore=true. Having said that, if that gets fixed, since we are disabling loading legacy wallets (#31250) if someone calls restorewallet RPC with load_on_startup=true on a legacy wallet, next time the node or QT starts it would fail and the app will be closed. Another place with similar issue (that’s not happening due to this PR) is when we select “Restore Wallet…” from the File menu in QT for a legacy wallet, we set load_on_startup=true in the wallet interface.load_after_restore, added in this PR, was set to true by default.