phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-09 11:27:28 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

wallet: migration, don't create spendable wallet from a watch-only legacy wallet

Browse source 
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.
This commit is contained in:
furszy 2024-12-04 12:48:18 -05:00
parent 788bb48f67
commit 8eb2736de4
No known key found for this signature in database
GPG key ID: 5DD23CCC686AA623
2 changed files with 57 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

52
src/wallet/wallet.cpp
View file

@ -4078,6 +4078,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()) {
@ -4109,9 +4112,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);
@ -4261,6 +4264,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
}
@ -4269,7 +4280,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);
@ -4378,6 +4389,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;
});
}
@ -4487,6 +4503,14 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
}
}
// 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
@ -4495,7 +4519,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
// 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);
@ -4509,20 +4533,32 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
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) {

14
test/functional/wallet_migration.py
View file

@ -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
@ -111,6 +111,8 @@ class WalletMigrationTest(BitcoinTestFramework):
# 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)
# Always verify backup path exist after migration
assert os.path.exists(migrate_info['backup_path'])
return migrate_info, self.master_node.get_wallet_rpc(wallet_name)
def test_basic(self):
@ -1018,6 +1020,12 @@ class WalletMigrationTest(BitcoinTestFramework):
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):
@ -1062,6 +1070,10 @@ class WalletMigrationTest(BitcoinTestFramework):
# 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 run_test(self):