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 1b0253cfef5..3ec4285a3f9 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -264,25 +264,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 6a1c51290b3..247c0f2de4a 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 63153fb55db..f7f7fcdbf0e 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 535f74f24fe..966c6d2c4ba 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(update_non_range_descriptor, TestingSetup) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 0abd032c537..4a4aa837eaa 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3725,13 +3725,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); @@ -3757,8 +3756,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 @@ -3766,8 +3764,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) { @@ -3783,7 +3780,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 01033b7f19d..fbc3bed2ab6 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -1018,7 +1018,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 142243cf32b..e367e59a193 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")