diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 1946e43eac4b0..3b5971591768a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3927,6 +3927,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } } + // Get best block locator so that we can copy it to the watchonly and solvables + CBlockLocator best_block_locator; + if (!WalletBatch(GetDatabase()).ReadBestBlock(best_block_locator)) { + error = _("Error: Unable to read wallet's best block locator record"); + return false; + } + // Check if the transactions in the wallet are still ours. Either they belong here, or they belong in the watchonly wallet. // We need to go through these in the tx insertion order so that lookups to spends works. std::vector txids_to_delete; @@ -3937,32 +3944,47 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) LOCK(data.watchonly_wallet->cs_wallet); data.watchonly_wallet->nOrderPosNext = nOrderPosNext; watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext); + // Write the best block locator to avoid rescanning on reload + if (!watchonly_batch->WriteBestBlock(best_block_locator)) { + error = _("Error: Unable to write watchonly wallet best block locator record"); + return false; + } + } + if (data.solvable_wallet) { + // Write the best block locator to avoid rescanning on reload + if (!WalletBatch(data.solvable_wallet->GetDatabase()).WriteBestBlock(best_block_locator)) { + error = _("Error: Unable to write solvable wallet best block locator record"); + return false; + } } 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 - const uint256& hash = wtx->GetHash(); - const CWalletTx& to_copy_wtx = *wtx; - if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { - if (!new_tx) return false; - ins_wtx.SetTx(to_copy_wtx.tx); - ins_wtx.CopyFrom(to_copy_wtx); - return true; - })) { - error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); - return false; - } - watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); - // 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 + const uint256& hash = wtx->GetHash(); + const CWalletTx& to_copy_wtx = *wtx; + if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) { + if (!new_tx) return false; + ins_wtx.SetTx(to_copy_wtx.tx); + ins_wtx.CopyFrom(to_copy_wtx); + return true; + })) { + error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex()); + return false; + } + watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash)); + // 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(hash); - 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; @@ -4194,11 +4216,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. @@ -4219,8 +4243,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->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + if (was_loaded) { + reload_wallet(local_wallet); + } return util::Error{_("Error: This wallet is already a descriptor wallet")}; } @@ -4229,27 +4265,33 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle fs::path backup_filename = fs::PathFromString(strprintf("%s-%d.legacy.bak", wallet_name, 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}; @@ -4270,24 +4312,19 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle std::set wallet_dirs; if (success) { // Migration successful, unload all wallets locally, then reload them. - const auto& reload_wallet = [&](std::shared_ptr& to_reload) { - assert(to_reload.use_count() == 1); - std::string name = to_reload->GetName(); - wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path()); - to_reload.reset(); - to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings); - return to_reload != nullptr; - }; // Reload the main wallet + wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(local_wallet); res.wallet = local_wallet; res.wallet_name = wallet_name; if (success && res.watchonly_wallet) { // Reload watchonly + wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.watchonly_wallet); } if (success && res.solvables_wallet) { // Reload solvables + wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path()); success = reload_wallet(res.solvables_wallet); } } diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index b466d3c54520d..f9919716be655 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -60,6 +60,18 @@ def create_legacy_wallet(self, wallet_name, **kwargs): assert_equal(info["format"], "bdb") 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"]) @@ -104,7 +116,7 @@ def test_basic(self): assert_equal(old_change_addr_info["hdkeypath"], "m/0'/1'/0'") # Note: migration could take a while. - basic0.migratewallet() + self.migrate_wallet(basic0) # Verify created descriptors assert_equal(basic0.getwalletinfo()["descriptors"], True) @@ -145,7 +157,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) @@ -186,7 +198,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) @@ -208,7 +220,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"]) @@ -243,7 +255,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) @@ -313,27 +325,31 @@ def test_other_watchonly(self): send = default.sendall(recipients=[default.getnewaddress()], inputs=[received_sent_watchonly_utxo]) 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) received_watchonly_tx_info = imports0.gettransaction(received_watchonly_txid, True) received_sent_watchonly_tx_info = imports0.gettransaction(received_sent_watchonly_utxo["txid"], True) 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) # Mock time forward a bit so we can check that tx metadata is preserved self.nodes[0].setmocktime(int(time.time()) + 100) # 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) assert_raises_rpc_error(-5, "Invalid or non-wallet transaction id", imports0.gettransaction, received_sent_watchonly_utxo['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) @@ -349,9 +365,10 @@ def test_other_watchonly(self): assert_equal(received_sent_watchonly_tx_info["time"], received_sent_migrated_watchonly_tx_info["time"]) assert_equal(received_sent_watchonly_tx_info["timereceived"], received_sent_migrated_watchonly_tx_info["timereceived"]) 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") @@ -379,7 +396,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) @@ -411,7 +428,7 @@ def test_no_privkeys(self): # Before migrating, we can fetch addr1 from the keypool assert_equal(watchonly1.getnewaddress(address_type="bech32"), addr1) - watchonly1.migratewallet() + self.migrate_wallet(watchonly1) info = watchonly1.getwalletinfo() assert_equal(info["descriptors"], True) assert_equal(info["private_keys_enabled"], False) @@ -431,7 +448,7 @@ def test_pk_coinbases(self): bals = wallet.getbalances() - wallet.migratewallet() + self.migrate_wallet(wallet) assert_equal(bals, wallet.getbalances()) @@ -450,7 +467,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) @@ -512,7 +529,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") @@ -534,7 +551,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") @@ -622,7 +639,7 @@ def check(info, node): check(addr_info, wallet) # Migrate wallet - info_migration = wallet.migratewallet() + info_migration = self.migrate_wallet(wallet) wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"]) wallet_solvables = self.nodes[0].get_wallet_rpc(info_migration["solvables_name"]) @@ -717,7 +734,7 @@ def send_to_script(script, amount): wallet.rpc.importaddress(address=script_sh_pkh.hex(), label=label_sh_pkh, rescan=False, p2sh=True) # 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". @@ -780,7 +797,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) @@ -813,7 +830,7 @@ def test_hybrid_pubkey(self): p2wpkh_addr = key_to_p2wpkh(hybrid_pubkey) wallet.importaddress(p2wpkh_addr) - migrate_info = wallet.migratewallet() + migrate_info = self.migrate_wallet(wallet) # Both addresses should only appear in the watchonly wallet p2pkh_addr_info = wallet.getaddressinfo(p2pkh_addr)