cli: Handle arguments that can be either JSON or string #33230

achow101 · 2025-08-20T21:20:53Z

There are some RPCs where the argument can be either JSON that needs to be parsed, or a string that we can pass straight through. However, bitcoin-cli would always parse those arguments as JSON which makes for some cumbersome argument passing when using those RPCs. Notably, hash_or_height in getblockstats and gettxoutsetinfo do this, and results in a more cumbersome command of bitcoin-cli getblockstats '"<hash>"'. Otherwise, using a normal invocation of bitcoin-cli getblockstats <hash> results in error: Error parsing JSON. This PR marks those particular options as also being a string so that when bitcoin-cli fails to parse the argument as JSON, it will assume that the argument is a string and pass it straight through.

DrahtBot · 2025-08-20T21:20:58Z

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/33230.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pablomartin4btc, kannapoix, enirox001
Concept ACK	nervana21
Approach ACK	w0xlt
Stale ACK	kevkevinpal

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33192 (refactor: unify container presence checks (without PR conflicts) by l0rinc)
#33031 (wallet: Set descriptor cache upgraded flag for migrated wallets by achow101)
#32928 (test: add logging to mock external signers by Sjors)
#32821 (rpc: Handle -named argument parsing where '=' character is used by zaidmstrr)
#32784 (wallet: derivehdkey RPC to get xpub at arbitrary path by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2025-08-20T21:39:13Z

