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

pablomartin4btc commented at 11:51 PM on June 30, 2025: member

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 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 )

...

DrahtBot commented at 11:52 PM on June 30, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32845.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, furszy, achow101
Stale ACK	maflcko, BrandonOdiwuor

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

fanquake renamed this:
~~rpc, test: Fix JSON parsing errors in RPC calls (unloadwallet and getdescriptoractivity) raising RPC_INVALID_PARAMETER with appropriate description~~
rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs
on Jul 1, 2025

in src/rpc/blockchain.cpp:2672 in 488a01b002 outdated

2668 | @@ -2669,6 +2669,11 @@ static RPCHelpMan getdescriptoractivity()
2669 |          },
2670 |          [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2671 |  {
2672 | +    if (request.params[0].isNull() || request.params[1].isNull()) {

stickies-v commented at 10:33 AM on July 1, 2025:

This should be && instead of ||. And the reason that the functional test didn't fail with this change is an indication that the .isNull() check is not really reliable either, because it will happily accept an empty array.

Do we actually need to ensure that at least one such (valid) parameter is provided? If so, we should probably check that blockindexes_sorted and scripts_to_watch are not both empty. Otherwise, I think just making sure we only execute the request.params[n].get_array().getValues() statements if we've first checked that !request.params[n].isNull() should do the trick?

pablomartin4btc commented at 3:00 PM on July 1, 2025:

This should be && instead of ||.

Yeah, my bad.

.isNull() check is not really reliable either, because it will happily accept an empty array.

At the moment you could do:

./build/bin/bitcoin-cli -signet -datadir=/tmp/btc getdescriptoractivity '[]' '[]'
{
  "activity": [
  ]
}

And that doesn't make sense either.

I think just making sure we only execute the request.params[n].get_array().getValues() statements if we've first checked that !request.params[n].isNull() should do the trick?

I agree, will do that. Thank you!

pablomartin4btc commented at 4:31 PM on July 1, 2025:

Done! Also added the empty array case to the test and updated the description in before & after sections with it.

stickies-v commented at 6:05 PM on July 9, 2025:

Do we actually need to ensure that at least one such (valid) parameter is provided? If so, we should probably check that blockindexes_sorted and scripts_to_watch are not both empty. Otherwise, I think just making sure we only execute the request.params[n].get_array().getValues() statements if we've first checked that !request.params[n].isNull() should do the trick?

I don't think my comment was addressed, and e.g. this test still failing with the below diff. According to the PR description and code changes it should pass since at least 1 blockhash or descriptor is passed.

diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py
index 6dadfa10f6..5dac541e01 100755
--- a/test/functional/rpc_getdescriptoractivity.py
+++ b/test/functional/rpc_getdescriptoractivity.py
@@ -237,6 +237,7 @@ class GetBlocksActivityTest(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "At least 1 blockhash or 1 descriptor must be specified.", node.getdescriptoractivity)
         self.log.info("Test that calling getdescriptoractivity with an empty array raises also RPC_INVALID_PARAMETER (-8)")
         assert_raises_rpc_error(-8, "At least 1 blockhash or 1 descriptor must be specified.", node.getdescriptoractivity, [])
+        node.getdescriptoractivity([self.generate(self.nodes[0], 1)[0]])
 
 
 if __name__ == '__main__':

</details>

The conceptual question raised in my earlier question was: do we just want to prevent .get_array() from throwing or do we actually need to ensure (valid or any) blockhashes or descriptors are passed. We'll need to answer this before we can decide on the best fix for this problem, I think. If the former, we might not need to raise any JSONRPCError. If the latter, we should probably inspect blockindexes_sorted and scripts_to_watch.

pablomartin4btc commented at 9:56 PM on July 9, 2025:

Thanks for the follow-up. The current fix is wrong. Looking at the original getdescriptoractivity implementation PR samples and tests, also checking the logic in the RPC, the second array, descriptors, has to be provided. Now, the first one, blockhashes, could be empty and activities may be returned in result if there are any in the mempool (3rd parameter include_mempool is true by default, but if the user sets it to false, the RPC is still usable -> no errors, just empty activity array will be returned).

As explained above, calling getdescriptoractivity with both blockhashes and descriptors arrays being empty doesn't make sense because it won't produce any results (but won't raise any errors), what's actually essential to be present in the call is the descriptors array, 2nd param, which will be used to filter the blocks and only if any is found will be added into the results.

So, I think, the error should say: "Both blockhashes and descriptors arrays must be specified", and the .empty() check should be removed for both. If we want to be more specific could check only for the emptiness of descriptors and will have to change the error message accordingly.

pablomartin4btc commented at 11:21 PM on July 9, 2025:

just want to prevent .get_array() from throwing

Yes, plus we need to validate both arrays... we could make blockhashes not optional but we can't do the same with descriptors (scan_objects_arg_desc) as it's used in different places.

stickies-v commented at 11:00 AM on July 10, 2025:

So, I think, the error should say: "Both blockhashes and descriptors arrays must be specified", and the .empty() check should be removed for both.

I think I agree with that. It seems that the RPC works fine (did not properly inspect the call graph/code) with empty blockhashes and descriptors (modulo not calling .get_array()):

% cli getdescriptoractivity '[]' '[]'
{
  "activity": [
  ]
}

I don't think we need to protect the user against providing empty arrays if it is safe (even if useless) and in line with documented behaviour to do so, so it seems like just setting the "blockhashes" and "descriptors" arguments to RPCArg::Optional::NO is the most straightforward and helpful approach imo:

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index c80c7f2308..c7f2591afa 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 = 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,
     },
     RPCArgOptions{.oneline_description="[scanobjects,...]"},
 };
@@ -2632,10 +2636,12 @@ 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{"descriptors", RPCArg::Type::ARR, RPCArg::Optional::NO, "The list of descriptors to examine for activity.\n", {
+                output_descriptor
+            }},
             {"include_mempool", RPCArg::Type::BOOL, RPCArg::Default{true}, "Whether to include unconfirmed activity"},
         },
         RPCResult{
@@ -2670,10 +2676,6 @@ static RPCHelpMan getdescriptoractivity()
         },
         [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    if (std::ranges::all_of(std::array{request.params[0], request.params[1]},
-                        [](const auto& p) {return p.isNull() || p.empty();})) {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, "At least 1 blockhash or 1 descriptor must be specified.");
-    }
     UniValue ret(UniValue::VOBJ);
     UniValue activity(UniValue::VARR);
     NodeContext& node = EnsureAnyNodeContext(request.context);

</details>

This yields a helpful response when parameters are missing:

% cli getdescriptoractivity '[]'
error code: -1
error message:
getdescriptoractivity ["blockhash",...] [{"desc":"str","range":n or [n,n]},...] ( 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)
...

Moreover, it removes the incorrect reference in the help documentation to a "start" action, which is not present in this RPC:

2. scanobjects                   (json array, optional) Array of scan objects. Required for "start" action

Note:

"descriptors" documentation could be improved, just quickly wrote something down. E.g. not sure if order matters here, or if there's anything else to look out for.
rawtransaction.cpp also has a bunch of "descriptors" parameters that could reuse the output_descriptor definition, but may be out of scope for this PR.

maflcko commented at 11:30 AM on July 10, 2025:

Yeah, thanks @stickies-v. Sorry for missing this. Obviously commit 529d2ce24570aae426ce19e87cb22f124dd09470 is not quite right. It should either be dropped, because there isn't really any problem, or the already existing RPCArg::Optional logic should be used.

stickies-v commented at 12:47 PM on July 10, 2025:

"descriptors" documentation could be improved, just quickly wrote something down. E.g. not sure if order matters here, or if there's anything else to look out for.

We should probably just leave the name as "scanobjects" (instead of my change to "descriptors"), to avoid introducing backwards incompatible API changes. The description should be change-able without issue.

pablomartin4btc commented at 2:28 AM on July 11, 2025:

Done! Thanks!

pablomartin4btc force-pushed on Jul 1, 2025

DrahtBot added the label CI failed on Jul 1, 2025

DrahtBot commented at 4:32 PM on July 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45149016067 LLM reason (✨ experimental): The CI failure is caused by a trailing whitespace error in the code, detected during the lint check.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

pablomartin4btc commented at 4:39 PM on July 1, 2025: member

<ins>Updates</ins>:

Addressed @stickies-v's feedback correcting the error condition for getdescriptoractivity, adding also the empty array case in its corresponding test and updating the PR description in the before & after sections accordingly.

furszy commented at 4:50 PM on July 1, 2025: member

I think this worth a general RPC util function like (haven't tested it):

template <typename... Value>
bool AreParamsNullOrEmpty(const Value&... params) {
    return ((params.isNull() || params.empty()) && ...);
}

Which, for example should work fine for getdescriptoractivity :

if (AreParamsNullOrEmpty(request.params[0], request.params[1])) {
    throw JSONRPCError(RPC_INVALID_PARAMETER, "At least 1 blockhash or 1 descriptor must be specified.");
}

in src/wallet/rpc/wallet.cpp:465 in 2e4a0f79a0 outdated

 461 | @@ -462,6 +462,9 @@ static RPCHelpMan unloadwallet()
 462 |              throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets");
 463 |          }
 464 |      } else {
 465 | +        if (request.params[0].isNull()) {

maflcko commented at 5:36 PM on July 1, 2025:

seems easier and less brittle to modify GetWalletNameFromJSONRPCRequest to:

const std::string wallet_name{GetWalletNameFromJSONRPCRequest(request, self.MaybeArg<std::string>("wallet_name")};

pablomartin4btc commented at 7:11 PM on July 1, 2025:

... it would make the code clearer... and we can handle the errors within that function instead... Thanks! Checking it...

pablomartin4btc commented at 8:22 PM on July 1, 2025:

Also, RPC migratewallet calls the same function so I could refactor that instance too.

pablomartin4btc commented at 4:50 AM on July 8, 2025:

Done! Added you as co-author on that commit.

DrahtBot removed the label CI failed on Jul 1, 2025

pablomartin4btc force-pushed on Jul 8, 2025

pablomartin4btc commented at 5:18 AM on July 8, 2025: member

<ins>Updates</ins>:

Addressed feedback from @maflcko (co-authored): extended MaybeArg to return a std::optional<std::string> for use in the GetWalletNameFromJSONRPCRequest overload. Also refactored the migratewallet handling of wallet_name to align with the existing behavior in unloadwallet.
Incorporated @furszy's suggestion (co-authored): added a new template in RPC utils to check whether all provided parameters are null or empty, now used in the getdescriptoractivity RPC.

DrahtBot added the label CI failed on Jul 8, 2025

DrahtBot commented at 6:47 AM on July 8, 2025: contributor

🚧 At least one of the CI tasks failed. Task tidy: https://github.com/bitcoin/bitcoin/runs/45527858084 LLM reason (✨ experimental): The CI failure is caused by a clang-tidy error in util.cpp due to an improperly qualified return type.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

in src/rpc/util.cpp:728 in 0104bbb92e outdated

 724 | @@ -725,6 +725,7 @@ TMPL_INST(nullptr, const UniValue*, maybe_arg;);
 725 |  TMPL_INST(nullptr, std::optional<double>, maybe_arg ? std::optional{maybe_arg->get_real()} : std::nullopt;);
 726 |  TMPL_INST(nullptr, std::optional<bool>, maybe_arg ? std::optional{maybe_arg->get_bool()} : std::nullopt;);
 727 |  TMPL_INST(nullptr, const std::string*, maybe_arg ? &maybe_arg->get_str() : nullptr;);
 728 | +TMPL_INST(nullptr, std::optional<std::string>, maybe_arg ? std::optional{maybe_arg->get_str()} : std::nullopt;);

maflcko commented at 8:29 AM on July 8, 2025:

why is this needed? A pointer can already hold the state of null, so i don't see why the translation to optional is needed

pablomartin4btc commented at 8:07 PM on July 8, 2025:

I do agree, just seemed more "idiomatic" to handle the wallet_name variable with the has_value(), but yeah, not very practical in the end. I've removed the MaybeArg extension commit making the wallet_name to hold a pointer in the GetWalletNameFromJSONRPCRequest overload.

stickies-v commented at 11:30 PM on July 15, 2025:

I think we can avoid using raw pointers and move to a more idiomatic interface by updating the arg helpers to use string_view instead of string, making it trivially copyable. I've implemented that in #32983. Shouldn't block progress - but if it gets merged first, it might be worth adopting that interface here too.

pablomartin4btc commented at 12:40 AM on July 16, 2025:

I thought to use <std::string_view> too when I did the TMPL_INST but didn't go forward. We can keep it this way if this gets merged first or if it does yours I can incorporate it here yeah. I'll try to review your PR tmw. Thanks!

in src/rpc/util.h:481 in 0104bbb92e outdated

 476 | @@ -477,8 +477,8 @@ class RPCHelpMan
 477 |      {
 478 |          auto i{GetParamIndex(key)};
 479 |          // Return optional argument (without default).
 480 | -        if constexpr (std::is_integral_v<R> || std::is_floating_point_v<R>) {
 481 | -            // Return numbers by value, wrapped in optional.

maflcko commented at 8:30 AM on July 8, 2025:

this just makes it confusing to special-case string here and doesn't seem ideal having to create a full copy of the possibly long string for no reason

pablomartin4btc commented at 8:07 PM on July 8, 2025:

As in previous comment, already removed the commit containing also this change. I've analysed the discussion around the string copy in the original implementation of MaybeArg and even if we could go around it and "fix"/ adding it for the string specific case/ condition (~return std::optional<std::string_view>{params[i].get_str_ref()}?) perhaps makes it too complicated for this simple case that can be resolved much easier with the pointer as you said.

in src/rpc/util.h:508 in 0104bbb92e outdated

 503 | +     *
 504 | +     * @param[in] params Variadic list of values to check.
 505 | +     * @return true if all parameters are null or empty; false otherwise.
 506 | +     */
 507 | +    template <typename... Value>
 508 | +    bool AreParamsNullOrEmpty(const Value&... params) const

maflcko commented at 8:33 AM on July 8, 2025:

why is this a member method? It doesn't use any member fields. Also, it seems a bit too trivial to create a helper for a method that is basically a one-line wrapper around std::ranges::all_of. Seems easier to just inline it in the rare cases where it is needed.

pablomartin4btc commented at 8:07 PM on July 8, 2025:

True, not really needed there, but resolved the getdescriptoractivity RPC issue using std::ranges::all_of) instead, as you suggested.

pablomartin4btc force-pushed on Jul 8, 2025

pablomartin4btc commented at 9:56 PM on July 8, 2025: member

<ins>Updates</ins>

Addressed @maflcko's feedback:
- dropped MaybeArg extension commit, since a pointer already can hold the null state needed in the validations within the GetWalletNameFromJSONRPCRequest overload;
- removed previously added AreParamsNullOrEmpty and replaced it with std::ranges::all_of in the getdescriptoractivity RPC fix instead, as suggested.

DrahtBot removed the label CI failed on Jul 8, 2025

furszy commented at 10:57 PM on July 8, 2025: member

utACK 529d2ce24570aae426ce19e87cb22f124dd09470

maflcko commented at 6:21 AM on July 9, 2025: member

review ACK 529d2ce24570aae426ce19e87cb22f124dd09470 🌦

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 529d2ce24570aae426ce19e87cb22f124dd09470 🌦
i0Xj78FXRdB9CQDK5n9I6JUcpJoQazD1SiiJh0xulnWWa6FTENw1vzhnN5u1kMpUxYbtq4Cw1hzp8s8XDz+LDQ==

</details>

in src/wallet/rpc/util.h:41 in 529d2ce245 outdated

  37 | @@ -38,6 +38,7 @@ static const RPCResult RESULT_LAST_PROCESSED_BLOCK { RPCResult::Type::OBJ, "last
  38 |   */
  39 |  std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request);
  40 |  bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name);
  41 | +std::string GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, const std::string* wallet_name);

stickies-v commented at 5:44 PM on July 9, 2025:

I think these overloads are very confusing, and as such at higher risk for introducing bugs. Overloading on std::string& wallet_name vs const std::string* wallet_name when one is an out-parameter returning the wallet endpoint name, and the other an in-parameter representing a query parameter is not great. One overload throwing, and the other not, is even worse.

I'm still considering what the better alternatives would be - it seems like doing it properly would probably add quite a bit of code changes, but I think that's something we need to consider before what is in my view adding tech debt for this particular fix.

pablomartin4btc commented at 6:48 PM on July 9, 2025:

I could use a better naming for this function, not being an overload. My idea was to fix the issue without touching much and later refactor both GetWalletNameFromJSONRPCRequest and GetWalletForJSONRPCRequest on a separate PR. Currently GetWalletForJSONRPCRequest, which is used broadly, calls internally GetWalletNameFromJSONRPCRequest and wanted to keep them untouched for now.

maflcko commented at 7:13 AM on July 10, 2025:

I think the function from a functionality perspective is fine, but symbols in the signature could be renamed for clarity.

stickies-v commented at 11:25 AM on July 10, 2025:

"adding tech debt" was probably too strong of a term, because yes just different function signature symbols would address my immediate concerns, even if it looks like this functionality can be streamlined in future PRs.

/**
 * Ensures exactly one wallet name is specified across the endpoint and wallet_arg. Throws 
 * `RPC_INVALID_PARAMETER` if no or multiple wallet names are specified.
 */
std::string EnsureUniqueWalletName(const JSONRPCRequest& request, const std::string* wallet_arg);

pablomartin4btc commented at 2:30 AM on July 11, 2025:

Applied a very tiny tweak to it. Thanks!

stickies-v commented at 6:07 PM on July 9, 2025: contributor

I think the proposed fix for getdescriptoractivity is not robust, and the GetWalletNameFromJSONRPCRequest() overload confusing, as detailed in comments.

pablomartin4btc force-pushed on Jul 11, 2025

DrahtBot added the label CI failed on Jul 11, 2025

DrahtBot commented at 2:33 AM on July 11, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45769239128 LLM reason (✨ experimental): The CI failure is caused by a failing lint check due to trailing whitespace in the test/functional/rpc_getdescriptoractivity.py file.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

pablomartin4btc force-pushed on Jul 11, 2025

pablomartin4btc commented at 3:04 AM on July 11, 2025: member

<ins>Updates</ins>

Addressed @stickies-v's feedback:
- converted the GetWalletNameFromJSONRPCRequest overload to EnsureUniqueWalletName as proposed - no changes in logic;
- corrected the getdescriptoractivity fix making both blockhashes and descriptors arrays as mandatory, returning the RPC Help for the command when any of them or both are missing. Updated the use cases in the PR description.

pablomartin4btc force-pushed on Jul 11, 2025

pablomartin4btc commented at 2:02 PM on July 11, 2025: member

Fixed functional test test_no_args_passed() to avoid CI failure (multiprocess, i686, DEBUG) adapting the code to when CLI option call is used (--usecli).

DrahtBot removed the label CI failed on Jul 11, 2025

in test/functional/rpc_getdescriptoractivity.py:245 in 01688e1753 outdated

 240 | +
 241 | +        self.log.info("Test that calling getdescriptoractivity with no arguments returns the RPC help for the command")
 242 | +        assert_raises_rpc_error(-1, rpc_help_output, node.getdescriptoractivity)
 243 | +
 244 | +        self.log.info("Test that calling getdescriptoractivity with one required array missing raises also its RPC help")
 245 | +        assert_raises_rpc_error(-1, rpc_help_output, node.getdescriptoractivity, [])

stickies-v commented at 10:45 AM on July 14, 2025:

It seems like our usual way to test this is to use the RPC name as the search text. I also don't think we need to be overly verbose in logging every single statement. Could simplify this test to:

    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, [])

diff --git a/test/functional/rpc_getdescriptoractivity.py b/test/functional/rpc_getdescriptoractivity.py
index d83f07e012..510d7692ed 100755
--- a/test/functional/rpc_getdescriptoractivity.py
+++ b/test/functional/rpc_getdescriptoractivity.py
@@ -31,7 +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_no_args_passed(node)
+        self.test_required_args(node)
 
     def test_no_activity(self, node):
         self.log.info("Test that no activity is found for an unused address")
@@ -232,18 +232,10 @@ 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_no_args_passed(self, node):
-        rpc_help_output = node.help('getdescriptoractivity')
-
-        if self.options.usecli:
-            rpc_help_output = rpc_help_output.splitlines()[0]
-
-        self.log.info("Test that calling getdescriptoractivity with no arguments returns the RPC help for the command")
-        assert_raises_rpc_error(-1, rpc_help_output, node.getdescriptoractivity)
-
-        self.log.info("Test that calling getdescriptoractivity with one required array missing raises also its RPC help")
-        assert_raises_rpc_error(-1, rpc_help_output, node.getdescriptoractivity, [])
-
+    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__':

</details>

pablomartin4btc commented at 5:22 PM on July 22, 2025:

Done!

in src/rpc/blockchain.cpp:2202 in 01688e1753 outdated

2205 | +    "descriptor", RPCArg::Type::STR, RPCArg::Optional::OMITTED, "An output descriptor"
2206 | +};
2207 | +
2208 | +static const auto scanobjects_oneline_description = RPCArgOptions{
2209 | +    .oneline_description="[scanobjects,...]"
2210 | +};

