RPC: Accept options as named-only parameters

[ryanofsky]

Allow RPC methods which take an options parameter (importmulti, listunspent, fundrawtransaction, bumpfee, send, sendall, walletcreatefundedpsbt, simulaterawtransaction), to accept the options as named parameters, without the need for nested JSON objects.

This makes it possible to make calls like:

src/bitcoin-cli -named bumpfee txid fee_rate=10

instead of

src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 10}'

RPC help is also updated to show options as top level named arguments instead of as nested objects.

diff

@@ -15,16 +15,17 @@
 
 Arguments:
 1. txid                           (string, required) The txid to be bumped
-2. options                        (json object, optional)
+2. options                        (json object, optional) Options object that can be used to pass named arguments, listed below.
+                 
+Named Arguments:
-     {
-       "conf_target": n,          (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
+conf_target                       (numeric, optional, default=wallet -txconfirmtarget) Confirmation target in blocks
 
-       "fee_rate": amount,        (numeric or string, optional, default=not set, fall back to wallet fee estimation) 
+fee_rate                          (numeric or string, optional, default=not set, fall back to wallet fee estimation) 
                                   Specify a fee rate in sat/vB instead of relying on the built-in fee estimator.
                                   Must be at least 1.000 sat/vB higher than the current transaction fee rate.
                                   WARNING: before version 0.21, fee_rate was in BTC/kvB. As of 0.21, fee_rate is in sat/vB.
                                   
-       "replaceable": bool,       (boolean, optional, default=true) Whether the new transaction should still be
+replaceable                       (boolean, optional, default=true) Whether the new transaction should still be
                                   marked bip-125 replaceable. If true, the sequence numbers in the transaction will
                                   be left unchanged from the original. If false, any input sequence numbers in the
                                   original transaction that were less than 0xfffffffe will be increased to 0xfffffffe
@@ -32,11 +33,10 @@
                                   still be replaceable in practice, for example if it has unconfirmed ancestors which
                                   are replaceable).
                                   
-       "estimate_mode": "str",    (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
+estimate_mode                     (string, optional, default="unset") The fee estimate mode, must be one of (case insensitive):
                                   "unset"
                                   "economical"
                                   "conservative"
-     }
 
 Result:
 {                    (json object)

Review suggestion: To understand this PR, it is probably easiest to review the commits in reverse order because the last commit shows the external API changes, the middle commit shows the internal API changes, and the first commit contains the low-level implementation.

[ryanofsky]

This is a draft because it needs more unit tests and is not tested very much yet in general. Also probably should have release notes and updates to generated documentation.

UPDATE: These things are completed, so no longer in draft status

[luke-jr]

Concept ACK. Another approach might be #17356. Not sure which is better (17356 is probably more readable code?)

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101
Concept ACK	luke-jr, aureleoules, TheCharlatan
Stale ACK	ajtowns

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:

• #bitcoin-core/gui/733 (Deniability - a tool to automatically improve coin ownership privacy by denavila)
• #27792 (wallet: Deniability API (Unilateral Transaction Meta-Privacy) by denavila)
• #27788 (rpc: Be able to access RPC parameters by name by achow101)
• #27101 (Support JSON-RPC 2.0 when requested by client by pinheadmz)
• #25243 (test: autogenerate bash completion by suhailsaqan)
• #24963 (RPC/Wallet: Convert walletprocesspsbt to use options parameter by luke-jr)
• #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.

[ryanofsky]

@MarcoFalke you may have thoughts on RPCHelpMan changes in this PR. I didn't make any changes to generated documentation yet, but I'm thinking adding a new "Named arguments" section between the existing "Arguments" and "Result" sections:

Arguments:
1. arg1 (type) description
2. arg2 (type) description

Named arguments:
option1 (type) description
option2 (type) description

Result:
result (type) description

For RPC methods that take explicit options arguments (existing RPC methods like like importmulti, listunspent, etc), the documentation lines for those arguments could look like:

3. options (json object, optional) Options object that can be used to pass named arguments

[luke-jr]

Concept ACK on a simple RPC help for "options", but I wonder if there's a better way to avoid duplicating all positional argument docs... Unless your thought was to have only non-positional ones under the Named arguments list?

[ryanofsky]

Concept ACK on a simple RPC help for "options", but I wonder if there's a better way to avoid duplicating all positional argument docs... Unless your thought was to have only non-positional ones under the Named arguments list?

Right, no duplication. Arguments that can be named and positional shown with names and numbers in "Arguments" section. Arguments that can only be passed by name shown without numbers in the "Named arguments"

[ryanofsky]

Rebased e1b5be5 -> fae439b (pr/nonly.3 -> pr/nonly.4, compare) after base PR #19762 merge. Also added unit tests and implemented changes to generated documentation discussed above.
Rebased fae439b -> cea5e37 (pr/nonly.4 -> pr/nonly.5, compare) due to conflict with #26601.

[achow101]

(I wouldn't expect bitcoin-cli to convert the types)

Then this ends up being fairly useless, e.g.:

$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
error code: -3
error message:
JSON value of type null is not of expected type string

[ajtowns]

(I wouldn't expect bitcoin-cli to convert the types)

Then this ends up being fairly useless, e.g.:

$ src/bitcoin-cli -regtest -rpcwallet=funds -named send '[{"bcrt1q4ul7y0p3p9d6sx6atnq9kucxng7jpde9tqfrt6":1}]' change_position=0
error code: -3
error message:
JSON value of type null is not of expected type string

Do you mean JSON value of type string for field change_position is not of expected type number ? (If not, I can't duplicate your error...)

Yeah, that does suck, and I don't see an obvious fix -- presumably we'd want named-only parameters to be treated as json rather than a string (unless overridden), but if it's not overridden, then bitcoin-cli has no way of knowing if change_position here is a positional param or a named one...

[achow101]

Do you mean JSON value of type string for field change_position is not of expected type number ? (If not, I can't duplicate your error...)

Oops, yes, I don't think I was on the HEAD commit.

Yeah, that does suck, and I don't see an obvious fix -- presumably we'd want named-only parameters to be treated as json rather than a string (unless overridden), but if it's not overridden, then bitcoin-cli has no way of knowing if change_position here is a positional param or a named one...

The obvious fix is to update the table src/rpc/client.cpp to contain all of the named-only parameters as we do for all the other parameters. It's just a bit tedious. They could just use the same position index as the existing options to avoid converting any other positionals.

But also we should probably figure out a better way to deal with the conversions in general.

[ryanofsky]

But also we should probably figure out a better way to deal with the conversions in general.

I think it was an unfortunate decision to hardcode parameter information into the client, and could think of various alternatives.

One alternative could be to avoid the need to process arguments on the client side at all and and add a new exec RPC method that takes an args array of strings and an optional stdin string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An exec method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new -exec or -dumb option to invoke the exec RPC instead of trying to convert arguments to JSON itself. Or -exec could just be the default client behavior.

Another alternative could be to have the client download parameter information from the server instead of hardcoding it, maybe through a schema specification like https://open-rpc.org/getting-started. Probably the client would need to cache this information to avoid a performance penalty, and it could be complicated deciding when to invalidate the cache. Though in the most common case when the client is using a cookie file, the server could just write an rpc-schema.json file next to the cookie and the client could just use that without downloading anything.

[achow101]

One alternative could be to avoid the need to process arguments on the client side at all and and add a new exec RPC method that takes an args array of strings and an optional stdin string parameter to figure out on the server side where there is full type information what RPC method to call and how to convert the string args to JSON. An exec method could also do more interpretation like #20273 if that is desirable. The bitcoin client could have a new -exec or -dumb option to invoke the exec RPC instead of trying to convert arguments to JSON itself. Or -exec could just be the default client behavior.

Could we just have the server automatically convert the arguments as needed? This can still be done before the RPC execution since we have type information in the RPCHelpMan.

[ryanofsky]

Could we just have the server automatically convert the arguments as needed? This can still be done before the RPC execution since we have type information in the RPCHelpMan.

Oh you're right. That seems like a better idea, simpler than either of my suggestions. And I think it could allow getting rid of the vRPCConvertParams table entirely?

I think the only possible downside of having the server automatically convert unexpected string parameters to JSON numbers/bools/objects would be if it reduced type checking for other RPC clients that are more strongly typed. For example if the python client were allowed to pass "true" instead of True or "3" instead of 3, it could make python code more fragile and less safe. But the looser type checking could be controlled with an header to avoid applying it to all clients.

(This idea actually seems like an mirror version of #15448, which changed the client to pass unparseable JSON arguments as strings instead of raising an error. This suggestion would change the server to parse unexpected string arguments as JSON instead of raising an error.)

[achow101]

@ryanofsky Are you planning on making any further changes here?

[ryanofsky]

@ryanofsky Are you planning on making any further changes here?

I'm not working on anything. The only suggestion I saw was to rename the fr variable, but that is a preexisting variable so would prefer not to change that here.

[achow101]

Ah okay, I wasn't sure if you were going to add the named-only parameters to the conversion table, but based on the above discussion, it sounds like we're going to move towards eliminating the table in a followup.

ACK 2808c33

[ryanofsky]

I wasn't sure if you were going to add the named-only parameters to the conversion table

Sorry, I just missed the part of your earlier comment where you suggested to this. I didn't realize the conversion table could handle parameters not passed by position. Should be fixed now though.

Updated 2808c33 -> 5559ad2 (pr/nonly.15 -> pr/nonly.16, compare) with rpc/client.cpp entries

[achow101]

It looks like the conversion table is missing the arguments that take amounts; these also need to be converted. The test is also failing as the named only arguments are not being output in the dump_all_command_conversions, the regex can't handle negatives, and psbt has mismatched conversions.

The following diff should fix those issues:

diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index be4d777169..7bd66ebbd2 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -101,7 +101,10 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listunspent", 2, "addresses" },
     { "listunspent", 3, "include_unsafe" },
     { "listunspent", 4, "query_options" },
+    { "listunspent", -1, "minimumAmount" },
+    { "listunspent", -1, "maximumAmount" },
     { "listunspent", -1, "maximumCount" },
+    { "listunspent", -1, "minimumSumAmount" },
     { "listunspent", -1, "include_immature_coinbase" },
     { "getblock", 1, "verbosity" },
     { "getblock", 1, "verbose" },
@@ -134,6 +137,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "fundrawtransaction", -1, "changePosition"},
     { "fundrawtransaction", -1, "includeWatching"},
     { "fundrawtransaction", -1, "lockUnspents"},
+    { "fundrawtransaction", -1, "fee_rate"},
+    { "fundrawtransaction", -1, "feeRate"},
     { "fundrawtransaction", -1, "subtractFeeFromOutputs"},
     { "fundrawtransaction", -1, "input_weights"},
     { "fundrawtransaction", -1, "conf_target"},
@@ -151,6 +156,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "walletcreatefundedpsbt", -1, "changePosition"},
     { "walletcreatefundedpsbt", -1, "includeWatching"},
     { "walletcreatefundedpsbt", -1, "lockUnspents"},
+    { "walletcreatefundedpsbt", -1, "fee_rate"},
+    { "walletcreatefundedpsbt", -1, "feeRate"},
     { "walletcreatefundedpsbt", -1, "subtractFeeFromOutputs"},
     { "walletcreatefundedpsbt", -1, "conf_target"},
     { "walletcreatefundedpsbt", -1, "replaceable"},
@@ -186,6 +193,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "send", -1, "maxconf"},
     { "send", -1, "add_to_wallet"},
     { "send", -1, "change_position"},
+    { "send", -1, "fee_rate"},
     { "send", -1, "include_watching"},
     { "send", -1, "inputs"},
     { "send", -1, "locktime"},
@@ -200,6 +208,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "sendall", 3, "fee_rate"},
     { "sendall", 4, "options" },
     { "sendall", -1, "add_to_wallet"},
+    { "sendall", -1, "fee_rate"},
     { "sendall", -1, "include_watching"},
     { "sendall", -1, "inputs"},
     { "sendall", -1, "locktime"},
@@ -244,10 +253,12 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "gettxspendingprevout", 0, "outputs" },
     { "bumpfee", 1, "options" },
     { "bumpfee", -1, "conf_target"},
+    { "bumpfee", -1, "fee_rate"},
     { "bumpfee", -1, "replaceable"},
     { "bumpfee", -1, "outputs"},
     { "psbtbumpfee", 1, "options" },
     { "psbtbumpfee", -1, "conf_target"},
+    { "psbtbumpfee", -1, "fee_rate"},
     { "psbtbumpfee", -1, "replaceable"},
     { "psbtbumpfee", -1, "outputs"},
     { "logging", 0, "include" },
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index c35706ce24..2a1061d49f 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -706,17 +706,30 @@ std::string RPCHelpMan::ToString() const
 UniValue RPCHelpMan::GetArgMap() const
 {
     UniValue arr{UniValue::VARR};
+
+    auto push_back_arg_info = [&arr](const std::string& rpc_name, int pos, const std::string& arg_name, const RPCArg::Type& type) {
+        UniValue map{UniValue::VARR};
+        map.push_back(rpc_name);
+        map.push_back(pos);
+        map.push_back(arg_name);
+        map.push_back(type == RPCArg::Type::STR ||
+                      type == RPCArg::Type::STR_HEX);
+        arr.push_back(map);
+    };
+
     for (int i{0}; i < int(m_args.size()); ++i) {
         const auto& arg = m_args.at(i);
         std::vector<std::string> arg_names = SplitString(arg.m_names, '|');
         for (const auto& arg_name : arg_names) {
-            UniValue map{UniValue::VARR};
-            map.push_back(m_name);
-            map.push_back(i);
-            map.push_back(arg_name);
-            map.push_back(arg.m_type == RPCArg::Type::STR ||
-                          arg.m_type == RPCArg::Type::STR_HEX);
-            arr.push_back(map);
+            push_back_arg_info(m_name, i, arg_name, arg.m_type);
+            if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
+                for (const auto& inner : arg.m_inner) {
+                    std::vector<std::string> inner_names = SplitString(inner.m_names, '|');
+                    for (const std::string& inner_name : inner_names) {
+                        push_back_arg_info(m_name, -1, inner_name, inner.m_type);
+                    }
+                }
+            }
         }
     }
     return arr;
diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py
index 7acc3cbbd5..dbd4b3047e 100755
--- a/test/functional/rpc_help.py
+++ b/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]+) *, *("[^"]*") *},', line)
                     assert m, 'No match to table expression: %s' % line
                     name = parse_string(m.group(1))
                     idx = int(m.group(2))
