wallet: Use util::Error throughout AddWalletDescriptor

achow101 · achow101 · commit 785e1407b0a3 · 2025-05-19T18:09:56.000-07:00
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&lt;DescriptorScriptPubKeyMan&gt;. 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.
diff --git 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<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 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
@@ -215,8 +215,7 @@ 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);
-    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
@@ -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
@@ -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
@@ -80,12 +80,9 @@ static std::optional<std::pair<WalletDescriptor, FlatSigningProvider>> 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<DescriptorScriptPubKeyMan*>(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
@@ -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
@@ -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<DescriptorScriptPubKeyMan*>(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<DescriptorScriptPubKeyMan*>(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
@@ -35,8 +35,7 @@ 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);
-        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<MockableDatabase&>(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
@@ -116,7 +116,7 @@ class MockableDatabase : public WalletDatabase
 std::unique_ptr<WalletDatabase> 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
@@ -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
@@ -3713,13 +3713,12 @@ std::optional<bool> CWallet::IsInternalScriptPubKeyMan(ScriptPubKeyMan* spk_man)
     return GetScriptPubKeyMan(*type, /* internal= */ true) == desc_spk_man;
 }
 
-util::Result<ScriptPubKeyMan*> CWallet::AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal)
+util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> 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,17 +3744,15 @@ util::Result<ScriptPubKeyMan*> 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
     // Note: we disable labels for ranged descriptors
     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<ScriptPubKeyMan*> 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
@@ -1020,7 +1020,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
-    util::Result<ScriptPubKeyMan*> AddWalletDescriptor(WalletDescriptor& desc, const FlatSigningProvider& signing_provider, const std::string& label, bool internal) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
+    util::Result<std::reference_wrapper<DescriptorScriptPubKeyMan>> 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
@@ -284,11 +284,11 @@ def run_test(self):
         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")

Original file line number	Diff line number	Diff line change
`@@ -50,8 +50,7 @@ static void WalletIsMine(benchmark::Bench& bench, int num_combo = 0)`
`50`	`50`	`std::string error;`
`51`	`51`	`std::vector<std::unique_ptr<Descriptor>> desc = Parse("combo(" + EncodeSecret(key) + ")", keys, error, /require_checksum=/false);`
`52`	`52`	`WalletDescriptor w_desc(std::move(desc.at(0)), /creation_time=/0, /range_start=/0, /range_end=/0, /next_index=/0);`
`53`		`- auto spk_manager = Assert(wallet->AddWalletDescriptor(w_desc, keys, /label=/"", /internal=*/false));`
`54`		`- assert(spk_manager);`
	`53`	`+ Assert(wallet->AddWalletDescriptor(w_desc, keys, /label=/"", /internal=/false));`
`55`	`54`	`}`
`56`	`55`	`}`
`57`	`56`
Original file line number	Diff line number	Diff line change
`@@ -59,9 +59,8 @@ 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 = Assert(wallet->AddWalletDescriptor(w_desc, keys, /label=*/"", internal));`
`63`		`- assert(spk_manager);`
`64`		`- wallet->AddActiveScriptPubKeyMan(spk_manager->GetID(), *Assert(w_desc.descriptor->GetOutputType()), internal);`
	`62`	`+ auto& spk_manager = Assert(wallet->AddWalletDescriptor(w_desc, keys, /label=/"", internal))->get();`
	`63`	`+ wallet->AddActiveScriptPubKeyMan(spk_manager.GetID(), *Assert(w_desc.descriptor->GetOutputType()), internal);`
`65`	`64`	`}`
`66`	`65`	`}`
`67`	`66`	`}`
Original file line number	Diff line number	Diff line change
`@@ -262,25 +262,21 @@ static UniValue ProcessDescriptorImport(CWallet& wallet, const UniValue& data, c`
`262`	`262`	`auto spk_manager_res = wallet.AddWalletDescriptor(w_desc, keys, label, desc_internal);`
`263`	`263`
`264`	`264`	`if (!spk_manager_res) {`
`265`		`- throw JSONRPCError(RPC_INVALID_PARAMETER, util::ErrorString(spk_manager_res).original);`
	`265`	`+ throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Could not add descriptor '%s': %s", descriptor, util::ErrorString(spk_manager_res).original));`
`266`	`266`	`}`
`267`	`267`
`268`		`- auto spk_manager = spk_manager_res.value();`
`269`		`-`
`270`		`- if (spk_manager == nullptr) {`
`271`		`- throw JSONRPCError(RPC_WALLET_ERROR, strprintf("Could not add descriptor '%s'", descriptor));`
`272`		`- }`
	`268`	`+ auto& spk_manager = spk_manager_res.value().get();`
`273`	`269`
`274`	`270`	`// Set descriptor as active if necessary`
`275`	`271`	`if (active) {`
`276`	`272`	`if (!w_desc.descriptor->GetOutputType()) {`
`277`	`273`	`warnings.push_back("Unknown output type, cannot set descriptor to active.");`
`278`	`274`	`} else {`
`279`		`- wallet.AddActiveScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), desc_internal);`
	`275`	`+ wallet.AddActiveScriptPubKeyMan(spk_manager.GetID(), *w_desc.descriptor->GetOutputType(), desc_internal);`
`280`	`276`	`}`
`281`	`277`	`} else {`
`282`	`278`	`if (w_desc.descriptor->GetOutputType()) {`
`283`		`- wallet.DeactivateScriptPubKeyMan(spk_manager->GetID(), *w_desc.descriptor->GetOutputType(), desc_internal);`
	`279`	`+ wallet.DeactivateScriptPubKeyMan(spk_manager.GetID(), *w_desc.descriptor->GetOutputType(), desc_internal);`
`284`	`280`	`}`
`285`	`281`	`}`
`286`	`282`	`}`
Original file line number	Diff line number	Diff line change
`@@ -27,8 +27,7 @@ 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`		`- auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));`
`31`		`- assert(spk_manager);`
	`30`	`+ Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));`
`32`	`31`	`}`
`33`	`32`
`34`	`33`	`BOOST_AUTO_TEST_CASE(psbt_updater_test)`
Original file line number	Diff line number	Diff line change
`@@ -66,8 +66,7 @@ static void AddKey(CWallet& wallet, const CKey& key)`
`66`	`66`	`assert(descs.size() == 1);`
`67`	`67`	`auto& desc = descs.at(0);`
`68`	`68`	`WalletDescriptor w_desc(std::move(desc), 0, 0, 1, 1);`
`69`		`- auto spk_manager = *Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));`
`70`		`- assert(spk_manager);`
	`69`	`+ Assert(wallet.AddWalletDescriptor(w_desc, provider, "", false));`
`71`	`70`	`}`
`72`	`71`
`73`	`72`	`BOOST_FIXTURE_TEST_CASE(scan_for_wallet_transactions, TestChain100Setup)`