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

zaidmstrr commented at 11:43 am on June 27, 2025: contributor

The [PR #31375](https://github.com/bitcoin/bitcoin/pull/31375) got merged and enables -named by default in the bitcoin rpc interface; bitcoin rpc corresponds to bitcoin-cli -named as it’s just a wrapper. Now, the problem arises when we try to parse the positional paramater which might contain “=” character. This splits the parameter into two parts first, before the “=” character, which treats this as the parameter name, but the other half is mostly passed as an empty string. Here, the first part of the string is an unknown parameter name; thus, an error is thrown. These types of errors are only applicable to those RPCs which might contain the = character as a parameter. Some examples are finalizepsbt, decodepsbt, verifymessage etc.

This is the one example of the error in finalizepsbt RPC:

0./bitcoin-cli -named -regtest finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
1error code: -8
2error message:
3Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA

This PR fixes this by updating the vRPCConvertParams table that identifies parameters that need special handling in -named parameter mode. The parser now recognises these parameters and handles strings with “=” char correctly, preventing them from being incorrectly split as parameter assignments.

DrahtBot added the label RPC/REST/ZMQ on Jun 27, 2025

DrahtBot commented at 11:43 am on June 27, 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/32821.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	yuvicc
Stale ACK	BrandonOdiwuor, ryanofsky, Prabhat1308

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:

#32966 (Silent Payments: Receiving by Eunovo)
#32741 (rpc: add optional peer_ids param to filter getpeerinfo by waketraindev)
#21283 (Implement BIP 370 PSBTv2 by achow101)

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.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

parameter parameter -> parameter [duplicated word; remove the extra “parameter” for clarity]

drahtbot_id_5_m

in src/rpc/client.cpp:319 in 14e2125ee0 outdated

310@@ -311,6 +311,26 @@ static const CRPCConvertParam vRPCConvertParams[] =
311     { "addnode", 2, "v2transport" },
312     { "addconnection", 2, "v2transport" },
313 };
314+
315+/**
316+ * Specify a (method, idx, name) here if the argument is a string RPC
317+ * parameter that should be recognized as a known parameter name but NOT
318+ * converted from JSON. This is primarily used for Base64-encoded parameters
319+ * that might contain '=' padding characters which could interfere with

maflcko commented at 11:58 am on June 27, 2025:

not sure about limiting this to base64. There are many other args that can hold values with an = in them. For example, labels.

zaidmstrr commented at 12:18 pm on June 27, 2025:

I think you are right; I will generalise it to any string args which might contain =

Sjors commented at 12:02 pm on June 27, 2025: member

I ran into this issue in #32784 while editing the multisig tutorial. 14e2125ee0297f0cb046ca91b6ead20052f24678 fixes it.

ryanofsky commented at 12:20 pm on June 27, 2025: contributor

Concept ACK 14e2125ee0297f0cb046ca91b6ead20052f24678. Thanks for the fix!

If possible it would be good to add python tests to check the new behavior and I also agree with comment that this change doesn’t need to be limited to base64 parameters and could probably be extended to other string parameters. But maybe the “If parameter is unknown for a given known string method, then treat the whole string as positional” behavior might have to be dropped in that case.

I think this fix is little different from my suggestion in #31375 (review) because I was thinking of continuing to treat unknown parameters as named not positional, but maybe this approach is better. Would have to think about it. The implementation is also a little different than what I had in mind because I was thinking of extending the vRPCConvertParams table with type information instead of adding a separate table, but your approach might be simpler.

zaidmstrr commented at 2:09 pm on June 27, 2025: contributor

If possible it would be good to add python tests to check the new behavior

Sure, I’m also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists = padding or in between, so while creating a PSBT, I need to look at how I can achieve this. Or maybe by calling RPCs with a label.

I also agree with #32821 (review) that this change doesn’t need to be limited to base64 parameters and could probably be extended to other string parameters. But maybe the “If parameter is unknown for a given known string method, then treat the whole string as positional” behavior might have to be dropped in that case.

To extend this feature to any string parameters, we simply need to add an entry in the vRPCStringParams table. I tested with getnewaddress which requires a label (optionally) and it’s working fine. Without adding the entry in vRPCStringParams it doesn’t work if we provide a label that consists of the = char in it.

And the line you mentioned to remove is problematic if we remove it, as it’s only got invoked if the RPC method is correct and matches with our table. And if the parameter name is not known then it’s invoked; otherwise not. This is safe to do because we already make sure that the calling method is present in the table. Otherwise, if the user passed the correct method name but an incorrect parameter, then the targeted RPC will fail with an error.

Sjors commented at 2:28 pm on June 27, 2025: member

If possible it would be good to add python tests to check the new behavior

Sure, I’m also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists = padding or in between, so while creating a PSBT, I need to look at how I can achieve this.

You could just manually generate a random PSBT on regtest, hardcode it into a test and call analyzepsbt. It doesn’t matter if it’s unspendable.

Or you could even just call these methods with abc=efg since it doesn’t even need to be a validly encoded psbt.

zaidmstrr force-pushed on Jun 28, 2025

zaidmstrr commented at 6:59 pm on June 28, 2025: contributor

I generalised this change to any string parameter which might contain the = char in it. I also added the functional test for most of the RPCs I included in the new table. For any new RPC method which might need to be parsed this way with -named enabled, one can add the new entry of that RPC in the vRPCStringParams table.

zaidmstrr renamed this:
~~rpc: Handle -named argument parsing where Base64 encoding is used~~
rpc: Handle -named argument parsing where '=' character is used
on Jun 29, 2025

yuvicc commented at 2:24 pm on June 29, 2025: contributor

Concept ACK

0./build/bin/bitcoin-cli -named  finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O=g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
1
2error code: -8
3error message:
4Unknown named parameter cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O

Commit 59fcefcd1f490f8ae276150421cbd895a10f3d4a

0./build/bin//bitcoin-cli -named  finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
1{
2  "psbt": "cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=",
3  "complete": false
4}

Will review the code soon!

zaidmstrr requested review from maflcko on Jun 29, 2025

BrandonOdiwuor commented at 5:07 pm on June 29, 2025: contributor

Tested ACK e246741db1f2f056fac32d0f3967f55306e78b49

Confirmed that commit 59fcefcd1f490f8ae276150421cbd895a10f3d4a correctly handles named argument parsing for parameters containing = characters—an issue currently reproducible on master.

<commit: 59fcefcd1f490f8ae276150421cbd895a10f3d4a>

<branch: master>

DrahtBot requested review from ryanofsky on Jun 29, 2025

in src/rpc/client.cpp:381 in e246741db1 outdated

329+    { "walletprocesspsbt", 0, "psbt"},
330+    { "descriptorprocesspsbt", 0, "psbt"},
331+    { "utxoupdatepsbt", 0, "psbt"},
332+    { "verifymessage", 1, "signature"},
333+    { "getnewaddress", 0, "label"},
334+};

maflcko commented at 6:34 am on June 30, 2025:

this list is incomplete. Also, I wonder how this should be maintained. There needs to be an automated check that the list is up-to-date, like for the other list.

zaidmstrr commented at 11:37 am on June 30, 2025:

Can you please tell me about the automated check you are referring to? I saw a test in test/functional/rpc_help.py which does similar things.

maflcko commented at 12:59 pm on June 30, 2025:

Can you please tell me about the automated check you are referring to? I saw a test in test/functional/rpc_help.py which does similar things.

Yes, that is the one I was referring to.

maflcko commented at 1:43 pm on July 3, 2025:

Also, it doesn’t seem to be working right now anyway, see the CI failure (needs rebase)

zaidmstrr commented at 9:08 pm on July 3, 2025:

Rebased and fixed the previous CI error. But now CI is showing a different error I’m investigating for this one.

DrahtBot added the label CI failed on Jul 3, 2025

zaidmstrr force-pushed on Jul 3, 2025

zaidmstrr force-pushed on Jul 4, 2025

Sjors commented at 6:35 am on July 4, 2025: member

From CI:

0assert not "non_witness_utxo" in mining_node.decodepsbt(signed_psbt["psbt"])["inputs"][0]

I didn’t check the code, but that’s usually a cryptic way of saying that signing the psbt failed.

zaidmstrr commented at 9:38 am on July 4, 2025: contributor

Signing the PSBT works fine while running locally, and removing non-witness UTXO from the PSBT.

maflcko commented at 10:34 am on July 4, 2025: member

Signing the PSBT works fine while running locally, and removing non-witness UTXO from the PSBT.

What command did you use to run locally?

zaidmstrr commented at 11:07 am on July 4, 2025: contributor

What command did you use to run locally?

I first checked by adding two log statements in rpc_psbt.py the first log to check the psbt before signing and the second log after walletprocesspsbt where CI is reporting an error. And then manually parsing the result with the help of decodepsbt to check whether the PSBT afterwalletprocesspsbt contains non-witness UTXO, but it doesn’t in my case, so the test passed successfully.

I also ran all the functional test cases with —extended and all tests passed.

maflcko commented at 11:22 am on July 4, 2025: member

you’ll have to use --usecli to run the tests, otherwise the cli is not used

Prabhat1308 commented at 3:40 pm on July 7, 2025: contributor

Trying to debug the issue I see some irregularities in the how the test behaves in rpc mode and cli mode.

 0
 1diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
 2index a241978b5f..f92e8d6e88 100755
 3--- a/test/functional/rpc_psbt.py
 4+++ b/test/functional/rpc_psbt.py
 5@@ -135,8 +135,64 @@ class PSBTTest(BitcoinTestFramework):
 6         assert "non_witness_utxo" in mining_node.decodepsbt(psbt_new.to_base64())["inputs"][0]
 7
 8         # Have the offline node sign the PSBT (which will remove the non-witness UTXO)
 9+
10+        decoded_before = mining_node.decodepsbt(psbt_new.to_base64())
11+
12+        self.log.info(f"=== DEBUG: CLI Mode = {self.options.usecli} ===")
13+
14+        # Log address and input details
15+        offline_addr_info = offline_node.getaddressinfo(offline_addr)
16+        self.log.info(f"Offline address: {offline_addr}")
17+        self.log.info(f"Address type: {offline_addr_info.get('type', 'unknown')}")
18+
19+        decoded_before = mining_node.decodepsbt(psbt_new.to_base64())
20+
21+        self.log.info(f"=== DEBUG: CLI Mode = {self.options.usecli} ===")
22+
23+        # Log address and input details
24+        offline_addr_info = offline_node.getaddressinfo(offline_addr)
25+        self.log.info(f"Offline address: {offline_addr}")
26+        self.log.info(f"Address type: {offline_addr_info.get('type', 'unknown')}")
27+        self.log.info(f"Script type: {offline_addr_info.get('script', 'unknown')}")
28+        self.log.info(f"Is witness: {offline_addr_info.get('iswitness', False)}")
29+        self.log.info(f"Witness version: {offline_addr_info.get('witness_version', 'none')}")
30+        self.log.info(f"Witness program: {offline_addr_info.get('witness_program', 'none')}")
31+
32+        # Log PSBT input details before signing
33+        self.log.info(f"BEFORE signing - input[0] keys: {list(decoded_before['inputs'][0].keys())}")
34+        self.log.info(f"BEFORE signing - has non_witness_utxo: {'non_witness_utxo' in decoded_before['inputs'][0]}")
35+        self.log.info(f"BEFORE signing - has witness_utxo: {'witness_utxo' in decoded_before['inputs'][0]}")
36+
37+        # Check witness_utxo script details
38+        if 'witness_utxo' in decoded_before['inputs'][0]:
39+            witness_utxo = decoded_before['inputs'][0]['witness_utxo']
40+            self.log.info(f"BEFORE signing - witness_utxo script type: {witness_utxo['scriptPubKey'].get('type', 'unknown')}")
41+            self.log.info(f"BEFORE signing - witness_utxo hex: {witness_utxo['scriptPubKey'].get('hex', 'none')}")
42+
43+        # Log the exact walletprocesspsbt call being made
44+        psbt_b64 = psbt_new.to_base64()
45+        self.log.info(f"walletprocesspsbt call: offline_node.walletprocesspsbt('{psbt_b64[:50]}...', True, 'ALL', True, True)")
46+
47         signed_psbt = offline_node.walletprocesspsbt(psbt_new.to_base64())
48-        assert not "non_witness_utxo" in mining_node.decodepsbt(signed_psbt["psbt"])["inputs"][0]
49+        decoded_after = mining_node.decodepsbt(signed_psbt["psbt"])
50+
51+        # Log PSBT input details after signing
52+        self.log.info(f"AFTER signing - input[0] keys: {list(decoded_after['inputs'][0].keys())}")
53+        self.log.info(f"AFTER signing - has non_witness_utxo: {'non_witness_utxo' in decoded_after['inputs'][0]}")
54+        self.log.info(f"AFTER signing - has witness_utxo: {'witness_utxo' in decoded_after['inputs'][0]}")
55+        self.log.info(f"walletprocesspsbt result complete: {signed_psbt.get('complete', 'not present')}")
56+
57+        # Check for Taproot vs regular segwit indicators
58+        has_taproot_derivs = 'taproot_bip32_derivs' in decoded_before['inputs'][0] or 'taproot_bip32_derivs' in decoded_after['inputs'][0]
59+        has_taproot_key = 'taproot_internal_key' in decoded_before['inputs'][0] or 'taproot_internal_key' in decoded_after['inputs'][0]
60+        has_regular_derivs = 'bip32_derivs' in decoded_before['inputs'][0] or 'bip32_derivs' in decoded_after['inputs'][0]
61+
62+        self.log.info(f"Input type detection - Taproot derivs: {has_taproot_derivs}, Taproot key: {has_taproot_key}, Regular derivs: {has_regular_derivs}")
63+
64+        # Log the actual walletprocesspsbt call details for comparison
65+        if self.options.usecli:
66+            self.log.info("Using CLI mode - parameters passed as separate arguments")
67+        else:
68+            self.log.info("Using RPC mode - parameters passed as JSON")
69+
70+        # More detailed assertion with context
71+        if "non_witness_utxo" in decoded_after["inputs"][0]:
72+            self.log.error("FAILURE: non_witness_utxo should be removed for segwit v1+ inputs!")
73+            self.log.error(f"This suggests the input is being treated as segwit v0 instead of v1+ in CLI mode")
74+
75+

 02025-07-07T15:20:33.852283Z TestFramework (INFO): Check that non-witness UTXOs are removed for segwit v1+ inputs
 12025-07-07T15:20:34.004885Z TestFramework (INFO): === DEBUG: CLI Mode = True ===
 22025-07-07T15:20:34.008380Z TestFramework (INFO): Offline address: bcrt1q2skwujcxhaawtf0hlqawfmk3gclpdz24a84znw
 32025-07-07T15:20:34.008437Z TestFramework (INFO): Address type: unknown
 42025-07-07T15:20:34.008459Z TestFramework (INFO): Script type: unknown
 52025-07-07T15:20:34.008479Z TestFramework (INFO): Is witness: True
 62025-07-07T15:20:34.008497Z TestFramework (INFO): Witness version: 0
 72025-07-07T15:20:34.008513Z TestFramework (INFO): Witness program: 542cee4b06bf7ae5a5f7f83ae4eed1463e168955
 82025-07-07T15:20:34.008532Z TestFramework (INFO): BEFORE signing - input[0] keys: ['witness_utxo', 'non_witness_utxo', 'bip32_derivs']
 92025-07-07T15:20:34.008550Z TestFramework (INFO): BEFORE signing - has non_witness_utxo: True
102025-07-07T15:20:34.008565Z TestFramework (INFO): BEFORE signing - has witness_utxo: True
112025-07-07T15:20:34.008579Z TestFramework (INFO): BEFORE signing - witness_utxo script type: witness_v0_keyhash
122025-07-07T15:20:34.008595Z TestFramework (INFO): BEFORE signing - witness_utxo hex: 0014542cee4b06bf7ae5a5f7f83ae4eed1463e168955
132025-07-07T15:20:34.008688Z TestFramework (INFO): walletprocesspsbt call: offline_node.walletprocesspsbt('cHNidP8BAFICAAAAAQVChcz/DF+d62weFt4XJAYjd8cLM81FZV...', True, 'ALL', True, True)
142025-07-07T15:20:34.016020Z TestFramework (INFO): AFTER signing - input[0] keys: ['witness_utxo', 'non_witness_utxo', 'final_scriptwitness']
152025-07-07T15:20:34.016093Z TestFramework (INFO): AFTER signing - has non_witness_utxo: True
162025-07-07T15:20:34.016117Z TestFramework (INFO): AFTER signing - has witness_utxo: True
172025-07-07T15:20:34.016135Z TestFramework (INFO): walletprocesspsbt result complete: True
182025-07-07T15:20:34.016155Z TestFramework (INFO): Input type detection - Taproot derivs: False, Taproot key: False, Regular derivs: True
192025-07-07T15:20:34.016174Z TestFramework (INFO): Using CLI mode - parameters passed as separate arguments
202025-07-07T15:20:34.016193Z TestFramework (ERROR): FAILURE: non_witness_utxo should be removed for segwit v1+ inputs!
212025-07-07T15:20:34.016209Z TestFramework (ERROR): This suggests the input is being treated as segwit v0 instead of v1+ in CLI mode
222025-07-07T15:20:34.016234Z TestFramework (ERROR): Assertion failed

 0
 12025-07-07T15:19:10.430768Z TestFramework (INFO): === DEBUG: CLI Mode = False ===
 22025-07-07T15:19:10.431566Z TestFramework (INFO): Offline address: bcrt1pxmju835wnuw3z5thftuz84q8rt96vqd97jaxwdn2uqlxwwka923q8fcsp8
 32025-07-07T15:19:10.431613Z TestFramework (INFO): Address type: unknown
 42025-07-07T15:19:10.431645Z TestFramework (INFO): Script type: unknown
 52025-07-07T15:19:10.431672Z TestFramework (INFO): Is witness: True
 62025-07-07T15:19:10.431699Z TestFramework (INFO): Witness version: 1
 72025-07-07T15:19:10.431729Z TestFramework (INFO): Witness program: 36e5c3c68e9f1d1151774af823d4071acba601a5f4ba67366ae03e673add2aa2
 82025-07-07T15:19:10.431760Z TestFramework (INFO): BEFORE signing - input[0] keys: ['witness_utxo', 'non_witness_utxo', 'taproot_bip32_derivs', 'taproot_internal_key']
 92025-07-07T15:19:10.431789Z TestFramework (INFO): BEFORE signing - has non_witness_utxo: True
102025-07-07T15:19:10.431831Z TestFramework (INFO): BEFORE signing - has witness_utxo: True
112025-07-07T15:19:10.431857Z TestFramework (INFO): BEFORE signing - witness_utxo script type: witness_v1_taproot
122025-07-07T15:19:10.431881Z TestFramework (INFO): BEFORE signing - witness_utxo hex: 512036e5c3c68e9f1d1151774af823d4071acba601a5f4ba67366ae03e673add2aa2
132025-07-07T15:19:10.431929Z TestFramework (INFO): walletprocesspsbt call: offline_node.walletprocesspsbt('cHNidP8BAF4CAAAAAVteg2JVDVvQHLSH4zmmgu7l4bYJdyIoFq...', True, 'ALL', True, True)
142025-07-07T15:19:10.433261Z TestFramework (INFO): AFTER signing - input[0] keys: ['witness_utxo', 'final_scriptwitness']
152025-07-07T15:19:10.433305Z TestFramework (INFO): AFTER signing - has non_witness_utxo: False
162025-07-07T15:19:10.433336Z TestFramework (INFO): AFTER signing - has witness_utxo: True
172025-07-07T15:19:10.433364Z TestFramework (INFO): walletprocesspsbt result complete: True
182025-07-07T15:19:10.433399Z TestFramework (INFO): Input type detection - Taproot derivs: True, Taproot key: True, Regular derivs: False
192025-07-07T15:19:10.433426Z TestFramework (INFO): Using RPC mode - parameters passed as JSON

inputs are being treated differently in the cli and non-cli mode
even the address being generated in rpc mode is different (larger as compared to cli mode)
non-witness utxo remains in cli mode.

zaidmstrr force-pushed on Jul 8, 2025

DrahtBot removed the label CI failed on Jul 8, 2025

zaidmstrr force-pushed on Jul 9, 2025

in src/rpc/client.cpp:515 in bc2241ab3d outdated

510+
511+        if (rpcCvtTable.HasKnownStringParams(strMethod, positional_args.size())) {
512+            // If this method has known string parameters, check if this specific parameter is one of them
513+            if (rpcCvtTable.IsStringParam(strMethod, name)) {
514+                // Pass only the value as positional
515+                positional_args.push_back(rpcCvtTable.ArgToUniValue(value, strMethod, positional_args.size()));

ryanofsky commented at 11:49 pm on July 9, 2025:

In commit “rpc: Handle -named argument parsing where Base64 encoding is used” (bc2241ab3d01a38f19588cb85134e5330b77e3f4)

Thanks for the updates!

Looking at more closely at this solution, some things don’t make sense to me. This code from lines 513-515 is checking to see if a command line argument that parses as NAME=VALUE has a NAME that is listed in the table above as a known string parameter. But if it is instead of adding the parameter to params and passing the argument by name, it is discarding the name, adding the parameter to positional_args and passing the argument by position, which seems like the opposite of what it should be doing. This looks like a bug, but it might be easy to fix and the else condition below does look right.

I’m also pretty confused by the IsOptionalStringParam code above. It seems complex and I don’t actually understand what cases that is intended to handle.

Also when experimenting with this, I realized the original bug of = characters causing positional parameters to be mistakenly interpreted as named parameters is not just limited to string parameters, but can also happen with JSON parameters. This easy to see with following example:

0build/bin/bitcoin-cli -regtest -named echojson '["ok"]'
1build/bin/bitcoin-cli -regtest -named echojson '["fail=yes"]'

which fails with Unknown named parameter ["fail. Fixing this might be out of scope for this PR, but it should be easier to fix than the original problem because the JSON representation is not ambiguous. So it could be nice to fix here.

To understand the problem better, I implemented df4f39143d868fe4303ab7b3875757a26b7ac5a5 (tag) on top of your PR which I think takes a simpler and more general approach and should fix this problem more fully.

If it would be helpful, feel free to use any of that code in this PR. And if you think the current approach is better, that could also be the case. I mostly just don’t understand the optional string aspect of bc2241ab3d01a38f19588cb85134e5330b77e3f4 and what cases it is handling.

zaidmstrr commented at 4:14 am on July 13, 2025:

Thank you for the detailed review!

I think you’re right, my current approach regarding named parameter handling is too verbose/complex, and I looked at your implementation, and I really liked the way you’re handling named parameters in the RPCConvertNamedValues function. I will implement that approach in my next update commit. Besides that, I also looked at the json parsing issue you’re mentioning, and it’s easily reproducible. I experimented with both the approaches, the one you provided and the current one, and I’m trying to get the best things from both of them. I will try to fix the json parsing issue in this PR because it’s closely related to the same issue.

ryanofsky commented at 6:59 pm on July 21, 2025:

re: #32821 (review)

Thanks! it appears all issues are solved now with the latest push (56b1da77b5998c44b3bb799689a6c0449e9223fc)

zaidmstrr force-pushed on Jul 14, 2025

zaidmstrr requested review from ryanofsky on Jul 15, 2025

zaidmstrr requested review from maflcko on Jul 20, 2025

in src/rpc/client.cpp:494 in e20cba675b outdated

489+             *
490+             * 2. The RPC method was listed in the vRPCConvertParams table with a given index but the parameter is not a known convert parameter
491+             *    in such cases we first parse the whole parameter as JSON and then treat as positional.
492+             */
493+            bool is_string_method = rpcCvtTable.IsStringParamByIndex(strMethod, positional_args.size());
494+            bool is_convert_method = rpcCvtTable.IsConvertParamByIndex(strMethod, positional_args.size());

