Merge bitcoin/bitcoin#32845: rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs

achow101 · achow101 · commit b08041cac863 · 2025-07-25T12:46:13.000-07:00
c5c1960 doc: Add release notes for changes in RPCs (pablomartin4btc) 90fd5ac rpc, test: Fix error message in getdescriptoractivity (pablomartin4btc) 39fef1d test: Add missing logging info for each test (pablomartin4btc) 53ac704 rpc, test: Fix error message in unloadwallet (pablomartin4btc) 1fc3a8e rpc, test: Add EnsureUniqueWalletName tests (pablomartin4btc) b635bc0 rpc, util: Add EnsureUniqueWalletName (pablomartin4btc) Pull request description: Currently, `unloadwallet` RPC call fails with a JSON parsing error when no `wallet_name` argument is provided. This behavior is misleading because the error originates from a low-level JSON type mismatch, rather than clearly indicating that the wallet name or RPC endpoint (`-rpcwallet=...`) is missing. Also, found out that the [issue](bitcoin/bitcoin#13111 (comment)) was noticed during its implementation but never addressed. In addition, I've verified all RPC commands calls finding that `getdescriptoractivity` had the same problem, but related to the array input types (blockhashes & descriptors), so I've corrected that RPC as well. For consistency I've added the missing logging info for each test case in `test/functional/rpc_getdescriptoractivity.py` in preparation for the new test. **_-Before_** ``` ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet error code: -3 error message: JSON value of type number is not of expected type string ``` ``` ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity error code: -3 error message: JSON value of type null is not of expected type array ``` ``` ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]' error code: -3 error message: JSON value of type null is not of expected type array ``` **_-After_** ``` ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet error code: -8 error message: Either the RPC endpoint wallet or the wallet name parameter must be provided ``` ``` ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity error code: -1 error message: getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool ) Get spend and receive activity associated with a set of descriptors for a set of blocks. This command pairs well with the `relevant_blocks` output of `scanblocks()`. This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0) Arguments: 1. blockhashes (json array, required) The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown. [ "blockhash", (string) A valid blockhash ... ] 2. scanobjects (json array, required) Array of scan objects. Every scan object is either a string descriptor or an object: [ "descriptor", (string) An output descriptor { (json object) An object with output descriptor and metadata "desc": "str", (string, required) An output descriptor "range": n or [n,n], (numeric or array, optional, default=1000) The range of HD chain indexes to explore (either end or [begin,end]) }, ... ] 3. include_mempool (boolean, optional, default=true) Whether to include unconfirmed activity ... ``` ``` ./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]' error code: -1 error message: getdescriptoractivity ["blockhash",...] [scanobjects,...] ( include_mempool ) ... ``` ACKs for top commit: achow101: ACK c5c1960 stickies-v: re-ACK c5c1960 furszy: ACK c5c1960 Tree-SHA512: e831ff1acbfd15d2ce3a69bb408cce94664c0b63b2aa2f4627a05c6c052241ae3b5cc238219ef1b30afb489a4a3f4c3030e2168b0c8f08b4d20805d050d810f5
diff --git a/doc/release-notes-32845.md b/doc/release-notes-32845.md
@@ -0,0 +1,9 @@
+Updated RPCs
+------------
+
+- `unloadwallet` - Return RPC_INVALID_PARAMETER when both the RPC wallet endpoint
+and wallet_name parameters are unspecified. Previously the RPC failed with a JSON
+parsing error.
+- `getdescriptoractivity` - Mark blockhashes and scanobjects arguments as required,
+so the user receives a clear help message when either is missing. As in `unloadwallet`,
+previously the RPC failed with a JSON parsing error.
diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
@@ -2194,16 +2194,20 @@ static const auto scan_action_arg_desc = RPCArg{
         "\"status\" for progress report (in %) of the current scan"
 };
 
+static const auto output_descriptor_obj = RPCArg{
+    "", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "An object with output descriptor and metadata",
+    {
+        {"desc", RPCArg::Type::STR, RPCArg::Optional::NO, "An output descriptor"},
+        {"range", RPCArg::Type::RANGE, RPCArg::Default{1000}, "The range of HD chain indexes to explore (either end or [begin,end])"},
+    }
+};
+
 static const auto scan_objects_arg_desc = RPCArg{
     "scanobjects", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "Array of scan objects. Required for \"start\" action\n"
         "Every scan object is either a string descriptor or an object:",
     {
         {"descriptor", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "An output descriptor"},
-        {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "An object with output descriptor and metadata",
-            {
-                {"desc", RPCArg::Type::STR, RPCArg::Optional::NO, "An output descriptor"},
-                {"range", RPCArg::Type::RANGE, RPCArg::Default{1000}, "The range of HD chain indexes to explore (either end or [begin,end])"},
-            }},
+        output_descriptor_obj,
     },
     RPCArgOptions{.oneline_description="[scanobjects,...]"},
 };
@@ -2633,10 +2637,16 @@ static RPCHelpMan getdescriptoractivity()
         "This command pairs well with the `relevant_blocks` output of `scanblocks()`.\n"
         "This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
         {
-            RPCArg{"blockhashes", RPCArg::Type::ARR, RPCArg::Optional::OMITTED, "The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown.\n", {
+            RPCArg{"blockhashes", RPCArg::Type::ARR, RPCArg::Optional::NO, "The list of blockhashes to examine for activity. Order doesn't matter. Must be along main chain or an error is thrown.\n", {
                 {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A valid blockhash"},
             }},
-            scan_objects_arg_desc,
+            RPCArg{"scanobjects", RPCArg::Type::ARR, RPCArg::Optional::NO, "The list of descriptors (scan objects) to examine for activity. Every scan object is either a string descriptor or an object:",
+                {
+                    {"descriptor", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "An output descriptor"},
+                    output_descriptor_obj,
+                },
+                RPCArgOptions{.oneline_description="[scanobjects,...]"},
+            },
             {"include_mempool", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether to include unconfirmed activity"},
         },
         RPCResult{
diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp
@@ -29,6 +29,27 @@ bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param) {
     return avoid_reuse;
 }
 
+std::string EnsureUniqueWalletName(const JSONRPCRequest& request, const std::string* wallet_name)
+{
+    std::string endpoint_wallet;
+    if (GetWalletNameFromJSONRPCRequest(request, endpoint_wallet)) {
+        // wallet endpoint was used
+        if (wallet_name && *wallet_name != endpoint_wallet) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER,
+                "The RPC endpoint wallet and the wallet name parameter specify different wallets");
+        }
+        return endpoint_wallet;
+    }
+
+    // Not a wallet endpoint; parameter must be provided
+    if (!wallet_name) {
+        throw JSONRPCError(RPC_INVALID_PARAMETER,
+            "Either the RPC endpoint wallet or the wallet name parameter must be provided");
+    }
+
+    return *wallet_name;
+}
+
 bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
 {
     if (request.URI.starts_with(WALLET_ENDPOINT_BASE)) {
diff --git a/src/wallet/rpc/util.h b/src/wallet/rpc/util.h
@@ -38,6 +38,11 @@ static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "last
  */
 std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
 bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name);
+/**
+ * Ensures that a wallet name is specified across the endpoint and wallet_name.
+ * Throws `RPC_INVALID_PARAMETER` if none or different wallet names are specified.
+ */
+std::string EnsureUniqueWalletName(const JSONRPCRequest& request, const std::string* wallet_name);
 
 void EnsureWalletIsUnlocked(const CWallet&);
 WalletContext& EnsureWalletContext(const std::any& context);
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
@@ -438,8 +438,8 @@ static RPCHelpMan createwallet()
 static RPCHelpMan unloadwallet()
 {
     return RPCHelpMan{"unloadwallet",
-                "Unloads the wallet referenced by the request endpoint, otherwise unloads the wallet specified in the argument.\n"
-                "Specifying the wallet name on a wallet endpoint is invalid.",
+                "Unloads the wallet referenced by the request endpoint or the wallet_name argument.\n"
+                "If both are specified, they must be identical.",
                 {
                     {"wallet_name", RPCArg::Type::STR, RPCArg::DefaultHint{"the wallet name from the RPC endpoint"}, "The name of the wallet to unload. If provided both here and in the RPC endpoint, the two must be identical."},
                     {"load_on_startup", RPCArg::Type::BOOL, RPCArg::Optional::OMITTED, "Save wallet name to persistent settings and load on startup. True to add wallet to startup list, false to remove, null to leave unchanged."},
@@ -456,14 +456,7 @@ static RPCHelpMan unloadwallet()
                 },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    std::string wallet_name;
-    if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
-        if (!(request.params[0].isNull() || request.params[0].get_str() == wallet_name)) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets");
-        }
-    } else {
-        wallet_name = request.params[0].get_str();
-    }
+    const std::string wallet_name{EnsureUniqueWalletName(request, self.MaybeArg<std::string>("wallet_name"))};
 
     WalletContext& context = EnsureWalletContext(request.context);
     std::shared_ptr<CWallet> wallet = GetWallet(context, wallet_name);
@@ -685,17 +678,7 @@ static RPCHelpMan migratewallet()
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
         {
-            std::string wallet_name;
-            if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
-                if (!(request.params[0].isNull() || request.params[0].get_str() == wallet_name)) {
-                    throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets");
-                }
-            } else {
-                if (request.params[0].isNull()) {
-                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Either RPC endpoint wallet or wallet_name parameter must be provided");
-                }
-                wallet_name = request.params[0].get_str();
-            }
+            const std::string wallet_name{EnsureUniqueWalletName(request, self.MaybeArg<std::string>("wallet_name"))};
 
             SecureString wallet_pass;
             wallet_pass.reserve(100);
diff --git a/src/wallet/test/CMakeLists.txt b/src/wallet/test/CMakeLists.txt
@@ -19,6 +19,7 @@ target_sources(test_bitcoin
     scriptpubkeyman_tests.cpp
     spend_tests.cpp
     wallet_crypto_tests.cpp
+    wallet_rpc_tests.cpp
     wallet_tests.cpp
     wallet_transaction_tests.cpp
     walletdb_tests.cpp
diff --git a/src/wallet/test/wallet_rpc_tests.cpp b/src/wallet/test/wallet_rpc_tests.cpp
@@ -0,0 +1,41 @@
+// Copyright (c) 2025-present The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <rpc/request.h>
+#include <test/util/setup_common.h>
+#include <univalue.h>
+#include <wallet/rpc/util.h>
+
+#include <boost/test/unit_test.hpp>
+
+#include <optional>
+#include <string>
+
+namespace wallet {
+static std::string TestWalletName(const std::string& endpoint, std::optional<std::string> parameter = std::nullopt)
+{
+    JSONRPCRequest req;
+    req.URI = endpoint;
+    return EnsureUniqueWalletName(req, parameter ? &*parameter : nullptr);
+}
+
+BOOST_FIXTURE_TEST_SUITE(wallet_rpc_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(ensure_unique_wallet_name)
+{
+    // EnsureUniqueWalletName should only return if exactly one unique wallet name is provided
+    BOOST_CHECK_EQUAL(TestWalletName("/wallet/foo"), "foo");
+    BOOST_CHECK_EQUAL(TestWalletName("/wallet/foo", "foo"), "foo");
+    BOOST_CHECK_EQUAL(TestWalletName("/", "foo"), "foo");
+    BOOST_CHECK_EQUAL(TestWalletName("/bar", "foo"), "foo");
+
+    BOOST_CHECK_THROW(TestWalletName("/"), UniValue);
+    BOOST_CHECK_THROW(TestWalletName("/foo"), UniValue);
+    BOOST_CHECK_THROW(TestWalletName("/wallet/foo", "bar"), UniValue);
+    BOOST_CHECK_THROW(TestWalletName("/wallet/foo", "foobar"), UniValue);
+    BOOST_CHECK_THROW(TestWalletName("/wallet/foobar", "foo"), UniValue);
+}
+
+BOOST_AUTO_TEST_SUITE_END()
+} // namespace wallet
diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py
@@ -31,13 +31,16 @@ def run_test(self):
         self.test_confirmed_and_unconfirmed(node, wallet)
         self.test_receive_then_spend(node, wallet)
         self.test_no_address(node, wallet)
+        self.test_required_args(node)
 
     def test_no_activity(self, node):
+        self.log.info("Test that no activity is found for an unused address")
         _, _, addr_1 = getnewdestination()
         result = node.getdescriptoractivity([], [f"addr({addr_1})"], True)
         assert_equal(len(result['activity']), 0)
 
     def test_activity_in_block(self, node, wallet):
+        self.log.info("Test that receive activity is correctly reported in a mined block")
         _, spk_1, addr_1 = getnewdestination(address_type='bech32m')
         txid = wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)['txid']
         blockhash = self.generate(node, 1)[0]
@@ -67,6 +70,7 @@ def test_activity_in_block(self, node, wallet):
 
 
     def test_no_mempool_inclusion(self, node, wallet):
+        self.log.info("Test that mempool transactions are not included when include_mempool argument is False")
         _, spk_1, addr_1 = getnewdestination()
         wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)
 
@@ -81,6 +85,7 @@ def test_no_mempool_inclusion(self, node, wallet):
         assert_equal(len(result['activity']), 0)
 
     def test_multiple_addresses(self, node, wallet):
+        self.log.info("Test querying multiple addresses returns all activity correctly")
         _, spk_1, addr_1 = getnewdestination()
         _, spk_2, addr_2 = getnewdestination()
         wallet.send_to(from_node=node, scriptPubKey=spk_1, amount=1 * COIN)
@@ -113,6 +118,7 @@ def test_multiple_addresses(self, node, wallet):
         assert a2['amount'] == 2.0
 
     def test_invalid_blockhash(self, node, wallet):
+        self.log.info("Test that passing an invalid blockhash raises appropriate RPC error")
         self.generate(node, 20) # Generate to get more fees
 
         _, spk_1, addr_1 = getnewdestination()
@@ -125,6 +131,7 @@ def test_invalid_blockhash(self, node, wallet):
             node.getdescriptoractivity, [invalid_blockhash], [f"addr({addr_1})"], True)
 
     def test_invalid_descriptor(self, node, wallet):
+        self.log.info("Test that an invalid descriptor raises the correct RPC error")
         blockhash = self.generate(node, 1)[0]
         _, _, addr_1 = getnewdestination()
 
@@ -133,6 +140,7 @@ def test_invalid_descriptor(self, node, wallet):
             node.getdescriptoractivity, [blockhash], [f"addrx({addr_1})"], True)
 
     def test_confirmed_and_unconfirmed(self, node, wallet):
+        self.log.info("Test that both confirmed and unconfirmed transactions are reported correctly")
         self.generate(node, 20) # Generate to get more fees
 
         _, spk_1, addr_1 = getnewdestination()
@@ -162,6 +170,7 @@ def test_confirmed_and_unconfirmed(self, node, wallet):
 
     def test_receive_then_spend(self, node, wallet):
         """Also important because this tests multiple blockhashes."""
+        self.log.info("Test receive and spend activities across different blocks are reported consistently")
         self.generate(node, 20) # Generate to get more fees
 
         sent1 = wallet.send_self_transfer(from_node=node)
@@ -189,11 +198,13 @@ def test_receive_then_spend(self, node, wallet):
         assert_equal(result, node.getdescriptoractivity(
             [blockhash_1, blockhash_2], [wallet.get_descriptor()], True))
 
+        self.log.info("Test that duplicated blockhash request does not report duplicated results")
         # Test that duplicating a blockhash yields the same result.
         assert_equal(result, node.getdescriptoractivity(
             [blockhash_1, blockhash_2, blockhash_2], [wallet.get_descriptor()], True))
 
     def test_no_address(self, node, wallet):
+        self.log.info("Test that activity is still reported for scripts without an associated address")
         raw_wallet = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK)
         self.generate(raw_wallet, 100)
 
@@ -221,6 +232,11 @@ def test_no_address(self, node, wallet):
         assert_equal(list(a2['output_spk'].keys()), ['asm', 'desc', 'hex', 'type'])
         assert a2['amount'] == Decimal(no_addr_tx["tx"].vout[0].nValue) / COIN
 
+    def test_required_args(self, node):
+        self.log.info("Test that required arguments must be passed")
+        assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity)
+        assert_raises_rpc_error(-1, "getdescriptoractivity", node.getdescriptoractivity, [])
+
 
 if __name__ == '__main__':
     GetBlocksActivityTest(__file__).main()
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
@@ -524,8 +524,8 @@ def test_encrypted(self):
     def test_nonexistent(self):
         self.log.info("Check migratewallet errors for nonexistent wallets")
         default = self.master_node.get_wallet_rpc(self.default_wallet_name)
