diff --git a/src/bench/wallet_ismine.cpp b/src/bench/wallet_ismine.cpp index 441448c9b19..6e91559a169 100644 --- 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> 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 index 8d945a1b869..d0602eef11a 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -223,7 +223,8 @@ std::shared_ptr 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 index 1f04b5dbf1a..8c27d0414e2 100644 --- 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 index 291c3bb9430..39d85090074 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -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)); } diff --git a/src/wallet/scriptpubkeyman.cpp b/src/wallet/scriptpubkeyman.cpp index 04287581f5a..80f25c62c2b 100644 --- a/src/wallet/scriptpubkeyman.cpp +++ b/src/wallet/scriptpubkeyman.cpp @@ -2827,12 +2827,12 @@ void DescriptorScriptPubKeyMan::UpgradeDescriptorCache() } } -void DescriptorScriptPubKeyMan::UpdateWalletDescriptor(WalletDescriptor& descriptor) +util::Result 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 index 4dbb2838d71..8af16319c1f 100644 --- a/src/wallet/scriptpubkeyman.h +++ b/src/wallet/scriptpubkeyman.h @@ -694,7 +694,7 @@ public: bool AddCryptedKey(const CKeyID& key_id, const CPubKey& pubkey, const std::vector& crypted_key); bool HasWalletDescriptor(const WalletDescriptor& desc) const; - void UpdateWalletDescriptor(WalletDescriptor& descriptor); + util::Result 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 index 0c17a6bf7a9..63768c89afc 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -80,8 +80,12 @@ static std::optional> 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(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 index 09057114a0b..c44ea496339 100644 --- 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 index 8cf58800514..f84e488f0a2 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -35,7 +35,8 @@ std::unique_ptr 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 index b5de4b4b3d3..bb76e4219ca 100644 --- 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 index 9ae0fb5329e..deffaef75da 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -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(spk_man_pair.second.get()); + DescriptorScriptPubKeyMan* spk_manager = dynamic_cast(spk_man_pair->second.get()); if (spk_manager != nullptr && spk_manager->HasWalletDescriptor(desc)) { return spk_manager; } @@ -3946,7 +3948,7 @@ std::optional 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 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(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 diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index c475718eb94..659e3854e71 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1039,7 +1039,7 @@ public: std::optional 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 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.