stickies-v commented at 10:51 AM on July 14, 2025:

I don't really see the benefit of replacing these brief and easy-to-read definitions with a variable that needs to be inspected out-of-line, especially since descriptions may need to be tailored to the specific RPC functionality. Would prefer keeping those 2 as-is?

Also nit: I find output_descriptor_obj a better name than obj_with_output_descriptor

pablomartin4btc commented at 12:35 AM on July 16, 2025:

Would prefer keeping those 2 as-is?

Ok, I did it that way just for reuse purpose, to avoid having a typo on the strings and so on.

obj_with_output_descriptor comes from the definition: "An object with output descriptor and metadata", but I'm ok with output_descriptor_obj.

pablomartin4btc commented at 5:22 PM on July 22, 2025:

Done!

in src/wallet/rpc/util.cpp:39 in 01688e1753 outdated

  34 | +    std::string endpoint_wallet;
  35 | +    if (GetWalletNameFromJSONRPCRequest(request, endpoint_wallet)) {
  36 | +        // wallet endpoint was used
  37 | +        if (wallet_name && *wallet_name != endpoint_wallet) {
  38 | +            throw JSONRPCError(RPC_INVALID_PARAMETER,
  39 | +                "RPC endpoint wallet and wallet_name parameter specify different wallets");

stickies-v commented at 7:41 PM on July 15, 2025:

nit: parameter names are defined per RPC, so assuming it's called "wallet_name" is not ideal, even if at the moment it seems that the assumption is correct for all RPCs (as far as I can see). I don't have a better solution yet, but I'll think about it more.

pablomartin4btc commented at 3:16 AM on July 16, 2025:

I haven't chosen wallet_name to match the RPC parameter. The function is meant to ensure consistency between two potential sources of a wallet name—the one in the endpoint and the one provided explicitly—regardless of where the second one comes from. So wallet_name here refers to the generic concept of a wallet name, not necessarily a parameter (RPC?) with that specific name.

That said, I see your point that parameter names vary between RPCs, and if a future RPC used this function with a differently named argument, it might cause some confusion at the call site. I’m open to renaming the variable (e.g., provided_wallet_name or similar) if that helps reduce ambiguity, though I think the function is still sufficiently generic in its current form.

stickies-v commented at 7:09 AM on July 16, 2025:

Oh no I'm not talking about the helper function's parameter name, that's fine imo, I'm talking about the error message that's thrown and eventually returned to the user, referencing an RPC parameter that potentially doesn't exist. Again, no biggie, just a symptom that concerns aren't entirely separated.

pablomartin4btc commented at 5:03 PM on July 16, 2025:

Ok, I see what you meant, the "wallet_name" parameter could be named differently from outside/ the RPC call. Is it possible to pass the request.param object itself? And get its name from it? I couldn't find any usage of it and also checked the UniValue lib... maybe I missed it, I'll double check but that would solve the potential issue that could happen as you describe as if a dev creates another RPC calling this function and not using "wallet_name" as an argument of the RPC call (some new RPC could receive 2 wallets, like "source_wallet" & "target_wallet", just to give an example).

pablomartin4btc commented at 5:22 PM on July 22, 2025:

I've re-tweaked the error descriptions to make them more generic and to don't depend on the name of the parameter specified in the RPC (otherwise we'd need to pass to the function the argument name defined in the RPC). Please let me know what you think.

BrandonOdiwuor commented at 8:47 AM on July 16, 2025: contributor

Tested ACK 01688e17534d3c9011cce92cff9f7935691bb80c

Was able to verify that this fixes the JSON parsing error on unloadwallet and getdescriptoractivity RPCs

Tested on macOS 15.5

Branch: Master <img width="870" height="203" alt="Screenshot 2025-07-16 at 11 41 41" src="https://github.com/user-attachments/assets/88508a93-3dfb-4676-9040-d8789677da1a" />

Commit: 01688e17534d3c9011cce92cff9f7935691bb80c <img width="1511" height="887" alt="Screenshot 2025-07-15 at 14 36 44" src="https://github.com/user-attachments/assets/72cfcc8f-29fa-4fce-85a1-bbc1c48c6cdd" /> <img width="1512" height="922" alt="Screenshot 2025-07-15 at 14 38 06" src="https://github.com/user-attachments/assets/dbba14d8-859e-468f-b874-76d463978188" />

DrahtBot requested review from furszy on Jul 16, 2025

DrahtBot requested review from maflcko on Jul 16, 2025

in src/wallet/rpc/wallet.cpp:459 in 01688e1753 outdated

 462 | -            throw JSONRPCError(RPC_INVALID_PARAMETER, "RPC endpoint wallet and wallet_name parameter specify different wallets");
 463 | -        }
 464 | -    } else {
 465 | -        wallet_name = request.params[0].get_str();
 466 | -    }
 467 | +    const std::string wallet_name{EnsureUniqueWalletName(request, self.MaybeArg<std::string>("wallet_name"))};

stickies-v commented at 10:42 AM on July 16, 2025:

side note: the docstring for this RPC seems out of date, could be sensible to fix in this PR? e.g.

diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index 67b7aafc00..26033d2119 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."},

</details>

pablomartin4btc commented at 5:18 PM on July 22, 2025:

Done!

in src/rpc/blockchain.cpp:2650 in 01688e1753 outdated

2643 | @@ -2632,10 +2644,13 @@ static RPCHelpMan getdescriptoractivity()
2644 |          "This command pairs well with the `relevant_blocks` output of `scanblocks()`.\n"
2645 |          "This call may take several minutes. If you encounter timeouts, try specifying no RPC timeout (bitcoin-cli -rpcclienttimeout=0)",
2646 |          {
2647 | -            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", {
2648 | +            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", {
2649 |                  {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, "A valid blockhash"},
2650 |              }},
2651 | -            scan_objects_arg_desc,
2652 | +            RPCArg{"scanobjects", RPCArg::Type::ARR, RPCArg::Optional::NO, "Array of scan objects. Every scan object is either a string descriptor or an object:", {

stickies-v commented at 10:54 AM on July 16, 2025:

nit: Unfortunately we have to keep the name "scanobjects" for backwards compatibility, but I think the description should be made more relevant to what this RPC is doing, in line with the blockhashes documentation, e.g.

            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:", {

pablomartin4btc commented at 5:18 PM on July 22, 2025:

Done!

in src/rpc/blockchain.cpp:1 in 01688e1753

stickies-v commented at 11:03 AM on July 16, 2025:

nit: I think the first paragraph of the 01688e17534d3c9011cce92cff9f7935691bb80c commit message can be improved to:

Mark blockhashes and scanobjects arguments as required, so the user receives a clear help message when either is missing.

pablomartin4btc commented at 5:18 PM on July 22, 2025:

Done!

in src/wallet/rpc/util.cpp:32 in 01688e1753 outdated

  28 | @@ -29,6 +29,27 @@ bool GetAvoidReuseFlag(const CWallet& wallet, const UniValue& param) {
  29 |      return avoid_reuse;
  30 |  }
  31 |  
  32 | +std::string EnsureUniqueWalletName(const JSONRPCRequest& request, const std::string* wallet_name)

stickies-v commented at 11:43 AM on July 16, 2025:

I wrote some unit tests for this function, feel free to add to this PR if you think it's helpful: https://github.com/stickies-v/bitcoin/commit/60f9dc8af66bbd4eda81049a10262527f83356a1

pablomartin4btc commented at 5:18 PM on July 16, 2025:

Thanks! Makes sense, I'll add them.

pablomartin4btc commented at 5:18 PM on July 22, 2025:

Added!

stickies-v approved

stickies-v commented at 11:45 AM on July 16, 2025: contributor

ACK 01688e17534d3c9011cce92cff9f7935691bb80c . Left a couple improvement suggestions that I think make sense to address here, but also aren't blocking. Happy to quickly re-review if you incorporate.

I think adding (very brief) release notes to indicate the slight behaviour change for unloadwallet and getdescriptoractivity could be useful for downstream projects.

DrahtBot requested review from stickies-v on Jul 16, 2025

rpc, util: Add EnsureUniqueWalletName

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.

b635bc0896

pablomartin4btc force-pushed on Jul 22, 2025

DrahtBot added the label CI failed on Jul 22, 2025

DrahtBot commented at 5:05 PM on July 22, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/46494187141 LLM reason (✨ experimental): The CI failure is caused by a trailing newline check failing in the lint step.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

pablomartin4btc force-pushed on Jul 22, 2025

pablomartin4btc commented at 7:08 PM on July 22, 2025: member

<ins>Updates</ins>:

Addressed feedback from @stickies-v:
- Updated getdescriptoractivity test for required params;
- Corrected variables naming within RPC getdescriptoractivity;
- Updated RPC invalid params error descriptions to be more generic and not depending on the RPC param naming representing the walle name;
- Updated outdated docstring for unloadwallet RPC;
- Updated scanobjects argument description within getdescriptoractivity RPC;
- Updated getdescriptoractivity commit message;
- Incorporated EnsureUniqueWalletName unit tests;
- Added release notes for unloadwallet & getdescriptoractivity updates.

in src/wallet/test/wallet_rpc_tests.cpp:15 in 7009befc4c outdated

  10 | +#include <boost/test/unit_test.hpp>
  11 | +
  12 | +#include <optional>
  13 | +#include <string>
  14 | +
  15 | +namespace wallet {

stickies-v commented at 9:21 PM on July 22, 2025:

nit: what's the purpose of wrapping this in namespace wallet?

Relatedly, should probably make TestWalletName static.

pablomartin4btc commented at 2:11 AM on July 23, 2025:

Thanks for pointed that out. Forgot to remove wallet:: in the EnsureUniqueWalletName() call. The reason I wrapped it in namespace wallet is that it helps to avoid verbose wallet:: prefixes and reduces the chance of symbol collisions. It's also consistent with most of the other unit tests under src/wallet/test, which commonly wrap helper functions and test cases in the same namespace.

That said, I agree TestWalletName() should probably be marked static to indicate internal linkage — will fix that.

pablomartin4btc commented at 2:25 AM on July 23, 2025:

Done!

stickies-v commented at 8:42 AM on July 23, 2025:

It's also consistent with most of the other unit tests under src/wallet/test

Fair enough, I wasn't aware.

and reduces the chance of symbol collisions

I think all of this stuff is meant to have internal linkage, so if reducing symbol collisions is the goal (which seems sensible) an anonymous namespace might make more sense? Potentially with a using namespace wallet, even though at the moment we're only using one wallet symbol.

Anyway, not important, and following convention is usually a good default. Whatever you prefer.

in src/rpc/blockchain.cpp:2211 in af0ad72a0e outdated

2212 | -                {"range", RPCArg::Type::RANGE, RPCArg::Default{1000}, "The range of HD chain indexes to explore (either end or [begin,end])"},
2213 | -            }},
2214 | +        output_descriptor_obj
2215 |      },
2216 | -    RPCArgOptions{.oneline_description="[scanobjects,...]"},
2217 | +    RPCArgOptions{.oneline_description="[scanobjects,...]"}

