Skip to content

backport: partial bitcoin#28868 (wallet: reload wallet if migration exited early, keep mixed watchonly txs) #7793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
PastaPastaPasta merged 4 commits into from
Oct 6, 2026
Merged

backport: partial bitcoin#28868 (wallet: reload wallet if migration exited early, keep mixed watchonly txs) #7793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1f9bf08
fix(wallet): reload the wallet if migration exited early
PastaPastaPasta Oct 4, 2026
c305aab
test: make sure that migration test does not rescan on reloading
PastaPastaPasta Oct 4, 2026
03821b7
fix(wallet): keep txs that belong to both watchonly and migrated wallets
PastaPastaPasta Oct 4, 2026
17b0589
test: test migration of tx with both spendable and watchonly
PastaPastaPasta Oct 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 49 additions & 26 deletions src/wallet/wallet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint256> 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;
Expand Down Expand Up @@ -5004,11 +5007,13 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
std::vector<bilingual_str> 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.
Expand All @@ -5029,8 +5034,20 @@ util::Result<MigrationResult> 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<CWallet>& 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")};
}

Expand All @@ -5053,27 +5070,33 @@ util::Result<MigrationResult> 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};

Expand Down
57 changes: 37 additions & 20 deletions test/functional/wallet_migration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +54 to +64

@thepastaclaw thepastaclaw Oct 4, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⏭️ Deferred (maintainer decision; not blocking) at 17b05897; see the replies below.

🟡 Suggestion: Missing test prerequisite: bitcoin#28038 addressbook migration scenario

Upstream commit 71cb28e also routes the migration in test_addressbook through self.migrate_wallet(wallet), adding the no-Rescanning assertion to a scenario that creates both auxiliary wallets and checks migrated addressbook data. That scenario was introduced by 7ecc29a in bitcoin#28038 and is absent from both the Dash base and this head, so this upstream test hunk has not been carried over. The existing multisig scenario exercises both auxiliary wallets, but it does not reproduce the addressbook assertions. This does not establish a prerequisite for either production fix, and the PR explicitly advertises a partial backport; however, its omission list does not identify this additional dropped test hunk. Add the adapted scenario and use this helper, or explicitly document the omitted transformation and its deferred test prerequisite.

source: gpt-6.1-sol (phase2-reviewer: general, backport-reviewer, dash-core-commit-history)

@PastaPastaPasta PastaPastaPasta Oct 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that it was undocumented. The hunk cannot be carried over because test_addressbook comes from bitcoin#28038, which is not backported, so there is nothing for it to change here. The omission is now listed under "Dash adaptations and omitted hunks" in the PR description.

🤖 Posted autonomously by Claude on behalf of pasta.

@thepastaclaw thepastaclaw Oct 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that test_addressbook is absent from both the Dash base and this head, and the updated PR description now explicitly identifies the omitted helper transformation and its bitcoin#28038 prerequisite; that addresses my documentation concern, and I’m satisfied to leave the scenario outside this backport.

Noted as intentionally deferred; I will not press it further here.


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"])
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand All @@ -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"])
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -325,22 +337,26 @@ 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
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_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)
Expand All @@ -352,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")
Expand Down Expand Up @@ -382,7 +399,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)
Expand Down Expand Up @@ -414,7 +431,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)
Expand All @@ -434,7 +451,7 @@ def test_pk_coinbases(self):

bals = wallet.getbalances()

wallet.migratewallet()
self.migrate_wallet(wallet)

assert_equal(bals, wallet.getbalances())

Expand All @@ -453,7 +470,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)
Expand Down Expand Up @@ -521,7 +538,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.
Expand All @@ -540,7 +557,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")
Expand All @@ -562,7 +579,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")
Expand Down Expand Up @@ -595,7 +612,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".
Expand Down Expand Up @@ -630,7 +647,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)
Expand Down Expand Up @@ -689,7 +706,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)
Expand Down
Loading