ryanofsky commented at 6:55 pm on July 21, 2025:

In commit “rpc: Handle -named argument parsing where Base64 encoding is used” (e20cba675bfa5e17da1a0776dda0070e9c94aff1)

Not sure the names is_string_method and is_convert_method are really accurate here. is_string_method is true if the method’s next positional argument expects a string, and is_convert_method is true if it expects a non-string value. Could maybe consider calling them next_positional_arg_is_string or next_positional_arg_is_json or something like that. Code looks like it does the right thing though so feel free to ignore this suggestion.

zaidmstrr commented at 6:37 pm on July 22, 2025:

I think you’re right. I will make those more descriptive.

in test/functional/wallet_labels.py:65 in 56b1da77b5 outdated

60+        result = node.cli("-named", "getnewaddress", f"label={label_with_equals}").send_cli()
61+        address = result.strip()
62+        addr_info = node.getaddressinfo(address)
63+        assert_equal(addr_info.get('labels', []), [label_with_equals])
64+
65+        # Test that unknown parameter with '=' gets treated as positional and works correctly

ryanofsky commented at 7:14 pm on July 21, 2025:

In commit “test: Add functional tests for named argument parsing” (56b1da77b5998c44b3bb799689a6c0449e9223fc)

Note: this comment is not exactly describing what is happening during this test. It’s not true in general that unknown parameters with = are treated as positional. Unknown parameters with = are only treated as positional if the next positional parameter is known and either accepts a string (this case) or if it’s known and accepts JSON and the argument actually parses as json. Otherwise it is still treated as a named parameter. Could consider clarifying the comment.

