From 785e1407b0a39fef81a7b25554aab88d4cecd66b Mon Sep 17 00:00:00 2001 From: Ava Chow Date: Mon, 12 May 2025 17:43:08 -0700 Subject: [PATCH] wallet: Use util::Error throughout AddWalletDescriptor 32023 changed AddWalletDescriptor to return util::Error, but did not change all of the failure cases to do so. This may result in some callers continuing when there was actually an error. Unify all of the failure cases to use util::Error so that all callers handle AddWalletDescriptor errors in the same way. The encapsulated return type is changed from ScriptPubKeyMan* to std::reference_wrapper. This avoids having a value that can be interpreted as a bool, and also removes the need to constantly dynamic_cast the returned value. The only kind of ScriptPubKeyMan that can come out of AddWalletDescriptor is a DescriptorScriptPubKeyMan anyways. --- src/bench/wallet_ismine.cpp | 3 +-- src/qt/test/wallettests.cpp | 3 +-- src/test/fuzz/util/wallet.h | 5 ++--- src/wallet/rpc/backup.cpp | 12 ++++-------- src/wallet/test/fuzz/scriptpubkeyman.cpp | 9 +++------ src/wallet/test/psbt_wallet_tests.cpp | 3 +-- src/wallet/test/scriptpubkeyman_tests.cpp | 4 ++-- src/wallet/test/util.cpp | 7 +++---- src/wallet/test/util.h | 2 +- src/wallet/test/wallet_tests.cpp | 3 +-- src/wallet/wallet.cpp | 13 +++++-------- src/wallet/wallet.h | 2 +- test/functional/wallet_importdescriptors.py | 6 +++--- 13 files changed, 28 insertions(+), 44 deletions(-) diff --git a/src/bench/wallet_ismine.cpp b/src/bench/wallet_ismine.cpp index a0832e458f4..30c73eaaadf 100644 --- a/src/bench/wallet_ismine.cpp +++ b/src/bench/wallet_ismine.cpp @@ -50,8 +50,7 @@ static void WalletIsMine(benchmark::Bench& bench, int num_combo = 0) 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 spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false)); - assert(spk_manager); + Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", /*internal=*/false)); } } diff --git a/src/qt/test/wallettests.cpp b/src/qt/test/wallettests.cpp index ee410e7db16..5e65dcca318 100644 --- a/src/qt/test/wallettests.cpp +++ b/src/qt/test/wallettests.cpp @@ -215,8 +215,7 @@ 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); - auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, provider, "", false)); - assert(spk_manager); + Assert(wallet->AddWalletDescriptor(w_desc, provider, "", false)); const PKHash dest{test.coinbaseKey.GetPubKey()}; 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 8c27d0414e2..2646f73be75 100644 --- a/src/test/fuzz/util/wallet.h +++ b/src/test/fuzz/util/wallet.h @@ -59,9 +59,8 @@ 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 = *Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal)); - assert(spk_manager); - wallet->AddActiveScriptPubKeyMan(spk_manager->GetID(), *Assert(w_desc.descriptor->GetOutputType()), internal); + auto& spk_manager = Assert(wallet->AddWalletDescriptor(w_desc, keys, /*label=*/"", internal))->get(); + 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 56915b0a16c..671f3fb2271 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -262,25 +262,21 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c 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); + throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Could not add descriptor '%s': %s", descriptor, util::ErrorString(spk_manager_res).original)); } - auto spk_manager = spk_manager_res.value(); - - if (spk_manager == nullptr) { - throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Could not add descriptor '%s'", descriptor)); - } + auto& spk_manager = spk_manager_res.value().get(); // Set descriptor as active if necessary if (active) { if (!w_desc.descriptor->GetOutputType()) { warnings.push_back("Unknown output type, cannot set descriptor to active."); } else { - wallet.AddActiveScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), desc_internal); + wallet.AddActiveScriptPubKeyMan(spk_manager.GetID(), *w_desc.descriptor->GetOutputType(), desc_internal); } } else { if (w_desc.descriptor->GetOutputType()) { - wallet.DeactivateScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), desc_internal); + wallet.DeactivateScriptPubKeyMan(spk_manager.GetID(), *w_desc.descriptor->GetOutputType(), desc_internal); } } } diff --git a/src/wallet/test/fuzz/scriptpubkeyman.cpp b/src/wallet/test/fuzz/scriptpubkeyman.cpp index 63768c89afc..79fe99c3879 100644 --- a/src/wallet/test/fuzz/scriptpubkeyman.cpp +++ b/src/wallet/test/fuzz/scriptpubkeyman.cpp @@ -80,12 +80,9 @@ static std::optional> CreateWal static DescriptorScriptPubKeyMan* CreateDescriptor(WalletDescriptor& wallet_desc, FlatSigningProvider& keys, CWallet& keystore) { LOCK(keystore.cs_wallet); - 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; + auto spk_manager_res = keystore.AddWalletDescriptor(wallet_desc, keys, /*label=*/"", /*internal=*/false); + if (!spk_manager_res) return nullptr; + return &spk_manager_res.value().get(); }; 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 c44ea496339..fdb28b0890f 100644 --- a/src/wallet/test/psbt_wallet_tests.cpp +++ b/src/wallet/test/psbt_wallet_tests.cpp @@ -27,8 +27,7 @@ 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); - auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false)); - assert(spk_manager); + Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false)); } BOOST_AUTO_TEST_CASE(psbt_updater_test) diff --git a/src/wallet/test/scriptpubkeyman_tests.cpp b/src/wallet/test/scriptpubkeyman_tests.cpp index 420c4866b09..90fff119f03 100644 --- a/src/wallet/test/scriptpubkeyman_tests.cpp +++ b/src/wallet/test/scriptpubkeyman_tests.cpp @@ -25,13 +25,13 @@ BOOST_AUTO_TEST_CASE(DescriptorScriptPubKeyManTests) // Verify that a SigningProvider for a pubkey is only returned if its corresponding private key is available auto key_internal = GenerateRandomKey(); std::string desc_str = "tr(" + EncodeSecret(key_internal) + ",pk(" + HexStr(key_scriptpath.GetPubKey()) + "))"; - auto spk_man1 = dynamic_cast(CreateDescriptor(keystore, desc_str, true)); + auto spk_man1 = CreateDescriptor(keystore, desc_str, true); BOOST_CHECK(spk_man1 != nullptr); auto signprov_keypath_spendable = spk_man1->GetSigningProvider(key_internal.GetPubKey()); BOOST_CHECK(signprov_keypath_spendable != nullptr); desc_str = "tr(" + HexStr(XOnlyPubKey::NUMS_H) + ",pk(" + HexStr(key_scriptpath.GetPubKey()) + "))"; - auto spk_man2 = dynamic_cast(CreateDescriptor(keystore, desc_str, true)); + auto spk_man2 = CreateDescriptor(keystore, desc_str, true); BOOST_CHECK(spk_man2 != nullptr); auto signprov_keypath_nums_h = spk_man2->GetSigningProvider(XOnlyPubKey::NUMS_H.GetEvenCorrespondingCPubKey()); BOOST_CHECK(signprov_keypath_nums_h == nullptr); diff --git a/src/wallet/test/util.cpp b/src/wallet/test/util.cpp index f84e488f0a2..c06c6af5b55 100644 --- a/src/wallet/test/util.cpp +++ b/src/wallet/test/util.cpp @@ -35,8 +35,7 @@ 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); - auto spk_manager = *Assert(wallet->AddWalletDescriptor(w_desc, provider, "", false)); - assert(spk_manager); + Assert(wallet->AddWalletDescriptor(w_desc, provider, "", false)); } WalletRescanReserver reserver(*wallet); reserver.reserve(); @@ -194,7 +193,7 @@ MockableDatabase& GetMockableDatabase(CWallet& wallet) return dynamic_cast(wallet.GetDatabase()); } -wallet::ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success) +wallet::DescriptorScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success) { keystore.SetWalletFlag(WALLET_FLAG_DESCRIPTORS); @@ -211,6 +210,6 @@ wallet::ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& LOCK(keystore.cs_wallet); auto spkm = Assert(keystore.AddWalletDescriptor(w_desc, keys,/*label=*/"", /*internal=*/false)); - return spkm.value(); + return &spkm.value().get(); }; } // namespace wallet diff --git a/src/wallet/test/util.h b/src/wallet/test/util.h index dbefa6b9649..7a19806731b 100644 --- a/src/wallet/test/util.h +++ b/src/wallet/test/util.h @@ -116,7 +116,7 @@ public: std::unique_ptr CreateMockableWalletDatabase(MockableData records = {}); MockableDatabase& GetMockableDatabase(CWallet& wallet); -ScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success); +DescriptorScriptPubKeyMan* CreateDescriptor(CWallet& keystore, const std::string& desc_str, const bool success); } // namespace wallet #endif // BITCOIN_WALLET_TEST_UTIL_H diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 072935ee897..f52b0088590 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -66,8 +66,7 @@ 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); - auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false)); - assert(spk_manager); + Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false)); } BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index fc1c31ad27c..00beccdf69f 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3713,13 +3713,12 @@ std::optional CWallet::IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man) return GetScriptPubKeyMan(*type, /* internal= */ true) == desc_spk_man; } -util::Result 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); if (!IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) { - WalletLogPrintf("Cannot add WalletDescriptor to a non-descriptor wallet\n"); - return nullptr; + return util::Error{_("Cannot add WalletDescriptor to a non-descriptor wallet")}; } auto spk_man = GetDescriptorScriptPubKeyMan(desc); @@ -3745,8 +3744,7 @@ util::Result CWallet::AddWalletDescriptor(WalletDescriptor& de // Top up key pool, the manager will generate new scriptPubKeys internally if (!spk_man->TopUp()) { - WalletLogPrintf("Could not top up scriptPubKeys\n"); - return nullptr; + return util::Error{_("Could not top up scriptPubKeys")}; } // Apply the label if necessary @@ -3754,8 +3752,7 @@ util::Result CWallet::AddWalletDescriptor(WalletDescriptor& de if (!desc.descriptor->IsRange()) { auto script_pub_keys = spk_man->GetScriptPubKeys(); if (script_pub_keys.empty()) { - WalletLogPrintf("Could not generate scriptPubKeys (cache is empty)\n"); - return nullptr; + return util::Error{_("Could not generate scriptPubKeys (cache is empty)")}; } if (!internal) { @@ -3771,7 +3768,7 @@ util::Result CWallet::AddWalletDescriptor(WalletDescriptor& de // Save the descriptor to DB spk_man->WriteDescriptor(); - return spk_man; + return std::reference_wrapper(*spk_man); } bool CWallet::MigrateToSQLite(bilingual_str& error) diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 54e5c94200f..c8ee37923d3 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1020,7 +1020,7 @@ public: std::optional IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man) const; //! Add a descriptor to the wallet, return a ScriptPubKeyMan & associated output type - util::Result 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. diff --git a/test/functional/wallet_importdescriptors.py b/test/functional/wallet_importdescriptors.py index 5b76c196008..42674fcf1d6 100755 --- a/test/functional/wallet_importdescriptors.py +++ b/test/functional/wallet_importdescriptors.py @@ -284,11 +284,11 @@ class ImportDescriptorsTest(BitcoinTestFramework): assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) self.test_importdesc({**range_request, "range": [5, 10]}, wallet=wpriv, success=False, - error_code=-8, error_message='new range must include current range = [0,20]') + error_code=-4, error_message=f"Could not add descriptor '{range_request['desc']}': new range must include current range = [0,20]") self.test_importdesc({**range_request, "range": [0, 10]}, wallet=wpriv, success=False, - error_code=-8, error_message='new range must include current range = [0,20]') + error_code=-4, error_message=f"Could not add descriptor '{range_request['desc']}': new range must include current range = [0,20]") self.test_importdesc({**range_request, "range": [5, 20]}, wallet=wpriv, success=False, - error_code=-8, error_message='new range must include current range = [0,20]') + error_code=-4, error_message=f"Could not add descriptor '{range_request['desc']}': new range must include current range = [0,20]") assert_equal(wpriv.getwalletinfo()['keypoolsize'], 21) self.log.info("Check we can change descriptor internal flag")