removed duplicate calling of GetDescriptorScriptPubKeyMan

saikiran57 · saikiran57 · commit 55b931934a34 · 2025-03-24T17:27:27.000+05:30
Removed duplicate call to GetDescriptorScriptPubKeyMan and Instead of checking linearly I have used find method so time complexity reduced significantly for GetDescriptorScriptPubKeyMan after this fix improved performance of importdescriptor part refs bitcoin#32013.
diff --git a/src/bench/wallet_ismine.cpp b/src/bench/wallet_ismine.cpp
@@ -54,8 +54,8 @@ static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_co
             std::string error;
             std::vector<std::unique_ptr<Descriptor>> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /*require_checksum=*/false);
             WalletDescriptor w_desc(std::move(desc.at(0)), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/0, /*next_index=*/0);
-            auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false);
-            assert(spkm);
+            auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false));
+            assert(spk_manager);
         }
     }
 
diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp
@@ -223,7 +223,8 @@ std::shared_ptr<CWallet> SetupDescriptorsWallet(interfaces::Node& node, TestChai
     assert(descs.size() == 1);
     auto& desc = descs.at(0);
     WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
-    if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
+    auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, provider, "", false));
+    assert(spk_manager);
     CTxDestination dest = GetDestinationForKey(test.coinbaseKey.GetPubKey(), wallet->m_default_address_type);
     wallet->SetAddressBook(dest, "", wallet::AddressPurpose::RECEIVE);
     wallet->SetLastBlockProcessed(105, WITH_LOCK(node.context()->chainman->GetMutex(), return node.context()->chainman->ActiveChain().Tip()->GetBlockHash()));
diff --git a/src/test/fuzz/util/wallet.h b/src/test/fuzz/util/wallet.h
@@ -59,7 +59,7 @@ struct FuzzedWallet {
                 WalletDescriptor w_desc{std::move(parsed_desc), /*creation_time=*/0, /*range_start=*/0, /*range_end=*/1, /*next_index=*/0};
                 assert(!wallet->GetDescriptorScriptPubKeyMan(w_desc));
                 LOCK(wallet->cs_wallet);
-                auto spk_manager{wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal)};
+                auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal));
                 assert(spk_manager);
                 wallet->AddActiveScriptPubKeyMan(spk_manager->GetID(), *Assert(w_desc.descriptor->GetOutputType()), internal);
             }
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
@@ -1575,16 +1575,15 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c
 
             WalletDescriptor w_desc(std::move(parsed_desc), timestamp, range_start, range_end, next_index);
 
-            // Check if the wallet already contains the descriptor
-            auto existing_spk_manager = wallet.GetDescriptorScriptPubKeyMan(w_desc);
-            if (existing_spk_manager) {
-                if (!existing_spk_manager->CanUpdateToWalletDescriptor(w_desc, error)) {
-                    throw JSONRPCError(RPC_INVALID_PARAMETER, error);
-                }
+            // Add descriptor to the wallet
+            auto spk_manager_res = wallet.AddWalletDescriptor(w_desc, keys, label, desc_internal);
+
+            if (!spk_manager_res) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(spk_manager_res).original);
             }
 
-            // Add descriptor to the wallet
-            auto spk_manager = wallet.AddWalletDescriptor(w_desc, keys, label, desc_internal);
+            auto spk_manager = spk_manager_res.value();
+
             if (spk_manager == nullptr) {
                 throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Could not add descriptor '%s'", descriptor));
             }
diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp
@@ -2827,12 +2827,12 @@ void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()
     }
 }
 
-void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor)
+util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor)
 {
     LOCK(cs_desc_man);
     std::string error;
     if (!CanUpdateToWalletDescriptor(descriptor, error)) {
-        throw std::runtime_error(std::string(__func__) + ": " + error);
+        return util::Error{Untranslated(std::move(error))};
     }
 
     m_map_pubkeys.clear();
@@ -2841,6 +2841,7 @@ void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descrip
     m_wallet_descriptor = descriptor;
 
     NotifyFirstKeyTimeChanged(this, m_wallet_descriptor.creation_time);
+    return {};
 }
 
 bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error)
diff --git a/src/wallet/scriptpubkeyman.h b/src/wallet/scriptpubkeyman.h
@@ -694,7 +694,7 @@ class DescriptorScriptPubKeyMan : public ScriptPubKeyMan
     bool AddCryptedKey(const CKeyID& key_id, const CPubKey& pubkey, const std::vector<unsigned char>& crypted_key);
 
     bool HasWalletDescriptor(const WalletDescriptor& desc) const;
-    void UpdateWalletDescriptor(WalletDescriptor& descriptor);
+    util::Result<void> UpdateWalletDescriptor(WalletDescriptor& descriptor);
     bool CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error);
     void AddDescriptorKey(const CKey& key, const CPubKey &pubkey);
     void WriteDescriptor();
diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp
@@ -80,8 +80,12 @@ static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> CreateWal
 static DescriptorScriptPubKeyMan* CreateDescriptor(WalletDescriptor& wallet_desc, FlatSigningProvider& keys, CWallet& keystore)
 {
     LOCK(keystore.cs_wallet);
-    keystore.AddWalletDescriptor(wallet_desc, keys, /*label=*/"", /*internal=*/false);
-    return keystore.GetDescriptorScriptPubKeyMan(wallet_desc);
+    DescriptorScriptPubKeyMan* descriptor_spk_manager = nullptr;
+    auto spk_manager = *Assert(keystore.AddWalletDescriptor(wallet_desc, keys, /*label=*/"", /*internal=*/false));
+    if (spk_manager) {
+        descriptor_spk_manager = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_manager);
+    }
+    return descriptor_spk_manager;
 };
 
 FUZZ_TARGET(scriptpubkeyman, .init = initialize_spkm)
diff --git a/src/wallet/test/psbt_wallet_tests.cpp b/src/wallet/test/psbt_wallet_tests.cpp
@@ -27,7 +27,8 @@ static void import_descriptor(CWallet& wallet, const std::string& descriptor)
     assert(descs.size() == 1);
     auto& desc = descs.at(0);
     WalletDescriptor w_desc(std::move(desc), 0, 0, 10, 0);
-    wallet.AddWalletDescriptor(w_desc, provider, "", false);
+    auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));
+    assert(spk_manager);
 }
 
 BOOST_AUTO_TEST_CASE(psbt_updater_test)
diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp
@@ -35,7 +35,8 @@ std::unique_ptr<CWallet> CreateSyncedWallet(interfaces::Chain& chain, CChain& cc
         assert(descs.size() == 1);
         auto& desc = descs.at(0);
         WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
-        if (!wallet->AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
+        auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, provider, "", false));
+        assert(spk_manager);
     }
     WalletRescanReserver reserver(*wallet);
     reserver.reserve();
@@ -209,7 +210,7 @@ wallet::ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string&
     WalletDescriptor w_desc(std::move(desc), timestamp, range_start, range_end, next_index);
 
     LOCK(keystore.cs_wallet);
-
-    return Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false));
+    auto spkm = Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false));
+    return spkm.value();
 };
 } // namespace wallet
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
@@ -69,7 +69,8 @@ static void AddKey(CWallet& wallet, const CKey& key)
     assert(descs.size() == 1);
     auto& desc = descs.at(0);
     WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);
-    if (!wallet.AddWalletDescriptor(w_desc, provider, "", false)) assert(false);
+    auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));
+    assert(spk_manager);
 }
 
 BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -3909,9 +3909,11 @@ bool CWallet::IsLegacy() const
 
 DescriptorScriptPubKeyMan* CWallet::GetDescriptorScriptPubKeyMan(const WalletDescriptor& desc) const
 {
-    for (auto& spk_man_pair : m_spk_managers) {
+    auto spk_man_pair = m_spk_managers.find(desc.id);
+
+    if (spk_man_pair != m_spk_managers.end()) {
         // Try to downcast to DescriptorScriptPubKeyMan then check if the descriptors match
-        DescriptorScriptPubKeyMan* spk_manager = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man_pair.second.get());
+        DescriptorScriptPubKeyMan* spk_manager = dynamic_cast<DescriptorScriptPubKeyMan*>(spk_man_pair->second.get());
         if (spk_manager != nullptr && spk_manager->HasWalletDescriptor(desc)) {
             return spk_manager;
         }
@@ -3944,7 +3946,7 @@ std::optional<bool> CWallet::IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man)
     return GetScriptPubKeyMan(*type, /* internal= */ true) == desc_spk_man;
 }
 
-ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal)
+util::Result<ScriptPubKeyMan*> CWallet::AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal)
 {
     AssertLockHeld(cs_wallet);
 
@@ -3956,7 +3958,9 @@ ScriptPubKeyMan* CWallet::AddWalletDescriptor(WalletDescriptor& desc, const Flat
     auto spk_man = GetDescriptorScriptPubKeyMan(desc);
     if (spk_man) {
         WalletLogPrintf("Update existing descriptor: %s\n", desc.descriptor->ToString());
-        spk_man->UpdateWalletDescriptor(desc);
+        if (auto spkm_res = spk_man->UpdateWalletDescriptor(desc); !spkm_res) {
+            return util::Error{util::ErrorString(spkm_res)};
+        }
     } else {
         auto new_spk_man = std::unique_ptr<DescriptorScriptPubKeyMan>(new DescriptorScriptPubKeyMan(*this, desc, m_keypool_size));
         spk_man = new_spk_man.get();
@@ -4354,7 +4358,9 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
 
                 // Add to the wallet
                 WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0);
-                data->watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false);
+                if (auto spkm_res = data->watchonly_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
+                    throw std::runtime_error(util::ErrorString(spkm_res).original);
+                }
             }
 
             // Add the wallet to settings
@@ -4391,7 +4397,9 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
 
                 // Add to the wallet
                 WalletDescriptor w_desc(std::move(descs.at(0)), creation_time, 0, 0, 0);
