cryptogera/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-14 15:39:09 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#31423: wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet

Browse Source 
b78990734621b8fe46c68a6e7edaf1fbd2f7d351 wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet (furszy)
e86d71b749c08bde6002b9aa2baee824975a518a wallet: refactor, dedup wallet re-loading code (furszy)
1de423e0a08bbc63eed36c8772e9ef8b48e80fb8 wallet: introduce method to return all db created files (furszy)
d04f6a97ba9a55aa9455e1a805feeed4d630f59a refactor: remove sqlite dir path back-and-forth conversion (furszy)

Pull request description:

  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.

  TODO List:
  * Explain that `migratewallet` renames the watch-only after migration, and
  also that the wallet will not have keys enabled.

ACKs for top commit:
  achow101:
    ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
  pablomartin4btc:
    tACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351
  rkrux:
    LGTM ACK b78990734621b8fe46c68a6e7edaf1fbd2f7d351

Tree-SHA512: 1d583ac4b206fb477e9727daf4b5ad9c3e18b12d40e1ab4a61e8565da44c3d0327c892b51cf47b4894405d122e414cefb6b6366c357e02a74a7ca96e06762d83
This commit is contained in:
Ava Chow 2025-07-02 13:25:33 -07:00
commit 35cae56a92
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41
8 changed files with 86 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
src/qt/walletcontroller.cpp
View File