zaidmstrr commented at 6:38 pm on July 22, 2025:

Thanks for pointing this out. I will fix this.

ryanofsky approved

ryanofsky commented at 7:44 pm on July 21, 2025: contributor

Code review ACK 56b1da77b5998c44b3bb799689a6c0449e9223fc. Thanks for the update! The fix here seems right and should be practically useful. And the new tests seem to cover different scenarios where positional string parameters contain ‘=’.

Two comments:

I think it would good to add a test to cover cases where positional non-string parameters contain ‘=’, i.e. the bitcoin-cli -named echojson '["fail=yes"]' case which previously failed (because it treated everything before the ‘=’ character as a parameter name) and now succeeds.
I think the code change suggested in df4f39143d868fe4303ab7b3875757a26b7ac5a5 (tag) to combine vRPCConvertParams and vRPCStringParams tables into a single table, and document it better, and remove some layers of indirection could still be a good change to make. But it could be a followup and the current approach seems reasonable to me. Unless I missed something, the approach implemented there is the same approach implemented here currently, so this would just be a refactoring.

DrahtBot requested review from BrandonOdiwuor on Jul 21, 2025

zaidmstrr commented at 7:15 pm on July 22, 2025: contributor

I think it would good to add a test to cover cases where positional non-string parameters contain ‘=’, i.e. the bitcoin-cli -named echojson '["fail=yes"]' case which previously failed (because it treated everything before the ‘=’ character as a parameter name) and now succeeds.

