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

df67bb6fd8 test: Remove convert_to_json_for_cli (Ava Chow)
44a493e150 cli: Allow arguments to be both strings and json (Ava Chow)

Pull request description:

  There are some RPCs where the argument can be either JSON that needs to be parsed, or a string that we can pass straight through. However, `bitcoin-cli` would always parse those arguments as JSON which makes for some cumbersome argument passing when using those RPCs. Notably, `hash_or_height` in `getblockstats` and `gettxoutsetinfo` do this, and results in a more cumbersome command of `bitcoin-cli getblockstats '"<hash>"'`. Otherwise, using a normal invocation of `bitcoin-cli getblockstats <hash>` results in `error: Error parsing JSON`. This PR marks those particular options as also being a string so that when `bitcoin-cli` fails to parse the argument as JSON, it will assume that the argument is a string and pass it straight through.

ACKs for top commit:
  ryanofsky:
    Code review ACK df67bb6fd8, just rebased since last review. I do still think it would be good to improve the test (https://github.com/bitcoin/bitcoin/pull/33230#discussion_r2369570345)
  rkrux:
    Light code review, lgtm ACK df67bb6fd8
  mzumsande:
    Code Review ACK df67bb6fd8

Tree-SHA512: 6c488570fbb24d0cf10508416c56accfc7af5163b7a7187d22d78c812424a9e3ecc95906d3e295fbf6af54bf80903aa448fd879dd6a9944ba8b4d1a33eb29ef2

src/rpc/client.cpp

@@ -18,6 +18,7 @@ public:
     std::string methodName; //!< method whose params want conversion
     int paramIdx;           //!< 0-based idx of param to convert
     std::string paramName;  //!< parameter name
+    bool also_string{false}; //!< The parameter is also a string
 };
 
 // clang-format off
@@ -188,10 +189,10 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "gettxout", 1, "n" },
     { "gettxout", 2, "include_mempool" },
     { "gettxoutproof", 0, "txids" },
-    { "gettxoutsetinfo", 1, "hash_or_height" },
+    { "gettxoutsetinfo", 1, "hash_or_height", /*also_string=*/true },
     { "gettxoutsetinfo", 2, "use_index"},
     { "dumptxoutset", 2, "options" },
-    { "dumptxoutset", 2, "rollback" },
+    { "dumptxoutset", 2, "rollback", /*also_string=*/true },
     { "lockunspent", 0, "unlock" },
     { "lockunspent", 1, "transactions" },
     { "lockunspent", 2, "persistent" },
@@ -246,7 +247,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listdescriptors", 0, "private" },
     { "verifychain", 0, "checklevel" },
     { "verifychain", 1, "nblocks" },
-    { "getblockstats", 0, "hash_or_height" },
+    { "getblockstats", 0, "hash_or_height", /*also_string=*/true },
     { "getblockstats", 1, "stats" },
     { "pruneblockchain", 0, "height" },
     { "keypoolrefill", 0, "newsize" },
@@ -318,18 +319,21 @@ static const CRPCConvertParam vRPCConvertParams[] =
 // clang-format on
 
 /** Parse string to UniValue or throw runtime_error if string contains invalid JSON */
-static UniValue Parse(std::string_view raw)
+static UniValue Parse(std::string_view raw, bool also_string)
 {
     UniValue parsed;
-    if (!parsed.read(raw)) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw));
+    if (!parsed.read(raw)) {
+        if (!also_string) throw std::runtime_error(tfm::format("Error parsing JSON: %s", raw));
+        return raw;
+    }
     return parsed;
 }
 
 class CRPCConvertTable
 {
 private:
-    std::set<std::pair<std::string, int>> members;
+    std::map<std::pair<std::string, int>, bool> members;
-    std::set<std::pair<std::string, std::string>> membersByName;
+    std::map<std::pair<std::string, std::string>, bool> membersByName;
 
 public:
     CRPCConvertTable();
@@ -337,21 +341,29 @@ public:
     /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
     UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, int param_idx)
     {
-        return members.count({method, param_idx}) > 0 ? Parse(arg_value) : arg_value;
+        const auto& it = members.find({method, param_idx});
+        if (it != members.end()) {
+            return Parse(arg_value, it->second);
+        }
+        return arg_value;
     }
 
     /** Return arg_value as UniValue, and first parse it if it is a non-string parameter */
     UniValue ArgToUniValue(std::string_view arg_value, const std::string& method, const std::string& param_name)
     {
-        return membersByName.count({method, param_name}) > 0 ? Parse(arg_value) : arg_value;
+        const auto& it = membersByName.find({method, param_name});
+        if (it != membersByName.end()) {
+            return Parse(arg_value, it->second);
+        }
+        return arg_value;
     }
 };
 
 CRPCConvertTable::CRPCConvertTable()
 {
     for (const auto& cp : vRPCConvertParams) {
-        members.emplace(cp.methodName, cp.paramIdx);
+        members.emplace(std::make_pair(cp.methodName, cp.paramIdx), cp.also_string);
-        membersByName.emplace(cp.methodName, cp.paramName);
+        membersByName.emplace(std::make_pair(cp.methodName, cp.paramName), cp.also_string);
     }
 }
 

