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

f53dbbc505 test: Add functional tests for named argument parsing (zaidmstrr)
694f04e2bd rpc: Handle -named argument parsing where '=' character is used (zaidmstrr)

Pull request description:

  Addresses [comment](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2091886628) and [this](https://github.com/bitcoin/bitcoin/pull/31375#discussion_r2092039999).

  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:
  ```
  ./bitcoin-cli -named -regtest finalizepsbt cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA=
  error code: -8
  error message:
  Unknown 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.

ACKs for top commit:
  ryanofsky:
    Code review ACK f53dbbc505. Just applied comment & test suggestions since last review
  kannapoix:
    Code review ACK: f53dbbc505
  achow101:
    ACK f53dbbc505

Tree-SHA512: 1b517144efeff45a4c4256c27a39ddf187f1d6189d133402a45171678214a10ff2925c31edcfd556d67f85bd26d42f63c528b941b68c9880eab443f2c883e681

test/functional/interface_bitcoin_cli.py

@@ -94,8 +94,30 @@ class TestBitcoinCli(BitcoinTestFramework):
         assert re.match(rf"^{re.escape(self.config['environment']['CLIENT_NAME'])} client.+services nwl2?$", det[0])
         assert not any(line.startswith("Local services:") for line in det)
 
+    def test_echojson_positional_equals(self):
+        """Test JSON parameter parsing containing '=' with -named echojson"""
+        self.log.info("Test JSON parameter parsing containing '=' is handled correctly with -named")
+
+        # This should be treated as a positional JSON argument, not as a named
+        result = self.nodes[0].cli("-named", "echojson", '["key=value"]').send_cli()
+        assert_equal(result, [["key=value"]])
+
+        result = self.nodes[0].cli("-named", "echojson", '["key=value", "another=test"]').send_cli()
+        assert_equal(result, [["key=value", "another=test"]])
+
+        result = self.nodes[0].cli("-named", "echojson", '["data=test"]', "42").send_cli()
+        expected = [["data=test"], 42]
+        assert_equal(result, expected)
+
+        # This should be treated as a named parameter, as arg0 and arg1 are valid parameter names
+        result = self.nodes[0].cli("-named", "echojson", 'arg0=["data=test"]', 'arg1=42').send_cli()
+        expected = [["data=test"], 42]
+        assert_equal(result, expected)
+
     def run_test(self):
         """Main test logic"""
+        self.test_echojson_positional_equals()
+
         self.generate(self.nodes[0], BLOCKS)
 
         self.log.info("Compare responses from getblockchaininfo RPC and `bitcoin-cli getblockchaininfo`")

test/functional/rpc_help.py

@@ -17,30 +17,40 @@ def parse_string(s):
     assert s[-1] == '"'
     return s[1:-1]
 
-
 def process_mapping(fname):
     """Find and parse conversion table in implementation file `fname`."""
     cmds = []
+    string_params = []
     in_rpcs = False
     with open(fname, "r", encoding="utf8") as f:
         for line in f:
             line = line.rstrip()
             if not in_rpcs:
-                if line == 'static const CRPCConvertParam vRPCConvertParams[] =':
+                if re.match(r'static const CRPCConvertParam vRPCConvertParams\[] =', line):
                     in_rpcs = True
             else:
                 if line.startswith('};'):
                     in_rpcs = False
                 elif '{' in line and '"' in line:
-                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *(, \/\*also_string=\*\/true *)?},', line)
-                    assert m, 'No match to table expression: %s' % line
-                    name = parse_string(m.group(1))
-                    idx = int(m.group(2))
-                    argname = parse_string(m.group(3))
-                    cmds.append((name, idx, argname))
-    assert not in_rpcs and cmds
-    return cmds
+                    # Match lines with ParamFormat::STRING
+                    m_string = re.search(r'{ *("[^"]*") *, *([0-9]+) *, *("[^"]*") *, *ParamFormat::STRING *},?', line)
+                    if m_string:
+                        name = parse_string(m_string.group(1))
+                        idx = int(m_string.group(2))
+                        argname = parse_string(m_string.group(3))
+                        string_params.append((name, idx, argname))
+                        continue
 
+                    # Match lines with ParamFormat::JSON and ParamFormat::JSON_OR_STRING
+                    m_json = re.search(r'{ *("[^"]*") *, *([0-9]+) *, *("[^"]*") *(?:, *ParamFormat::(JSON_OR_STRING|JSON))? *},?', line)
+                    if m_json:
+                        name = parse_string(m_json.group(1))
+                        idx = int(m_json.group(2))
+                        argname = parse_string(m_json.group(3))
+                        cmds.append((name, idx, argname))
+
+    assert not in_rpcs
+    return cmds, string_params
 
 class HelpRpcTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -49,6 +59,7 @@ class HelpRpcTest(BitcoinTestFramework):
 
     def run_test(self):
         self.test_client_conversion_table()
+        self.test_client_string_conversion_table()
         self.test_categories()
         self.dump_help()
         if self.is_wallet_compiled():
@@ -56,7 +67,7 @@ class HelpRpcTest(BitcoinTestFramework):
 
     def test_client_conversion_table(self):
         file_conversion_table = os.path.join(self.config["environment"]["SRCDIR"], 'src', 'rpc', 'client.cpp')
-        mapping_client = process_mapping(file_conversion_table)
+        mapping_client, _ = process_mapping(file_conversion_table)
         # Ignore echojson in client table
         mapping_client = [m for m in mapping_client if m[0] != 'echojson']
 
@@ -85,6 +96,29 @@ class HelpRpcTest(BitcoinTestFramework):
                 # Only allow dummy and psbt to fail consistency check
                 assert argname in ['dummy', "psbt"], ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
 
+    def test_client_string_conversion_table(self):
+        file_conversion_table = os.path.join(self.config["environment"]["SRCDIR"], 'src', 'rpc', 'client.cpp')
+        _, string_params_client = process_mapping(file_conversion_table)
+        mapping_server = self.nodes[0].help("dump_all_command_conversions")
+        server_tuples = {tuple(m[:3]) for m in mapping_server}
+
+        # Filter string parameters based on wallet compilation status
+        if self.is_wallet_compiled():
+            # Check that every entry in string parameters exists on the server
+            stale_entries = [entry for entry in string_params_client if entry not in server_tuples]
+            if stale_entries:
+                raise AssertionError(f"String parameters contains entries not present on the server: {stale_entries}")
+            filtered_string_params = string_params_client
+        else:
+            available_string_params = [entry for entry in string_params_client if entry in server_tuples]
+            filtered_string_params = available_string_params
+
+        # Validate that all entries are legitimate server parameters
+        server_method_param_tuples = {(m[0], m[1], m[2]) for m in mapping_server}
+        invalid_entries = [entry for entry in filtered_string_params if entry not in server_method_param_tuples]
+        if invalid_entries:
+            raise AssertionError(f"String parameters contains invalid entries: {invalid_entries}")
+
     def test_categories(self):
         node = self.nodes[0]
 
@@ -128,6 +162,5 @@ class HelpRpcTest(BitcoinTestFramework):
         self.restart_node(0, extra_args=['-nowallet=1'])
         assert 'getnewaddress ( "label" "address_type" )' in self.nodes[0].help('getnewaddress')
 
-
 if __name__ == '__main__':
     HelpRpcTest(__file__).main()

test/functional/rpc_psbt.py

@@ -369,6 +369,43 @@ class PSBTTest(BitcoinTestFramework):
         changepos = psbtx["changepos"]
         assert_equal(decoded_psbt["tx"]["vout"][changepos]["scriptPubKey"]["type"], expected_type)
 
+    def test_psbt_named_parameter_handling(self):
+        """Test that PSBT Base64 parameters with '=' padding are handled correctly in -named mode"""
+        self.log.info("Testing PSBT Base64 parameter handling with '=' padding characters")
+        node = self.nodes[0]
+        psbt_with_padding = "cHNidP8BAJoCAAAAAqvNEjSrzRI0q80SNKvNEjSrzRI0q80SNKvNEjSrzRI0AAAAAAD9////NBLNqzQSzas0Es2rNBLNqzQSzas0Es2rNBLNqzQSzasBAAAAAP3///8CoIYBAAAAAAAWABQVQBGVs/sqFAmC8HZ8O+g1htqivkANAwAAAAAAFgAUir7MzgyzDnRMjdkVa7d+Dwr07jsAAAAAAAAAAAA="
+
+        # Test decodepsbt with explicit named parameter containing '=' padding
+        result = node.cli("-named", "decodepsbt", f"psbt={psbt_with_padding}").send_cli()
+        assert 'tx' in result
+
+        # Test decodepsbt with positional argument containing '=' padding
+        result = node.cli("-named", "decodepsbt", psbt_with_padding).send_cli()
+        assert 'tx' in result
+
+        # Test analyzepsbt with positional argument containing '=' padding
+        result = node.cli("-named", "analyzepsbt", psbt_with_padding).send_cli()
+        assert 'inputs' in result
+
+        # Test finalizepsbt with positional argument containing '=' padding
+        result = node.cli("-named", "finalizepsbt", psbt_with_padding, "extract=true").send_cli()
+        assert 'complete' in result
+
+        # Test walletprocesspsbt with positional argument containing '=' padding
+        result = node.cli("-named", "walletprocesspsbt", psbt_with_padding).send_cli()
+        assert 'complete' in result
+
+        # Test utxoupdatepsbt with positional argument containing '=' padding
+        result = node.cli("-named", "utxoupdatepsbt", psbt_with_padding).send_cli()
+        assert isinstance(result, str) and len(result) > 0
+
+        # Test that unknown parameter with '=' gets treated as positional and return error
+        unknown_psbt_param = "unknown_param_data=more_data="
+        # This should be treated as positional and fail with decode error, not parameter error
+        assert_raises_rpc_error(-22, "TX decode failed invalid base64", node.cli("-named", "finalizepsbt", unknown_psbt_param).send_cli)
+
+        self.log.info("PSBT parameter handling test completed successfully")
+
     def run_test(self):
         # Create and fund a raw tx for sending 10 BTC
         psbtx1 = self.nodes[0].walletcreatefundedpsbt([], {self.nodes[2].getnewaddress():10})['psbt']
@@ -1211,6 +1248,7 @@ class PSBTTest(BitcoinTestFramework):
         if not self.options.usecli:
             self.test_sighash_mismatch()
         self.test_sighash_adding()
+        self.test_psbt_named_parameter_handling()
 
 if __name__ == '__main__':
     PSBTTest(__file__).main()

test/functional/wallet_labels.py

@@ -50,6 +50,27 @@ class WalletLabelsTest(BitcoinTestFramework):
         for rpc_call in rpc_calls:
             assert_raises_rpc_error(-11, "Invalid label name", *rpc_call, label="*")
 
+    def test_label_named_parameter_handling(self):
+        """Test that getnewaddress with labels containing '=' characters is handled correctly in -named mode"""
+        self.log.info("Test getnewaddress label parameter handling")
+        node = self.nodes[0]
+
+        # Test getnewaddress with explicit named parameter containing '='
+        label_with_equals = "wallet=wallet"
+        result = node.cli("-named", "getnewaddress", f"label={label_with_equals}").send_cli()
+        address = result.strip()
+        addr_info = node.getaddressinfo(address)
+        assert_equal(addr_info.get('labels', []), [label_with_equals])
+
+        self.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")
+        equals_label = "my=label"
+        result = node.cli("-named", "getnewaddress", equals_label).send_cli()
+        address = result.strip()
+        addr_info = node.getaddressinfo(address)
+        assert_equal(addr_info.get('labels', []), [equals_label])
+
+        self.log.info("getnewaddress label parameter handling test completed successfully")
+
     def run_test(self):
         # Check that there's no UTXO on the node
         node = self.nodes[0]
@@ -159,6 +180,7 @@ class WalletLabelsTest(BitcoinTestFramework):
         change_label(node, labels[2].addresses[0], labels[2], labels[2])
 
         self.invalid_label_name_test()
+        self.test_label_named_parameter_handling()
 
         # This is a descriptor wallet test because of segwit v1+ addresses
         self.log.info('Check watchonly labels')