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

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.

Follow-up: Add an option to the GUI to not load the wallet after migrating.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

Concept ACK.

note to self - need release note

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()

@w0xlt @achow101 I think the issue you were pointing is solved.

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

I am actually not sure if we need the wallet.reset() as we are just overriding it after that. @w0xlt I picked your test and added you as a co-author.

Currently when load_wallet=false, the migration code still wrote the new wallet name(s) into settings.json's wallet array, so the wallets would silently be reloaded on the next node restart — defeating the whole point of load_wallet=false.

Suggestion:

<details> <summary>diff</summary>

diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 5cd47f6722..bd0367f3a4 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -244,6 +244,13 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
         Assert(RemoveWalletSetting(*chain, strprintf("wallet_%d", i)));
     },
                             /*num_expected_wallets=*/0);
+
+    Assert(chain->deleteRwSettings("wallet"));
+    BOOST_REQUIRE(chain->getRwSetting("wallet").isNull());
+    Assert(RemoveWalletSetting(*chain, ""));
+    auto wallets = chain->getRwSetting("wallet");
+    BOOST_CHECK(wallets.isArray());
+    BOOST_CHECK_EQUAL(wallets.getValues().size(), 0);
 }
 
 static int64_t AddTx(ChainstateManager& chainman, CWallet& wallet, uint32_t lockTime, int64_t mockTime, int64_t blockTime)
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 792aafaaa7..342e179c88 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -109,7 +109,14 @@ bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 {
     const auto update_function = [&wallet_name](common::SettingsValue& setting_value) {
-        if (!setting_value.isArray()) return interfaces::SettingsAction::SKIP_WRITE;
+        if (!setting_value.isArray()) {
+            if (wallet_name.empty() && setting_value.isNull()) {
+                // Empty setting suppresses backwards-compatible default wallet autoload.
+                setting_value.setArray();
+                return interfaces::SettingsAction::WRITE;
+            }
+            return interfaces::SettingsAction::SKIP_WRITE;
+        }
         common::SettingsValue new_value(common::SettingsValue::VARR);
         for (const auto& value : setting_value.getValues()) {
             if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value);
@@ -4177,7 +4184,7 @@ static std::string MigrationPrefixName(CWallet& wallet)
     return name.empty() ? "default_wallet" : name;
 }
 
-bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, MigrationResult& res, bool load_wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
 {
     AssertLockHeld(wallet.cs_wallet);
 
@@ -4239,8 +4246,10 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
                 }
             }
 
-            // Add the wallet to settings
-            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
+            if (load_wallet) {
+                // Add the wallet to settings
+                UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
+            }
         }
         if (data->solvable_descs.size() > 0) {
             wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n");
@@ -4278,8 +4287,10 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
                 }
             }
 
-            // Add the wallet to settings
-            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
+            if (load_wallet) {
+                // Add the wallet to settings
+                UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
+            }
         }
     }
 