stickies-v commented at 9:24 PM on July 22, 2025:

nit: can keep comma to avoid touching this line

pablomartin4btc commented at 2:25 AM on July 23, 2025:

Done!

in doc/release-notes-32845.md:6 in af0ad72a0e outdated

   0 | @@ -0,0 +1,9 @@
   1 | +Updated RPCs
   2 | +------------
   3 | +
   4 | +- `unloadwallet` - Raise RPC_INVALID_PARAMETER describing that either the RPC endpoint
   5 | +or the wallet name parameter must be provided. Previously the RPC failed with a JSON
   6 | +parsing error.

stickies-v commented at 9:30 PM on July 22, 2025:

- `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.

pablomartin4btc commented at 2:25 AM on July 23, 2025:

Done!

stickies-v approved

stickies-v commented at 9:46 PM on July 22, 2025: contributor

re-ACK af0ad72a0ed778c7e6f83b1a290705cefe23d78a

DrahtBot requested review from BrandonOdiwuor on Jul 22, 2025

DrahtBot removed the label CI failed on Jul 22, 2025

rpc, test: Add EnsureUniqueWalletName tests

Co-authored-by: stickies-v <stickies-v@users.noreply.github.com>

1fc3a8e8e7

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 <maflcko@users.noreply.github.com>

53ac704efd

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.

39fef1d203

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 <stickies-v@users.noreply.github.com>

90fd5acbe5

doc: Add release notes for changes in RPCs

Adding notes for both `unloadwallet` and `getdescriptoractivity`.

c5c1960f93

pablomartin4btc force-pushed on Jul 23, 2025

pablomartin4btc commented at 2:37 AM on July 23, 2025: member

Addressed feedback from @stickies-v.

stickies-v approved

stickies-v commented at 8:51 AM on July 23, 2025: contributor

re-ACK c5c1960f9350d6315cadbdc95fface5f85f25806

nit: clang-format doesn't agree with some of the spacing changes, if you force push again could be nice to run it on each commit to ensure consistency

furszy commented at 7:03 PM on July 23, 2025: member

ACK c5c1960f9350d6315cadbdc95fface5f85f25806

achow101 commented at 7:26 PM on July 25, 2025: member

ACK c5c1960f9350d6315cadbdc95fface5f85f25806

achow101 merged this on Jul 25, 2025

achow101 closed this on Jul 25, 2025