-        assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", default.migratewallet, "someotherwallet")
-        assert_raises_rpc_error(-8, "Either RPC endpoint wallet or wallet_name parameter must be provided", self.master_node.migratewallet)
+        assert_raises_rpc_error(-8, "The RPC endpoint wallet and the wallet name parameter specify different wallets", default.migratewallet, "someotherwallet")
+        assert_raises_rpc_error(-8, "Either the RPC endpoint wallet or the wallet name parameter must be provided", self.master_node.migratewallet)
         assert_raises_rpc_error(-4, "Error: Wallet does not exist", self.master_node.migratewallet, "notawallet")
 
     def test_unloaded_by_path(self):
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
@@ -343,10 +343,10 @@ def wallet_file(name):
         self.log.info("Test dynamic wallet unloading")
 
         # Test `unloadwallet` errors
-        assert_raises_rpc_error(-3, "JSON value of type null is not of expected type string", self.nodes[0].unloadwallet)
+        assert_raises_rpc_error(-8, "Either the RPC endpoint wallet or the wallet name parameter must be provided", self.nodes[0].unloadwallet)
         assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", self.nodes[0].unloadwallet, "dummy")
         assert_raises_rpc_error(-18, "Requested wallet does not exist or is not loaded", node.get_wallet_rpc("dummy").unloadwallet)
-        assert_raises_rpc_error(-8, "RPC endpoint wallet and wallet_name parameter specify different wallets", w1.unloadwallet, "w2"),
+        assert_raises_rpc_error(-8, "The RPC endpoint wallet and the wallet name parameter specify different wallets", w1.unloadwallet, "w2"),
 
         # Successfully unload the specified wallet name
         self.nodes[0].unloadwallet("w1")