@@ -4405,7 +4416,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 
         // Do the migration of keys and scripts for non-empty wallets, and cleanup if it fails
         if (HasLegacyRecords(*local_wallet)) {
-            success = DoMigration(*local_wallet, context, error, res);
+            success = DoMigration(*local_wallet, context, error, res, load_wallet);
             // No scripts mean empty wallet after migration
             empty_local_wallet = local_wallet->GetAllScriptPubKeyMans().empty();
         } else {
@@ -4439,6 +4450,9 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 
     if (success) {
         Assume(!res.wallet); // We will set it here.
+        if (!load_wallet) {
+            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/false, warnings);
+        }
         // Check if the local wallet is empty after migration
         if (empty_local_wallet) {
             // This wallet has no records. We can safely remove it.
@@ -4455,13 +4469,16 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
                 // Track db path
                 track_for_cleanup(*wallet);
                 assert(wallet.use_count() == 1);
-                std::string wallet_name = wallet->GetName();
+                const std::string migrated_wallet_name = wallet->GetName();
                 wallet.reset();
+                if (!load_wallet) {
+                    UpdateWalletSetting(*context.chain, migrated_wallet_name, /*load_on_startup=*/false, warnings);
+                }
                 if (load_wallet) {
-                    wallet = LoadWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
+                    wallet = LoadWallet(context, migrated_wallet_name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
                     if (!wallet) {
                         LogError("Failed to load wallet '%s' after migration. Rolling back migration to preserve consistency. "
-                                 "Error cause: %s\n", wallet_name, error.original);
+                                 "Error cause: %s\n", migrated_wallet_name, error.original);
                         success = false;
                         break;
                     }
@@ -4469,13 +4486,13 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
                 // Set the first wallet as the main one.
                 // The loop order is intentional and must always start with the local wallet.
                 if (res.wallet_name.empty()) {
-                    res.wallet_name = wallet_name;
+                    res.wallet_name = migrated_wallet_name;
                     if (load_wallet) res.wallet = std::move(wallet);
                 }
                 if (wallet_ptr == &res.watchonly_wallet) {
-                    res.watchonly_wallet_name = wallet_name;
+                    res.watchonly_wallet_name = migrated_wallet_name;
                 } else if (wallet_ptr == &res.solvables_wallet) {
-                    res.solvables_wallet_name = wallet_name;
+                    res.solvables_wallet_name = migrated_wallet_name;
                 }
             }
         }
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 36eba41282..44bd7744bb 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -4,13 +4,14 @@
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Test Migrating a wallet from legacy to descriptor."""
 from contextlib import suppress
-from pathlib import Path
+import json
 import os.path
 import random
 import shutil
 import struct
 import time
 from decimal import Decimal
+from pathlib import Path
 
 from test_framework.address import (
     key_to_p2pkh,
@@ -1547,6 +1548,12 @@ class WalletMigrationTest(BitcoinTestFramework):
         _, pubkey = generate_keypair(compressed=True, wif=True)
         wallet.addmultisigaddress(nrequired=1, keys=[pubkey.hex()])
 
+        self.master_node.createwallet(wallet_name=wallet_name, load_on_startup=True)
+        self.master_node.unloadwallet(wallet_name)
+        shutil.rmtree(self.master_node.wallets_path / wallet_name)
+        with (self.master_node.chain_path / "settings.json").open(encoding="utf8") as settings_file:
+            assert wallet_name in json.load(settings_file).get("wallet", [])
+
         self.old_node.unloadwallet(wallet_name)
         shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name)
 
@@ -1559,6 +1566,12 @@ class WalletMigrationTest(BitcoinTestFramework):
         assert f"{wallet_name}_watchonly" not in self.master_node.listwallets()
         assert f"{wallet_name}_solvables" not in self.master_node.listwallets()
 
+        with (self.master_node.chain_path / "settings.json").open(encoding="utf8") as settings_file:
+            startup_wallets = json.load(settings_file).get("wallet", [])
+        assert wallet_name not in startup_wallets
+        assert f"{wallet_name}_watchonly" not in startup_wallets
+        assert f"{wallet_name}_solvables" not in startup_wallets
+
     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")

</details>

Currently when load_wallet=false, the migration code still wrote the new wallet name(s) into settings.json's wallet array, so the wallets would silently be reloaded on the next node restart — defeating the whole point of load_wallet=false.

Good catch!! I was testing it with only wallet_name, but unless you have a previous wallet created with that name it does not add it to settings.json.

I (think) I fixed it with a more simpler approach, your diff had some changes that I think are not necessary. Why the changes in RemoveWalletSetting?

The RemoveWalletSetting change is needed for the unnamed wallet case.

For old unnamed wallets, if settings.json has no wallet key, startup may still treat the existing top-level wallet.dat specially and load it for backwards compatibility. So “no wallet setting” does not mean “load no wallets”.

Bitcoin Core no longer auto-creates a default wallet, but it still has backwards-compatible auto-load behavior for an existing top-level unnamed wallet (src/wallet/load.cpp):

  // For backwards compatibility if an unnamed top level wallet exists...
  if (!args.IsArgSet("wallet")) {
      ...
      if (MakeWalletDatabase("", ...)) {
          wallets.push_back("");
          chain.overwriteRwSetting("wallet", wallets, SKIP_WRITE);
      }
  }

When migratewallet("", load_wallet=false) runs, we need to persist the user’s intent that the unnamed wallet should not be loaded on the next startup. But current RemoveWalletSetting("", ...) sees the missing/null wallet setting and returns SKIP_WRITE, so nothing is written. After restart, Core can again see no wallet key and autoload the unnamed wallet.

The patch suggested above makes RemoveWalletSetting("", ...) write an explicit empty array:

"wallet": []

That means “there are intentionally zero startup wallets”, which suppresses the backwards-compatible unnamed-wallet autoload. For named wallets, the old no-op behavior when the setting is missing is still fine.

The test below shows this behavior. It will fail with current code and succeed in the suggested above patch:

<details> <summary>diff</summary>

diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index cd4411682d..f563efd172 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -183,6 +183,17 @@ class WalletMigrationTest(BitcoinTestFramework):
 
         return migrate_info, wallet
 
+    def remove_wallet_setting(self, node):
+        settings_path = node.chain_path / "settings.json"
+        if not settings_path.exists():
+            return
+        with settings_path.open(encoding="utf8") as settings_file:
+            settings = json.load(settings_file)
+        settings.pop("wallet", None)
+        with settings_path.open("w", encoding="utf8") as settings_file:
+            json.dump(settings, settings_file, indent=4)
+            settings_file.write("\n")
+
     def test_basic(self):
         default = self.master_node.get_wallet_rpc(self.default_wallet_name)
 
@@ -1540,6 +1551,42 @@ class WalletMigrationTest(BitcoinTestFramework):
         loaded_wallet.unloadwallet()
 
 
+    def test_no_load_unnamed_wallet_does_not_autoload(self):
+        self.log.info("Test no-load migration of unnamed wallet suppresses default autoload")
+        self.master_node = self.nodes[0]
+        self.old_node = self.nodes[1]
+
+        wallet = self.create_legacy_wallet("", load_on_startup=False)
+        wallet.unloadwallet()
+
+        self.stop_node(0)
+        self.remove_wallet_setting(self.master_node)
+        (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True)
+        shutil.copyfile(self.old_node.wallets_path / "wallet.dat", self.master_node.wallets_path / "wallet.dat")
+        self.start_node(0)
+
+        assert_equal(self.master_node.listwallets(), [])
+
+        migrate_info = self.master_node.migratewallet(wallet_name="", load_wallet=False)
+        assert_equal(migrate_info["wallet_name"], "")
+        backup_path = Path(migrate_info["backup_path"])
+        assert "" not in self.master_node.listwallets()
+
+        with (self.master_node.chain_path / "settings.json").open(encoding="utf8") as settings_file:
+            wallet_setting_after_migration = json.load(settings_file).get("wallet")
+
+        self.restart_node(0)
+        wallets_after_restart = self.master_node.listwallets()
+
+        if "" in wallets_after_restart:
+            self.master_node.unloadwallet("")
+        self.clear_default_wallet(backup_path)
+        self.master_node.loadwallet(self.default_wallet_name, load_on_startup=True)
+
+        assert_equal(wallet_setting_after_migration, [])
+        assert "" not in wallets_after_restart
+
+
     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"
@@ -1711,6 +1758,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         self.test_miniscript()
         self.test_taproot()
         self.test_no_load_after_migration()
+        self.test_no_load_unnamed_wallet_does_not_autoload()
         self.test_no_load_reports_auxiliary_wallet_names()
         self.test_solvable_no_privs()
         self.test_loading_failure_after_migration()

</details>

Oh I see, I didn't catch that odd case. Thanks @w0xlt. Taking your patch :)

ACK 565037e30f2b005d22f3757111c9aba2902a8786

Concept ACK

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.

Is that because currently in master at the end of the migration, the once migrated descriptor wallet can't be loaded (due to missing blocks in the pruned node)? (I'd clarify that's the descriptor wallet that can't be loaded). Even now with your PR, the loading is optional, by default is true, so the user will get the error first, the process will fail and the user will have to run it again (and somehow finding out that they need to pass this new arg set to false)? I don't know if possible but perhaps the migration should finish with success in the case that the problem is only loading the migrated wallet due to its sync and the user should be informed of that? Unless there are other issues that could be hidden with this approach, but if we allow them to set the load migrated wallet to false, it's the same case.

Loading is not necessary for migration...

So the legacy wallet (BERKELEY_RO db) it's getting loaded (CWallet::LoadExisting(empty_context..) but because it has an empty context doesn't need the rescan?

Even now with your PR, the loading is optional, by default is true, so the user will get the error first, the process will fail and the user will have to run it again (and somehow finding out that they need to pass this new arg set to false)?

Yes, which would be similar as setting it to false by default and users trying to load it manually and failing.

I don't know if possible but perhaps the migration should finish with success in the case that the problem is only loading the migrated wallet due to its sync and the user should be informed of that?

I don't think this is a good idea, ideally the wallet should be able to be loaded to be sure the migration is correct and there is no corruption or any other error. This PR only adds an option to not load it bc in a specific case where a user is unable to load it because the node is pruned and has no other option.

Making it check if the reason for not loading it is due to being pruned adds some complexity that I don't see worth adding. And IIUC errors are passed as strings so it's something I don't find "pretty" to use as conditional.

Also at the end this is a "one"-time use RPC call, it's not a big deal to fail the migration and start it again.

So the legacy wallet (BERKELEY_RO db) it's getting loaded (CWallet::LoadExisting(empty_context..) but because it has an empty context doesn't need the rescan?

Correct :)

Force pushed to fix some nits @pablomartin4btc taken with a few tweaks.

Force pushed to address some nits and a small test correction

ACK 0cdd817a82a26be53efa6038dae85fceb0cf2248

ACK 0cdd817a82a26be53efa6038dae85fceb0cf2248

ACK https://github.com/bitcoin/bitcoin/commit/0cdd817a82a26be53efa6038dae85fceb0cf2248

Left a suggestion, not a blocker.