test/functional/feature_assumeutxo.py

@@ -481,7 +481,7 @@ class AssumeutxoTest(BitcoinTestFramework):
 
         # Use a hash instead of a height
         prev_snap_hash = n0.getblockhash(prev_snap_height)
-        dump_output5 = n0.dumptxoutset('utxos5.dat', rollback=self.convert_to_json_for_cli(prev_snap_hash))
+        dump_output5 = n0.dumptxoutset('utxos5.dat', rollback=prev_snap_hash)
         assert_equal(sha256sum_file(dump_output4['path']), sha256sum_file(dump_output5['path']))
 
         # Ensure n0 is back at the tip

test/functional/feature_coinstatsindex.py

@@ -103,7 +103,7 @@ class CoinStatsIndexTest(BitcoinTestFramework):
             assert_equal(res0, res2)
 
             # Fetch old stats by hash
-            res3 = index_node.gettxoutsetinfo(hash_option, self.convert_to_json_for_cli(res0['bestblock']))
+            res3 = index_node.gettxoutsetinfo(hash_option, res0['bestblock'])
             del res3['block_info'], res3['total_unspendable_amount']
             res3.pop('muhash', None)
             assert_equal(res0, res3)
@@ -242,7 +242,7 @@ class CoinStatsIndexTest(BitcoinTestFramework):
         assert_equal(res12, res10)
 
         self.log.info("Test obtaining info for a non-existent block hash")
-        assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height=self.convert_to_json_for_cli("ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"), use_index=True)
+        assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height="ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", use_index=True)
 
     def _test_use_index_option(self):
         self.log.info("Test use_index option for nodes running the index")
@@ -277,7 +277,7 @@ class CoinStatsIndexTest(BitcoinTestFramework):
         assert_not_equal(res["muhash"], res_invalid["muhash"])
 
         # Test that requesting reorged out block by hash is still returning correct results
-        res_invalid2 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=self.convert_to_json_for_cli(reorg_block))
+        res_invalid2 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=reorg_block)
         assert_equal(res_invalid2["muhash"], res_invalid["muhash"])
         assert_not_equal(res["muhash"], res_invalid2["muhash"])
 

test/functional/feature_index_prune.py

@@ -67,7 +67,7 @@ class FeatureIndexPruneTest(BitcoinTestFramework):
         for node in filter_nodes:
             assert_greater_than(len(node.getblockfilter(tip)['filter']), 0)
         for node in stats_nodes:
-            assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=self.convert_to_json_for_cli(tip))['muhash']
+            assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash']
 
         self.generate(self.nodes[0], 500)
         self.sync_index(height=700)
@@ -85,14 +85,14 @@ class FeatureIndexPruneTest(BitcoinTestFramework):
         for node in filter_nodes:
             assert_greater_than(len(node.getblockfilter(tip)['filter']), 0)
         for node in stats_nodes:
-            assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=self.convert_to_json_for_cli(tip))['muhash']
+            assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=tip)['muhash']
 
         self.log.info("check if we can access the blockfilter and coinstats of a pruned block")
         height_hash = self.nodes[0].getblockhash(2)
         for node in filter_nodes:
             assert_greater_than(len(node.getblockfilter(height_hash)['filter']), 0)
         for node in stats_nodes:
-            assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=self.convert_to_json_for_cli(height_hash))['muhash']
+            assert node.gettxoutsetinfo(hash_type="muhash", hash_or_height=height_hash)['muhash']
 
         # mine and sync index up to a height that will later be the pruneheight
         self.generate(self.nodes[0], 51)
