phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-04-29 14:59:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#32023: wallet: removed duplicate call to GetDescriptorScriptPubKeyMan

Browse source 
55b931934a removed duplicate calling of GetDescriptorScriptPubKeyMan (Saikiran)

Pull request description:

  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 https://github.com/bitcoin/bitcoin/issues/32013.

  **Steps to reproduce in testnet environment**

  **Input size:** 2 million address in the wallet

  **Step1:** call importaddresdescriptor rpc method
  observe the time it has taken.

  **With the provided fix:**
  Do the same steps again
  observe the time it has taken.

  There is a huge improvement in the performance. (previously it may take 5 to 6 seconds now it will take 1 seconds or less)

  main changes i've made during this pr:

  1. remove duplicate call to GetDescriptorScriptPubKeyMan method
  2. And inside GetDescriptorScriptPubKeyMan method previously we checking **each address linearly** so each time it is calling HasWallet method which has aquired lock.
  3. Now i've modified this logic call **find method on the map (O(logn)**) time it is taking, so only once we calling HasWallet method.

  **Note:** Smaller inputs in the wallet you may not see the issue but huge wallet size it will definitely impact the performance.

ACKs for top commit:
  achow101:
    ACK 55b931934a
  w0xlt:
    ACK 55b931934a

Tree-SHA512: 4a7fdbcbb4e55bd034e9cf28ab4e7ee3fb1745fc8847adb388c98a19c952a1fb66d7b54f0f28b4c2a75a42473923742b4a99fb26771577183a98e0bcbf87a8ca
This commit is contained in:
Ava Chow 2025-04-23 13:51:48 -07:00
commit bd158ab4e3
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
12 changed files with 45 additions and 29 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

4
src/bench/wallet_ismine.cpp
View file

@ -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);
}
}

3
src/qt/test/wallettests.cpp
View file

@ -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()));

2
src/test/fuzz/util/wallet.h
View file

@ -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);
}

15
src/wallet/rpc/backup.cpp
View file

@ -1580,16 +1580,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));
}

5
src/wallet/scriptpubkeyman.cpp
View file

@ -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)

2
src/wallet/scriptpubkeyman.h
View file

@ -694,7 +694,7 @@ public:
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();

8
src/wallet/test/fuzz/scriptpubkeyman.cpp
View file

@ -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)

3
src/wallet/test/psbt_wallet_tests.cpp
View file

@ -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)

7
src/wallet/test/util.cpp
View file

@ -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

3
src/wallet/test/wallet_tests.cpp
View file

@ -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)

20
src/wallet/wallet.cpp
View file

@ -3911,9 +3911,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;
}
@ -3946,7 +3948,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);
@ -3958,7 +3960,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();
@ -4356,7 +4360,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
@ -4393,7 +4399,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

2
src/wallet/wallet.h
View file

@ -1039,7 +1039,7 @@ public:
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.