From 1f9bf081d58ba7042ca39f84aacddeaf8daffa4d Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 01:10:11 -0400 Subject: [PATCH 1/4] fix(wallet): reload the wallet if migration exited early Backport of bitcoin/bitcoin@78ba0e6748d2 from bitcoin#28868. Migration will unload loaded wallets prior to beginning. It will then perform some checks which may exit early. Such unloaded wallets should be reloaded prior to exiting. Adapted to Dash: the "already a descriptor wallet" check keeps using GetLegacyScriptPubKeyMan(), the Dash backup filename logic is kept, and the successful-migration path still reloads the wallet directly because bitcoin#28609 (which introduced the reload_wallet helper there) has not been backported, so the upstream hunk moving the helper out of that branch does not apply. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/wallet/wallet.cpp | 46 +++++++++++++++++++++++++++++++------------ 1 file changed, 33 insertions(+), 13 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7a2977c0f349..61d642065694 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5004,11 +5004,13 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle std::vector warnings; // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it + bool was_loaded = false; if (auto wallet = GetWallet(context, wallet_name)) { if (!RemoveWallet(context, wallet, /*load_on_start=*/std::nullopt, warnings)) { return util::Error{_("Unable to unload the wallet before migrating")}; } UnloadWallet(std::move(wallet)); + was_loaded = true; } // Load the wallet but only in the context of this function. @@ -5029,8 +5031,20 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle return util::Error{Untranslated("Wallet loading failed.") + Untranslated(" ") + error}; } + // Helper to reload as normal for some of our exit scenarios + const auto& reload_wallet = [&](std::shared_ptr& to_reload) { + assert(to_reload.use_count() == 1); + std::string name = to_reload->GetName(); + to_reload.reset(); + to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); + return to_reload != nullptr; + }; + // Before anything else, check if there is something to migrate. if (!local_wallet->GetLegacyScriptPubKeyMan()) { + if (was_loaded) { + reload_wallet(local_wallet); + } return util::Error{_("Error: This wallet is already a descriptor wallet")}; } @@ -5053,27 +5067,33 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", backup_prefix, GetTime())); fs::path backup_path = this_wallet_dir / backup_filename; if (!local_wallet->BackupWallet(fs::PathToString(backup_path))) { + if (was_loaded) { + reload_wallet(local_wallet); + } return util::Error{_("Error: Unable to make a backup of your wallet")}; } res.backup_path = backup_path; bool success = false; - { - LOCK(local_wallet->cs_wallet); - // Unlock the wallet if needed - if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) { - if (passphrase.find('\0') == std::string::npos) { - return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; - } else { - return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " - "The passphrase contains a null character (ie - a zero byte). " - "If this passphrase was set with a version of this software prior to 25.0, " - "please try again with only the characters up to — but not including — " - "the first null character.")}; - } + // Unlock the wallet if needed + if (local_wallet->IsLocked() && !local_wallet->Unlock(passphrase)) { + if (was_loaded) { + reload_wallet(local_wallet); + } + if (passphrase.find('\0') == std::string::npos) { + return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect.")}; + } else { + return util::Error{Untranslated("Error: Wallet decryption failed, the wallet passphrase entered was incorrect. " + "The passphrase contains a null character (ie - a zero byte). " + "If this passphrase was set with a version of this software prior to 25.0, " + "please try again with only the characters up to — but not including — " + "the first null character.")}; } + } + { + LOCK(local_wallet->cs_wallet); // First change to using SQLite if (!local_wallet->MigrateToSQLite(error)) return util::Error{error}; From c305aab8318fbbf009cadce749e9a13a60494ea2 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 01:10:11 -0400 Subject: [PATCH 2/4] test: make sure that migration test does not rescan on reloading Backport of bitcoin/bitcoin@71cb28ea8cb5 from bitcoin#28868. We want to make sure that all of the transactions are being copied to the watchonly and solvable wallets as expected. The automatic rescanning behavior can cause us to pass a test by finding the transaction on loading rather than having it be copied as expected. The helper also reloads the wallet before each migration, so it fails if an earlier failed migratewallet call left the wallet unloaded. The Dash-only test_wallet_name_with_slashes migration goes through the helper as well. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/functional/wallet_migration.py | 44 ++++++++++++++++++----------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 0899c7d3cb5f..63f8821e8db1 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -51,6 +51,18 @@ def create_legacy_wallet(self, wallet_name, disable_private_keys=False, blank=Fa assert_equal(info["private_keys_enabled"], not disable_private_keys) return wallet + def migrate_wallet(self, wallet_rpc, *args, **kwargs): + # Helper to ensure that only migration happens + # Since we may rescan on loading of a wallet, make sure that the best block + # is written before beginning migration + # Reload to force write that record + wallet_name = wallet_rpc.getwalletinfo()["walletname"] + wallet_rpc.unloadwallet() + self.nodes[0].loadwallet(wallet_name) + # Migrate, checking that rescan does not occur + with self.nodes[0].assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]): + return wallet_rpc.migratewallet(*args, **kwargs) + def assert_addr_info_equal(self, addr_info, addr_info_old): assert_equal(addr_info["address"], addr_info_old["address"]) assert_equal(addr_info["scriptPubKey"], addr_info_old["scriptPubKey"]) @@ -90,7 +102,7 @@ def test_basic(self): legacy_mnemonic_passphrase = legacy_hdinfo["mnemonicpassphrase"] # Note: migration could take a while. - basic0.migratewallet() + self.migrate_wallet(basic0) # Verify created descriptors assert_equal(basic0.getwalletinfo()["descriptors"], True) @@ -141,7 +153,7 @@ def test_basic(self): txs = basic1.listtransactions() addr_gps = basic1.listaddressgroupings() - basic1_migrate = basic1.migratewallet() + basic1_migrate = self.migrate_wallet(basic1) assert_equal(basic1.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("basic1") assert_equal(basic1.getbalance(), bal) @@ -198,7 +210,7 @@ def test_basic(self): basic2_txs = basic2.listtransactions() # Now migrate and test that we still see have the same balance/transactions - basic2.migratewallet() + self.migrate_wallet(basic2) assert_equal(basic2.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("basic2") assert_equal(basic2.getbalance(), basic2_balance) @@ -220,7 +232,7 @@ def test_multisig(self): ms_info = multisig0.addmultisigaddress(2, [addr1, addr2, addr3]) - multisig0.migratewallet() + self.migrate_wallet(multisig0) assert_equal(multisig0.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("multisig0") ms_addr_info = multisig0.getaddressinfo(ms_info["address"]) @@ -255,7 +267,7 @@ def test_multisig(self): # Migrating multisig1 should see the multisig is no longer part of multisig1 # A new wallet multisig1_watchonly is created which has the multisig address # Transaction to multisig is in multisig1_watchonly and not multisig1 - multisig1.migratewallet() + self.migrate_wallet(multisig1) assert_equal(multisig1.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("multisig1") assert_equal(multisig1.getaddressinfo(addr1)["ismine"], False) @@ -333,7 +345,7 @@ def test_other_watchonly(self): assert_equal(len(imports0.listtransactions(include_watchonly=True)), 4) # Migrate - imports0.migratewallet() + self.migrate_wallet(imports0) assert_equal(imports0.getwalletinfo()["descriptors"], True) self.assert_is_sqlite("imports0") assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_watchonly_txid) @@ -382,7 +394,7 @@ def test_no_privkeys(self): default.sendtoaddress(addr, 10) self.generate(self.nodes[0], 1) - watchonly0.migratewallet() + self.migrate_wallet(watchonly0) assert_equal("watchonly0_watchonly" in self.nodes[0].listwallets(), False) info = watchonly0.getwalletinfo() assert_equal(info["descriptors"], True) @@ -414,7 +426,7 @@ def test_no_privkeys(self): # Before migrating, we can fetch addr1 from the keypool assert_equal(watchonly1.getnewaddress(), addr1) - watchonly1.migratewallet() + self.migrate_wallet(watchonly1) info = watchonly1.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["private_keys_enabled"], False) @@ -434,7 +446,7 @@ def test_pk_coinbases(self): bals = wallet.getbalances() - wallet.migratewallet() + self.migrate_wallet(wallet) assert_equal(bals, wallet.getbalances()) @@ -453,7 +465,7 @@ def test_encrypted(self): assert_raises_rpc_error(-4, "Error: Wallet decryption failed, the wallet passphrase was not provided or was incorrect", wallet.migratewallet, None, "badpass") assert_raises_rpc_error(-4, "The passphrase contains a null character", wallet.migratewallet, None, "pass\0with\0null") - wallet.migratewallet(passphrase="pass") + self.migrate_wallet(wallet, passphrase="pass") info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) @@ -521,7 +533,7 @@ def test_wallet_name_with_slashes(self): wallet = self.create_legacy_wallet(nested_name) wallet.getnewaddress() - migrate_result = wallet.migratewallet() + migrate_result = self.migrate_wallet(wallet) backup_path = migrate_result["backup_path"] # Backup must live directly inside the wallet's own directory. @@ -540,7 +552,7 @@ def test_default_wallet(self): self.log.info("Test migration of the wallet named as the empty string") wallet = self.create_legacy_wallet("") - wallet.migratewallet() + self.migrate_wallet(wallet) info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["format"], "sqlite") @@ -562,7 +574,7 @@ def test_direct_file(self): assert_equal(info["descriptors"], False) assert_equal(info["format"], "bdb") - wallet.migratewallet() + self.migrate_wallet(wallet) info = wallet.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["format"], "sqlite") @@ -595,7 +607,7 @@ def send_to_script(script, amount): assert_equal(wallet.getbalances()['watchonly']['trusted'], 2) # Migrate wallet and re-check balance - info_migration = wallet.migratewallet() + info_migration = self.migrate_wallet(wallet) wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) # Watch-only balance is under "mine". @@ -630,7 +642,7 @@ def test_hybrid_pubkey(self): assert_equal(p2pkh_addr_info["iswatchonly"], True) assert_equal(p2pkh_addr_info["ismine"], False) # Things involving hybrid pubkeys are not spendable - migrate_info = wallet.migratewallet() + migrate_info = self.migrate_wallet(wallet) # The address should only appear in the watchonly wallet p2pkh_addr_info = wallet.getaddressinfo(p2pkh_addr) @@ -689,7 +701,7 @@ def test_conflict_txs(self): assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) - wallet.migratewallet() + self.migrate_wallet(wallet) assert_equal(wallet.gettransaction(txid=parent_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=child_txid)["confirmations"], -1) assert_equal(wallet.gettransaction(txid=conflict_txid)["confirmations"], 1) From 03821b7bdcad5634399f4d5b74f172f29c59d376 Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 01:12:08 -0400 Subject: [PATCH 3/4] fix(wallet): keep txs that belong to both watchonly and migrated wallets Backport of bitcoin/bitcoin@c62a8d03a862 from bitcoin#28868. It is possible for a transaction that has an output that belongs to the migrated wallet, and another output that belongs to the watchonly wallet. Such transaction should appear in both wallets during migration. Adapted to Dash: the watchonly copy still goes through AddToWallet() because bitcoin#28125 (LoadToWallet()/CopyFrom() and the shared watchonly WalletBatch) has not been backported. Co-Authored-By: Claude Opus 5.5 (1M context) --- src/wallet/wallet.cpp | 29 ++++++++++++++++------------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 61d642065694..45ba30f211a1 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4757,22 +4757,25 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // We need to go through these in the tx insertion order so that lookups to spends works. std::vector txids_to_delete; for (const auto& [_pos, wtx] : wtxOrdered) { - if (!IsMine(*wtx->tx) && !IsFromMe(*wtx->tx)) { - // Check it is the watchonly wallet's - // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for - if (data.watchonly_wallet) { - LOCK(data.watchonly_wallet->cs_wallet); - if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { - // Add to watchonly wallet - if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) { - error = _("Error: Could not add watchonly tx to watchonly wallet"); - return false; - } - // Mark as to remove from this wallet + // Check it is the watchonly wallet's + // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for + bool is_mine = IsMine(*wtx->tx) || IsFromMe(*wtx->tx); + if (data.watchonly_wallet) { + LOCK(data.watchonly_wallet->cs_wallet); + if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) { + // Add to watchonly wallet + if (!data.watchonly_wallet->AddToWallet(wtx->tx, wtx->m_state)) { + error = _("Error: Could not add watchonly tx to watchonly wallet"); + return false; + } + // Mark as to remove from the migrated wallet only if it does not also belong to it + if (!is_mine) { txids_to_delete.push_back(wtx->GetHash()); - continue; } + continue; } + } + if (!is_mine) { // Both not ours and not in the watchonly wallet error = strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex()); return false; From 17b05897c6f8d67eb9e6a90edb3a734d7bf4ed1d Mon Sep 17 00:00:00 2001 From: pasta Date: Sun, 4 Oct 2026 01:12:08 -0400 Subject: [PATCH 4/4] test: test migration of tx with both spendable and watchonly Backport of bitcoin/bitcoin@4da76ca24725 from bitcoin#28868. Co-Authored-By: Claude Opus 5.5 (1M context) --- test/functional/wallet_migration.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 63f8821e8db1..c5cc9737b60c 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -337,12 +337,15 @@ def test_other_watchonly(self): send = default.sendall(recipients=[default.getnewaddress()], options={"inputs": [{"txid": received_sent_watchonly_txid, "vout": received_sent_watchonly_vout}]}) sent_watchonly_txid = send["txid"] - self.generate(self.nodes[0], 1) + # Tx that has both a watchonly and spendable output + watchonly_spendable_txid = default.send(outputs=[{received_addr: 1}, {import_addr:1}])["txid"] + + self.generate(self.nodes[0], 2) balances = imports0.getbalances() spendable_bal = balances["mine"]["trusted"] watchonly_bal = balances["watchonly"]["trusted"] - assert_equal(len(imports0.listtransactions(include_watchonly=True)), 4) + assert_equal(len(imports0.listtransactions(include_watchonly=True)), 6) # Migrate self.migrate_wallet(imports0) @@ -351,8 +354,9 @@ def test_other_watchonly(self): assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_watchonly_txid) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_sent_watchonly_txid) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, sent_watchonly_txid) - assert_equal(len(imports0.listtransactions(include_watchonly=True)), 1) + assert_equal(len(imports0.listtransactions(include_watchonly=True)), 2) imports0.gettransaction(received_txid) + imports0.gettransaction(watchonly_spendable_txid) assert_equal(imports0.getbalance(), spendable_bal) assert_equal("imports0_watchonly" in self.nodes[0].listwallets(), True) @@ -364,9 +368,10 @@ def test_other_watchonly(self): watchonly.gettransaction(received_watchonly_txid) watchonly.gettransaction(received_sent_watchonly_txid) watchonly.gettransaction(sent_watchonly_txid) + watchonly.gettransaction(watchonly_spendable_txid) assert_equal(watchonly.getbalance(), watchonly_bal) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", watchonly.gettransaction, received_txid) - assert_equal(len(watchonly.listtransactions(include_watchonly=True)), 3) + assert_equal(len(watchonly.listtransactions(include_watchonly=True)), 4) # Check that labels were migrated and persisted to watchonly wallet self.nodes[0].unloadwallet("imports0_watchonly")