You are right here, I will add a test for non-string params like echojson.

I think the code change suggested in df4f391 (tag) to combine vRPCConvertParams and vRPCStringParams tables into a single table, and document it better, and remove some layers of indirection could still be a good change to make.

I intentionally choose this approach to use both the vRPCStringParams and vRPCConvertParams tables because the string table has some additional cases, like adding all the string params for a given method in the table for consistency and valid lookups. So with all these things, separating the tables is a good and less complex way between both logics.

zaidmstrr force-pushed on Jul 26, 2025

ryanofsky approved

ryanofsky commented at 3:04 pm on July 28, 2025: contributor

Code review ACK 8fa1a0e50544a5ad987955aec5443857a1711ca7. Thanks for the updates! Only change since last review was clarifying a comment and some variable names and adding new tests to cover cases where non-string positional parameters containing ‘=’ are passed.

achow101 commented at 6:53 pm on July 31, 2025: member

I’m wondering if there’s a different way we can go about this without adding another table of arguments that need special treatment. Perhaps we could move named argument handling and string to json conversion server side? That’s a much more significant refactor, but the RPC server already knows every parameter type and name, so it could do the conversion and named argument check as well.

ryanofsky commented at 7:37 pm on July 31, 2025: contributor

I’m wondering if there’s a different way we can go about this without adding another table of arguments that need special treatment.

I’m also not a fan of separate tables and suggested the following change to unify them earlier: b998cc52d51b48db9271fdba0bd69e9aaccb7999 (tag). This change is just a refactoring and could be a followup.

Perhaps we could move named argument handling and string to json conversion server side?

I think moving logic server side would avoid need for duplicate tables, but not actually make the code or logic simpler because the current syntax for distinguishing named parameters is inherently ambiguous. (It probably would have been better to require named parameters to begin with - to avoid ambiguity. It could also be better to try to parse every argument as JSON and just fall back to passing strings if they are not valid JSON to avoid the need for the conversion table.) I feel like current PR just makes some small tweaks to parsing to make the current syntax work a little better, and it adds good test coverage. If we follow up this PR with b998cc52d51b48db9271fdba0bd69e9aaccb7999 or incorporate those changes here, the client code should be better documented and more maintainable too.

achow101 commented at 8:35 pm on July 31, 2025: member

because the current syntax for distinguishing named parameters is inherently ambiguous.

But since the server knows the names of all parameters, it can also check whether the incoming string starts with the name of a parameter.

Alternatively, maybe we can make the rpc client aware of all of the rpcs so it can do that conversion? Although that would likely have the effect of reducing the utility of bitcoin-cli across versions.

ryanofsky commented at 8:59 pm on July 31, 2025: contributor

Alternatively, maybe we can make the rpc client aware of all of the rpcs so it can do that conversion?

That’s basically what the PR does implicitly and what my refactoring of the PR does more explicitly with string/json formats in rpc/client.cpp.

I think if you look at that file you will see that the the parsing logic is not that complicated and can be well explained. The code there should actually be simpler than current client code in master because it gets rid of intermediate lookup tables and the overloaded overloaded ArgToUniValue method.

Although that would likely have the effect of reducing the utility of bitcoin-cli across versions.

I think that’s just the current reality. The fact that client needs to know about parameter names and types makes it more fragile, but shouldn’t be made worse by the changes here, as far as I can tell.

achow101 commented at 9:20 pm on July 31, 2025: member

That’s basically what the PR does implicitly and what my refactoring of the PR does more explicitly with string/json formats

Yes, but I meant more so in having access to RPCHelpMan which already has the names and types of all parameters.

