rpc: Handle -named argument parsing where '=' character is used #32821
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32821. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK 14e2125. 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 (comment) 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. |
Sure, I'm also thinking of adding a python test, but the main problem is, this issue is only reproducible when there exists
To extend this feature to any string parameters, we simply need to add an entry in the 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. |
You could just manually generate a random PSBT on regtest, hardcode it into a test and call Or you could even just call these methods with |
|
I generalised this change to any string parameter which might contain the |
|
Concept ACK Commit 59fcefc Will review the code soon! |
537c120 to
03c1f0d
Compare
|
From CI: I didn't check the code, but that's usually a cryptic way of saying that signing the psbt failed. |
|
Signing the PSBT works fine while running locally, and removing non-witness UTXO from the PSBT. |
What command did you use to run locally? |
I first checked by adding two log statements in I also ran all the functional test cases with |
|
you'll have to use |
|
Trying to debug the issue I see some irregularities in the how the test behaves in diffdiff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
index a241978b5f..f92e8d6e88 100755
--- a/test/functional/rpc_psbt.py
+++ b/test/functional/rpc_psbt.py
@@ -135,8 +135,64 @@ class PSBTTest(BitcoinTestFramework):
assert "non_witness_utxo" in mining_node.decodepsbt(psbt_new.to_base64())["inputs"][0]
# Have the offline node sign the PSBT (which will remove the non-witness UTXO)
+
+ decoded_before = mining_node.decodepsbt(psbt_new.to_base64())
+
+ self.log.info(f"=== DEBUG: CLI Mode = {self.options.usecli} ===")
+
+ # Log address and input details
+ offline_addr_info = offline_node.getaddressinfo(offline_addr)
+ self.log.info(f"Offline address: {offline_addr}")
+ self.log.info(f"Address type: {offline_addr_info.get('type', 'unknown')}")
+
+ decoded_before = mining_node.decodepsbt(psbt_new.to_base64())
+
+ self.log.info(f"=== DEBUG: CLI Mode = {self.options.usecli} ===")
+
+ # Log address and input details
+ offline_addr_info = offline_node.getaddressinfo(offline_addr)
+ self.log.info(f"Offline address: {offline_addr}")
+ self.log.info(f"Address type: {offline_addr_info.get('type', 'unknown')}")
+ self.log.info(f"Script type: {offline_addr_info.get('script', 'unknown')}")
+ self.log.info(f"Is witness: {offline_addr_info.get('iswitness', False)}")
+ self.log.info(f"Witness version: {offline_addr_info.get('witness_version', 'none')}")
+ self.log.info(f"Witness program: {offline_addr_info.get('witness_program', 'none')}")
+
+ # Log PSBT input details before signing
+ self.log.info(f"BEFORE signing - input[0] keys: {list(decoded_before['inputs'][0].keys())}")
+ self.log.info(f"BEFORE signing - has non_witness_utxo: {'non_witness_utxo' in decoded_before['inputs'][0]}")
+ self.log.info(f"BEFORE signing - has witness_utxo: {'witness_utxo' in decoded_before['inputs'][0]}")
+
+ # Check witness_utxo script details
+ if 'witness_utxo' in decoded_before['inputs'][0]:
+ witness_utxo = decoded_before['inputs'][0]['witness_utxo']
+ self.log.info(f"BEFORE signing - witness_utxo script type: {witness_utxo['scriptPubKey'].get('type', 'unknown')}")
+ self.log.info(f"BEFORE signing - witness_utxo hex: {witness_utxo['scriptPubKey'].get('hex', 'none')}")
+
+ # Log the exact walletprocesspsbt call being made
+ psbt_b64 = psbt_new.to_base64()
+ self.log.info(f"walletprocesspsbt call: offline_node.walletprocesspsbt('{psbt_b64[:50]}...', True, 'ALL', True, True)")
+
signed_psbt = offline_node.walletprocesspsbt(psbt_new.to_base64())
- assert not "non_witness_utxo" in mining_node.decodepsbt(signed_psbt["psbt"])["inputs"][0]
+ decoded_after = mining_node.decodepsbt(signed_psbt["psbt"])
+
+ # Log PSBT input details after signing
+ self.log.info(f"AFTER signing - input[0] keys: {list(decoded_after['inputs'][0].keys())}")
+ self.log.info(f"AFTER signing - has non_witness_utxo: {'non_witness_utxo' in decoded_after['inputs'][0]}")
+ self.log.info(f"AFTER signing - has witness_utxo: {'witness_utxo' in decoded_after['inputs'][0]}")
+ self.log.info(f"walletprocesspsbt result complete: {signed_psbt.get('complete', 'not present')}")
+
+ # Check for Taproot vs regular segwit indicators
+ has_taproot_derivs = 'taproot_bip32_derivs' in decoded_before['inputs'][0] or 'taproot_bip32_derivs' in decoded_after['inputs'][0]
+ has_taproot_key = 'taproot_internal_key' in decoded_before['inputs'][0] or 'taproot_internal_key' in decoded_after['inputs'][0]
+ has_regular_derivs = 'bip32_derivs' in decoded_before['inputs'][0] or 'bip32_derivs' in decoded_after['inputs'][0]
+
+ self.log.info(f"Input type detection - Taproot derivs: {has_taproot_derivs}, Taproot key: {has_taproot_key}, Regular derivs: {has_regular_derivs}")
+
+ # Log the actual walletprocesspsbt call details for comparison
+ if self.options.usecli:
+ self.log.info("Using CLI mode - parameters passed as separate arguments")
+ else:
+ self.log.info("Using RPC mode - parameters passed as JSON")
+
+ # More detailed assertion with context
+ if "non_witness_utxo" in decoded_after["inputs"][0]:
+ self.log.error("FAILURE: non_witness_utxo should be removed for segwit v1+ inputs!")
+ self.log.error(f"This suggests the input is being treated as segwit v0 instead of v1+ in CLI mode")
+
+
with cli2025-07-07T15:20:33.852283Z TestFramework (INFO): Check that non-witness UTXOs are removed for segwit v1+ inputs
2025-07-07T15:20:34.004885Z TestFramework (INFO): === DEBUG: CLI Mode = True ===
2025-07-07T15:20:34.008380Z TestFramework (INFO): Offline address: bcrt1q2skwujcxhaawtf0hlqawfmk3gclpdz24a84znw
2025-07-07T15:20:34.008437Z TestFramework (INFO): Address type: unknown
2025-07-07T15:20:34.008459Z TestFramework (INFO): Script type: unknown
2025-07-07T15:20:34.008479Z TestFramework (INFO): Is witness: True
2025-07-07T15:20:34.008497Z TestFramework (INFO): Witness version: 0
2025-07-07T15:20:34.008513Z TestFramework (INFO): Witness program: 542cee4b06bf7ae5a5f7f83ae4eed1463e168955
2025-07-07T15:20:34.008532Z TestFramework (INFO): BEFORE signing - input[0] keys: ['witness_utxo', 'non_witness_utxo', 'bip32_derivs']
2025-07-07T15:20:34.008550Z TestFramework (INFO): BEFORE signing - has non_witness_utxo: True
2025-07-07T15:20:34.008565Z TestFramework (INFO): BEFORE signing - has witness_utxo: True
2025-07-07T15:20:34.008579Z TestFramework (INFO): BEFORE signing - witness_utxo script type: witness_v0_keyhash
2025-07-07T15:20:34.008595Z TestFramework (INFO): BEFORE signing - witness_utxo hex: 0014542cee4b06bf7ae5a5f7f83ae4eed1463e168955
2025-07-07T15:20:34.008688Z TestFramework (INFO): walletprocesspsbt call: offline_node.walletprocesspsbt('cHNidP8BAFICAAAAAQVChcz/DF+d62weFt4XJAYjd8cLM81FZV...', True, 'ALL', True, True)
2025-07-07T15:20:34.016020Z TestFramework (INFO): AFTER signing - input[0] keys: ['witness_utxo', 'non_witness_utxo', 'final_scriptwitness']
2025-07-07T15:20:34.016093Z TestFramework (INFO): AFTER signing - has non_witness_utxo: True
2025-07-07T15:20:34.016117Z TestFramework (INFO): AFTER signing - has witness_utxo: True
2025-07-07T15:20:34.016135Z TestFramework (INFO): walletprocesspsbt result complete: True
2025-07-07T15:20:34.016155Z TestFramework (INFO): Input type detection - Taproot derivs: False, Taproot key: False, Regular derivs: True
2025-07-07T15:20:34.016174Z TestFramework (INFO): Using CLI mode - parameters passed as separate arguments
2025-07-07T15:20:34.016193Z TestFramework (ERROR): FAILURE: non_witness_utxo should be removed for segwit v1+ inputs!
2025-07-07T15:20:34.016209Z TestFramework (ERROR): This suggests the input is being treated as segwit v0 instead of v1+ in CLI mode
2025-07-07T15:20:34.016234Z TestFramework (ERROR): Assertion failedwithout cli2025-07-07T15:19:10.430768Z TestFramework (INFO): === DEBUG: CLI Mode = False ===
2025-07-07T15:19:10.431566Z TestFramework (INFO): Offline address: bcrt1pxmju835wnuw3z5thftuz84q8rt96vqd97jaxwdn2uqlxwwka923q8fcsp8
2025-07-07T15:19:10.431613Z TestFramework (INFO): Address type: unknown
2025-07-07T15:19:10.431645Z TestFramework (INFO): Script type: unknown
2025-07-07T15:19:10.431672Z TestFramework (INFO): Is witness: True
2025-07-07T15:19:10.431699Z TestFramework (INFO): Witness version: 1
2025-07-07T15:19:10.431729Z TestFramework (INFO): Witness program: 36e5c3c68e9f1d1151774af823d4071acba601a5f4ba67366ae03e673add2aa2
2025-07-07T15:19:10.431760Z TestFramework (INFO): BEFORE signing - input[0] keys: ['witness_utxo', 'non_witness_utxo', 'taproot_bip32_derivs', 'taproot_internal_key']
2025-07-07T15:19:10.431789Z TestFramework (INFO): BEFORE signing - has non_witness_utxo: True
2025-07-07T15:19:10.431831Z TestFramework (INFO): BEFORE signing - has witness_utxo: True
2025-07-07T15:19:10.431857Z TestFramework (INFO): BEFORE signing - witness_utxo script type: witness_v1_taproot
2025-07-07T15:19:10.431881Z TestFramework (INFO): BEFORE signing - witness_utxo hex: 512036e5c3c68e9f1d1151774af823d4071acba601a5f4ba67366ae03e673add2aa2
2025-07-07T15:19:10.431929Z TestFramework (INFO): walletprocesspsbt call: offline_node.walletprocesspsbt('cHNidP8BAF4CAAAAAVteg2JVDVvQHLSH4zmmgu7l4bYJdyIoFq...', True, 'ALL', True, True)
2025-07-07T15:19:10.433261Z TestFramework (INFO): AFTER signing - input[0] keys: ['witness_utxo', 'final_scriptwitness']
2025-07-07T15:19:10.433305Z TestFramework (INFO): AFTER signing - has non_witness_utxo: False
2025-07-07T15:19:10.433336Z TestFramework (INFO): AFTER signing - has witness_utxo: True
2025-07-07T15:19:10.433364Z TestFramework (INFO): walletprocesspsbt result complete: True
2025-07-07T15:19:10.433399Z TestFramework (INFO): Input type detection - Taproot derivs: True, Taproot key: True, Regular derivs: False
2025-07-07T15:19:10.433426Z TestFramework (INFO): Using RPC mode - parameters passed as JSON
|
There was a problem hiding this comment.
Code review ACK 56b1da7. 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 df4f391 (tag) to combine
vRPCConvertParamsandvRPCStringParamstables 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.
You are right here, I will add a test for non-string params like I intentionally choose this approach to use both the |
There was a problem hiding this comment.
Code review ACK 8fa1a0e. 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.
|
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. |
I'm also not a fan of separate tables and suggested the following change to unify them earlier: b998cc5 (tag). This change is just a refactoring and could be a followup.
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 |
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. |
That's basically what the PR does implicitly and what my refactoring of the PR does more explicitly with string/json formats in 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
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. |
Yes, but I meant more so in having access to Or if we had a machine parseable schema thing (see #29912) that bitcon-cli refers to. |
|
Rebased on top latest master. |
There was a problem hiding this comment.
Code review ACK 780bec6, just rebased since last review to avoid minor conflict with #31886
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. |
| class RpcStringParam { | ||
| public: | ||
| std::string method_name; //!< method name this parameter belongs to | ||
| int param_idx; //!< 0-based idx of param | ||
| std::string param_name; //!< parameter name | ||
| }; |
There was a problem hiding this comment.
In a4f2144 "rpc: Handle -named argument parsing where '=' character is used"
This seems unnecessary, it's the same thing CRPCConvertParam.
There was a problem hiding this comment.
The definition might be the same, but the purpose is entirely different. This is specifically for string RPC methods.
There was a problem hiding this comment.
I don't think the meaning is different enough to warrant a whole new class.
There was a problem hiding this comment.
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.
| std::set<std::pair<std::string, std::string>> stringParams; | ||
| std::set<std::pair<std::string, int>> stringParamsByIndex; |
There was a problem hiding this comment.
In a4f2144 "rpc: Handle -named argument parsing where '=' character is used"
nit: these should use the new naming style.
| bool next_positional_arg_is_string = rpcCvtTable.IsStringParamByIndex(strMethod, positional_args.size()); | ||
| bool next_positional_arg_is_json = rpcCvtTable.IsConvertParamByIndex(strMethod, positional_args.size()); |
There was a problem hiding this comment.
In a4f2144 "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.
There was a problem hiding this comment.
re: #32821 (comment)
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 b998cc5 does not include these variables and includes a fuller explanation of what this code is doing:
Lines 422 to 439 in b998cc5
| bool next_positional_arg_is_string = rpcCvtTable.IsStringParamByIndex(strMethod, positional_args.size()); | ||
| bool next_positional_arg_is_json = rpcCvtTable.IsConvertParamByIndex(strMethod, positional_args.size()); | ||
| UniValue value; | ||
| if(next_positional_arg_is_json && value.read(s)){ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
In a4f2144 "rpc: Handle -named argument parsing where '=' character is used"
Instead of parsing the argument directly, we should be using either
ParseorArgToUniValue
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.
| * converted from JSON. This is used for string parameters that might | ||
| * contain '=' character which could interfere with named parameter parsing. |
There was a problem hiding this comment.
In a4f2144 "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.
There was a problem hiding this comment.
re: #32821 (comment)
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 b998cc5 adds a comment explaining this:
Lines 36 to 42 in b998cc5
There was a problem hiding this comment.
There was a @note at around lines 328-330 explaining this behaviour.
There was a problem hiding this comment.
My point was more that the criteria appears to be applied inconsistently.
There was a problem hiding this comment.
According to my analysis, there is nothing more than specified criteria applied inconsistently in the codebase. Can you point out something?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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:
Lines 36 to 42 in b998cc5
There was a problem hiding this comment.
It seems like this might also make it more difficult to use
bitcoin-cliacross versions. Ifbitcoindis 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 abitcoin-clifrom 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
|
Code review ACK 6de6c78. 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 df4f391 (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. |
I would also prefer that. |
Addresses comment and this.
The PR #31375 got merged and enables
-namedby default in thebitcoin rpcinterface;bitcoin rpccorresponds tobitcoin-cli -namedas 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 arefinalizepsbt,decodepsbt,verifymessageetc.This is the one example of the error in
finalizepsbtRPC:This PR fixes this by adding a
vRPCStringParamstable that identifies parameters that need special handling in-namedparameter mode. The parser now recognises these parameters and handles strings with "=" char correctly, preventing them from being incorrectly split as parameter assignments.