Skip to content

Commit

Permalink
wallet: migration, don't create spendable wallet from a watch-only le…
Browse files Browse the repository at this point in the history 
…gacy wallet

Currently, the migration process creates a brand-new descriptor wallet with no
connection to the user's legacy wallet when the legacy wallet lacks key material
and contains only watch-only scripts. This behavior is not aligned with user
expectations. If the legacy wallet contains only watch-only scripts, the migration
process should only generate a watch-only wallet instead.

This commit implements this behavior.
  • Loading branch information
@furszy
furszy committed Feb 14, 2025
1 parent a48d810 commit 33b1707
Show file tree
Hide file tree
Showing 2 changed files with 60 additions and 9 deletions.
52 changes: 44 additions & 8 deletions src/wallet/wallet.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4092,6 +4092,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))};
}

// When the legacy wallet has no spendable scripts, it will be replaced by the watch-only/solvable wallets.
bool empty_local_wallet = data.desc_spkms.empty() && !data.master_key.key.IsValid();

// Get all invalid or non-watched scripts that will not be migrated
std::set<CTxDestination> not_migrated_dests;
for (const auto& script : legacy_spkm->GetNotMineScriptPubKeys()) {
Expand Down Expand Up @@ -4123,9 +4126,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
m_external_spk_managers.clear();
m_internal_spk_managers.clear();

// Setup new descriptors
// Setup new descriptors (only if we are migrating any key material)
SetWalletFlagWithDB(local_wallet_batch, WALLET_FLAG_DESCRIPTORS);
if (!IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
if (!empty_local_wallet && !IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
// Use the existing master key if we have it
if (data.master_key.key.IsValid()) {
SetupDescriptorScriptPubKeyMans(local_wallet_batch, data.master_key);
Expand Down Expand Up @@ -4275,6 +4278,14 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
}
}

// If there was no key material in the main wallet, there should be no records on it anymore.
// This wallet will be discarded at the end of the process. Only wallets that contain the
// migrated records will be presented to the user.
if (empty_local_wallet) {
if (!m_address_book.empty()) return util::Error{_("Error: Not all address book records were migrated")};
if (!mapWallet.empty()) return util::Error{_("Error: Not all transaction records were migrated")};
}

return {}; // all good
}

Expand All @@ -4283,7 +4294,7 @@ bool CWallet::CanGrindR() const
return !IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER);
}

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& empty_local_wallet) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
{
AssertLockHeld(wallet.cs_wallet);

Expand Down Expand Up @@ -4392,6 +4403,11 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
return false;
}
wallet.WalletLogPrintf("Wallet migration complete.\n");

// When the legacy wallet had no spendable scripts, the local wallet will contain no information after migration.
// In such case, we notify the caller to not add it to the result. The user is not expecting to have a spendable
// wallet.
empty_local_wallet = data->desc_spkms.empty() && !data->master_key.key.IsValid();
return true;
});
}
Expand Down Expand Up @@ -4513,6 +4529,14 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
}
}

// Indicates whether the current wallet is empty after migration.
// Notes:
// When non-empty: the local wallet becomes the main spendable wallet.
// When empty: The local wallet is excluded from the result, as the
// user does not expect a spendable wallet after migrating
// only watch-only scripts.
bool empty_local_wallet = false;

{
LOCK(local_wallet->cs_wallet);
// First change to using SQLite
Expand All @@ -4521,7 +4545,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Do the migration of keys and scripts for non-blank wallets, and cleanup if it fails
success = local_wallet->IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET);
if (!success) {
success = DoMigration(*local_wallet, context, error, res);
success = DoMigration(*local_wallet, context, error, res, empty_local_wallet);
} else {
// Make sure that descriptors flag is actually set
local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
Expand All @@ -4535,20 +4559,32 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
std::set<fs::path> wallet_dirs;
if (success) {
// Migration successful, unload all wallets locally, then reload them.
// 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;
// Reload and set the main spendable wallet only if needed
if (!empty_local_wallet) {
success = reload_wallet(local_wallet);
res.wallet = local_wallet;
res.wallet_name = wallet_name;
} else {
// At this point, the main wallet is empty after migration, meaning it has no records.
// Therefore, we can safely remove it.
std::vector<fs::path> paths_to_remove = local_wallet->GetDatabase().Files();
local_wallet.reset();
for (const auto& path_to_remove : paths_to_remove) fs::remove_all(path_to_remove);
}
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);
// When no spendable wallet is created, set the wallet name to the watch-only wallet name
if (res.wallet_name.empty()) res.wallet_name = res.watchonly_wallet->GetName();
}
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);
// When no spendable neither watch-only wallets are created, set the wallet name to the solvables-only wallet name
if (res.wallet_name.empty()) res.wallet_name = res.solvables_wallet->GetName();
}
}
if (!success) {
Expand Down
17 changes: 16 additions & 1 deletion test/functional/wallet_migration.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
# Distributed under the MIT software license, see the accompanying
# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test Migrating a wallet from legacy to descriptor."""

import os.path
import random
import shutil
import struct
Expand Down Expand Up @@ -114,9 +114,14 @@ def migrate_and_get_rpc(self, wallet_name, **kwargs):
# Migrate, checking that rescan does not occur
with self.master_node.assert_debug_log(expected_msgs=[], unexpected_msgs=["Rescanning"]):
migrate_info = self.master_node.migratewallet(wallet_name=wallet_name, **kwargs)
# Update wallet name in case the initial wallet was completely migrated to a watch-only wallet
# (in which case the wallet name would be suffixed by the 'watchonly' term)
wallet_name = migrate_info['wallet_name']
wallet = self.master_node.get_wallet_rpc(wallet_name)
assert_equal(wallet.getwalletinfo()["descriptors"], True)
self.assert_is_sqlite(wallet_name)
# Always verify backup path exist after migration
assert os.path.exists(migrate_info['backup_path'])
return migrate_info, wallet

def test_basic(self):
Expand Down Expand Up @@ -1021,6 +1026,12 @@ def test_migrate_simple_watch_only(self):
res, _ = self.migrate_and_get_rpc("bare_p2pk")
wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name'])
assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})'))

# Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet.
assert_equal('bare_p2pk_watchonly', res['wallet_name'])
assert "bare_p2pk" not in self.master_node.listwallets()
assert "bare_p2pk" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]]

wo_wallet.unloadwallet()

def test_manual_keys_import(self):
Expand Down Expand Up @@ -1061,6 +1072,10 @@ def test_manual_keys_import(self):
# Verify all expected descriptors were migrated
migrated_desc = [item['desc'] for item in wo_wallet.listdescriptors()['descriptors']]
assert_equal(expected_descs, migrated_desc)
# Ensure that migrating a wallet with watch-only scripts does not create a spendable wallet.
assert_equal('import_pubkeys_watchonly', res['wallet_name'])
assert "import_pubkeys" not in self.master_node.listwallets()
assert "import_pubkeys" not in [w["name"] for w in self.master_node.listwalletdir()["wallets"]]
wo_wallet.unloadwallet()

def test_p2wsh(self):
Expand Down

0 comments on commit 33b1707

Please sign in to comment.