@ -468,7 +468,7 @@ void MigrateWalletActivity::migrate(const std::string& name)
auto res{node().walletLoader().migrateWallet(name, passphrase)};
if (res) {
m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->wallet->getWalletName())));
m_success_message = tr("The wallet '%1' was migrated successfully.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(name)));
if (res->watchonly_wallet_name) {
m_success_message += QChar(' ') + tr("Watchonly scripts have been migrated to a new wallet named '%1'.").arg(GUIUtil::HtmlEscape(GUIUtil::WalletDisplayName(res->watchonly_wallet_name.value())));
}

3
src/wallet/db.h
View File

@ -155,6 +155,9 @@ public:
/** Return path to main database file for logs and error messages. */
virtual std::string Filename() = 0;
/** Return paths to all database created files */
virtual std::vector<fs::path> Files() = 0;
virtual std::string Format() = 0;
/** Make a DatabaseBatch connected to this database */

1
src/wallet/migrate.h
View File

@ -50,6 +50,7 @@ public:
/** Return path to main database file for logs and error messages. */
std::string Filename() override { return fs::PathToString(m_filepath); }
std::vector<fs::path> Files() override { return {m_filepath}; }
std::string Format() override { return "bdb_ro"; }

6
src/wallet/sqlite.cpp
View File

@ -112,12 +112,12 @@ Mutex SQLiteDatabase::g_sqlite_mutex;
int SQLiteDatabase::g_sqlite_count = 0;
SQLiteDatabase::SQLiteDatabase(const fs::path& dir_path, const fs::path& file_path, const DatabaseOptions& options, bool mock)
: WalletDatabase(), m_mock(mock), m_dir_path(fs::PathToString(dir_path)), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync)
: WalletDatabase(), m_mock(mock), m_dir_path(dir_path), m_file_path(fs::PathToString(file_path)), m_write_semaphore(1), m_use_unsafe_sync(options.use_unsafe_sync)
{
{
LOCK(g_sqlite_mutex);
LogPrintf("Using SQLite Version %s\n", SQLiteDatabaseVersion());
LogPrintf("Using wallet %s\n", m_dir_path);
LogPrintf("Using wallet %s\n", fs::PathToString(m_dir_path));
if (++g_sqlite_count == 1) {
// Setup logging
@ -253,7 +253,7 @@ void SQLiteDatabase::Open()
if (m_db == nullptr) {
if (!m_mock) {
TryCreateDirectories(fs::PathFromString(m_dir_path));
TryCreateDirectories(m_dir_path);
}
int ret = sqlite3_open_v2(m_file_path.c_str(), &m_db, flags, nullptr);
if (ret != SQLITE_OK) {

10
src/wallet/sqlite.h
View File

@ -104,7 +104,7 @@ class SQLiteDatabase : public WalletDatabase
private:
const bool m_mock{false};
const std::string m_dir_path;
const fs::path m_dir_path;
const std::string m_file_path;
@ -147,6 +147,14 @@ public:
bool Backup(const std::string& dest) const override;
std::string Filename() override { return m_file_path; }
/** Return paths to all database created files */
std::vector<fs::path> Files() override
{
std::vector<fs::path> files;
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path));
files.emplace_back(m_dir_path / fs::PathFromString(m_file_path + "-journal"));
return files;
}
std::string Format() override { return "sqlite"; }
/** Make a SQLiteBatch connected to this database */

1
src/wallet/test/util.h
View File

@ -109,6 +109,7 @@ public:
void Close() override {}
std::string Filename() override { return "mockable"; }
std::vector<fs::path> Files() override { return {}; }
std::string Format() override { return "mock"; }
std::unique_ptr<DatabaseBatch> MakeBatch() override { return std::make_unique<MockableBatch>(m_records, m_pass); }
};

63
src/wallet/wallet.cpp
View File

@ -3846,6 +3846,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
return util::Error{Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing"))};
}
// Note: when the legacy wallet has no spendable scripts, it must be empty at the end of the process.
bool has_spendable_material = !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()) {
@ -3877,9 +3880,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 (has_spendable_material && !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);
@ -4033,6 +4036,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 (!has_spendable_material) {
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
}
@ -4270,6 +4281,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 an empty spendable wallet after
// migrating only watch-only scripts.
bool empty_local_wallet = false;
{
LOCK(local_wallet->cs_wallet);
// First change to using SQLite
@ -4278,6 +4297,8 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// Do the migration of keys and scripts for non-empty wallets, and cleanup if it fails
if (HasLegacyRecords(*local_wallet)) {
success = DoMigration(*local_wallet, context, error, res);
// No scripts mean empty wallet after migration
empty_local_wallet = local_wallet->GetAllScriptPubKeyMans().empty();
} else {
// Make sure that descriptors flag is actually set
local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
@ -4291,21 +4312,31 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
// fails to reload.
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;
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);
Assume(!res.wallet); // We will set it here.
// Check if the local wallet is empty after migration
if (empty_local_wallet) {
// This wallet has no records. 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.solvables_wallet) {
// Reload solvables
wallet_dirs.insert(fs::PathFromString(res.solvables_wallet->GetDatabase().Filename()).parent_path());
success = reload_wallet(res.solvables_wallet);
// Migration successful, unload all wallets locally, then reload them.
// Note: We use a pointer to the shared_ptr to avoid increasing its reference count,
// as 'reload_wallet' expects to be the sole owner (use_count == 1).
for (std::shared_ptr<CWallet>* wallet_ptr : {&local_wallet, &res.watchonly_wallet, &res.solvables_wallet}) {
if (success && *wallet_ptr) {
std::shared_ptr<CWallet>& wallet = *wallet_ptr;
// Save db path and reload wallet
wallet_dirs.insert(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path());
success = reload_wallet(wallet);
// When no wallet is set, set the main wallet.
if (!res.wallet) {
res.wallet_name = wallet->GetName();
res.wallet = std::move(wallet);
}
}
}
}
if (!success) {

24
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
@ -121,9 +121,14 @@ 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)
# 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 the backup path exist after migration
assert os.path.exists(migrate_info['backup_path'])
return migrate_info, wallet
def test_basic(self):
@ -1024,8 +1029,15 @@ class WalletMigrationTest(BitcoinTestFramework):
wallet.importaddress(address=p2pk_script.hex())
# Migrate wallet in the latest node
res, _ = self.migrate_and_get_rpc("bare_p2pk")
wo_wallet = self.master_node.get_wallet_rpc(res['watchonly_name'])
wo_wallet = self.master_node.get_wallet_rpc(res['wallet_name'])
assert_equal(wo_wallet.listdescriptors()['descriptors'][0]['desc'], descsum_create(f'pk({pubkey.hex()})'))
assert_equal(wo_wallet.getwalletinfo()["private_keys_enabled"], False)
# 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):
@ -1055,7 +1067,9 @@ class WalletMigrationTest(BitcoinTestFramework):
res, _ = self.migrate_and_get_rpc("import_pubkeys")
# Same as before, there should be descriptors in the watch-only wallet for the imported pubkey
wo_wallet = self.nodes[0].get_wallet_rpc(res['watchonly_name'])
wo_wallet = self.nodes[0].get_wallet_rpc(res['wallet_name'])
# Assert this is a watch-only wallet
assert_equal(wo_wallet.getwalletinfo()["private_keys_enabled"], False)
# As we imported the pubkey only, there will be no key origin in the following descriptors
pk_desc = descsum_create(f'pk({pubkey_hex})')
pkh_desc = descsum_create(f'pkh({pubkey_hex})')
@ -1066,6 +1080,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 test_p2wsh(self):