From 8c127ff1edb6b9a607bf1ad247893347252a38e3 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 30 Nov 2023 16:00:02 -0500 Subject: [PATCH 1/4] wallet: Skip key and script migration for blank wallets Blank wallets don't have any keys or scripts to migrate --- src/wallet/wallet.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index bf8cfcb082a..3cf4e9bdd18 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4246,8 +4246,11 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle // First change to using SQLite if (!local_wallet->MigrateToSQLite(error)) return util::Error{error}; - // Do the migration, and cleanup if it fails - success = DoMigration(*local_wallet, context, error, res); + // 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); + } } // In case of reloading failure, we need to remember the wallet dirs to remove From b1d2c771d43b802db12768e620588ed179e92b29 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 30 Nov 2023 16:00:06 -0500 Subject: [PATCH 2/4] wallet: Check for descriptors flag before migration Previously we would check that there is no LegacySPKM in order to determine whether a wallet is already a descriptor wallet and doesn't need to be migrated. However blank legacy wallets will also not have a LegacySPKM, so we need to be checking for the descriptors flag instead. --- src/wallet/wallet.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3cf4e9bdd18..437b274cefd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4213,7 +4213,7 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } // Before anything else, check if there is something to migrate. - if (!local_wallet->GetLegacyScriptPubKeyMan()) { + if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { return util::Error{_("Error: This wallet is already a descriptor wallet")}; } From 563b2a60d6a371f26474410397da435547e58786 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 30 Nov 2023 16:00:08 -0500 Subject: [PATCH 3/4] wallet: Better error message when missing LegacySPKM during migration --- src/wallet/wallet.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 437b274cefd..b260e409e57 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3862,7 +3862,11 @@ std::optional CWallet::GetDescriptorsForLegacy(bilingual_str& err AssertLockHeld(cs_wallet); LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan(); - assert(legacy_spkm); + if (!Assume(legacy_spkm)) { + // This shouldn't happen + error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); + return std::nullopt; + } std::optional res = legacy_spkm->MigrateToDescriptor(); if (res == std::nullopt) { @@ -3877,8 +3881,9 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) AssertLockHeld(cs_wallet); LegacyScriptPubKeyMan* legacy_spkm = GetLegacyScriptPubKeyMan(); - if (!legacy_spkm) { - error = _("Error: This wallet is already a descriptor wallet"); + if (!Assume(legacy_spkm)) { + // This shouldn't happen + error = Untranslated(STR_INTERNAL_BUG("Error: Legacy wallet data missing")); return false; } From c11c404281d2d0e22967e30e16c0733db84f4eee Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Thu, 30 Nov 2023 16:00:12 -0500 Subject: [PATCH 4/4] tests: Test migration of blank wallets --- test/functional/wallet_migration.py | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 3d68dbe07ac..20e92dbef73 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -52,13 +52,12 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(file_magic, b'SQLite format 3\x00') assert_equal(self.nodes[0].get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite") - def create_legacy_wallet(self, wallet_name, disable_private_keys=False): - self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, disable_private_keys=disable_private_keys) + def create_legacy_wallet(self, wallet_name, **kwargs): + self.nodes[0].createwallet(wallet_name=wallet_name, descriptors=False, **kwargs) wallet = self.nodes[0].get_wallet_rpc(wallet_name) info = wallet.getwalletinfo() assert_equal(info["descriptors"], False) assert_equal(info["format"], "bdb") - assert_equal(info["private_keys_enabled"], not disable_private_keys) return wallet def assert_addr_info_equal(self, addr_info, addr_info_old): @@ -876,6 +875,13 @@ class WalletMigrationTest(BitcoinTestFramework): _, _, magic = struct.unpack("QII", data) assert_equal(magic, BTREE_MAGIC) + def test_blank(self): + self.log.info("Test that a blank wallet is migrated") + wallet = self.create_legacy_wallet("blank", blank=True) + assert_equal(wallet.getwalletinfo()["blank"], True) + wallet.migratewallet() + assert_equal(wallet.getwalletinfo()["blank"], True) + def test_avoidreuse(self): self.log.info("Test that avoidreuse persists after migration") @@ -987,6 +993,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_failed_migration_cleanup() self.test_avoidreuse() self.test_preserve_tx_extra_info() + self.test_blank() if __name__ == '__main__': WalletMigrationTest().main()