@@ -106,7 +106,7 @@ class FeatureIndexPruneTest(BitcoinTestFramework):
             assert_raises_rpc_error(-1, msg, node.getblockfilter, height_hash)
         for node in stats_nodes:
             msg = "Querying specific block heights requires coinstatsindex"
-            assert_raises_rpc_error(-8, msg, node.gettxoutsetinfo, "muhash", self.convert_to_json_for_cli(height_hash))
+            assert_raises_rpc_error(-8, msg, node.gettxoutsetinfo, "muhash", height_hash)
 
         self.generate(self.nodes[0], 749)
 

test/functional/rpc_getblockstats.py

@@ -119,7 +119,7 @@ class GetblockstatsTest(BitcoinTestFramework):
 
             # Check selecting block by hash too
             blockhash = self.expected_stats[i]['blockhash']
-            stats_by_hash = self.nodes[0].getblockstats(hash_or_height=self.convert_to_json_for_cli(blockhash))
+            stats_by_hash = self.nodes[0].getblockstats(hash_or_height=blockhash)
             assert_equal(stats_by_hash, self.expected_stats[i])
 
         # Make sure each stat can be queried on its own
@@ -161,10 +161,10 @@ class GetblockstatsTest(BitcoinTestFramework):
                                 self.nodes[0].getblockstats, hash_or_height=1, stats=['minfee', f'aaa{inv_sel_stat}'])
         # Mainchain's genesis block shouldn't be found on regtest
         assert_raises_rpc_error(-5, 'Block not found', self.nodes[0].getblockstats,
-                                hash_or_height=self.convert_to_json_for_cli('000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f'))
+                                hash_or_height='000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f')
 
         # Invalid number of args
-        assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats, self.convert_to_json_for_cli('00'), 1, 2)
+        assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats, '00', 1, 2)
         assert_raises_rpc_error(-1, 'getblockstats hash_or_height ( stats )', self.nodes[0].getblockstats)
 
         self.log.info('Test block height 0')
@@ -185,7 +185,7 @@ class GetblockstatsTest(BitcoinTestFramework):
         self.log.info("Test when only header is known")
         block = self.generateblock(self.nodes[0], output="raw(55)", transactions=[], submit=False)
         self.nodes[0].submitheader(block["hex"])
-        assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", lambda: self.nodes[0].getblockstats(self.convert_to_json_for_cli(block['hash'])))
+        assert_raises_rpc_error(-1, "Block not available (not fully downloaded)", lambda: self.nodes[0].getblockstats(block['hash']))
 
         self.log.info('Test when block is missing')
         (self.nodes[0].blocks_path / 'blk00000.dat').rename(self.nodes[0].blocks_path / 'blk00000.dat.backup')

test/functional/rpc_help.py

@@ -32,7 +32,7 @@ def process_mapping(fname):
                 if line.startswith('};'):
                     in_rpcs = False
                 elif '{' in line and '"' in line:
-                    m = re.search(r'{ *("[^"]*"), *([0-9]+) *, *("[^"]*") *},', 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))

test/functional/test_framework/test_framework.py

@@ -8,7 +8,6 @@ import configparser
 from enum import Enum
 import argparse
 from datetime import datetime, timezone
-import json
 import logging
 import os
 import platform
@@ -1106,11 +1105,6 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
     def has_blockfile(self, node, filenum: str):
         return (node.blocks_path/ f"blk{filenum}.dat").is_file()
 
-    def convert_to_json_for_cli(self, text):
-        if self.options.usecli:
-            return json.dumps(text)
-        return text
-
     def inspect_sqlite_db(self, path, fn, *args, **kwargs):
         try:
             import sqlite3 # type: ignore[import]
@@ -1121,4 +1115,3 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
             return result
         except ImportError:
             self.log.warning("sqlite3 module not available, skipping tests that inspect the database")
-

test/functional/wallet_reorgsrestore.py

@@ -104,7 +104,7 @@ class ReorgsRestoreTest(BitcoinTestFramework):
 
         # Disconnect tip and sync wallet state
         tip = wallet.getbestblockhash()
-        tip_height = wallet.getblockstats(hash_or_height=self.convert_to_json_for_cli(tip))["height"]
+        tip_height = wallet.getblockstats(hash_or_height=tip)["height"]
         wallet.invalidateblock(tip)
         wallet.syncwithvalidationinterfacequeue()