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. diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 8ee74fb0d0b..3588446b198 100644 --- 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 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); 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/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 diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py index a0dc43718bc..510d7692ed7 100755 --- a/test/functional/rpc_getdescriptoractivity.py +++ b/test/functional/rpc_getdescriptoractivity.py @@ -31,13 +31,16 @@ 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") _, _, 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 @@ 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 +85,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 +118,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 +131,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 +140,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 +170,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 +198,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) @@ -221,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() diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index a1b319b90bb..be8f6ea873b 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -524,8 +524,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")