Or if we had a machine parseable schema thing (see #29912) that bitcon-cli refers to.

DrahtBot added the label Needs rebase on Aug 8, 2025

zaidmstrr force-pushed on Aug 9, 2025

zaidmstrr commented at 8:45 am on August 9, 2025: contributor

Rebased on top latest master.

DrahtBot removed the label Needs rebase on Aug 9, 2025

ryanofsky approved

ryanofsky commented at 1:06 pm on August 13, 2025: contributor

Code review ACK 780bec6f1824651341ee51b2dffefa4cfa1f5690, just rebased since last review to avoid minor conflict with #31886

I still think https://github.com/bitcoin/bitcoin/commit/b998cc52d51b48db9271fdba0bd69e9aaccb7999 (‘rpc/client.cpp’) would be an nice simplification, but it’s just a refactoring so it could be a followup.

zaidmstrr commented at 3:42 pm on August 13, 2025: contributor

I still think b998cc5 (‘rpc/client.cpp’) would be an nice simplification, but it’s just a refactoring so it could be a followup.

Yes, that could be the next possible followup.

in src/rpc/client.cpp:318 in 3f10fe8763 outdated

309@@ -310,6 +310,72 @@ static const CRPCConvertParam vRPCConvertParams[] =
310     { "addnode", 2, "v2transport" },
311     { "addconnection", 2, "v2transport" },
312 };
313+
314+class CRPCStringParam {
315+public:
316+    std::string methodName; //!< method name this parameter belongs to
317+    int paramIdx;           //!< 0-based idx of param
318+    std::string paramName;  //!< parameter name

achow101 commented at 9:59 pm on August 20, 2025:

In 3f10fe8763bf4a1bdb6b347204987584761b9dbf “rpc: Handle -named argument parsing where ‘=’ character is used”

Please use the new class and variable naming style.

zaidmstrr commented at 2:38 pm on August 22, 2025:

Thanks for suggesting. Fixed

in src/rpc/client.cpp:332 in 3f10fe8763 outdated

327+ * @note Parameter indexes start from 0.
328+ * @note If you include an RPC method then you need to include all other
329+ * string parameters that correspond to the same method for consistency
330+ * and valid lookup from the table.
331+ */
332+static const CRPCStringParam vRPCStringParams[] =

achow101 commented at 9:59 pm on August 20, 2025:

In 3f10fe8763bf4a1bdb6b347204987584761b9dbf “rpc: Handle -named argument parsing where ‘=’ character is used”

Please use the new variable naming style.

zaidmstrr commented at 2:38 pm on August 22, 2025:

Fixed

in src/rpc/client.cpp:415 in 3f10fe8763 outdated

407@@ -340,6 +408,30 @@ class CRPCConvertTable
408     {
409         return membersByName.count({method, param_name}) > 0 ? Parse(arg_value) : arg_value;
410     }
411+
412+    /** Return true if parameter is in the string parameters table (vRPCStringParams) */
413+    bool IsStringParamByName(const std::string& method, const std::string& param) const
414+    {
415+        return stringParams.count({method, param}) > 0;

achow101 commented at 10:03 pm on August 20, 2025:

In 3f10fe8763bf4a1bdb6b347204987584761b9dbf “rpc: Handle -named argument parsing where ‘=’ character is used”

We can use contains here rather than count() > 0

zaidmstrr commented at 2:39 pm on August 22, 2025:

Thanks, done.

zaidmstrr force-pushed on Aug 22, 2025

in src/rpc/client.cpp:319 in a4f2144fac outdated

314+class RpcStringParam {
315+public:
316+    std::string method_name; //!< method name this parameter belongs to
317+    int param_idx;           //!< 0-based idx of param
318+    std::string param_name;  //!< parameter name
319+};

achow101 commented at 9:22 pm on August 25, 2025:

In a4f2144facb5d4dc2f749494ea65744fffcb628b “rpc: Handle -named argument parsing where ‘=’ character is used”

This seems unnecessary, it’s the same thing CRPCConvertParam.

zaidmstrr commented at 6:13 am on August 26, 2025:

The definition might be the same, but the purpose is entirely different. This is specifically for string RPC methods.

achow101 commented at 5:23 pm on August 26, 2025:

I don’t think the meaning is different enough to warrant a whole new class.

zaidmstrr commented at 5:53 pm on August 26, 2025:

The meaning is totally different, and it’s easier and much more maintainable right now. This class doesn’t depend on any other class/object. It’s an independent representation.

in src/rpc/client.cpp:395 in a4f2144fac outdated

390@@ -325,6 +391,8 @@ class CRPCConvertTable
391 private:
392     std::set<std::pair<std::string, int>> members;
393     std::set<std::pair<std::string, std::string>> membersByName;
394+    std::set<std::pair<std::string, std::string>> stringParams;
395+    std::set<std::pair<std::string, int>> stringParamsByIndex;

achow101 commented at 9:22 pm on August 25, 2025:

In a4f2144facb5d4dc2f749494ea65744fffcb628b “rpc: Handle -named argument parsing where ‘=’ character is used”

nit: these should use the new naming style.

in src/rpc/client.cpp:493 in a4f2144fac outdated

488+             *
489+             * 2. The RPC method was listed in the vRPCConvertParams table with a given index but the parameter is not a known convert parameter
490+             *    in such cases we first parse the whole parameter as JSON and then treat as positional.
491+             */
492+            bool next_positional_arg_is_string = rpcCvtTable.IsStringParamByIndex(strMethod, positional_args.size());
493+            bool next_positional_arg_is_json = rpcCvtTable.IsConvertParamByIndex(strMethod, positional_args.size());

achow101 commented at 9:24 pm on August 25, 2025:

In a4f2144facb5d4dc2f749494ea65744fffcb628b “rpc: Handle -named argument parsing where ‘=’ character is used”

I find the name ’next_…` to be confusing since this is really referring to the position of the current argument, not the next argument.

ryanofsky commented at 0:10 am on August 26, 2025:

re: #32821 (review)

I find the name ’next_…` to be confusing since this is really referring to the position of the current argument, not the next argument.

Technically these aren’t really referring to the current argument s. At this point in the loop, all we know about s is that it contains an = sign and that the beginning part of s before = is not a known parameter name. At this point s might be passed by-position if it is compatible with what we know about the next position in the args list (i.e. the positional_args vector). These variables are describing attributes of the next positional parameter. If s is compatible with them, it will be be passed by position, otherwise it will be passed by name.

I can see how next_positional_arg_* might be confusing though and maybe one solution is to just drop these variables and use the expressions directly below. My rewrite of this code in b998cc52d51b48db9271fdba0bd69e9aaccb7999 does not include these variables and includes a fuller explanation of what this code is doing:

https://github.com/bitcoin/bitcoin/blob/b998cc52d51b48db9271fdba0bd69e9aaccb7999/src/rpc/client.cpp#L422-L439

in src/rpc/client.cpp:495 in a4f2144fac outdated

490+             *    in such cases we first parse the whole parameter as JSON and then treat as positional.
491+             */
492+            bool next_positional_arg_is_string = rpcCvtTable.IsStringParamByIndex(strMethod, positional_args.size());
493+            bool next_positional_arg_is_json = rpcCvtTable.IsConvertParamByIndex(strMethod, positional_args.size());
494+            UniValue value;
495+            if(next_positional_arg_is_json && value.read(s)){

achow101 commented at 9:27 pm on August 25, 2025:

In a4f2144facb5d4dc2f749494ea65744fffcb628b “rpc: Handle -named argument parsing where ‘=’ character is used”

Instead of parsing the argument directly, we should be using either Parse or ArgToUniValue

ryanofsky commented at 0:00 am on August 26, 2025:

In a4f2144 “rpc: Handle -named argument parsing where ‘=’ character is used”

Instead of parsing the argument directly, we should be using either Parse or ArgToUniValue

I don’t think you can use Parse or ArgToUniValue unless you catch the exception they throw. No strong opinion, but I think it is probably simpler to use read.

in src/rpc/client.cpp:325 in a4f2144fac outdated

320+
321+/**
322+ * Specify a (method, idx, name) here if the argument is a string RPC
323+ * parameter that should be recognized as a known parameter name but NOT
324+ * converted from JSON. This is used for string parameters that might
325+ * contain '=' character which could interfere with named parameter parsing.

achow101 commented at 9:32 pm on August 25, 2025:

In a4f2144facb5d4dc2f749494ea65744fffcb628b “rpc: Handle -named argument parsing where ‘=’ character is used”

What is the criteria for including a parameter in this table? This text says “parameters that might contain ‘=’ character”, but this includes things that should never have = in them, like sighashtype, estimate_mode, or blockhash. But if the inclusion criteria is basically any string a user can provide, then there are several string parameters that should be included, like all of the other blockhash parameters.

ryanofsky commented at 11:54 pm on August 25, 2025:

re: #32821 (review)

What is the criteria for including a parameter in this table?

Basically if any string parameter for a method can contain =, all the other string parameters for the method should be listed too. My cleanup of this code in b998cc52d51b48db9271fdba0bd69e9aaccb7999 adds a comment explaining this:

https://github.com/bitcoin/bitcoin/blob/b998cc52d51b48db9271fdba0bd69e9aaccb7999/src/rpc/client.cpp#L36-L42

zaidmstrr commented at 6:14 am on August 26, 2025:

There was a @note at around lines 328-330 explaining this behaviour.

achow101 commented at 5:23 pm on August 26, 2025:

My point was more that the criteria appears to be applied inconsistently.

zaidmstrr commented at 5:47 pm on August 26, 2025:

According to my analysis, there is nothing more than specified criteria applied inconsistently in the codebase. Can you point out something?

achow101 commented at 6:06 pm on August 26, 2025:

listsinceblock’s blockhash is included in this list even though block hashes cannot include =. There are also a ton of other RPCs that take a block hash in the same way but are not included in this list.
descriptorprocesspsbt’s sighashtype is a parameter which has a list of fixed string values and none of them have an =. There are multiple RPCs that take a sighashtype but they are not included in this list.
sendmany’s dummy must be an empty string so it is reasonable to expect that no one will put an empty string argument there. There are other RPCs that take a dummy with similar restrictions but are not included in this list.
sendtoaddress’s estimate_mode is a parameter with a list of fixed string values and none of them have an =. There are multiple RPCs that have the same estimate_mode parameter but are not included in this list.

Since these arguments are there, but not all RPCs that have these arguments, is why I ask about the criteria. Either we are including all string parameters because they may contain an =, or we are only including parameters that might reasonably contain a =. In the former, which I think is a reasonable position, the list is incomplete and significant expansion. In the latter, which is also reasonable, the list has too many things in it.

zaidmstrr commented at 6:23 pm on August 26, 2025:

I think now I got what you are trying to say. So the things like blockhash, dummy, sighashtype etc are included because we are not mainly targeting these params; instead, we are targeting methods which might contain a = character. For example, listsinceblock contains a param named label which might contain an = char but according to the rule for consistency, we need to include all other string params related to the same method (listsinceblock). That’s why I included blockhash here, but that doesn’t mean we need to add all other methods which contain blockhash. That’s only needed for this method’s lookup correctness. Due to these rules, I made this table separate from the JSON one; this will make this logic and code more maintainable and readable.

achow101 commented at 6:42 pm on August 26, 2025:

What do you mean by “for consistency”? It should work fine without specifying the other parameters. Requiring that all string parameters need to be added “for consistency” just makes it more annoying to add new RPCs and parameters.

zaidmstrr commented at 6:49 pm on August 26, 2025:

Okay, so to verify my claim, please remove these params from the table and recompile the code, and then run the functional tests with CLI. You will see some errors because of inconsistent behaviour of the class. Meaning the code needs to know about all the string parameters for the given method for correctness. And that’s only needed for RPCs which might contain an = character; the rest can be part of the JSON table without any further rules.

ryanofsky commented at 6:51 pm on August 26, 2025:

Since these arguments are there, but not all RPCs that have these arguments, is why I ask about the criteria. Either we are including all string parameters because they may contain an =, or we are only including parameters that might reasonably contain a =

Any individual string that is allowed to contain an ‘=’ should be to be listed in the string table so the new code in this PR can function and detect that these string arguments should be passed as positional parameters, not named parameters.

But this is not sufficient, because methods can accept multiple string parameters. If a method accepts two string parameters and one of them may contain = and the other is not allowed to contain =, both parameters need to be listed in the table so the code does not make the opposite mistake and treat a named parameter as a positional parameter.

For example, say there is a wallet RPC called with ==mylabel== and type=bech32 as arguments. These are both string parameters and the label parameter can contain = and the type parameter can’t. Both parameters need to be listed in the table. The label one needs to be listed so ==mylabel== is passed by position instead of by name, and the type one needs to be listed so type=bech32 is passed by name instead of by position.

Either all of a method’s string parameters need to be listed in the table or none of them need to be listed for things to work properly.

achow101 commented at 6:55 pm on August 26, 2025:

You will see some errors

Then that is not just “for consistency” and that shouldn’t be listed as the reason for including the other parameters.

Either all of a method’s string parameters need to be listed in the table or none of them need to be listed for things to work properly.

I see. The comment does not explain this clearly.

It seems like this might also make it more difficult to use bitcoin-cli across versions. If bitcoind is a newer version with some RPC that has a new string parameter, it may not be possible to pass that new argument by name with a bitcoin-cli from an older version if the RPC has an entry in this table. (This is more likely to be an issue for developers, at least in my workflow, I typically always use the bitcoin-cli from master even when working on a branch.)

ryanofsky commented at 6:58 pm on August 26, 2025:

I see. The comment does not explain this clearly.

If it helps, as mentioned earlier I wrote a comment which does try to explain this: https://github.com/bitcoin/bitcoin/blob/b998cc52d51b48db9271fdba0bd69e9aaccb7999/src/rpc/client.cpp#L36-L42

ryanofsky commented at 7:33 pm on August 26, 2025:

It seems like this might also make it more difficult to use bitcoin-cli across versions. If bitcoind is a newer version with some RPC that has a new string parameter, it may not be possible to pass that new argument by name with a bitcoin-cli from an older version if the RPC has an entry in this table.

This is true, and we know a similar issue already happens if a new JSON parameter is added: old clients will incorrectly pass that parameter as a string instead of a JSON value. This PR is adding to the problem by not only using the table to determine whether arguments are JSON vs. string, but whether they are positional vs named.

I still think this PR is an improvement though because the positional vs named table lookups only need to be used as a heuristic to disambiguate genuinely ambiguous bitcoin-cli calls. If you want to pass a string argument that contains = by name, you do that 100% reliably by using -named mode and specifying parameter names. If you want to pass the argument by position, you can reliably do that by using -nonamed mode and not specifying parameter names. However, when you make -named calls without specifying parameter names, those calls are genuinely ambiguous, so bitcoin-cli needs to use a heuristic to decide what to do with them. This PR just improves that heuristic.

I do think we may want to think about supporting another syntax for passing named and positional parameters together that is not ambiguous. One syntax could be accepting –name=value arguments and accepting – as a separator for specifying position arguments that begin with –. Another syntax which I always liked was jonasschnelli’s python-like syntax from #20273

ryanofsky commented at 7:14 pm on August 26, 2025: contributor

Code review ACK 6de6c78c676abd1edb96597bfe8b40bd5fbe7122. Just style and naming changes since last review.

Just to restate my opinion of this PR from earlier, I think it is not good to have separate tables for string and json parameters. I think it will make this code harder to maintain and it would be better in long run to have a single table with clearly spelled out rules for what parameters need to be included in the table, as implemented in my suggested code change df4f39143d868fe4303ab7b3875757a26b7ac5a5 (tag) . But I’d be happy to see this PR merged as is and cleanup done as a followup if both things can’t be done in the same PR.

achow101 commented at 7:55 pm on August 26, 2025: member

I think it is not good to have separate tables for string and json parameters.

I would also prefer that.

zaidmstrr force-pushed on Sep 14, 2025

zaidmstrr commented at 9:00 am on September 14, 2025: contributor

Since most of the reviewers are in favour of using a single table instead of two tables for string and convert params. My recent commit does this; the implementation is motivated by the ryanofsky suggested commit b998cc5.

zaidmstrr force-pushed on Sep 14, 2025

zaidmstrr requested review from achow101 on Sep 15, 2025

in src/rpc/client.cpp:319 in 9aaf553971 outdated

379+    { "signmessage", 1, "message", ParamFormat::STRING },
380+    { "signmessagewithprivkey", 1, "message", ParamFormat::STRING },
381+    { "walletpassphrasechange", 0, "oldpassphrase", ParamFormat::STRING },
382+    { "walletpassphrasechange", 1, "newpassphrase", ParamFormat::STRING },
383 };
384-// clang-format on

achow101 commented at 11:00 pm on September 17, 2025:

In 9aaf55397159bb0e839c19d3d4578dfa144a059b “rpc: Handle -named argument parsing where ‘=’ character is used”

This shouldn’t be deleted.

zaidmstrr commented at 10:27 am on September 18, 2025:

Thanks, fixed.

in src/rpc/client.cpp:407 in 9aaf553971 outdated

423+            return p.methodName == method && p.paramName == name;
424+        });
425+
426+        return it == std::end(vRPCConvertParams) ? nullptr : &*it;
427     }
428+

achow101 commented at 11:02 pm on September 17, 2025:

In 9aaf55397159bb0e839c19d3d4578dfa144a059b “rpc: Handle -named argument parsing where ‘=’ character is used”

Extraneous newline

zaidmstrr commented at 10:27 am on September 18, 2025:

Fixed.

in src/rpc/client.cpp:441 in 9aaf553971 outdated

466     UniValue params(UniValue::VARR);
467 
468-    for (unsigned int idx = 0; idx < strParams.size(); idx++) {
469-        std::string_view value{strParams[idx]};
470-        params.push_back(rpcCvtTable.ArgToUniValue(value, strMethod, idx));
471+    for (std::string_view s: strParams) {

achow101 commented at 11:03 pm on September 17, 2025:

In 9aaf55397159bb0e839c19d3d4578dfa144a059b “rpc: Handle -named argument parsing where ‘=’ character is used”

nit: space between s and :

0    for (std::string_view s : strParams) {

zaidmstrr commented at 10:28 am on September 18, 2025:

Fixed.

zaidmstrr force-pushed on Sep 18, 2025

in src/rpc/client.cpp:326 in fbeacfabe8 outdated

388+class RPCConvertTable
389 {
390-    UniValue parsed;
391-    if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw));
392-    return parsed;
393-}

ryanofsky commented at 6:16 pm on September 18, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Could consider just leaving this function above the RPCConvertTable class instead of moving it below, since it is not changing. This would make the diff a little smaller.

zaidmstrr commented at 6:37 pm on September 20, 2025:

Thanks, fixed.

in src/rpc/client.cpp:410 in fbeacfabe8 outdated

426+        return it == std::end(vRPCConvertParams) ? nullptr : &*it;
427     }
428 };
429 
430-CRPCConvertTable::CRPCConvertTable()
431+static const RPCConvertTable g_rpc_convert_table;

ryanofsky commented at 6:20 pm on September 18, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Could consider dropping the RPCConvertTable class and g_rpc_convert_table global variable. The RPCConvertTable class has no state and only has two standalone methods which could be plain functions.

zaidmstrr commented at 6:37 pm on September 20, 2025:

Although using class has no performance issues, but I think the stateless point is valid, and using namespace here is a better choice to group related functions and for readability. This will also remove indirection function calls.

in src/rpc/client.cpp:45 in fbeacfabe8 outdated

44+ * needed for string parameters that are guaranteed not to contain an '=' character.
45+ *
46+ * IMPORTANT: If one string parameter is listed for a method, other string
47+ * parameters for that method need to be listed as well so bitcoin-cli does not
48+ * make the opposite mistake and pass other arguments by position instead of
49+ * name because it does not recognize their names.

ryanofsky commented at 8:57 pm on September 18, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Just IMO, but I feel like the example is confusing because “key=store” looks like a case where “key” actually is a named parameter and = sign should be parsed, not a case where it shouldn’t be parsed. Also the next two sentences seem to just be repeating what the first two sentences say without out adding anything new. And I’m not sure the “IMPORTANT” section is really so important. I think the comment starts with a good one-line summary, and after that things listed go from most to least important. The important requirement is to list JSON parameters, everything else just provides nicer behavior when ambiguous -named command lines are used.

I suggested some documentation edits addressing this in da919fb63c519ac91c9704c58f1bbe522d7a8879 (tag) in case it might be useful.

zaidmstrr commented at 6:39 pm on September 20, 2025:

Thanks for suggesting. Fixed in the latest commit.

in src/rpc/client.cpp:473 in fbeacfabe8 outdated

470+             * 1. The RPC method was listed in the vRPCConvertParams table with a given index, but the parameter is not a known parameter
471+             *    and has a param format STRING; in such cases we pass the whole parameter as positional.
472+             *
473+             * 2. The RPC method was listed in the vRPCConvertParams table with a given index, but the parameter is not a known parameter
474+             *    and has a param format JSON; in such cases we first parse the whole parameter as JSON and then treat as positional.
475+             */

ryanofsky commented at 9:00 pm on September 18, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Maybe consider deleting this comment. It seems like it is mostly just repeating what code is doing without explaining why it is doing these things. If you do think a comment would be helpful here, probably the most useful thing might be some examples of the things being parsed. I suggested some documentation with examples in da919fb63c519ac91c9704c58f1bbe522d7a8879 (tag) in case it might be useful.

zaidmstrr commented at 6:41 pm on September 20, 2025:

Agreed. Now in the latest commit, I removed this and shifted it above the function with more descriptive info and an example command.

in src/rpc/client.cpp:17 in fbeacfabe8 outdated

11@@ -12,25 +12,47 @@
12 #include <string>
13 #include <string_view>
14 
15+enum ParamFormat { JSON, STRING };

ryanofsky commented at 9:06 pm on September 18, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (fbeacfabe802c24b427bee2cd82da1f12ef9899d)

Note: with #33230 this could have JSON_OR_STRING added as well

zaidmstrr commented at 3:05 pm on September 24, 2025:

It makes sense to add JSON_OR_STRING and related parsing logic after this is merged.

ryanofsky approved

ryanofsky commented at 9:11 pm on September 18, 2025: contributor

Code review ACK d1be44d3a672fc343eb3e64f4c0fea1a7e3030aa. Thanks for the update. I do feel like this is simpler with just one table. I left some suggestions below which are mostly about documentation and not important.

zaidmstrr force-pushed on Sep 20, 2025

in src/rpc/client.cpp:394 in 8eafb1e9ee outdated

399-public:
400-    CRPCConvertTable();
401-
402-    /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
403-    UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx)
404+    const CRPCConvertParam* FromPosition(std::string_view method, int pos)

ryanofsky commented at 6:52 pm on September 22, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)

Note: namespace contents is typically not indented: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c

I also think entire namespace could be replaced with a ParamFind function

https://github.com/bitcoin/bitcoin/blob/da919fb63c519ac91c9704c58f1bbe522d7a8879/src/rpc/client.cpp#L384-L389

Which would deduplicate and remove a level of indirection from this code. But feel free to keep current version if preferred.

zaidmstrr commented at 1:57 pm on September 24, 2025:

I think using namespace is better here, as it provides clear intent of both the functions and how they are related. Also I find more readability using this approach.

in src/rpc/client.cpp:451 in 8eafb1e9ee outdated

470+ *      is listed as a ParamFormat::STRING, the entire "PARAM_NAME=PARAM_VALUE" string is treated
471+ *      as a single positional string value.
472+ *
473+ * For example, the command `bitcoin-cli -named createwallet "my=wallet"`, the
474+ * parser initially sees "my=wallet" and attempts to process it as a named
475+ * parameter with PARAM_NAME="my". When it finds that "my" is not a valid

ryanofsky commented at 7:09 pm on September 22, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)

