From b27682593266c99507c720855cb34f5f7d363dd2 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 14:16:43 -0500 Subject: [PATCH 1/7] bench: Add a benchmark for ismine --- src/Makefile.bench.include | 2 + src/bench/wallet_ismine.cpp | 74 +++++++++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+) create mode 100644 src/bench/wallet_ismine.cpp diff --git a/src/Makefile.bench.include b/src/Makefile.bench.include index 9e5366f0b40..b24405ce19c 100644 --- a/src/Makefile.bench.include +++ b/src/Makefile.bench.include @@ -88,6 +88,8 @@ bench_bench_bitcoin_SOURCES += bench/wallet_balance.cpp bench_bench_bitcoin_SOURCES += bench/wallet_create.cpp bench_bench_bitcoin_SOURCES += bench/wallet_loading.cpp bench_bench_bitcoin_SOURCES += bench/wallet_create_tx.cpp +bench_bench_bitcoin_SOURCES += bench/wallet_ismine.cpp + bench_bench_bitcoin_LDADD += $(BDB_LIBS) $(SQLITE_LIBS) endif diff --git a/src/bench/wallet_ismine.cpp b/src/bench/wallet_ismine.cpp new file mode 100644 index 00000000000..261c95c7c8d --- /dev/null +++ b/src/bench/wallet_ismine.cpp @@ -0,0 +1,74 @@ +// Copyright (c) 2022 The Bitcoin Core developers +// Distributed under the MIT software license, see the accompanying +// file COPYING or http://www.opensource.org/licenses/mit-license.php. + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +namespace wallet { +static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_combo = 0) +{ + const auto test_setup = MakeNoLogFileContext(); + + WalletContext context; + context.args = &test_setup->m_args; + context.chain = test_setup->m_node.chain.get(); + + // Setup the wallet + // Loading the wallet will also create it + uint64_t create_flags = 0; + if (!legacy_wallet) { + create_flags = WALLET_FLAG_DESCRIPTORS; + } + auto database = CreateMockableWalletDatabase(); + auto wallet = TestLoadWallet(std::move(database), context, create_flags); + + // For a descriptor wallet, fill with num_combo combo descriptors with random keys + // This benchmarks a non-HD wallet migrated to descriptors + if (!legacy_wallet && num_combo > 0) { + LOCK(wallet->cs_wallet); + for (int i = 0; i < num_combo; ++i) { + CKey key; + key.MakeNewKey(/*fCompressed=*/true); + FlatSigningProvider keys; + std::string error; + std::unique_ptr desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false); + WalletDescriptor w_desc(std::move(desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0); + auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false); + assert(spkm); + } + } + + const CScript script = GetScriptForDestination(DecodeDestination(ADDRESS_BCRT1_UNSPENDABLE)); + + bench.run([&] { + LOCK(wallet->cs_wallet); + isminetype mine = wallet->IsMine(script); + assert(mine == ISMINE_NO); + }); + + TestUnloadWallet(std::move(wallet)); +} + +#ifdef USE_BDB +static void WalletIsMineLegacy(benchmark::Bench& bench) { WalletIsMine(bench, /*legacy_wallet=*/true); } +BENCHMARK(WalletIsMineLegacy, benchmark::PriorityLevel::LOW); +#endif + +#ifdef USE_SQLITE +static void WalletIsMineDescriptors(benchmark::Bench& bench) { WalletIsMine(bench, /*legacy_wallet=*/false); } +static void WalletIsMineMigratedDescriptors(benchmark::Bench& bench) { WalletIsMine(bench, /*legacy_wallet=*/false, /*num_combo=*/2000); } +BENCHMARK(WalletIsMineDescriptors, benchmark::PriorityLevel::LOW); +BENCHMARK(WalletIsMineMigratedDescriptors, benchmark::PriorityLevel::LOW); +#endif +} // namespace wallet From 99a0cddbc04e8bfea3748a6cb4c0107392fab94f Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 14:16:46 -0500 Subject: [PATCH 2/7] wallet: Introduce a callback called after TopUp completes After TopUp completes, the wallet containing each SPKM will want to know what new scriptPubKeys were generated. In order for all TopUp calls (including ones internal the the SPKM), we use a callback function in the WalletStorage interface. --- src/wallet/scriptpubkeyman.cpp | 10 ++++++++++ src/wallet/scriptpubkeyman.h | 3 +++ src/wallet/wallet.cpp | 10 ++++++++++ src/wallet/wallet.h | 5 +++++ 4 files changed, 28 insertions(+) diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index dc5c442b95d..72f07f11787 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -1290,6 +1290,9 @@ bool LegacyScriptPubKeyMan::TopUp(unsigned int kpSize) } if (!batch.TxnCommit()) throw std::runtime_error(strprintf("Error during keypool top up. Cannot commit changes for wallet %s", m_storage.GetDisplayName())); NotifyCanGetAddressesChanged(); + // Note: Unlike with DescriptorSPKM, LegacySPKM does not need to call + // m_storage.TopUpCallback() as we do not know what new scripts the LegacySPKM is + // watching for. CWallet's scriptPubKey cache is not used for LegacySPKMs. return true; } @@ -2154,6 +2157,7 @@ bool DescriptorScriptPubKeyMan::TopUp(unsigned int size) bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int size) { LOCK(cs_desc_man); + std::set new_spks; unsigned int target_size; if (size > 0) { target_size = size; @@ -2184,6 +2188,7 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz if (!m_wallet_descriptor.descriptor->Expand(i, provider, scripts_temp, out_keys, &temp_cache)) return false; } // Add all of the scriptPubKeys to the scriptPubKey set + new_spks.insert(scripts_temp.begin(), scripts_temp.end()); for (const CScript& script : scripts_temp) { m_map_script_pub_keys[script] = i; } @@ -2209,6 +2214,7 @@ bool DescriptorScriptPubKeyMan::TopUpWithDB(WalletBatch& batch, unsigned int siz // By this point, the cache size should be the size of the entire range assert(m_wallet_descriptor.range_end - 1 == m_max_cached_index); + m_storage.TopUpCallback(new_spks, this); NotifyCanGetAddressesChanged(); return true; } @@ -2624,6 +2630,7 @@ uint256 DescriptorScriptPubKeyMan::GetID() const void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) { 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; @@ -2632,6 +2639,7 @@ void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) throw std::runtime_error("Error: Unable to expand wallet descriptor from cache"); } // Add all of the scriptPubKeys to the scriptPubKey set + new_spks.insert(scripts_temp.begin(), scripts_temp.end()); for (const CScript& script : scripts_temp) { if (m_map_script_pub_keys.count(script) != 0) { throw std::runtime_error(strprintf("Error: Already loaded script at index %d as being at index %d", i, m_map_script_pub_keys[script])); @@ -2649,6 +2657,8 @@ void DescriptorScriptPubKeyMan::SetCache(const DescriptorCache& cache) } m_max_cached_index++; } + // Make sure the wallet knows about our new spks + m_storage.TopUpCallback(new_spks, this); } bool DescriptorScriptPubKeyMan::AddKey(const CKeyID& key_id, const CKey& key) diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h index b63ba5bda04..43edc0a3f86 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -31,6 +31,7 @@ struct bilingual_str; namespace wallet { struct MigrationData; +class ScriptPubKeyMan; // Wallet storage things that ScriptPubKeyMans need in order to be able to store things to the wallet database. // It provides access to things that are part of the entire wallet and not specific to a ScriptPubKeyMan such as @@ -51,6 +52,8 @@ public: virtual bool WithEncryptionKey(std::function cb) const = 0; virtual bool HasEncryptionKeys() const = 0; virtual bool IsLocked() const = 0; + //! Callback function for after TopUp completes containining any scripts that were added by a SPKMan + virtual void TopUpCallback(const std::set&, ScriptPubKeyMan*) = 0; }; //! Constant representing an unknown spkm creation time diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e93cd4b4b9b..e98baeff548 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4334,4 +4334,14 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle } return res; } + +void CWallet::CacheNewScriptPubKeys(const std::set& spks, ScriptPubKeyMan* spkm) +{ +} + +void CWallet::TopUpCallback(const std::set& spks, ScriptPubKeyMan* spkm) +{ + // Update scriptPubKey cache + CacheNewScriptPubKeys(spks, spkm); +} } // namespace wallet diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index cc961068a54..d13a9308c42 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1042,6 +1042,11 @@ public: //! Whether the (external) signer performs R-value signature grinding bool CanGrindR() const; + + //! Add scriptPubKeys for this ScriptPubKeyMan into the scriptPubKey cache + void CacheNewScriptPubKeys(const std::set& spks, ScriptPubKeyMan* spkm); + + void TopUpCallback(const std::set& spks, ScriptPubKeyMan* spkm) override; }; /** From 37232332bd7253422ea92a8c9eeb36b12fc84b56 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 14:16:49 -0500 Subject: [PATCH 3/7] wallet: Cache scriptPubKeys for all DescriptorSPKMs Have CWallet maintain a cache of all known scriptPubKeys for its DescriptorSPKMs in order to improve performance of the functions that require searching for scriptPubKeys. --- src/wallet/wallet.cpp | 5 +++++ src/wallet/wallet.h | 3 +++ 2 files changed, 8 insertions(+) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e98baeff548..a416276d050 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3891,6 +3891,8 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) if (ExtractDestination(script, dest)) not_migrated_dests.emplace(dest); } + Assume(!m_cached_spks.empty()); + for (auto& desc_spkm : data.desc_spkms) { if (m_spk_managers.count(desc_spkm->GetID()) > 0) { error = _("Error: Duplicate descriptors created during migration. Your wallet may be corrupted."); @@ -4337,6 +4339,9 @@ util::Result MigrateLegacyToDescriptor(const std::string& walle void CWallet::CacheNewScriptPubKeys(const std::set& spks, ScriptPubKeyMan* spkm) { + for (const auto& script : spks) { + m_cached_spks[script].push_back(spkm); + } } void CWallet::TopUpCallback(const std::set& spks, ScriptPubKeyMan* spkm) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index d13a9308c42..3360d2515ac 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -422,6 +422,9 @@ private: // Same as 'AddActiveScriptPubKeyMan' but designed for use within a batch transaction context void AddActiveScriptPubKeyManWithDb(WalletBatch& batch, uint256 id, OutputType type, bool internal); + //! Cache of descriptor ScriptPubKeys used for IsMine. Maps ScriptPubKey to set of spkms + std::unordered_map, SaltedSipHasher> m_cached_spks; + /** * Catch wallet up to current chain, scanning new blocks, updating the best * block locator and m_last_block_processed, and registering for From edf4e73a163739a64eb9a54cd95413583a0e5c1f Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 14:16:54 -0500 Subject: [PATCH 4/7] wallet: Use scriptPubKey cache in IsMine --- src/wallet/wallet.cpp | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index a416276d050..0c61a9b3fa2 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1571,11 +1571,22 @@ isminetype CWallet::IsMine(const CTxDestination& dest) const isminetype CWallet::IsMine(const CScript& script) const { AssertLockHeld(cs_wallet); - isminetype result = ISMINE_NO; - for (const auto& spk_man_pair : m_spk_managers) { - result = std::max(result, spk_man_pair.second->IsMine(script)); + + // Search the cache so that IsMine is called only on the relevant SPKMs instead of on everything in m_spk_managers + const auto& it = m_cached_spks.find(script); + if (it != m_cached_spks.end()) { + isminetype res = ISMINE_NO; + for (const auto& spkm : it->second) { + res = std::max(res, spkm->IsMine(script)); + } + Assume(res == ISMINE_SPENDABLE); + return res; } - return result; + + // Legacy wallet + if (IsLegacy()) return GetLegacyScriptPubKeyMan()->IsMine(script); + + return ISMINE_NO; } bool CWallet::IsMine(const CTransaction& tx) const From b410f68791143800968f4c628beda1c9f898b4f6 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 14:17:02 -0500 Subject: [PATCH 5/7] wallet: Use scriptPubKey cache in GetScriptPubKeyMans --- src/wallet/wallet.cpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0c61a9b3fa2..e43243cb554 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3436,12 +3436,18 @@ ScriptPubKeyMan* CWallet::GetScriptPubKeyMan(const OutputType& type, bool intern std::set CWallet::GetScriptPubKeyMans(const CScript& script) const { std::set spk_mans; - SignatureData sigdata; - for (const auto& spk_man_pair : m_spk_managers) { - if (spk_man_pair.second->CanProvide(script, sigdata)) { - spk_mans.insert(spk_man_pair.second.get()); - } + + // Search the cache for relevant SPKMs instead of iterating m_spk_managers + const auto& it = m_cached_spks.find(script); + if (it != m_cached_spks.end()) { + spk_mans.insert(it->second.begin(), it->second.end()); } + SignatureData sigdata; + Assume(std::all_of(spk_mans.begin(), spk_mans.end(), [&script, &sigdata](ScriptPubKeyMan* spkm) { return spkm->CanProvide(script, sigdata); })); + + // Legacy wallet + if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) spk_mans.insert(GetLegacyScriptPubKeyMan()); + return spk_mans; } From 39640dd34e980e69d13664ddbc2a7612a1888ab4 Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Fri, 2 Feb 2024 14:17:05 -0500 Subject: [PATCH 6/7] wallet: Use scriptPubKeyCache in GetSolvingProvider --- src/wallet/wallet.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e43243cb554..ad09dcd38d8 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3467,11 +3467,17 @@ std::unique_ptr CWallet::GetSolvingProvider(const CScript& scri std::unique_ptr CWallet::GetSolvingProvider(const CScript& script, SignatureData& sigdata) const { - for (const auto& spk_man_pair : m_spk_managers) { - if (spk_man_pair.second->CanProvide(script, sigdata)) { - return spk_man_pair.second->GetSolvingProvider(script); - } + // Search the cache for relevant SPKMs instead of iterating m_spk_managers + const auto& it = m_cached_spks.find(script); + if (it != m_cached_spks.end()) { + // All spkms for a given script must already be able to make a SigningProvider for the script, so just return the first one. + Assume(it->second.at(0)->CanProvide(script, sigdata)); + return it->second.at(0)->GetSolvingProvider(script); } + + // Legacy wallet + if (IsLegacy() && GetLegacyScriptPubKeyMan()->CanProvide(script, sigdata)) return GetLegacyScriptPubKeyMan()->GetSolvingProvider(script); + return nullptr; } From e041ed9b755468d205ed48b29f6c4b9e9df8bc9f Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Thu, 15 Feb 2024 11:43:13 -0500 Subject: [PATCH 7/7] wallet: Retrieve ID from loaded DescSPKM directly Instead of iterating m_spk_managers a DescriptorSPKM has been loaded in order to get it's ID to compare, have LoadDescriptorSPKM return a reference to the loaded DescriptorSPKM so it can be queried directly. --- src/wallet/wallet.cpp | 11 ++++++----- src/wallet/wallet.h | 2 +- src/wallet/walletdb.cpp | 4 ++-- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ad09dcd38d8..4259565a83e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3556,15 +3556,16 @@ void CWallet::ConnectScriptPubKeyManNotifiers() } } -void CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) +DescriptorScriptPubKeyMan& CWallet::LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc) { + DescriptorScriptPubKeyMan* spk_manager; if (IsWalletFlagSet(WALLET_FLAG_EXTERNAL_SIGNER)) { - auto spk_manager = std::unique_ptr(new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size)); - AddScriptPubKeyMan(id, std::move(spk_manager)); + spk_manager = new ExternalSignerScriptPubKeyMan(*this, desc, m_keypool_size); } else { - auto spk_manager = std::unique_ptr(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size)); - AddScriptPubKeyMan(id, std::move(spk_manager)); + spk_manager = new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size); } + AddScriptPubKeyMan(id, std::unique_ptr(spk_manager)); + return *spk_manager; } void CWallet::SetupDescriptorScriptPubKeyMans(const CExtKey& master_key) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 3360d2515ac..52779703f7b 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -994,7 +994,7 @@ public: void ConnectScriptPubKeyManNotifiers(); //! Instantiate a descriptor ScriptPubKeyMan from the WalletDescriptor and load it - void LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc); + DescriptorScriptPubKeyMan& LoadDescriptorScriptPubKeyMan(uint256 id, WalletDescriptor& desc); //! 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 f3dd5b328e7..e60c951a480 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -804,10 +804,10 @@ static DBErrors LoadDescriptorWalletRecords(CWallet* pwallet, DatabaseBatch& bat strErr = strprintf("%s\nDetails: %s", strErr, e.what()); return DBErrors::UNKNOWN_DESCRIPTOR; } - pwallet->LoadDescriptorScriptPubKeyMan(id, desc); + DescriptorScriptPubKeyMan& spkm = pwallet->LoadDescriptorScriptPubKeyMan(id, desc); // Prior to doing anything with this spkm, verify ID compatibility - if (id != pwallet->GetDescriptorScriptPubKeyMan(desc)->GetID()) { + if (id != spkm.GetID()) { strErr = "The descriptor ID calculated by the wallet differs from the one in DB"; return DBErrors::CORRUPT; }