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 adding a vRPCStringParams 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

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:

#33230 (cli: Handle arguments that can be either JSON or string 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.

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:378 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.

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

test: Add functional tests for named argument parsing 6de6c78c67

zaidmstrr force-pushed on Aug 22, 2025

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

Code Coverage & Benchmarks

Reviews

Conflicts