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

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

0./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
1error code: -3
2error message:
3JSON value of type number is not of expected type string

0./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
1error code: -3
2error message:
3JSON value of type null is not of expected type array

0./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
1error code: -3
2error message:
3JSON value of type null is not of expected type array

-After

0./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc unloadwallet
1error code: -8
2error message:
3Either RPC endpoint wallet or wallet_name parameter must be provided

0./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity
1error code: -8
2error message:
3At least 1 blockhash or 1 descriptor must be specified.

0./build/bin/bitcoin-cli -regtest -datadir=/tmp/btc getdescriptoractivity '[]'
1error code: -8
2error message:
3At least 1 blockhash or 1 descriptor must be specified.

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.

788b3ae274

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.

d5de9a53aa

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. A summary of reviews will appear here.

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:

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

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.

pablomartin4btc force-pushed on Jul 1, 2025

rpc, test: Fix error message in getdescriptoractivity

Raise RPC_INVALID_PARAMETER indicating that at least 1 blockhash or 1 descriptor
must be passed when calling getdescriptoractivity with no argument, otherwise
the command fails with a low-level JSON parsing error lacking of clarity.

Considering also that calling getdescriptoractivity with an empty array raises
the same error.

Adding a new functional test for this use case.

2e4a0f79a0

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.

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 commented at 4:39 pm on July 1, 2025: member

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 commented at 4:50 pm on July 1, 2025: member

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

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

Which, for example should work fine for getdescriptoractivity :

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

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

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:

0const 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.

DrahtBot removed the label CI failed on Jul 1, 2025

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

Code Coverage & Benchmarks

Reviews