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

pull achow101 wants to merge 2 commits into bitcoin:master from achow101:cli-strong-or-json changing 8 files +37 −31
  1. achow101 commented at 9:20 pm on August 20, 2025: member
    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.
  2. DrahtBot added the label Scripts and tools on Aug 20, 2025
  3. DrahtBot commented at 9:20 pm on August 20, 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/33230.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK pablomartin4btc, kannapoix
    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.

  4. achow101 force-pushed on Aug 20, 2025
  5. DrahtBot added the label CI failed on Aug 20, 2025
  6. DrahtBot commented at 9:39 pm on August 20, 2025: contributor

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

    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.

  7. cli: Allow arguments to be both strings and json 009c972c09
  8. test: Remove convert_to_json_for_cli aabf1f6093
  9. achow101 force-pushed on Aug 20, 2025
  10. ryanofsky commented at 9:53 pm on August 20, 2025: contributor
    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.
  11. polespinasa commented at 10:00 pm on August 20, 2025: contributor

    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: https://github.com/bitcoin/bitcoin/pull/32468

  12. w0xlt commented at 11:28 pm on August 20, 2025: contributor
    Approach ACK
  13. DrahtBot removed the label CI failed on Aug 20, 2025
  14. pablomartin4btc commented at 6:32 pm on August 21, 2025: member

    ACK aabf1f6093892d372544c8b5d55d260699dab69c

    This is quite practical, went thru these errors.

  15. DrahtBot requested review from w0xlt on Aug 21, 2025
  16. in test/functional/rpc_help.py:35 in aabf1f6093
    31@@ -32,7 +32,7 @@ def process_mapping(fname):
    32                 if line.startswith('};'):
    33                     in_rpcs = False
    34                 elif '{' in line and '"' in line:
    35-                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', line)
    36+                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *(, \/\*also_string=\*\/true *)?},', line)
    


    luke-jr commented at 11:50 am on August 22, 2025:
    Might be a good idea to actually test this is correct?

    achow101 commented at 7:32 pm on August 22, 2025:
    This is here because the test fails without it.
  17. luke-jr commented at 11:55 am on August 22, 2025: member

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

  18. luke-jr referenced this in commit 111938ce2c on Aug 22, 2025
  19. achow101 commented at 7:39 pm on August 22, 2025: member

    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.

  20. nervana21 commented at 2:34 am on August 23, 2025: contributor
    Concept ACK
  21. kannapoix commented at 4:17 am on August 23, 2025: none

    ACK aabf1f6093892d372544c8b5d55d260699dab69c

    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. https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/rpc/blockchain.cpp#L1943

  22. kevkevinpal commented at 1:19 pm on August 26, 2025: contributor

    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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-28 21:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me