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

[pablomartin4btc]

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]

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.

[DrahtBot]

🚧 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.}

Hints

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.

[pablomartin4btc]

Updates:

• 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]

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.");
}

[pablomartin4btc]

Updates:

• 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]

🚧 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.}

Hints

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.

[pablomartin4btc]

Updates

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

[furszy]

utACK 529d2ce

[maflcko]

review ACK 529d2ce 🌦

Show signature

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

[stickies-v]

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

[DrahtBot]

🚧 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.}

Hints

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.

[pablomartin4btc]

Updates:

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

[stickies-v]

re-ACK af0ad72

[pablomartin4btc]

Addressed feedback from @stickies-v.

[stickies-v]

re-ACK c5c1960

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]

ACK c5c1960

[achow101]

ACK c5c1960