🚧 At least one of the CI tasks failed.
_{Task lint: https://github.com/bitcoin/bitcoin/runs/48530907189}
_{LLM reason (✨ experimental): The CI failure is caused by a linting error detected by ruff due to an unused import in the Python test code.}

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.

ryanofsky · 2025-08-20T21:53:58Z

Seems like a reasonable change. I wonder if an alternate solution might have the client treat hash_or_height as a string parameter instead of a JSON parameter, and update the server to accept height strings? But I guess that might not be backwards compatible if there are old scripts calling bitcoin-cli with extra quotes around the hash string.

polespinasa · 2025-08-20T22:00:35Z

I wonder if an alternate solution might have the client treat hash_or_height as a string parameter instead of a JSON parameter, and update the server to accept height strings?

Even if there is, there are other use cases taking lists and strings as parameters. See: #32468

w0xlt

Approach ACK

pablomartin4btc

ACK aabf1f6

This is quite practical, went thru these errors.

luke-jr

I guess this basically works for the existing params, since a block hash is never valid JSON (which forbids leading zeros), but could be dangerous to use for new parameters that are ambiguous.

With that in mind, it might be worth instead having a new RPCResult::Type::BLOCK_HEIGHT_OR_HASH type to sanity check this. Or at least a warning comment (but I don't know where you'd put it to ensure it's seen).

luke-jr · 2025-08-22T11:50:30Z

test/functional/rpc_help.py

@@ -32,7 +32,7 @@ def process_mapping(fname):
                if line.startswith('};'):
                    in_rpcs = False
                elif '{' in line and '"' in line:
-                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
+                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *(, \/\*also_string=\*\/true *)?},', line)


Might be a good idea to actually test this is correct?

This is here because the test fails without it.

Github-Pull: bitcoin#33230 Rebased-From: 009c972

achow101 · 2025-08-22T19:39:34Z

I guess this basically works for the existing params, since a block hash is never valid JSON (which forbids leading zeros), but could be dangerous to use for new parameters that are ambiguous.

I don't think we have any types where that would be the case. But this is also something that needs to be turned on for a parameter explicitly, so I don't expect this to be problematic.

With that in mind, it might be worth instead having a new RPCResult::Type::BLOCK_HEIGHT_OR_HASH type to sanity check this.

Part of the impetus for this is #32468 where the parameter can be either is a JSON array or a string, so something that is more generic is better.

Or at least a warning comment (but I don't know where you'd put it to ensure it's seen).

I think with it being default off makes that not really necessary.

nervana21 · 2025-08-23T02:34:24Z

Concept ACK

kannapoix · 2025-08-23T04:17:37Z

ACK aabf1f6

nit: It may be a good idea to also update the example command in the help message of getblockstats. Given that this PR eliminates the need for quotes around arguments, it would be good to update the example accordingly by using plain text instead of the quoted format.

bitcoin/src/rpc/blockchain.cpp

Line 1943 in 73220fc

    
           HelpExampleCli("getblockstats", R"('"00000000c937983704a73af28acdec37b049d214adbda81d7e2a3dd146f6ed09"' '["minfeerate","avgfeerate"]')") +

kevkevinpal · 2025-08-26T13:19:58Z

ACK 111938c

This change makes sense. I've definitely run into this issue, and being able to use a regular string is less confusing

I checked that with the change to src/rpc/client.cpp, the old tests using convert_to_json_for_cli still passed.

I was wondering if it makes sense to keep one test using convert_to_json_for_cli to ensure that these parameters can still be used as JSON for backward compatibility.

nervana21 · 2025-08-29T02:07:13Z

src/rpc/client.cpp

    { "gettxoutsetinfo", 2, "use_index"},
    { "dumptxoutset", 2, "options" },
-    { "dumptxoutset", 2, "rollback" },
+    { "dumptxoutset", 2, "rollback", /*also_string=*/true },


Will this entry have an effect? In the positional map members, the key ("dumptxoutset", 2) is first set to false. My understanding is that subsequent encounters of the same key are ignored by std::map::emplace. Is this the desired behavior, or should the subsequent value overwrite the first?

{ "dumptxoutset", 2, "options", /*also_string=*/false }, // inserted { "dumptxoutset", 2, "rollback", /*also_string=*/true }, // ignored or used to overwrite?

enirox001

ACK aabf1f6 — improves CLI ergonomics for mixed JSON/string RPC params

DrahtBot added the Scripts and tools label Aug 20, 2025

achow101 force-pushed the cli-strong-or-json branch from 281802b to 01039d3 Compare August 20, 2025 21:38

DrahtBot added the CI failed label Aug 20, 2025

achow101 added 2 commits August 20, 2025 14:40

cli: Allow arguments to be both strings and json

009c972

test: Remove convert_to_json_for_cli

aabf1f6

achow101 force-pushed the cli-strong-or-json branch from 01039d3 to aabf1f6 Compare August 20, 2025 21:40

w0xlt reviewed Aug 20, 2025

View reviewed changes

DrahtBot removed the CI failed label Aug 20, 2025

This was referenced Aug 21, 2025

refactor: unify container presence checks (without PR conflicts) #33192

Open

wallet: Set descriptor cache upgraded flag for migrated wallets #33031

Open

test: add logging to mock external signers #32928

Open

pablomartin4btc reviewed Aug 21, 2025

View reviewed changes

DrahtBot requested a review from w0xlt August 21, 2025 18:32

This was referenced Aug 21, 2025

rpc: Handle -named argument parsing where '=' character is used #32821

Open

wallet: derivehdkey RPC to get xpub at arbitrary path #32784

Draft

luke-jr reviewed Aug 22, 2025

View reviewed changes

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Aug 22, 2025

cli: Allow arguments to be both strings and json

111938c

Github-Pull: bitcoin#33230 Rebased-From: 009c972

nervana21 reviewed Aug 29, 2025

View reviewed changes

DrahtBot requested a review from nervana21 August 29, 2025 02:07

enirox001 reviewed Aug 30, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

cli: Handle arguments that can be either JSON or string #33230

cli: Handle arguments that can be either JSON or string #33230

achow101 commented Aug 20, 2025

Uh oh!

DrahtBot commented Aug 20, 2025 •

edited

Loading

Uh oh!

DrahtBot commented Aug 20, 2025

Uh oh!

ryanofsky commented Aug 20, 2025

Uh oh!

polespinasa commented Aug 20, 2025 •

edited

Loading

Uh oh!

w0xlt left a comment

Uh oh!

pablomartin4btc left a comment

Uh oh!

luke-jr left a comment

Uh oh!

luke-jr Aug 22, 2025

Uh oh!

achow101 Aug 22, 2025

Uh oh!

achow101 commented Aug 22, 2025

Uh oh!

nervana21 commented Aug 23, 2025 •

edited

Loading

Uh oh!

kannapoix commented Aug 23, 2025 •

edited

Loading

Uh oh!

kevkevinpal commented Aug 26, 2025

Uh oh!

nervana21 Aug 29, 2025

Uh oh!

enirox001 left a comment •

edited

Loading

Uh oh!

Uh oh!

cli: Handle arguments that can be either JSON or string #33230

Are you sure you want to change the base?

cli: Handle arguments that can be either JSON or string #33230

Conversation

achow101 commented Aug 20, 2025

Uh oh!

DrahtBot commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Aug 20, 2025

Uh oh!

ryanofsky commented Aug 20, 2025

Uh oh!

polespinasa commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

luke-jr Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

achow101 commented Aug 22, 2025

Uh oh!

nervana21 commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kannapoix commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kevkevinpal commented Aug 26, 2025

Uh oh!

nervana21 Aug 29, 2025

Choose a reason for hiding this comment

Uh oh!

enirox001 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 20, 2025 •

edited

Loading

polespinasa commented Aug 20, 2025 •

edited

Loading

nervana21 commented Aug 23, 2025 •

edited

Loading

kannapoix commented Aug 23, 2025 •

edited

Loading

enirox001 left a comment •

edited

Loading