From 42c13141b529b10360c73cdc8bf69bfeb1755eb6 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Wed, 26 Mar 2025 15:44:32 -0300 Subject: [PATCH 1/2] wallet, refactor: Decouple into HasLegacyRecords() The new helper will be used to fix a crash in the wallet migration process (watch-only, non-blank, private keys disabled, empty wallet - no scripts or addresses imported). Co-authored-by: Matias Furszyfer --- src/wallet/test/walletdb_tests.cpp | 3 ++ src/wallet/walletdb.cpp | 49 +++++++++++++++++++----------- src/wallet/walletdb.h | 4 +++ 3 files changed, 39 insertions(+), 17 deletions(-) diff --git a/src/wallet/test/walletdb_tests.cpp b/src/wallet/test/walletdb_tests.cpp index fee8c85873e..91952245d70 100644 --- a/src/wallet/test/walletdb_tests.cpp +++ b/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)); } } diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index a19c03d70ef..3ae081735cc 100644 --- a/src/wallet/walletdb.cpp +++ b/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 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 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; diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 70d69870126..5cdd449a6fd 100644 --- a/src/wallet/walletdb.h +++ b/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 From 0f602c5693ef5d0c63b1e5b7dc0990dced3655d6 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Wed, 26 Mar 2025 19:13:44 -0300 Subject: [PATCH 2/2] wallet, migration: Fix crash on empty wallet Same as with a blank wallet, wallets with no legacy records (i.e. empty, non-blank, watch-only wallet) do not require to be migrated. --- src/wallet/wallet.cpp | 6 +++--- test/functional/wallet_migration.py | 9 +++++++++ 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 40d52b90f00..9ae0fb5329e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4534,13 +4534,13 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // 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; } } diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index ce8dc19460d..c467d6ad36d 100755 --- a/test/functional/wallet_migration.py +++ b/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")