After a wallet is migrated and we are trying to load it, if it could not be loaded, don’t try to set the wallet name. Otherwise we have a segfault.
This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.
After a wallet is migrated and we are trying to load it, if it could not be
loaded, don't try to set the wallet name.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32984.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, davidgumberg, rkrux, w0xlt, pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
I was unable to write a working functional test for this behavior.
It should be pretty trivial, see https://github.com/bitcoin/bitcoin/pull/32988
1386+
1387+ # Check the wallet we tried to migrate is still BDB
1388+ with open(self.master_node.wallets_path / wallet_name / "wallet.dat", "rb") as f:
1389+ data = f.read(16)
1390+ _, _, magic = struct.unpack("QII", data)
1391+ assert_equal(magic, BTREE_MAGIC)
ACK 060695c22ae7b2b0f2a1d
I think this change is good since we can’t ever be certain that loading will always succeed after migration, but I think cases where we know migration will succeed but loading will fail should be addressed. i.e. shouldn’t we also change migration so that the case used in the functional test never successfully migrated in the first place?
Nit: Update PR description now that functional test is included.
1383+ # Disable network sync and prevent disk space warning on small (tmp)fs
1384+ self.start_node(self.old_node.index, extra_args=self.old_node.extra_args + ["-maxconnections=0", "-prune=550"])
1385+
1386+ wallet_name = "failed_load_after_migrate"
1387+ self.create_legacy_wallet(wallet_name)
1388+ assert_raises_rpc_error(-4, "Wallet loading failed. Wallet files should not be reused across chains.", lambda: self.migrate_and_get_rpc(wallet_name))
In 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542 “test: Failed load after migrate should restore backup”
Adding a review note because it took me some time to understand why this error pops up only after migration is complete and not during the process as this particular error comes from inside CWallet::AttachChain that is a part of CWallet::Create. The reason I found is that all the CWallet::Create calls inside the migration process pass an empty context that doesn’t contain the chain due to which the chain doesn’t need to be attached. Only while loading the wallet after migration, the proper wallet context is passed that triggers this error.
I verified that the migration-before-loading was triggered/completed by using the below patch in the migrate_and_get_rpc function. The below patch can be added separately as well (not necessary in this PR) because all the tests pass.
0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
1index a1b319b90b..890478da26 100755
2--- a/test/functional/wallet_migration.py
3+++ b/test/functional/wallet_migration.py
4@@ -125,7 +125,7 @@ class WalletMigrationTest(BitcoinTestFramework):
5 if w["name"] == wallet_name:
6 assert_equal(w["warnings"], ["This wallet is a legacy wallet and will need to be migrated with migratewallet before it can be loaded"])
7 # Migrate, checking that rescan does not occur
8- with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
9+ with self.master_node.assert_debug_log(expected_msgs=["Migrating wallet storage database from BerkeleyDB to SQLite."], unexpected_msgs=["Rescanning"]):
10 migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
11 # Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
12 # (in which case the wallet name would be suffixed by the 'watchonly' term)
ACK 060695c22ae7b2b0f2a1dd1417ed1b9d5a5ab542
On a side note: should we not check the wallet chain before executing the migration on it instead of letting it go that far?