rpc, wallet: add an option to not load the wallet after migrating

[polespinasa]

This PR is motivated by this Stack Exchange question.

Long story short, someone who has a node pruned before his legacy wallet birthday, is unable to migrate the wallet as it is not possible to load it.

Loading is not necessary for migration, and migrating without wanting to use the wallet in that node is a valid use-case.

This PR adds a new RPC argument to migratewallet that allow the user disabling the wallet loading.
Second commits adds tests for it.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35266.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	w0xlt

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
• #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

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.

[w0xlt]

Concept ACK.

[achow101]

In 6d3916f "rpc, wallet: add option to not load the wallet after migration"

For watchonly_wallet and solvables_wallet, without loading those wallets, we lose the output to the user that those wallets were created. MigrationResult should be updated to include the watchonly and solvables wallets names so we can return that info to the user when those wallets are not being loaded.

[polespinasa]

We only lose that output when not loading the wallet? I mean is it something this PR broke?

[achow101]

When those wallets are not loaded, we no longer inform the user that those wallets have been created in the RPC result. But we should still do that, so MigrationResult should have fields for those wallet names.

[w0xlt]

The current code fails when migration creates watchonly and/or solvables wallets.

This happens in the post-migration loop inside MigrateLegacyToDescriptor():

for (std::shared_ptr<CWallet>* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) { ... }

Inside the loop, the wallet pointer is reset before the optional reload:

wallet.reset();

When load_wallet=false, the wallet is not reloaded, so res.wallet remains null. The code then uses !res.wallet to decide whether the primary wallet name still needs to be set:

if (!res.wallet) {
    res.wallet_name = wallet_name;
}

Because res.wallet remains null throughout the loop, this condition is true for the primary wallet, the watchonly wallet, and the solvables wallet. As a result, res.wallet_name is overwritten and ends up containing the last auxiliary wallet name, e.g. <wallet>_solvables, instead of the primary wallet name.

The same no-load path also clears res.watchonly_wallet and res.solvables_wallet, so the RPC response cannot derive watchonly_name or solvables_name from those pointers.

Test:

diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 48ae1fb051..78730aa3fe 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1537,6 +1537,27 @@ class WalletMigrationTest(BitcoinTestFramework):
         assert_equal(loaded_wallet.getbalance(), bals["mine"]["trusted"])
         loaded_wallet.unloadwallet()
 
+    def test_no_load_reports_auxiliary_wallet_names(self):
+        self.log.info("Test no-load migration reports auxiliary wallet names")
+        wallet_name = "no_load_auxiliary_names"
+        wallet = self.create_legacy_wallet(wallet_name)
+
+        wallet.importaddress(address=self.master_node.get_wallet_rpc(self.default_wallet_name).getnewaddress(), rescan=False)
+        _, pubkey = generate_keypair(compressed=True, wif=True)
+        wallet.addmultisigaddress(nrequired=1, keys=[pubkey.hex()])
+
+        self.old_node.unloadwallet(wallet_name)
+        shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name)
+
+        migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, load_wallet=False)
+
+        assert_equal(migrate_info["wallet_name"], wallet_name)
+        assert_equal(migrate_info["watchonly_name"], f"{wallet_name}_watchonly")
+        assert_equal(migrate_info["solvables_name"], f"{wallet_name}_solvables")
+        assert wallet_name not in self.master_node.listwallets()
+        assert f"{wallet_name}_watchonly" not in self.master_node.listwallets()
+        assert f"{wallet_name}_solvables" not in self.master_node.listwallets()
+
     def test_solvable_no_privs(self):
         self.log.info("Test migrating a multisig that we do not have any private keys for")
         wallet = self.create_legacy_wallet("multisig_noprivs")
@@ -1669,6 +1690,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         self.test_miniscript()
         self.test_taproot()
         self.test_no_load_after_migration()
+        self.test_no_load_reports_auxiliary_wallet_names()
         self.test_solvable_no_privs()
         self.test_loading_failure_after_migration()