From b635bc0896294af5afa1b18a35f307dfae441bb8 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Mon, 7 Jul 2025 13:39:25 -0300 Subject: [PATCH 1/6] rpc, util: Add EnsureUniqueWalletName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a new function called EnsureUniqueWalletNamet that returns the selected wallet name across the RPC request endpoint and wallet_name. Supports the case where the wallet_name argument may be omitted—either when using a wallet endpoint, or when not provided at all. In the latter case, if no wallet endpoint is used, an error is raised. Internally reuses the existing implementation to avoid redundant URL decoding and logic duplication. This is a preparatory change for upcoming refactoring of unloadwallet and migratewallet, which will adopt EnsureUniqueWalletName for improved clarity and consistency. --- src/wallet/rpc/util.cpp | 21 +++++++++++++++++++++ src/wallet/rpc/util.h | 5 +++++ 2 files changed, 26 insertions(+) diff --git a/src/wallet/rpc/util.cpp b/src/wallet/rpc/util.cpp index eaca74b6e65..79703985354 100644 --- 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 index a73fb177b3e..0c586be4a49 100644 --- 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 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); From 1fc3a8e8e7ae4698ac5cd5292a7e7e37097d37ce Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Tue, 22 Jul 2025 12:56:17 -0300 Subject: [PATCH 2/6] rpc, test: Add EnsureUniqueWalletName tests Co-authored-by: stickies-v --- src/wallet/test/CMakeLists.txt | 1 + src/wallet/test/wallet_rpc_tests.cpp | 41 ++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+) create mode 100644 src/wallet/test/wallet_rpc_tests.cpp diff --git a/src/wallet/test/CMakeLists.txt b/src/wallet/test/CMakeLists.txt index 8564eface55..524c7218f4e 100644 --- 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 new file mode 100644 index 00000000000..fb33d3668da --- /dev/null +++ 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 +#include +#include +#include + +#include + +#include +#include + +namespace wallet { +static std::string TestWalletName(const std::string& endpoint, std::optional 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 From 53ac704efd668f7d4ad74158e628023c9a34141f Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Mon, 7 Jul 2025 23:54:02 -0300 Subject: [PATCH 3/6] rpc, test: Fix error message in unloadwallet The unloadwallet RPC previously failed with a low-level JSON parsing error when called without any arguments (wallet_name). Although this issue was first identified during review of the original unloadwallet implementation in #13111, it was never addressed. Raise RPC_INVALID_PARAMETER instead describing that either the RPC endpoint or wallet name must be provided. Adding a new functional test for this use case. Refactor migratewallet to use the same logic as the wallet_name argument handling is identical. Co-authored-by: maflcko --- src/wallet/rpc/wallet.cpp | 25 ++++--------------------- test/functional/wallet_migration.py | 4 ++-- test/functional/wallet_multiwallet.py | 4 ++-- 3 files changed, 8 insertions(+), 25 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 1c417695788..26033d21191 100644 --- 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("wallet_name"))}; WalletContext& context = EnsureWalletContext(request.context); std::shared_ptr 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("wallet_name"))}; SecureString wallet_pass; wallet_pass.reserve(100); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index d10a8757591..04bb219f736 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -518,8 +518,8 @@ class WalletMigrationTest(BitcoinTestFramework): 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 index a003f97e3e4..25e66c95e70 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -343,10 +343,10 @@ class MultiWalletTest(BitcoinTestFramework): 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") From 39fef1d203678291020aa1adb2e420a117f86169 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Mon, 30 Jun 2025 17:41:50 -0300 Subject: [PATCH 4/6] test: Add missing logging info for each test Add self.log.info(...) calls at the beginning of each test in GetBlocksActivityTest. This improves readability and provides debugging information by logging the purpose of each test upon its correct execution. This is in preparation for the next commit, which adds a new test with log info, and it would look inconsistent without this commit. --- test/functional/rpc_getdescriptoractivity.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py index a0dc43718bc..94c871c58cd 100755 --- a/test/functional/rpc_getdescriptoractivity.py +++ b/test/functional/rpc_getdescriptoractivity.py @@ -33,11 +33,13 @@ class GetBlocksActivityTest(BitcoinTestFramework): self.test_no_address(node, wallet) 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 +69,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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 +84,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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 +117,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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 +130,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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 +139,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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 +169,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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 +197,13 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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) From 90fd5acbe57edb219a5dcbdc1095e11ae5398da5 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Tue, 8 Jul 2025 01:14:50 -0300 Subject: [PATCH 5/6] rpc, test: Fix error message in getdescriptoractivity Mark blockhashes and scanobjects arguments as required, so the user receives a clear help message when either is missing. Added a new functional test for this use case. Co-authored-by: stickies-v --- src/rpc/blockchain.cpp | 24 ++++++++++++++------ test/functional/rpc_getdescriptoractivity.py | 6 +++++ 2 files changed, 23 insertions(+), 7 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index ca872cf6ac8..9fcc685ae03 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -2193,16 +2193,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,...]"}, }; @@ -2632,10 +2636,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/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py index 94c871c58cd..510d7692ed7 100755 --- a/test/functional/rpc_getdescriptoractivity.py +++ b/test/functional/rpc_getdescriptoractivity.py @@ -31,6 +31,7 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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") @@ -231,6 +232,11 @@ class GetBlocksActivityTest(BitcoinTestFramework): 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() From c5c1960f9350d6315cadbdc95fface5f85f25806 Mon Sep 17 00:00:00 2001 From: pablomartin4btc Date: Tue, 22 Jul 2025 13:43:12 -0300 Subject: [PATCH 6/6] doc: Add release notes for changes in RPCs Adding notes for both `unloadwallet` and `getdescriptoractivity`. --- doc/release-notes-32845.md | 9 +++++++++ 1 file changed, 9 insertions(+) create mode 100644 doc/release-notes-32845.md diff --git a/doc/release-notes-32845.md b/doc/release-notes-32845.md new file mode 100644 index 00000000000..4730f29dea8 --- /dev/null +++ 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.