Woud suggest replacing parameter with PARAM_NAME="my". with parameter named "my" so this is shorter and more understandable without the previous context.

zaidmstrr commented at 1:53 pm on September 24, 2025:

Seems reasonable.

in src/rpc/client.cpp:447 in 8eafb1e9ee outdated

466+ *      expected positional parameter is listed as ParamFormat::JSON. If so, and if the entire
467+ *      "PARAM_NAME=PARAM_VALUE" string parses as valid JSON, it's treated as positional.
468+ *
469+ *   2. If the PARAM_NAME is not a known parameter name and the next expected positional parameter
470+ *      is listed as a ParamFormat::STRING, the entire "PARAM_NAME=PARAM_VALUE" string is treated
471+ *      as a single positional string value.

ryanofsky commented at 7:12 pm on September 22, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (8eafb1e9ee4fff3a9ab4a7dc899bd2d57c29d04f)

New example below is nice, and feel free to ignore this feedback, but IMO lines 437-447 are not as good as the earlier suggestion for a few reasons:

They is saying what code is doing without reasons for doing those things.
They are not acknowledging the -named syntax is ambiguous, which seems an important and non-obvious fact.
The first two sentences seem to contradict each other
PARAM_NAME and PARAM_VALUE look and sound similar and seem to distinguish than NAME and VALUE
Examples are dropped from the two special cases and there is no indication of why they are present.

