Merge bitcoin/bitcoin#32149: wallet, migration: Fix empty wallet crash

0f602c5693 wallet, migration: Fix crash on empty wallet (pablomartin4btc)
42c13141b5 wallet, refactor: Decouple into HasLegacyRecords() (pablomartin4btc)

Pull request description:

  Same as with a blank wallet (#28976), wallets with no legacy records (i.e. empty, non-blank, watch-only wallet) do not require to be migrated.

  Steps to reproduce the issue:

  1.- `createwallet "empty_wo_noblank_legacy_wallet" true false "" false false`
  2.- `migratewallet`

  ```
  wallet/wallet.cpp:4071 GetDescriptorsForLegacy: Assertion `legacy_spkm' failed.
  Aborted (core dumped)
  ```

ACKs for top commit:
  davidgumberg:
    untested reACK 0f602c5693
  fjahr:
    re-ACK 0f602c5693
  achow101:
    ACK 0f602c5693
  furszy:
    ACK 0f602c5693
  BrandonOdiwuor:
    Code Review ACK 0f602c5693

Tree-SHA512: 796c3f0b1946281097f7ffc3563bc79f879e80a98237012535cc530a4a2239fd2d71a17b4f54e30258886dc9f0b83206d7a5d50312e4fc6d0abe4f559fbe07ec

src/wallet/test/walletdb_tests.cpp

@@ -47,11 +47,14 @@ BOOST_AUTO_TEST_CASE(walletdb_read_write_deadlock)
         // Create legacy spkm
         LOCK(wallet->cs_wallet);
         auto legacy_spkm = wallet->GetOrCreateLegacyScriptPubKeyMan();
+        BOOST_CHECK(!HasLegacyRecords(*wallet));
         BOOST_CHECK(legacy_spkm->SetupGeneration(true));
+        BOOST_CHECK(HasLegacyRecords(*wallet));
         wallet->Flush();
 
         // Now delete all records, which performs a read write operation.
         BOOST_CHECK(wallet->GetLegacyScriptPubKeyMan()->DeleteRecords());
+        BOOST_CHECK(!HasLegacyRecords(*wallet));
     }
 }
 

src/wallet/wallet.cpp

@@ -4534,13 +4534,13 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
         // First change to using SQLite
         if (!local_wallet->MigrateToSQLite(error)) return util::Error{error};
 
-        // 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) {
+        // 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);
         } else {
             // Make sure that descriptors flag is actually set
             local_wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+            success = true;
         }
     }
 

src/wallet/walletdb.cpp

@@ -540,6 +540,35 @@ static LoadResult LoadRecords(CWallet* pwallet, DatabaseBatch& batch, const std:
     return LoadRecords(pwallet, batch, key, prefix, load_func);
 }
 
+bool HasLegacyRecords(CWallet& wallet)
+{
+    const auto& batch = wallet.GetDatabase().MakeBatch();
+    return HasLegacyRecords(wallet, *batch);
+}
+
+bool HasLegacyRecords(CWallet& wallet, DatabaseBatch& batch)
+{
+    for (const auto& type : DBKeys::LEGACY_TYPES) {
+        DataStream key;
+        DataStream value{};
+        DataStream prefix;
+
+        prefix << type;
+        std::unique_ptr<DatabaseCursor> cursor = batch.GetNewPrefixCursor(prefix);
+        if (!cursor) {
+            // Could only happen on a closed db, which means there is an error in the code flow.
+            wallet.WalletLogPrintf("Error getting database cursor for '%s' records", type);
+            throw std::runtime_error(strprintf("Error getting database cursor for '%s' records", type));
+        }
+
+        DatabaseCursor::Status status = cursor->Next(key, value);
+        if (status != DatabaseCursor::Status::DONE) {
+            return true;
+        }
+    }
+    return false;
+}
+
 static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch, int last_client) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)
 {
     AssertLockHeld(pwallet->cs_wallet);
@@ -547,23 +576,9 @@ static DBErrors LoadLegacyWalletRecords(CWallet* pwallet, DatabaseBatch& batch,
 
     // Make sure descriptor wallets don't have any legacy records
     if (pwallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
-        for (const auto& type : DBKeys::LEGACY_TYPES) {
-            DataStream key;
-            DataStream value{};
-
-            DataStream prefix;
-            prefix << type;
-            std::unique_ptr<DatabaseCursor> cursor = batch.GetNewPrefixCursor(prefix);
-            if (!cursor) {
-                pwallet->WalletLogPrintf("Error getting database cursor for '%s' records\n", type);
-                return DBErrors::CORRUPT;
-            }
-
-            DatabaseCursor::Status status = cursor->Next(key, value);
-            if (status != DatabaseCursor::Status::DONE) {
-                pwallet->WalletLogPrintf("Error: Unexpected legacy entry found in descriptor wallet %s. The wallet might have been tampered with or created with malicious intent.\n", pwallet->GetName());
-                return DBErrors::UNEXPECTED_LEGACY_ENTRY;
-            }
+        if (HasLegacyRecords(*pwallet, batch)) {
+            pwallet->WalletLogPrintf("Error: Unexpected legacy entry found in descriptor wallet %s. The wallet might have been tampered with or created with malicious intent.\n", pwallet->GetName());
+            return DBErrors::UNEXPECTED_LEGACY_ENTRY;
         }
 
         return DBErrors::LOAD_OK;

src/wallet/walletdb.h

@@ -333,6 +333,10 @@ bool LoadKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::stri
 bool LoadCryptedKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
 bool LoadEncryptionKey(CWallet* pwallet, DataStream& ssKey, DataStream& ssValue, std::string& strErr);
 bool LoadHDChain(CWallet* pwallet, DataStream& ssValue, std::string& strErr);
+
+//! Returns true if there are any DBKeys::LEGACY_TYPES record in the wallet db
+bool HasLegacyRecords(CWallet& wallet);
+bool HasLegacyRecords(CWallet& wallet, DatabaseBatch& batch);
 } // namespace wallet
 
 #endif // BITCOIN_WALLET_WALLETDB_H

test/functional/wallet_migration.py

@@ -445,6 +445,15 @@ class WalletMigrationTest(BitcoinTestFramework):
         # After migrating, the "keypool" is empty
         assert_raises_rpc_error(-4, "Error: This wallet has no available keys", watchonly1.getnewaddress)
 
+        self.log.info("Test migration of a watch-only empty wallet")
+        for idx, is_blank in enumerate([True, False], start=1):
+            wallet_name = f"watchonly_empty{idx}"
+            self.create_legacy_wallet(wallet_name, disable_private_keys=True, blank=is_blank)
+            _, watchonly_empty = self.migrate_and_get_rpc(wallet_name)
+            info = watchonly_empty.getwalletinfo()
+            assert_equal(info["private_keys_enabled"], False)
+            assert_equal(info["blank"], is_blank)
+
     def test_pk_coinbases(self):
         self.log.info("Test migration of a wallet using old pk() coinbases")
         wallet = self.create_legacy_wallet("pkcb")