Merge bitcoin/bitcoin#28976: wallet: Fix migration of blank wallets

c11c404281d2d0e22967e30e16c0733db84f4eee tests: Test migration of blank wallets (Andrew Chow)
563b2a60d6a371f26474410397da435547e58786 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow)
b1d2c771d43b802db12768e620588ed179e92b29 wallet: Check for descriptors flag before migration (Andrew Chow)
8c127ff1edb6b9a607bf1ad247893347252a38e3 wallet: Skip key and script migration for blank wallets (Andrew Chow)

Pull request description:

  Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets.

  Fixes the issue mentioned in https://github.com/bitcoin/bitcoin/pull/28868#issuecomment-1809641110

ACKs for top commit:
  furszy:
    reACK c11c404281d2d0e22967e30e16c0733db84f4eee. CI failure is unrelated.
  ryanofsky:
    Code review ACK c11c404281d2d0e22967e30e16c0733db84f4eee

Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4

2 files changed, 24 insertions, 9 deletions

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index e93cd4b4b9..33b3ad6e91 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3864,7 +3864,11 @@ std::optional<MigrationData> 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<MigrationData> res = legacy_spkm->MigrateToDescriptor();
     if (res == std::nullopt) {
@@ -3879,8 +3883,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;
     }
 
@@ -4215,7 +4220,7 @@ util::Result<MigrationResult> 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")};
     }
 
@@ -4248,8 +4253,11 @@ util::Result<MigrationResult> 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
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 3d68dbe07a..20e92dbef7 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()