For reference, earlier suggestion was:

https://github.com/bitcoin/bitcoin/blob/da919fb63c519ac91c9704c58f1bbe522d7a8879/src/rpc/client.cpp#L432-L447

zaidmstrr commented at 1:49 pm on September 24, 2025:

Thanks, I implemented the suggested changes.

ryanofsky approved

ryanofsky commented at 7:16 pm on September 22, 2025: contributor

Code review ACK ad6d740623f01bbe90cbf328590ccfae602c2a3f, just updating documentation and converting class into a namespace since last review. I like the new example in the RPCConvertNamedValues documentation.

I left some comments below, but feel free to ignore them, they are just explaining previous suggestions not adding anything new.

zaidmstrr force-pushed on Sep 24, 2025

in src/rpc/client.cpp:394 in 03068e67cd outdated

393+namespace rpc_convert
394 {
395-private:
396-    std::set<std::pair<std::string, int>> members;
397-    std::set<std::pair<std::string, std::string>> membersByName;
398+const CRPCConvertParam* FromPosition(std::string_view method, int pos)

ryanofsky commented at 2:50 pm on September 24, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (03068e67cd9a62aa9105ca6a4c971b6261a783c5)

Every place this function is called, it called with a size_t value cast to an int. It would seem nice to make it take an a size_t value directly, since it is a new function. (Could also change the CRPCConvertParam struct to use size_t instead of int but would suggest not doing that to keep the changes minimal.)

zaidmstrr commented at 10:21 am on September 25, 2025:

Seems good. Fixed.

in src/rpc/client.cpp:45 in 03068e67cd outdated

42+ * This prevents `bitcoin-cli` from splitting strings like "my=wallet" into a named
43+ * argument "my" and value "wallet" when the whole string is intended to be a
44+ * single positional argument. And if one string parameter is listed for a method,
45+ * other string parameters for that method need to be listed as well so bitcoin-cli
46+ * does not make the opposite mistake and pass other arguments by position instead of
47+ * name because it does not recognize their names.

ryanofsky commented at 3:01 pm on September 24, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (03068e67cd9a62aa9105ca6a4c971b6261a783c5)

Maybe add a sentence like “See \ref RPCConvertNamedValues for more information on how named and positional arguments are distinguished with -named.” since this comment is only describing what entries need to be added to the table, not exactly how the entries are used.

zaidmstrr commented at 10:21 am on September 25, 2025:

Added in the latest commit.

in src/rpc/client.cpp:452 in 03068e67cd outdated

473+ *   1. The case where NAME is not a known parameter name, and the next
474+ *      positional parameter requires a JSON value, and the argument parses as
475+ *      JSON. E.g. ["list", "with", "="].
476+ *
477+ *   2. The case where NAME is not a known parameter name and the next
478+ *      positional parameter requires a string value. E.g. "==label==".

ryanofsky commented at 3:07 pm on September 24, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (03068e67cd9a62aa9105ca6a4c971b6261a783c5)

Could change “==label==” to “my=wallet” to be consistent with example below. The “my=wallet” example is probably clearer anyway since there is only one = and it’s in the middle of the string, and wallet names are probably more familiar than labels.

zaidmstrr commented at 10:21 am on September 25, 2025:

Done.

ryanofsky approved

ryanofsky commented at 3:11 pm on September 24, 2025: contributor

Code review ACK 6730abb137eed82073814bdb8dbd577e79520b59. Just some comments and indentation changed since last review. Thanks for considering all the suggestions

zaidmstrr force-pushed on Sep 25, 2025

ryanofsky approved

ryanofsky commented at 2:50 pm on September 25, 2025: contributor

Code review ACK 7dd85d13e22f14940cce9ed9a5bbc2afc5c5c2f4. Only change since last review is applying int->size_t and documentation tweaks. Thanks for the update!

DrahtBot added the label Needs rebase on Sep 25, 2025

zaidmstrr force-pushed on Sep 26, 2025

zaidmstrr commented at 3:26 pm on September 26, 2025: contributor

Rebased. And addressed the changes done in [PR #33230](https://github.com/bitcoin/bitcoin/pull/33230).

DrahtBot removed the label Needs rebase on Sep 26, 2025

in src/rpc/client.cpp:54 in 04e7859541 outdated

52+ * Parameters that can be either a JSON value or a string should be listed
53+ * with format `ParamFormat::JSON_OR_STRING`. This makes bitcoin-cli first attempt
54+ * to parse the argument as JSON. If parsing fails, it falls back to treating the
55+ * argument as a raw string. This is useful for arguments like hash_or_height,
56+ * allowing invocations such as `bitcoin-cli getblockstats <hash>` without
57+ * needing to quote the hash string as JSON (`'"<hash>"'`).

ryanofsky commented at 3:21 pm on September 29, 2025:

In commit “rpc: Handle -named argument parsing where ‘=’ character is used” (04e78595410c557c970eb3994a40586231d5b38e)

This new paragraph is nice, but I think it would make sense to move it earlier in the description so JSON parameters (which are more important to list) are covered first, and STRING parameters (which are less important to list) are covered second). This should also be clearer than talking about JSON parameters, then string parameters, then JSON parameters again. Would suggest something like:

 0--- a/src/rpc/client.cpp
 1+++ b/src/rpc/client.cpp
 2@@ -31,8 +31,14 @@ public:
 3  * argument and needs to be converted from JSON, or if it is a string argument
 4  * passed to a method that accepts '=' characters in any string arguments.
 5  *
 6- * JSON parameters need to be listed here to make bitcoin-cli treat command line
 7- * arguments as JSON values instead of strings.
 8+ * JSON parameters need to be listed here to make bitcoin-cli parse command line
 9+ * arguments as JSON, instead of passing them as raw strings. `JSON` and
10+ * `JSON_OR_STRING` formats both make `bitcoin-cli` attempt to parse the
11+ * argument as JSON. But if parsing fails, the former triggers an error while
12+ * the latter falls back to passing the argument as a raw string. This is
13+ * useful for arguments like hash_or_height, allowing invocations such as
14+ * `bitcoin-cli getblockstats <hash>` without needing to quote the hash string
15+ * as JSON (`'"<hash>"'`).
16  *
17  * String parameters that may contain an '=' character (e.g. base64 strings,
18  * filenames, or labels) need to be listed here with format `ParamFormat::STRING`
19@@ -46,13 +52,6 @@ public:
20  * for more information on how named and positional arguments are distinguished with
21  * -named.
22  *
23- * Parameters that can be either a JSON value or a string should be listed
24- * with format `ParamFormat::JSON_OR_STRING`. This makes bitcoin-cli first attempt
25- * to parse the argument as JSON. If parsing fails, it falls back to treating the
26- * argument as a raw string. This is useful for arguments like hash_or_height,
27- * allowing invocations such as `bitcoin-cli getblockstats <hash>` without
28- * needing to quote the hash string as JSON (`'"<hash>"'`).
29- *
30  * [@note](/bitcoin-bitcoin/contributor/note/) Parameter indexes start from 0.
31  */
32 static const CRPCConvertParam vRPCConvertParams[] =

zaidmstrr commented at 3:45 pm on October 1, 2025:

Thanks. Done.

in test/functional/rpc_help.py:50 in 04e7859541 outdated

65+                    m_json = re.search(r'{ *("[^"]*") *, *([0-9]+) *, *("[^"]*") *(?:, *ParamFormat::JSON *)? *}', line)
66+                    if m_json:
67+                        name = parse_string(m_json.group(1))
68+                        idx = int(m_json.group(2))
69+                        argname = parse_string(m_json.group(3))
70+                        cmds.append((name, idx, argname))

ryanofsky commented at 3:26 pm on September 29, 2025:

In commit “test: Add functional tests for named argument parsing” (24391ed7804a724a62034b21150c89e45ac9b625)

It seems like this is getting pretty repetitive. Maybe just match once with a (STRING|JSON_OR_STRING|JSON) pattern?

zaidmstrr commented at 3:44 pm on October 1, 2025:

We can match JSON and JSON_OR_STRING because both are added to cmds list and follow the same rule, and STRING is added to string_params list. Recent changes simplify this.

ryanofsky approved

ryanofsky commented at 3:27 pm on September 29, 2025: contributor

Code review ACK 24391ed7804a724a62034b21150c89e45ac9b625. Just rebased after #33230

in test/functional/wallet_labels.py:66 in 24391ed780

61+        address = result.strip()
62+        addr_info = node.getaddressinfo(address)
63+        assert_equal(addr_info.get('labels', []), [label_with_equals])
64+
65+        # Test that unknown parameter with '=' gets treated as positional only if the next
66+        # positional parameter is known and accepts a string or valid JSON.

ryanofsky commented at 1:02 pm on September 30, 2025:

In commit “test: Add functional tests for named argument parsing” (24391ed7804a724a62034b21150c89e45ac9b625)

Not important, but I feel like “only if” in this comment is a little misleading, since the test is not confirming parameter is treated as positional only in these cases. Also it is not checking the “valid JSON” case at all. Could also be nice to replace comments in this function with log.info or log.debug calls. And it might be nice to make this example mirror the my=wallet example mentioned in client.cpp. Maybe consider updating this to something like:

0self.log.info("Test bitcoin-cli -named passes parameter containing '=' by position if it does not specify a known parameter name and is in a string position")
1equals_label = "my=label"
2result = node.cli("-named", "getnetaddress", equals_label).send_cli()

zaidmstrr commented at 3:45 pm on October 1, 2025:

Agreed. Now fixed.

in src/rpc/client.cpp:408 in 24391ed780 outdated

411-    std::map<std::pair<std::string, int>, bool> members;
412-    std::map<std::pair<std::string, std::string>, bool> membersByName;
413+const CRPCConvertParam* FromPosition(std::string_view method, size_t pos)
414+{
415+    auto it = std::ranges::find_if(vRPCConvertParams, [&](const auto& p) {
416+        return p.methodName == method && p.paramIdx == static_cast<int>(pos);

Prabhat1308 commented at 8:53 pm on September 30, 2025:

This feels a little wierd . can maybe change it to something like p.methodName == rpcMethodName or p.rpcMethod == method

zaidmstrr commented at 3:46 pm on October 1, 2025:

Since the definition of both the FromPosition and FromName functions tells that they’re returning the pointer to the CRPCConvertParam class, thus the parameter names here represent the true meaning in their names according to me. And here p.rpcMethod == method if we change methodName to rpcMethod we need to change the existing name of the member of the class.

in src/rpc/client.cpp:417 in 24391ed780 outdated

431-        return arg_value;
432-    }
433+const CRPCConvertParam* FromName(std::string_view method, std::string_view name)
434+{
435+    auto it = std::ranges::find_if(vRPCConvertParams, [&](const auto& p) {
436+        return p.methodName == method && p.paramName == name;

Prabhat1308 commented at 8:53 pm on September 30, 2025:

Same here

in src/rpc/client.cpp:411 in 24391ed780 outdated

416+        return p.methodName == method && p.paramIdx == static_cast<int>(pos);
417+    });
418 
419-public:
420-    CRPCConvertTable();
421+    return it == std::end(vRPCConvertParams) ? nullptr : &*it;

Prabhat1308 commented at 8:55 pm on September 30, 2025:

maybe change here to &(*it) ? just looks a bit cleaner.

zaidmstrr commented at 3:54 pm on October 1, 2025:

As we are not obtaining some value from the iterator the current implementation seems simpler.

in src/rpc/client.cpp:420 in 24391ed780 outdated

444-            return Parse(arg_value, it->second);
445-        }
446-        return arg_value;
447-    }
448-};
449+    return it == std::end(vRPCConvertParams) ? nullptr : &*it;

Prabhat1308 commented at 8:56 pm on September 30, 2025:

similar change to &(*it) here

Prabhat1308 commented at 9:00 pm on September 30, 2025: contributor

Code Review ACK 24391ed

Just left a few non-blocking nits

rpc: Handle -named argument parsing where '=' character is used 694f04e2bd

zaidmstrr force-pushed on Oct 1, 2025

test: Add functional tests for named argument parsing f53dbbc505

zaidmstrr force-pushed on Oct 1, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)