From 5cf1c7855936ff8ce5bda3d5dc2ba23db6661b6a Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:33 -0500 Subject: [PATCH 1/7] wallet: Consolidate generation setup callers into one function --- src/wallet/wallet.cpp | 89 ++++++++++++++++++------------------------- src/wallet/wallet.h | 3 ++ 2 files changed, 41 insertions(+), 51 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index d010d90be38..dd4871462dd 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -438,6 +438,11 @@ 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 (!wallet->EncryptWallet(passphrase)) { @@ -445,33 +450,6 @@ std::shared_ptr CreateWallet(WalletContext& context, const std::string& 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); - if (wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - wallet->SetupDescriptorScriptPubKeyMans(); - } else { - for (auto spk_man : wallet->GetActiveScriptPubKeyMans()) { - if (!spk_man->SetupGeneration()) { - error = Untranslated("Unable to generate initial keys"); - status = DatabaseStatus::FAILED_CREATE; - return nullptr; - } - } - } - } - - // Relock the wallet - wallet->Lock(); - } } NotifyWalletLoaded(context, wallet); @@ -882,16 +860,9 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase) Lock(); Unlock(strWalletPassphrase); - // If we are using descriptors, make new descriptors with a new seed - if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS) && !IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET)) { - SetupDescriptorScriptPubKeyMans(); - } else if (auto spk_man = GetLegacyScriptPubKeyMan()) { - // if we are using HD, replace the HD seed with a new one - if (spk_man->IsHDEnabled()) { - if (!spk_man->SetupGeneration(true)) { - return false; - } - } + // Generate new descriptors or seed if not blank or disable private keys + if (!SetupWalletGeneration()) { + return false; } Lock(); @@ -3075,20 +3046,9 @@ std::shared_ptr CWallet::Create(WalletContext& context, const std::stri // 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))) { - LOCK(walletInstance->cs_wallet); - if (walletInstance->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - walletInstance->SetupDescriptorScriptPubKeyMans(); - // SetupDescriptorScriptPubKeyMans already calls SetupGeneration for us so we don't need to call SetupGeneration separately - } else { - // Legacy wallets need SetupGeneration here. - for (auto spk_man : walletInstance->GetActiveScriptPubKeyMans()) { - if (!spk_man->SetupGeneration()) { - error = _("Unable to generate initial keys"); - return nullptr; - } - } - } + if (!walletInstance->SetupWalletGeneration()) { + error = _("Unable to generate initial keys"); + return nullptr; } if (chain) { @@ -3854,6 +3814,33 @@ void CWallet::SetupDescriptorScriptPubKeyMans() } } +bool CWallet::SetupWalletGeneration() +{ + LOCK(cs_wallet); + // Skip if blank or no privkeys + if (!IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER) && + (IsWalletFlagSet(WALLET_FLAG_BLANK_WALLET) || IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS))) { + // Just make sure that there's a legacy spkm if this wallet is legacy + if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + SetupLegacyScriptPubKeyMan(); + } + return true; + } + // If we are using descriptors, make new descriptors with a new seed + if (IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { + SetupDescriptorScriptPubKeyMans(); + } else if (auto spk_man = GetOrCreateLegacyScriptPubKeyMan()) { + // if we are using HD, set or replace the HD seed with a new one + // non-HD always has key generation enabled by default + if (CanSupportFeature(FEATURE_HD)) { + if (!spk_man->SetupGeneration(true)) { + return false; + } + } + } + return true; +} + 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 659e3854e71..8b8fec96809 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1030,6 +1030,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 + bool SetupWalletGeneration(); + //! Return the DescriptorScriptPubKeyMan for a WalletDescriptor if it is already in the wallet DescriptorScriptPubKeyMan* GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const; From 449d41e1c3b486a6f939b4e6b26a7fde8dabae07 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:36 -0500 Subject: [PATCH 2/7] wallet: Remove SetupGeneration from ScriptPubKeyMan interface --- src/wallet/scriptpubkeyman.h | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 8af16319c1f..77cfd011fdb 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -205,12 +205,6 @@ public: */ virtual std::vector MarkUnusedAddresses(const CScript& script) { return {}; } - /** Sets up the key generation stuff, i.e. generates new HD seeds and sets them as active. - * Returns false if already setup or setup fails, true if setup is successful - * Set force=true to make it re-setup if already setup, used for upgrades - */ - virtual bool SetupGeneration(bool force = false) { return false; } - /* Returns true if HD is enabled */ virtual bool IsHDEnabled() const { return false; } @@ -474,7 +468,11 @@ public: bool IsHDEnabled() const override; - bool SetupGeneration(bool force = false) override; + /** Sets up the key generation stuff, i.e. generates new HD seeds and sets them as active. + * Returns false if already setup or setup fails, true if setup is successful + * Set force=true to make it re-setup if already setup, used for upgrades + */ + bool SetupGeneration(bool force = false); bool Upgrade(int prev_version, int new_version, bilingual_str& error) override; From fd03fb9493a1128b43bc10b691227c0c1e72a5ac Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:38 -0500 Subject: [PATCH 3/7] 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 constructor when loading. --- src/wallet/external_signer_scriptpubkeyman.h | 4 +-- src/wallet/scriptpubkeyman.cpp | 34 +++++++++----------- src/wallet/scriptpubkeyman.h | 26 +++++++-------- src/wallet/wallet.cpp | 7 ++-- src/wallet/wallet.h | 2 +- src/wallet/walletdb.cpp | 30 ++++++++--------- 6 files changed, 49 insertions(+), 54 deletions(-) diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 10d67d2ab47..44cb8d213ca 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -15,8 +15,8 @@ namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { public: - ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size) - : DescriptorScriptPubKeyMan(storage, descriptor, keypool_size) + ExternalSignerScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) + : DescriptorScriptPubKeyMan(storage, id, descriptor, keypool_size, keys, ckeys) {} ExternalSignerScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) : DescriptorScriptPubKeyMan(storage, keypool_size) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 80f25c62c2b..527fbcf4cc6 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2074,6 +2074,22 @@ bool LegacyDataSPKM::DeleteRecordsWithDB(WalletBatch& batch) return batch.EraseRecords(DBKeys::LEGACY_TYPES); } +DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, 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 (!m_map_keys.empty() && !m_map_crypted_keys.empty()) { + throw std::runtime_error("Error: Wallet contains both unencrypted and encrypted keys"); + } + if (id != GetID()) { + throw std::runtime_error("The descriptor ID calculated by the wallet differs from the one in DB"); + } + SetCache(m_wallet_descriptor.cache); +} + 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 @@ -2721,24 +2737,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 77cfd011fdb..12074414a39 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -273,15 +273,18 @@ static const std::unordered_set LEGACY_OUTPUT_TYPES { class DescriptorScriptPubKeyMan; +using WatchOnlySet = std::set; +using WatchKeyMap = std::map; +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: - using WatchOnlySet = std::set; - using WatchKeyMap = std::map; - using CryptedKeyMap = std::map>>; - CryptedKeyMap mapCryptedKeys GUARDED_BY(cs_KeyStore); WatchOnlySet setWatchOnly GUARDED_BY(cs_KeyStore); WatchKeyMap mapWatchKeys GUARDED_BY(cs_KeyStore); @@ -588,11 +591,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; @@ -617,6 +615,8 @@ 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 SetCache(const DescriptorCache& cache); + protected: WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); @@ -624,11 +624,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) {} + //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) + DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, 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) @@ -686,11 +689,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 dd4871462dd..8b45b45e0c9 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3706,16 +3706,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; if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { - spk_manager = new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size); + spk_manager = new ExternalSignerScriptPubKeyMan(*this, id, desc, m_keypool_size, keys, ckeys); } else { - spk_manager = new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size); + spk_manager = new DescriptorScriptPubKeyMan(*this, id, desc, m_keypool_size, keys, ckeys); } AddScriptPubKeyMan(id, std::unique_ptr(spk_manager)); - return *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 8b8fec96809..c2d9cb43a48 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1001,7 +1001,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 5538d810527..915aadf9ccb 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -841,13 +841,6 @@ 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()) { - strErr = "The descriptor ID calculated by the wallet differs from the one in DB"; - return DBErrors::CORRUPT; - } DescriptorCache cache; @@ -903,15 +896,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 + std::map 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; @@ -946,16 +938,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 + std::map>> 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; @@ -969,12 +962,19 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat std::vector privkey; value >> privkey; - spk_man->AddCryptedKey(pubkey.GetID(), pubkey, privkey); + ckeys[pubkey.GetID()] = 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 9f92bf378237749aa012a4a9162059cdadeff9c1 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Wed, 3 Apr 2024 18:38:51 -0400 Subject: [PATCH 4/7] fuzz: Skip adding descriptor to wallet if it cannot be expanded --- src/wallet/test/fuzz/scriptpubkeyman.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index 63768c89afc..f433f0dd3a9 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -73,6 +73,11 @@ static std::optional> CreateWal std::vector> parsed_descs = Parse(desc_str.value(), keys, error, false); if (parsed_descs.empty()) return std::nullopt; + 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 dc4c61f7ecb9b31542abe16dcc4325b3beab8634 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:42 -0500 Subject: [PATCH 5/7] 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 | 55 +++++++++++++++++++++++++++------- src/wallet/scriptpubkeyman.h | 13 ++++---- src/wallet/wallet.cpp | 16 ++-------- 3 files changed, 52 insertions(+), 32 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 527fbcf4cc6..1d2d9aea6c6 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1851,9 +1851,8 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() 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); + keys.keys.emplace(key.GetPubKey().GetID(), key); + auto desc_spk_man = std::make_unique(batch, m_storage, w_desc, /*keypool_size=*/0, keys); auto desc_spks = desc_spk_man->GetScriptPubKeys(); // Remove the scriptPubKeys from our current set @@ -1897,9 +1896,8 @@ std::optional LegacyDataSPKM::MigrateToDescriptor() 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); + keys.keys.emplace(master_key.key.GetPubKey().GetID(), master_key.key); + auto desc_spk_man = std::make_unique(batch, m_storage, w_desc, /*keypool_size=*/0, keys); auto desc_spks = desc_spk_man->GetScriptPubKeys(); // Remove the scriptPubKeys from our current set @@ -1973,16 +1971,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 = std::make_unique(batch, m_storage, 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()); @@ -2074,6 +2071,25 @@ bool LegacyDataSPKM::DeleteRecordsWithDB(WalletBatch& batch) return batch.EraseRecords(DBKeys::LEGACY_TYPES); } +DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider) + : ScriptPubKeyMan(storage), + m_keypool_size(keypool_size), + m_wallet_descriptor(descriptor) +{ + LOCK(cs_desc_man); + WalletBatch batch(m_storage.GetDatabase()); + UpdateWithSigningProvider(batch, provider); +} + +DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider) + : ScriptPubKeyMan(storage), + m_keypool_size(keypool_size), + m_wallet_descriptor(descriptor) +{ + LOCK(cs_desc_man); + UpdateWithSigningProvider(batch, provider); +} + DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) : ScriptPubKeyMan(storage), m_map_keys(keys), @@ -2825,7 +2841,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; @@ -2838,10 +2854,27 @@ 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; + AddDescriptorKeyWithDB(batch, key, key.GetPubKey()); + } + + // 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 12074414a39..893f151a22e 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -617,6 +617,9 @@ private: void SetCache(const DescriptorCache& cache); + void AddDescriptorKey(const CKey& key, const CPubKey &pubkey); + void UpdateWithSigningProvider(WalletBatch& batch, const FlatSigningProvider& signing_provider) EXCLUSIVE_LOCKS_REQUIRED(cs_desc_man); + protected: WalletDescriptor m_wallet_descriptor GUARDED_BY(cs_desc_man); @@ -625,11 +628,8 @@ protected: 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, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); + DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys); DescriptorScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) @@ -690,9 +690,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 8b45b45e0c9..372df10916e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3947,11 +3947,11 @@ util::Result CWallet::AddWalletDescriptor(WalletDescriptor& de 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 = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size, signing_provider)); spk_man = new_spk_man.get(); // Save the descriptor to memory @@ -3959,18 +3959,6 @@ util::Result CWallet::AddWalletDescriptor(WalletDescriptor& de 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()) { - WalletLogPrintf("Could not top up scriptPubKeys\n"); - return nullptr; - } - // Apply the label if necessary // Note: we disable labels for ranged descriptors if (!desc.descriptor->IsRange()) { From 39db554fe01ad6ddf4e850b56a1f1df30a97244d Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:44 -0500 Subject: [PATCH 6/7] wallet: Construct ExternalSignerSPKM with the new descriptor Instead of constructing then setting the descriptor with SetupDescriptor, just pass in that descriptor to the constructor. --- src/wallet/external_signer_scriptpubkeyman.cpp | 4 ++-- src/wallet/external_signer_scriptpubkeyman.h | 15 +++++---------- src/wallet/wallet.cpp | 3 +-- 3 files changed, 8 insertions(+), 14 deletions(-) diff --git a/src/wallet/external_signer_scriptpubkeyman.cpp b/src/wallet/external_signer_scriptpubkeyman.cpp index 32e9941453b..5d99ac9dec1 100644 --- a/src/wallet/external_signer_scriptpubkeyman.cpp +++ b/src/wallet/external_signer_scriptpubkeyman.cpp @@ -21,7 +21,8 @@ using common::PSBTError; namespace wallet { -bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::unique_ptr desc) +ExternalSignerScriptPubKeyMan::ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc) + : DescriptorScriptPubKeyMan(storage, keypool_size) { LOCK(cs_desc_man); assert(m_storage.IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)); @@ -42,7 +43,6 @@ bool ExternalSignerScriptPubKeyMan::SetupDescriptor(WalletBatch& batch, std::uni TopUpWithDB(batch); m_storage.UnsetBlankWalletFlag(batch); - return true; } ExternalSigner ExternalSignerScriptPubKeyMan::GetExternalSigner() { diff --git a/src/wallet/external_signer_scriptpubkeyman.h b/src/wallet/external_signer_scriptpubkeyman.h index 44cb8d213ca..e6f9a79798f 100644 --- a/src/wallet/external_signer_scriptpubkeyman.h +++ b/src/wallet/external_signer_scriptpubkeyman.h @@ -14,18 +14,13 @@ struct bilingual_str; namespace wallet { class ExternalSignerScriptPubKeyMan : public DescriptorScriptPubKeyMan { - public: - ExternalSignerScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) +public: + //! Create an ExternalSPKM from existing wallet data + ExternalSignerScriptPubKeyMan(WalletStorage& storage, const uint256& id, WalletDescriptor& descriptor, int64_t keypool_size, const KeyMap& keys, const CryptedKeyMap& ckeys) : DescriptorScriptPubKeyMan(storage, id, descriptor, keypool_size, keys, ckeys) {} - ExternalSignerScriptPubKeyMan(WalletStorage& storage, int64_t keypool_size) - : DescriptorScriptPubKeyMan(storage, keypool_size) - {} - - /** 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); + //! Create a new ExternalSPKM from just a descriptor + ExternalSignerScriptPubKeyMan(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, std::unique_ptr desc); static ExternalSigner GetExternalSigner(); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 372df10916e..0649f1eceef 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3800,8 +3800,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 = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*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 3356805e703b1e3c4bfd097d1848ca977b8a4a33 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Tue, 20 Feb 2024 11:48:46 -0500 Subject: [PATCH 7/7] 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 | 7 +++++++ src/wallet/scriptpubkeyman.h | 17 ++++++++++------- src/wallet/wallet.cpp | 3 +-- 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 1d2d9aea6c6..dd855589b11 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2106,6 +2106,13 @@ DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, con SetCache(m_wallet_descriptor.cache); } +DescriptorScriptPubKeyMan::DescriptorScriptPubKeyMan(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal) + : ScriptPubKeyMan(storage), + m_keypool_size(keypool_size) +{ + SetupDescriptorGeneration(batch, master_key, addr_type, internal); +} + 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 diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index 893f151a22e..d43b2923f86 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -620,7 +620,15 @@ 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 + bool SetupDescriptorGeneration(WalletBatch& batch, const CExtKey& master_key, OutputType addr_type, bool internal); + protected: + 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 @@ -632,10 +640,8 @@ public: DescriptorScriptPubKeyMan(WalletBatch& batch, WalletStorage& storage, WalletDescriptor& descriptor, int64_t keypool_size, const FlatSigningProvider& provider); //! Create a DescriptorScriptPubKeyMan from existing data (i.e. during loading) DescriptorScriptPubKeyMan(WalletStorage& storage, const uint256& id, 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) - {} + //! Create an automatically generated DescriptorScriptPubKeyMan + DescriptorScriptPubKeyMan(WalletStorage& storage, WalletBatch& batch, int64_t keypool_size, const CExtKey& master_key, OutputType addr_type, bool internal); mutable RecursiveMutex cs_desc_man; @@ -658,9 +664,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 0649f1eceef..4b9be86c2b7 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3720,7 +3720,7 @@ 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)); + auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, batch, m_keypool_size, master_key, output_type, internal)); if (IsCrypted()) { if (IsLocked()) { throw std::runtime_error(std::string(__func__) + ": Wallet is locked, cannot setup new descriptors"); @@ -3729,7 +3729,6 @@ DescriptorScriptPubKeyMan& CWallet::SetupDescriptorScriptPubKeyMan(WalletBatch& throw std::runtime_error(std::string(__func__) + ": Could not encrypt new descriptors"); } } - spk_manager->SetupDescriptorGeneration(batch, master_key, output_type, internal); DescriptorScriptPubKeyMan* out = spk_manager.get(); uint256 id = spk_manager->GetID(); AddScriptPubKeyMan(id, std::move(spk_manager));