From 0301c758ea0d0b95090d7492f1e5d30e6b447b9c Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 29 Apr 2026 14:13:28 -0700 Subject: [PATCH 01/10] wallet migration, fuzz: Migrate hd seed once If a wallet has multiple HD chains that have the same seed, we should only migrate that seed a single time. This fixes a fuzz crash that occurs once the return value of AddDescriptorKeyWithDB is checked during descriptor construction. --- src/wallet/scriptpubkeyman.cpp | 6 +++--- src/wallet/test/fuzz/scriptpubkeyman.cpp | 5 ++++- src/wallet/walletdb.h | 4 ++++ 3 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 8a1c0a45a56..a65dee60ac0 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -619,10 +619,10 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() } // Handle HD keys by using the CHDChains - std::vector chains; - chains.push_back(m_hd_chain); + std::set chains; + chains.insert(m_hd_chain); for (const auto& chain_pair : m_inactive_hd_chains) { - chains.push_back(chain_pair.second); + chains.insert(chain_pair.second); } bool can_support_hd_split_feature = m_hd_chain.nVersion >= CHDChain::VERSION_HD_CHAIN_SPLIT; diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index a627c770541..341543ff412 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -229,6 +229,7 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration) if (legacy_data.LoadKey(key, pub_key) && std::find(keys.begin(), keys.end(), key) == keys.end()) keys.push_back(key); } + size_t added_chains = 0; bool add_hd_chain{fuzzed_data_provider.ConsumeBool() && !keys.empty()}; CHDChain hd_chain; auto version{fuzzed_data_provider.ConsumeBool() ? CHDChain::VERSION_HD_CHAIN_SPLIT : CHDChain::VERSION_HD_BASE}; @@ -238,14 +239,17 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration) hd_chain.nVersion = version; hd_chain.seed_id = hd_key.GetPubKey().GetID(); legacy_data.LoadHDChain(hd_chain); + added_chains++; } bool add_inactive_hd_chain{fuzzed_data_provider.ConsumeBool() && !keys.empty()}; if (add_inactive_hd_chain) { hd_key = PickValue(fuzzed_data_provider, keys); hd_chain.nVersion = fuzzed_data_provider.ConsumeBool() ? CHDChain::VERSION_HD_CHAIN_SPLIT : CHDChain::VERSION_HD_BASE; + bool dup_chain = hd_chain.seed_id == hd_key.GetPubKey().GetID(); hd_chain.seed_id = hd_key.GetPubKey().GetID(); legacy_data.AddInactiveHDChain(hd_chain); + if (!dup_chain) added_chains++; } bool watch_only = false; @@ -323,7 +327,6 @@ FUZZ_TARGET(spkm_migration, .init = initialize_spkm_migration) auto result{legacy_data.MigrateToDescriptor()}; assert(result); - size_t added_chains{static_cast(add_hd_chain) + static_cast(add_inactive_hd_chain)}; if ((add_hd_chain && version >= CHDChain::VERSION_HD_CHAIN_SPLIT) || (!add_hd_chain && add_inactive_hd_chain)) { added_chains *= 2; } diff --git a/src/wallet/walletdb.h b/src/wallet/walletdb.h index 455fc745571..454435cfcb4 100644 --- a/src/wallet/walletdb.h +++ b/src/wallet/walletdb.h @@ -126,6 +126,10 @@ public: { return seed_id == chain.seed_id; } + bool operator<(const CHDChain& chain) const + { + return seed_id < chain.seed_id; + } }; class CKeyMetadata From cd912c4e10848baf7fc1d661b533cf2fb806d696 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:33 -0500 Subject: [PATCH 02/10] wallet: Consolidate generation setup callers into one function --- src/wallet/wallet.cpp | 50 +++++++++++++++++++++---------------------- src/wallet/wallet.h | 3 +++ 2 files changed, 27 insertions(+), 26 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 3724c3f2b3d..ade5b48fa51 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -429,30 +429,18 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& return nullptr; } + // Unset the blank flag if not specified by the user + if (!create_blank) { + wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET); + } + // Encrypt the wallet - if (!passphrase.empty() && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + if (!passphrase.empty()) { if (!wallet->EncryptWallet(passphrase)) { error = Untranslated("Error: Wallet created but failed to encrypt."); status = DatabaseStatus::FAILED_ENCRYPT; return nullptr; } - if (!create_blank) { - // Unlock the wallet - if (!wallet->Unlock(passphrase)) { - error = Untranslated("Error: Wallet was encrypted but could not be unlocked"); - status = DatabaseStatus::FAILED_ENCRYPT; - return nullptr; - } - - // Set a seed for the wallet - { - LOCK(wallet->cs_wallet); - wallet->SetupDescriptorScriptPubKeyMans(); - } - - // Relock the wallet - wallet->Lock(); - } } WITH_LOCK(wallet->cs_wallet, wallet->LogStats()); @@ -873,12 +861,12 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) encrypted_batch = nullptr; Lock(); - Unlock(strWalletPassphrase); - - // Make new descriptors with a new seed - if (!IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) { - SetupDescriptorScriptPubKeyMans(); + if (!Unlock(strWalletPassphrase)) { + return false; } + + SetupWalletGeneration(); + Lock(); // Need to completely rewrite the wallet file; if we don't, the database might keep @@ -3101,9 +3089,7 @@ std::shared_ptr CWallet::CreateNew(WalletContext& context, const std::s // Only descriptor wallets can be created assert(walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); - if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) || !(wallet_creation_flags & (WALLET_FLAG_DISABLE_PRIVATE_KEYS | WALLET_FLAG_BLANK_WALLET))) { - walletInstance->SetupDescriptorScriptPubKeyMans(); - } + walletInstance->SetupWalletGeneration(); if (chain) { std::optional tip_height = chain->getHeight(); @@ -3670,6 +3656,18 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } } +void CWallet::SetupWalletGeneration() +{ + AssertLockHeld(cs_wallet); + // Skip setup for non-external-signer wallets that are either blank + // or have private keys disabled (not having private keys implies blank). + if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) && + (IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS))) { + return; + } + SetupDescriptorScriptPubKeyMans(); +} + void CWallet::AddActiveScriptPubKeyMan(uint256 id, OutputType type, bool internal) { WalletBatch batch(GetDatabase()); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 81d40ec8938..9a1b4fbc5b0 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1033,6 +1033,9 @@ public: //! Create new seed and default DescriptorScriptPubKeyMans for this wallet void SetupOwnDescriptorScriptPubKeyMans(WalletBatch& batch) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Setup new descriptors or seed for new address generation + void SetupWalletGeneration() EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); + //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; From f713fd1725f09eaf1d12afd8b6b50e7b977c0789 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 22 Apr 2026 15:11:52 -0700 Subject: [PATCH 03/10] refactor: wallet: Don't reuse WALLET_BLANK flag for born-encrypted wallets. With the split between LoadWallet and CreateNew, it's no longer necessary to utilize the blank flag to prevent the wallet from having descriptors automatically being generated. Instead, CreateNew can take a separate parameter to indicate whether the wallet is to be born encrypted and therefore should not have any keys generated. --- src/wallet/test/util.cpp | 2 +- src/wallet/wallet.cpp | 30 +++++++++++------------------- src/wallet/wallet.h | 2 +- 3 files changed, 13 insertions(+), 21 deletions(-) diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index 9ee63f6e228..d2608fccf0b 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -51,7 +51,7 @@ std::shared_ptr TestCreateWallet(std::unique_ptr databa { bilingual_str _error; std::vector _warnings; - auto wallet = CWallet::CreateNew(context, "", std::move(database), create_flags, _error, _warnings); + auto wallet = CWallet::CreateNew(context, "", std::move(database), create_flags, /*born_encrypted=*/false, _error, _warnings); NotifyWalletLoaded(context, wallet); if (context.chain) { wallet->postInitProcess(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ade5b48fa51..c337c45d63a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -385,18 +385,12 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& uint64_t wallet_creation_flags = options.create_flags; const SecureString& passphrase = options.create_passphrase; + bool born_encrypted = !passphrase.empty(); // Only descriptor wallets can be created Assert(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS); options.require_format = DatabaseFormat::SQLITE; - // Indicate that the wallet is actually supposed to be blank and not just blank to make it encrypted - bool create_blank = (wallet_creation_flags & WALLET_FLAG_BLANK_WALLET); - - // Born encrypted wallets need to be created blank first. - if (!passphrase.empty()) { - wallet_creation_flags |= WALLET_FLAG_BLANK_WALLET; - } // Private keys must be disabled for an external signer wallet if ((wallet_creation_flags & WALLET_FLAG_EXTERNAL_SIGNER) && !(wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { @@ -406,7 +400,7 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& } // Do not allow a passphrase when private keys are disabled - if (!passphrase.empty() && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { + if (born_encrypted && (wallet_creation_flags & WALLET_FLAG_DISABLE_PRIVATE_KEYS)) { error = Untranslated("Passphrase provided but private keys are disabled. A passphrase is only used to encrypt private keys, so cannot be used for wallets with private keys disabled."); status = DatabaseStatus::FAILED_CREATE; return nullptr; @@ -422,20 +416,15 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& // Make the wallet context.chain->initMessage(_("Creating wallet…")); - std::shared_ptr wallet = CWallet::CreateNew(context, name, std::move(database), wallet_creation_flags, error, warnings); + std::shared_ptr wallet = CWallet::CreateNew(context, name, std::move(database), wallet_creation_flags, born_encrypted, error, warnings); if (!wallet) { error = Untranslated("Wallet creation failed.") + Untranslated(" ") + error; status = DatabaseStatus::FAILED_CREATE; return nullptr; } - // Unset the blank flag if not specified by the user - if (!create_blank) { - wallet->UnsetWalletFlag(WALLET_FLAG_BLANK_WALLET); - } - // Encrypt the wallet - if (!passphrase.empty()) { + if (born_encrypted) { if (!wallet->EncryptWallet(passphrase)) { error = Untranslated("Error: Wallet created but failed to encrypt."); status = DatabaseStatus::FAILED_ENCRYPT; @@ -3060,7 +3049,7 @@ bool CWallet::LoadWalletArgs(std::shared_ptr wallet, const WalletContex return true; } -std::shared_ptr CWallet::CreateNew(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings) +std::shared_ptr CWallet::CreateNew(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bool born_encrypted, bilingual_str& error, std::vector& warnings) { interfaces::Chain* chain = context.chain; const std::string& walletFile = database->Filename(); @@ -3089,7 +3078,10 @@ std::shared_ptr CWallet::CreateNew(WalletContext& context, const std::s // Only descriptor wallets can be created assert(walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); - walletInstance->SetupWalletGeneration(); + // Born encrypted wallets will have their keys generated later + if (!born_encrypted) { + walletInstance->SetupWalletGeneration(); + } if (chain) { std::optional tip_height = chain->getHeight(); @@ -4170,7 +4162,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return false; } - data->watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + data->watchonly_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, /*born_encrypted=*/false, error, warnings); if (!data->watchonly_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; @@ -4209,7 +4201,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error, return false; } - data->solvable_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, error, warnings); + data->solvable_wallet = CWallet::CreateNew(empty_context, wallet_name, std::move(database), options.create_flags, /*born_encrypted=*/false, error, warnings); if (!data->solvable_wallet) { error = _("Error: Failed to create new watchonly wallet"); return false; diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 9a1b4fbc5b0..360b7315b47 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -871,7 +871,7 @@ public: static bool LoadWalletArgs(std::shared_ptr wallet, const WalletContext& context, bilingual_str& error, std::vector& warnings); /* Initializes, creates and returns a new CWallet; returns a null pointer in case of an error */ - static std::shared_ptr CreateNew(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bilingual_str& error, std::vector& warnings); + static std::shared_ptr CreateNew(WalletContext& context, const std::string& name, std::unique_ptr database, uint64_t wallet_creation_flags, bool born_encrypted, bilingual_str& error, std::vector& warnings); /* Initializes, loads, and returns a CWallet from an existing wallet; returns a null pointer in case of an error */ static std::shared_ptr LoadExisting(WalletContext& context, const std::string& name, std::unique_ptr database, bilingual_str& error, std::vector& warnings); From 80b0c259921d03754320754f14fec7a21bce3126 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:38 -0500 Subject: [PATCH 04/10] wallet: Load everything into DescSPKM on construction Instead of creating a DescSPKM that is then progressively loaded, we should instead create it all at once in a factory function when loading. --- .../external_signer_scriptpubkeyman.cpp | 5 +++ src/wallet/external_signer_scriptpubkeyman.h | 12 ++++-- src/wallet/scriptpubkeyman.cpp | 39 +++++++++---------- src/wallet/scriptpubkeyman.h | 26 +++++++------ src/wallet/wallet.cpp | 11 +++--- src/wallet/wallet.h | 2 +- src/wallet/walletdb.cpp | 27 +++++++------ 7 files changed, 68 insertions(+), 54 deletions(-) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index f66686254a4..189f7698b50 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -21,6 +21,11 @@ using common::PSBTError; namespace wallet { +std::unique_ptr ExternalSignerScriptPubKeyMan::LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) +{ + return std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); +} + bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::unique_ptr desc) { LOCK(cs_desc_man); diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 0ccc243c256..1c3324f1915 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -15,14 +15,18 @@ struct bilingual_str; namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { - public: - ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) - : DescriptorScriptPubKeyMan(storage, descriptor, keypool_size) - {} +private: + ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) + : DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys) + {} + +public: ExternalSignerScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : DescriptorScriptPubKeyMan(storage, keypool_size) {} + static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + /** Provide a descriptor at setup time * Returns false if already setup or setup fails, true if setup is successful */ diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index a65dee60ac0..5ec51a737cd 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -821,6 +821,24 @@ bool LegacyDataSPKM::DeleteRecordsWithDB(WalletBatch& batch) return batch.EraseRecords(DBKeys::LEGACY_TYPES); } +DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) + : ScriptPubKeyMan(storage), + m_map_keys(keys), + m_map_crypted_keys(ckeys), + m_keypool_size(keypool_size), + m_wallet_descriptor(descriptor) +{ + if (!keys.empty() && !ckeys.empty()) { + throw std::runtime_error("Wallet contains both unencrypted and encrypted keys"); + } + Load(); +} + +std::unique_ptr DescriptorScriptPubKeyMan::LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) +{ + return std::unique_ptr(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); +} + util::Result DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type) { // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later @@ -1426,11 +1444,10 @@ uint256 DescriptorScriptPubKeyMan::GetID() const return m_wallet_descriptor.id; } -void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) +void DescriptorScriptPubKeyMan::Load() { LOCK(cs_desc_man); std::set new_spks; - m_wallet_descriptor.cache = cache; for (int32_t i = m_wallet_descriptor.range_start; i < m_wallet_descriptor.range_end; ++i) { FlatSigningProvider out_keys; std::vector scripts_temp; @@ -1460,24 +1477,6 @@ void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) m_storage.TopUpCallback(new_spks, this); } -bool DescriptorScriptPubKeyMan::AddKey(const CKeyID& key_id, const CKey& key) -{ - LOCK(cs_desc_man); - m_map_keys[key_id] = key; - return true; -} - -bool DescriptorScriptPubKeyMan::AddCryptedKey(const CKeyID& key_id, const CPubKey& pubkey, const std::vector& crypted_key) -{ - LOCK(cs_desc_man); - if (!m_map_keys.empty()) { - return false; - } - - m_map_crypted_keys[key_id] = make_pair(pubkey, crypted_key); - return true; -} - bool DescriptorScriptPubKeyMan::HasWalletDescriptor(const WalletDescriptor& desc) const { LOCK(cs_desc_man); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index cf024294bf3..2762ebc2e61 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -166,14 +166,18 @@ static const std::unordered_set LEGACY_OUTPUT_TYPES { OutputType::BECH32, }; +using KeyMap = std::map; +using CryptedKeyMap = std::map>>; +using ScriptPubKeyMap = std::map; // Map of scripts to descriptor range index +using PubKeyMap = std::map; // Map of pubkeys involved in scripts to descriptor range index + // Manages the data for a LegacyScriptPubKeyMan. // This is the minimum necessary to load a legacy wallet so that it can be migrated. class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider { -private: +protected: using WatchOnlySet = std::set; using WatchKeyMap = std::map; - using CryptedKeyMap = std::map>>; CryptedKeyMap mapCryptedKeys GUARDED_BY(cs_KeyStore); WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore); @@ -273,11 +277,6 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan { friend class LegacyDataSPKM; private: - using ScriptPubKeyMap = std::map; // Map of scripts to descriptor range index - using PubKeyMap = std::map; // Map of pubkeys involved in scripts to descriptor range index - using CryptedKeyMap = std::map>>; - using KeyMap = std::map; - ScriptPubKeyMap m_map_script_pub_keys GUARDED_BY(cs_desc_man); PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man); int32_t m_max_cached_index = -1; @@ -315,13 +314,19 @@ private: // Fetch the SigningProvider for a given index and optionally include private keys. Called by the above functions. std::unique_ptr GetSigningProvider(int32_t index, bool include_private = false) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + void Load(); + protected: + //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) + DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); //! Same as 'TopUp' but designed for use within a batch transaction context bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0); public: + //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import) DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size), @@ -332,6 +337,8 @@ public: m_keypool_size(keypool_size) {} + static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + mutable RecursiveMutex cs_desc_man; util::Result GetNewDestination(OutputType type) override; @@ -383,11 +390,6 @@ public: uint256 GetID() const override; - void SetCache(const DescriptorCache& cache); - - bool AddKey(const CKeyID& key_id, const CKey& key); - bool AddCryptedKey(const CKeyID& key_id, const CPubKey& pubkey, const std::vector& crypted_key); - bool HasWalletDescriptor(const WalletDescriptor& desc) const; util::Result UpdateWalletDescriptor(WalletDescriptor& descriptor); bool CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index c337c45d63a..7b94b659700 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3539,16 +3539,15 @@ void CWallet::ConnectScriptPubKeyManNotifiers() } } -DescriptorScriptPubKeyMan& CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) +void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc, const KeyMap& keys, const CryptedKeyMap& ckeys) { - DescriptorScriptPubKeyMan* spk_manager; + std::unique_ptr spk_manager; if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { - spk_manager = new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size); + spk_manager = ExternalSignerScriptPubKeyMan::LoadFromStorage(*this, desc, m_keypool_size, keys, ckeys); } else { - spk_manager = new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size); + spk_manager = DescriptorScriptPubKeyMan::LoadFromStorage(*this, desc, m_keypool_size, keys, ckeys); } - AddScriptPubKeyMan(id, std::unique_ptr(spk_manager)); - return *spk_manager; + AddScriptPubKeyMan(id, std::move(spk_manager)); } DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 360b7315b47..4b5df5c5006 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1004,7 +1004,7 @@ public: void ConnectScriptPubKeyManNotifiers(); //! Instantiate a descriptor ScriptPubKeyMan from the WalletDescriptor and load it - DescriptorScriptPubKeyMan& LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc); + void LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc, const KeyMap& keys, const CryptedKeyMap& ckeys); //! Adds the active ScriptPubKeyMan for the specified type and internal. Writes it to the wallet file //! @param[in] id The unique id for the ScriptPubKeyMan diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index c8b7ae53cd5..5ad29970d20 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -771,10 +771,8 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat strErr = strprintf("%s\nDetails: %s", strErr, e.what()); return DBErrors::UNKNOWN_DESCRIPTOR; } - DescriptorScriptPubKeyMan& spkm = pwallet->LoadDescriptorScriptPubKeyMan(id, desc); - // Prior to doing anything with this spkm, verify ID compatibility - if (id != spkm.GetID()) { + if (id != desc.id) { strErr = "The descriptor ID calculated by the wallet differs from the one in DB"; return DBErrors::CORRUPT; } @@ -833,15 +831,14 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat }); result = std::max(result, lh_cache_res.m_result); - // Set the cache for this descriptor - auto spk_man = (DescriptorScriptPubKeyMan*)pwallet->GetScriptPubKeyMan(id); - assert(spk_man); - spk_man->SetCache(cache); + // Set the cache to the WalletDescriptor + desc.cache = cache; // Get unencrypted keys + KeyMap keys; prefix = PrefixStream(DBKeys::WALLETDESCRIPTORKEY, id); LoadResult key_res = LoadRecords(pwallet, batch, DBKeys::WALLETDESCRIPTORKEY, prefix, - [&id, &spk_man] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& strErr) { + [&id, &keys] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& strErr) { uint256 desc_id; CPubKey pubkey; key >> desc_id; @@ -873,16 +870,17 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat strErr = "Error reading wallet database: descriptor unencrypted key CPrivKey corrupt"; return DBErrors::CORRUPT; } - spk_man->AddKey(pubkey.GetID(), privkey); + keys[pubkey.GetID()] = privkey; return DBErrors::LOAD_OK; }); result = std::max(result, key_res.m_result); num_keys = key_res.m_records; // Get encrypted keys + CryptedKeyMap ckeys; prefix = PrefixStream(DBKeys::WALLETDESCRIPTORCKEY, id); LoadResult ckey_res = LoadRecords(pwallet, batch, DBKeys::WALLETDESCRIPTORCKEY, prefix, - [&id, &spk_man] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { + [&id, &ckeys] (CWallet* pwallet, DataStream& key, DataStream& value, std::string& err) { uint256 desc_id; CPubKey pubkey; key >> desc_id; @@ -896,12 +894,19 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat std::vector privkey; value >> privkey; - spk_man->AddCryptedKey(pubkey.GetID(), pubkey, privkey); + ckeys[pubkey.GetID()] = std::make_pair(pubkey, privkey); return DBErrors::LOAD_OK; }); result = std::max(result, ckey_res.m_result); num_ckeys = ckey_res.m_records; + try { + pwallet->LoadDescriptorScriptPubKeyMan(id, desc, keys, ckeys); + } catch (std::runtime_error& e) { + strErr = e.what(); + return DBErrors::CORRUPT; + } + return result; }); From 8be5ee554bb9c8053de2e6addc6abed950555654 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 22 Apr 2026 18:14:17 -0700 Subject: [PATCH 05/10] test: wallet: Check that loading wallet with both unencrypted and encrypted keys fails. --- test/functional/wallet_descriptor.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/test/functional/wallet_descriptor.py b/test/functional/wallet_descriptor.py index 39b80bf79bf..ee8ea1927a0 100755 --- a/test/functional/wallet_descriptor.py +++ b/test/functional/wallet_descriptor.py @@ -12,6 +12,7 @@ except ImportError: import re from test_framework.blocktools import COINBASE_MATURITY +from test_framework.messages import ser_string from test_framework.test_framework import BitcoinTestFramework from test_framework.util import ( assert_not_equal, @@ -268,6 +269,25 @@ class WalletDescriptorTest(BitcoinTestFramework): conn.close() assert_raises_rpc_error(-4, "Unexpected legacy entry in descriptor wallet found.", self.nodes[0].loadwallet, "crashme") + self.log.info("Test that loading descriptor wallet containing both unencrypted and encrypted keys for same descriptor fails to load") + wallet_name = "mixed_crypt" + self.nodes[0].createwallet(wallet_name) + self.nodes[0].unloadwallet(wallet_name) + wallet_db = self.nodes[0].wallets_path / wallet_name / self.wallet_data_filename + conn = sqlite3.connect(wallet_db) + with conn: + key_prefix = ser_string(b"walletdescriptorkey") + ckey_prefix = ser_string(b"walletdescriptorckey") + rows = conn.execute('SELECT key, value FROM main').fetchall() + key_rows = [(k, v) for k, v in rows if k.startswith(key_prefix)] + # Test the test, want to be sure there is at least one unencrypted key. + assert len(key_rows) >= 1 + k, v = key_rows[0] + conn.execute('INSERT INTO main VALUES(?, ?)', (k.replace(key_prefix, ckey_prefix), v)) + conn.close() + with self.nodes[0].assert_debug_log(["Wallet contains both unencrypted and encrypted keys"]): + assert_raises_rpc_error(-4, "Wallet corrupted", self.nodes[0].loadwallet, wallet_name) + self.test_parent_descriptors() if __name__ == '__main__': From 6538f6913570814ddacd5645529ba42c2e41185c Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 3 Apr 2024 18:38:51 -0400 Subject: [PATCH 06/10] fuzz: Skip adding descriptor to wallet if it cannot be expanded --- src/wallet/test/fuzz/scriptpubkeyman.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index 341543ff412..d251df29a4a 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -70,6 +70,13 @@ static std::optional> CreateWal std::vector> parsed_descs = Parse(desc_str.value(), keys, error, false); if (parsed_descs.empty()) return std::nullopt; + // Verify expand succeeds before making WalletDescriptor + // Expansion results are not needed + FlatSigningProvider out_keys; + std::vector scripts_temp; + DescriptorCache temp_cache; + if (!parsed_descs.at(0)->Expand(0, keys, scripts_temp, out_keys, &temp_cache)) return std::nullopt; + WalletDescriptor w_desc{std::move(parsed_descs.at(0)), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/1}; return std::make_pair(w_desc, keys); } From aa4f7823aa18eea7e310a355981bbf70c0496b98 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:42 -0500 Subject: [PATCH 07/10] wallet: include keys when constructing DescriptorSPKM during import When importing a descriptor, all of the descriptor data should be provided at the same time in the constructor. --- src/wallet/scriptpubkeyman.cpp | 63 ++++++++++++++++++++++++++-------- src/wallet/scriptpubkeyman.h | 21 +++++++----- src/wallet/wallet.cpp | 15 ++------ 3 files changed, 63 insertions(+), 36 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 5ec51a737cd..98bf5542799 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -596,16 +596,15 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() // Construct the combo descriptor std::string desc_str = "combo(" + origin_str + HexStr(key.GetPubKey()) + ")"; - FlatSigningProvider keys; + FlatSigningProvider provider; std::string error; - std::vector> descs = Parse(desc_str, keys, error, false); + std::vector> descs = Parse(desc_str, provider, error, false); CHECK_NONFATAL(descs.size() == 1); // It shouldn't be possible to have an invalid or multipath descriptor WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0); // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys - auto desc_spk_man = std::make_unique(m_storage, w_desc, /*keypool_size=*/0); - WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey())); - desc_spk_man->TopUpWithDB(batch); + provider.keys.emplace(key.GetPubKey().GetID(), key); + auto desc_spk_man = DescriptorScriptPubKeyMan::CreateFromMigration(m_storage, batch, w_desc, /*keypool_size=*/0, provider); auto desc_spks = desc_spk_man->GetScriptPubKeys(); // Remove the scriptPubKeys from our current set @@ -644,17 +643,16 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() // Make the combo descriptor std::string xpub = EncodeExtPubKey(master_key.Neuter()); std::string desc_str = "combo(" + xpub + "/0h/" + ToString(i) + "h/*h)"; - FlatSigningProvider keys; + FlatSigningProvider provider; std::string error; - std::vector> descs = Parse(desc_str, keys, error, false); + std::vector> descs = Parse(desc_str, provider, error, false); CHECK_NONFATAL(descs.size() == 1); // It shouldn't be possible to have an invalid or multipath descriptor uint32_t chain_counter = std::max((i == 1 ? chain.nInternalChainCounter : chain.nExternalChainCounter), (uint32_t)0); WalletDescriptor w_desc(std::move(descs.at(0)), 0, 0, chain_counter, 0); // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys - auto desc_spk_man = std::make_unique(m_storage, w_desc, /*keypool_size=*/0); - WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, master_key.key, master_key.key.GetPubKey())); - desc_spk_man->TopUpWithDB(batch); + provider.keys.emplace(master_key.key.GetPubKey().GetID(), master_key.key); + auto desc_spk_man = DescriptorScriptPubKeyMan::CreateFromMigration(m_storage, batch, w_desc, /*keypool_size=*/0, provider); auto desc_spks = desc_spk_man->GetScriptPubKeys(); // Remove the scriptPubKeys from our current set @@ -727,16 +725,15 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() desc->Expand(0, provider, desc_spks, provider); } else { // Make the DescriptorScriptPubKeyMan and get the scriptPubKeys - WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); - auto desc_spk_man = std::make_unique(m_storage, w_desc, /*keypool_size=*/0); for (const auto& keyid : privkeyids) { CKey key; if (!GetKey(keyid, key)) { continue; } - WITH_LOCK(desc_spk_man->cs_desc_man, desc_spk_man->AddDescriptorKeyWithDB(batch, key, key.GetPubKey())); + keys.keys.emplace(key.GetPubKey().GetID(), key); } - desc_spk_man->TopUpWithDB(batch); + WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); + auto desc_spk_man = DescriptorScriptPubKeyMan::CreateFromMigration(m_storage, batch, w_desc, /*keypool_size=*/0, keys); auto desc_spks_set = desc_spk_man->GetScriptPubKeys(); desc_spks.insert(desc_spks.end(), desc_spks_set.begin(), desc_spks_set.end()); @@ -821,6 +818,23 @@ bool LegacyDataSPKM::DeleteRecordsWithDB(WalletBatch& batch) return batch.EraseRecords(DBKeys::LEGACY_TYPES); } +std::unique_ptr DescriptorScriptPubKeyMan::CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider) +{ + auto spkm = std::unique_ptr(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size)); + LOCK(spkm->cs_desc_man); + WalletBatch batch(storage.GetDatabase()); + spkm->UpdateWithSigningProvider(batch, provider); + return spkm; +} + +std::unique_ptr DescriptorScriptPubKeyMan::CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider) +{ + auto spkm = std::unique_ptr(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size)); + LOCK(spkm->cs_desc_man); + spkm->UpdateWithSigningProvider(batch, provider); + return spkm; +} + DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) : ScriptPubKeyMan(storage), m_map_keys(keys), @@ -1565,7 +1579,7 @@ void DescriptorScriptPubKeyMan::UpgradeDescriptorCache() } } -util::Result DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor) +util::Result DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor, const FlatSigningProvider& provider) { LOCK(cs_desc_man); std::string error; @@ -1578,10 +1592,29 @@ util::Result DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescr m_max_cached_index = -1; m_wallet_descriptor = descriptor; + WalletBatch batch(m_storage.GetDatabase()); + UpdateWithSigningProvider(batch, provider); NotifyFirstKeyTimeChanged(this, m_wallet_descriptor.creation_time); return {}; } +void DescriptorScriptPubKeyMan::UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) +{ + AssertLockHeld(cs_desc_man); + // Add the private keys to the descriptor + for (const auto& entry : signing_provider.keys) { + const CKey& key = entry.second; + if (!AddDescriptorKeyWithDB(batch, key, key.GetPubKey())) { + throw std::runtime_error(std::string(__func__) + ": writing descriptor private key failed"); + } + } + + // Top up key pool, to generate scriptPubKeys + if (!TopUpWithDB(batch)) { + throw std::runtime_error("Could not top up scriptPubKeys"); + } +} + bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error) { LOCK(cs_desc_man); diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 2762ebc2e61..238888fcf0e 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -303,6 +303,13 @@ private: */ mutable std::map m_musig2_secnonces; + //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import) + DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) + : ScriptPubKeyMan(storage), + m_keypool_size(keypool_size), + m_wallet_descriptor(descriptor) + {} + bool AddDescriptorKeyWithDB(WalletBatch& batch, const CKey& key, const CPubKey &pubkey) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); KeyMap GetKeys() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); @@ -316,6 +323,9 @@ private: void Load(); + void AddDescriptorKey(const CKey& key, const CPubKey &pubkey); + void UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + protected: //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); @@ -326,18 +336,14 @@ protected: bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0); public: - //! Create a new DescriptorScriptPubKeyMan from an existing descriptor (i.e. from an import) - DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) - : ScriptPubKeyMan(storage), - m_keypool_size(keypool_size), - m_wallet_descriptor(descriptor) - {} DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : ScriptPubKeyMan(storage), m_keypool_size(keypool_size) {} static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + static std::unique_ptr CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); + static std::unique_ptr CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); mutable RecursiveMutex cs_desc_man; @@ -391,9 +397,8 @@ public: uint256 GetID() const override; bool HasWalletDescriptor(const WalletDescriptor& desc) const; - util::Result UpdateWalletDescriptor(WalletDescriptor& descriptor); + util::Result UpdateWalletDescriptor(WalletDescriptor& descriptor, const FlatSigningProvider& provider); bool CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error); - void AddDescriptorKey(const CKey& key, const CPubKey &pubkey); void WriteDescriptor(); WalletDescriptor GetWalletDescriptor() const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7b94b659700..7f40fa7011f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3753,11 +3753,11 @@ util::Result> CWallet::AddWall auto spk_man = GetDescriptorScriptPubKeyMan(desc); if (spk_man) { WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString()); - if (auto spkm_res = spk_man->UpdateWalletDescriptor(desc); !spkm_res) { + if (auto spkm_res = spk_man->UpdateWalletDescriptor(desc, signing_provider); !spkm_res) { return util::Error{util::ErrorString(spkm_res)}; } } else { - auto new_spk_man = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size)); + auto new_spk_man = DescriptorScriptPubKeyMan::CreateFromImport(*this, desc, m_keypool_size, signing_provider); spk_man = new_spk_man.get(); // Save the descriptor to memory @@ -3765,17 +3765,6 @@ util::Result> CWallet::AddWall AddScriptPubKeyMan(id, std::move(new_spk_man)); } - // Add the private keys to the descriptor - for (const auto& entry : signing_provider.keys) { - const CKey& key = entry.second; - spk_man->AddDescriptorKey(key, key.GetPubKey()); - } - - // Top up key pool, the manager will generate new scriptPubKeys internally - if (!spk_man->TopUp()) { - return util::Error{_("Could not top up scriptPubKeys")}; - } - // Apply the label if necessary // Note: we disable labels for ranged descriptors if (!desc.descriptor->IsRange()) { From e20aaff70f005630d64fd93d93ccae5455a18d24 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:44 -0500 Subject: [PATCH 08/10] wallet: Construct ExternalSignerSPKM with the new descriptor Instead of constructing then setting the descriptor with SetupDescriptor, just pass in that descriptor to the constructor. --- .../external_signer_scriptpubkeyman.cpp | 20 ++++++++++--------- src/wallet/external_signer_scriptpubkeyman.h | 15 ++++++-------- src/wallet/wallet.cpp | 3 +-- 3 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 189f7698b50..e07de305d3a 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -26,28 +26,30 @@ std::unique_ptr ExternalSignerScriptPubKeyMan::Lo return std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); } -bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::unique_ptr desc) +std::unique_ptr ExternalSignerScriptPubKeyMan::CreateNew(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc) { - LOCK(cs_desc_man); - assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); - assert(m_storage.IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)); + auto spkm = std::unique_ptr(new ExternalSignerScriptPubKeyMan(storage, keypool_size)); + + LOCK(spkm->cs_desc_man); + assert(storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + assert(storage.IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)); int64_t creation_time = GetTime(); // Make the descriptor WalletDescriptor w_desc(std::move(desc), creation_time, 0, 0, 0); - m_wallet_descriptor = w_desc; + spkm->m_wallet_descriptor = w_desc; // Store the descriptor - if (!batch.WriteDescriptor(GetID(), m_wallet_descriptor)) { + if (!batch.WriteDescriptor(spkm->GetID(), spkm->m_wallet_descriptor)) { throw std::runtime_error(std::string(__func__) + ": writing descriptor failed"); } // TopUp - TopUpWithDB(batch); + spkm->TopUpWithDB(batch); - m_storage.UnsetBlankWalletFlag(batch); - return true; + storage.UnsetBlankWalletFlag(batch); + return spkm; } util::Result ExternalSignerScriptPubKeyMan::GetExternalSigner() { diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 1c3324f1915..3ee6b5cd0c7 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -16,21 +16,18 @@ namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { private: + //! Create an ExternalSPKM from existing wallet data ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) : DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys) {} + ExternalSignerScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) + : DescriptorScriptPubKeyMan(storage, keypool_size) + {} + public: - ExternalSignerScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) - : DescriptorScriptPubKeyMan(storage, keypool_size) - {} - static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); - - /** Provide a descriptor at setup time - * Returns false if already setup or setup fails, true if setup is successful - */ - bool SetupDescriptor(WalletBatch& batch, std::unique_ptrdesc); + static std::unique_ptr CreateNew(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc); static util::Result GetExternalSigner(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 7f40fa7011f..5a5af893f8c 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3634,8 +3634,7 @@ void CWallet::SetupDescriptorScriptPubKeyMans() continue; } OutputType t = *desc->GetOutputType(); - auto spk_manager = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*this, m_keypool_size)); - spk_manager->SetupDescriptor(batch, std::move(desc)); + auto spk_manager = ExternalSignerScriptPubKeyMan::CreateNew(*this, batch, m_keypool_size, std::move(desc)); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager)); AddActiveScriptPubKeyManWithDb(batch, id, t, internal); From 32946e0291fc4024439ceb7ec39f4e6a9c58bdc5 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:46 -0500 Subject: [PATCH 09/10] wallet: Setup new autogenerated descriptors on construction Instead of having a caller use SetupDescriptorGeneration, just have a constructor that takes those arguments and sets up the descriptor with the autogenerated key. --- src/wallet/scriptpubkeyman.cpp | 23 +++++++++++++++-------- src/wallet/scriptpubkeyman.h | 17 +++++++++-------- src/wallet/wallet.cpp | 12 +++--------- 3 files changed, 27 insertions(+), 25 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 98bf5542799..d0f899fed2d 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -853,6 +853,13 @@ std::unique_ptr DescriptorScriptPubKeyMan::LoadFromSt return std::unique_ptr(new DescriptorScriptPubKeyMan(storage, descriptor, keypool_size, keys, ckeys)); } +std::unique_ptr DescriptorScriptPubKeyMan::GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal) +{ + auto spkm = std::unique_ptr(new DescriptorScriptPubKeyMan(storage, keypool_size)); + spkm->SetupDescriptorGeneration(batch, master_key, addr_type, internal); + return spkm; +} + util::Result DescriptorScriptPubKeyMan::GetNewDestination(const OutputType type) { // Returns true if this descriptor supports getting new addresses. Conditions where we may be unable to fetch them (e.g. locked) are caught later @@ -1164,15 +1171,11 @@ bool DescriptorScriptPubKeyMan::AddDescriptorKeyWithDB(WalletBatch& batch, const } } -bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal) +void DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal) { LOCK(cs_desc_man); - assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); - - // Ignore when there is already a descriptor - if (m_wallet_descriptor.descriptor) { - return false; - } + Assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); + Assert(!m_wallet_descriptor.descriptor); m_wallet_descriptor = GenerateWalletDescriptor(master_key.Neuter(), addr_type, internal); @@ -1184,11 +1187,15 @@ bool DescriptorScriptPubKeyMan::SetupDescriptorGeneration(WalletBatch& batch, co throw std::runtime_error(std::string(__func__) + ": writing descriptor failed"); } + // Set m_decryption_thoroughly_checked for encrypted wallets + if (m_storage.HasEncryptionKeys()) { + m_decryption_thoroughly_checked = true; + } + // TopUp TopUpWithDB(batch); m_storage.UnsetBlankWalletFlag(batch); - return true; } bool DescriptorScriptPubKeyMan::IsHDEnabled() const diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 238888fcf0e..621630c9480 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -326,24 +326,28 @@ private: void AddDescriptorKey(const CKey& key, const CPubKey &pubkey); void UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + //! Setup descriptors based on the given CExtKey + void SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); + protected: //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); + DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) + : ScriptPubKeyMan(storage), + m_keypool_size(keypool_size) + {} + WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); //! Same as 'TopUp' but designed for use within a batch transaction context bool TopUpWithDB(WalletBatch& batch, unsigned int size = 0); public: - DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) - : ScriptPubKeyMan(storage), - m_keypool_size(keypool_size) - {} - static std::unique_ptr LoadFromStorage(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); static std::unique_ptr CreateFromImport(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); static std::unique_ptr CreateFromMigration(WalletStorage& storage, WalletBatch& batch, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); + static std::unique_ptr GenerateNewSingleSig(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal); mutable RecursiveMutex cs_desc_man; @@ -366,9 +370,6 @@ public: bool IsHDEnabled() const override; - //! Setup descriptors based on the given CExtkey - bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); - bool HavePrivateKeys() const override; bool HasPrivKey(const CKeyID& keyid) const EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); //! Retrieve the particular key if it is available. Returns nullopt if the key is not in the wallet, or if the wallet is locked. diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5a5af893f8c..8a8755d76e4 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3553,16 +3553,10 @@ void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc, DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch& batch, const CExtKey& master_key, const OutputType& output_type, bool internal) { AssertLockHeld(cs_wallet); - auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, m_keypool_size)); - if (HasEncryptionKeys()) { - if (IsLocked()) { - throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); - } - if (!spk_manager->CheckDecryptionKey(vMasterKey) && !spk_manager->Encrypt(vMasterKey, &batch)) { - throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); - } + if (IsLocked()) { + throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); } - spk_manager->SetupDescriptorGeneration(batch, master_key, output_type, internal); + auto spk_manager = DescriptorScriptPubKeyMan::GenerateNewSingleSig(*this, batch, m_keypool_size, master_key, output_type, internal); DescriptorScriptPubKeyMan* out = spk_manager.get(); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager)); From 451fdd26a4f638e4f68f076ba2a891222cea380c Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Thu, 23 Apr 2026 15:17:32 -0700 Subject: [PATCH 10/10] test: wallet: Constructing a DSPKM that can't TopUp() throws. --- src/wallet/scriptpubkeyman.h | 7 ++++--- src/wallet/test/scriptpubkeyman_tests.cpp | 13 +++++++++++++ 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 621630c9480..8b80a3da132 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -168,14 +168,12 @@ static const std::unordered_set LEGACY_OUTPUT_TYPES { using KeyMap = std::map; using CryptedKeyMap = std::map>>; -using ScriptPubKeyMap = std::map; // Map of scripts to descriptor range index -using PubKeyMap = std::map; // Map of pubkeys involved in scripts to descriptor range index // Manages the data for a LegacyScriptPubKeyMan. // This is the minimum necessary to load a legacy wallet so that it can be migrated. class LegacyDataSPKM : public ScriptPubKeyMan, public FillableSigningProvider { -protected: +private: using WatchOnlySet = std::set; using WatchKeyMap = std::map; @@ -277,6 +275,9 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan { friend class LegacyDataSPKM; private: + using ScriptPubKeyMap = std::map; // Map of scripts to descriptor range index + using PubKeyMap = std::map; // Map of pubkeys involved in scripts to descriptor range index + ScriptPubKeyMap m_map_script_pub_keys GUARDED_BY(cs_desc_man); PubKeyMap m_map_pubkeys GUARDED_BY(cs_desc_man); int32_t m_max_cached_index = -1; diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 9c62436d6c0..74389a54967 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include