@@ -85,8 +85,8 @@ class HelpRpcTest(BitcoinTestFramework):
 
         for argname, convert in converts_by_argname.items():
             if all(convert) != any(convert):
-                # Only allow dummy to fail consistency check
-                assert argname == 'dummy', ('WARNING: conversion mismatch for argument named %s (%s)' % (argname, list(zip(all_methods_by_argname[argname], converts_by_argname[argname]))))
+                # 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_categories(self):
         node = self.nodes[0]

[ryanofsky]

It looks like the conversion table is missing the arguments that take amounts; these also need to be converted.

Thank you! Will apply patch. I'm surprised amounts need to be converted, though. I thought AmountFromValue handled both numbers and strings so strings would be fine.

[achow101]

I'm surprised amounts need to be converted, though

It actually may just be the test that checks the conversion table that requires it since dump_all_command_conversions marks amounts as needed to be converted. However converting amounts is also consistent with all of the other RPCs that accept amounts as arguments, even prior to that test being added.

[ryanofsky]

Updated 5559ad2 -> 2cd28e9 (pr/nonly.16 -> pr/nonly.17, compare) with suggested test and conversion table fixes. I also changed the code to use actual position of the options parameters as originally suggested, instead of -1 values.

[achow101]

ACK 2cd28e9

Only changes since last are the suggested conversion table changes