-                data->solvable_wallet->AddWalletDescriptor(w_desc, keys, "", false);
+                if (auto spkm_res = data->solvable_wallet->AddWalletDescriptor(w_desc, keys, "", false); !spkm_res) {
+                    throw std::runtime_error(util::ErrorString(spkm_res).original);
+                }
             }
 
             // Add the wallet to settings
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
@@ -1039,7 +1039,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
     std::optional<bool> IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man) const;
 
     //! Add a descriptor to the wallet, return a ScriptPubKeyMan & associated output type
-    ScriptPubKeyMan* AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<ScriptPubKeyMan*> AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
 
     /** Move all records from the BDB database to a new SQLite database for storage.
      * The original BDB file will be deleted and replaced with a new SQLite file.

Original file line number	Diff line number	Diff line change
`@@ -54,8 +54,8 @@ static void WalletIsMine(benchmark::Bench& bench, bool legacy_wallet, int num_co`
`54`	`54`	`std::string error;`
`55`	`55`	`std::vector<std::unique_ptr<Descriptor>> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /require_checksum=/false);`
`56`	`56`	`WalletDescriptor w_desc(std::move(desc.at(0)), /creation_time=/0, /range_start=/0, /range_end=/0, /next_index=/0);`
`57`		`- auto spkm = wallet->AddWalletDescriptor(w_desc, keys, /label=/"", /internal=/false);`
`58`		`- assert(spkm);`
	`57`	`+ auto spk_manager = Assert(wallet->AddWalletDescriptor(w_desc, keys, /label=/"", /internal=*/false));`
	`58`	`+ assert(spk_manager);`
`59`	`59`	`}`
`60`	`60`	`}`
`61`	`61`
Original file line number	Diff line number	Diff line change
`@@ -59,7 +59,7 @@ struct FuzzedWallet {`
`59`	`59`	`WalletDescriptor w_desc{std::move(parsed_desc), /creation_time=/0, /range_start=/0, /range_end=/1, /next_index=/0};`
`60`	`60`	`assert(!wallet->GetDescriptorScriptPubKeyMan(w_desc));`
`61`	`61`	`LOCK(wallet->cs_wallet);`
`62`		`- auto spk_manager{wallet->AddWalletDescriptor(w_desc, keys, /label=/"", internal)};`
	`62`	`+ auto spk_manager = Assert(wallet->AddWalletDescriptor(w_desc, keys, /label=*/"", internal));`
`63`	`63`	`assert(spk_manager);`
`64`	`64`	`wallet->AddActiveScriptPubKeyMan(spk_manager->GetID(), *Assert(w_desc.descriptor->GetOutputType()), internal);`
`65`	`65`	`}`
Original file line number	Diff line number	Diff line change
`@@ -2827,12 +2827,12 @@ void DescriptorScriptPubKeyMan::UpgradeDescriptorCache()`
`2827`	`2827`	`}`
`2828`	`2828`	`}`
`2829`	`2829`
`2830`		`-void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor)`
	`2830`	`+util::Result<void> DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor)`
`2831`	`2831`	`{`
`2832`	`2832`	`LOCK(cs_desc_man);`
`2833`	`2833`	`std::string error;`
`2834`	`2834`	`if (!CanUpdateToWalletDescriptor(descriptor, error)) {`
`2835`		`- throw std::runtime_error(std::string(__func__) + ": " + error);`
	`2835`	`+ return util::Error{Untranslated(std::move(error))};`
`2836`	`2836`	`}`
`2837`	`2837`
`2838`	`2838`	`m_map_pubkeys.clear();`
`@@ -2841,6 +2841,7 @@ void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descrip`
`2841`	`2841`	`m_wallet_descriptor = descriptor;`
`2842`	`2842`
`2843`	`2843`	`NotifyFirstKeyTimeChanged(this, m_wallet_descriptor.creation_time);`
	`2844`	`+ return {};`
`2844`	`2845`	`}`
`2845`	`2846`
`2846`	`2847`	`bool DescriptorScriptPubKeyMan::CanUpdateToWalletDescriptor(const WalletDescriptor& descriptor, std::string& error)`
Original file line number	Diff line number	Diff line change
`@@ -27,7 +27,8 @@ static void import_descriptor(CWallet& wallet, const std::string& descriptor)`
`27`	`27`	`assert(descs.size() == 1);`
`28`	`28`	`auto& desc = descs.at(0);`
`29`	`29`	`WalletDescriptor w_desc(std::move(desc), 0, 0, 10, 0);`
`30`		`- wallet.AddWalletDescriptor(w_desc, provider, "", false);`
	`30`	`+ auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));`
	`31`	`+ assert(spk_manager);`
`31`	`32`	`}`
`32`	`33`
`33`	`34`	`BOOST_AUTO_TEST_CASE(psbt_updater_test)`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,8 @@ static void AddKey(CWallet& wallet, const CKey& key)`
`69`	`69`	`assert(descs.size() == 1);`
`70`	`70`	`auto& desc = descs.at(0);`
`71`	`71`	`WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);`
`72`		`- if (!wallet.AddWalletDescriptor(w_desc, provider, "", false)) assert(false);`
	`72`	`+ auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));`
	`73`	`+ assert(spk_manager);`
`73`	`74`	`}`
`74`	`75`
`75`	`76`	`BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)`