Merge bitcoin/bitcoin#26485: RPC: Accept options as named-only parameters

2cd28e9 rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
9623314 RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d RPC: Add add OBJ_NAMED_PARAMS type (Ryan Ofsky)

Pull request description:

  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:

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

  instead of

  ```sh
  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.

  <details><summary>diff</summary>
  <p>

  ```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)
  ```

  </p>
  </details>

  **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.

ACKs for top commit:
  achow101:
    ACK 2cd28e9

Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436

Author: achow101

doc/release-notes-26485.md

@@ -0,0 +1,16 @@
+JSON-RPC
+---
+
+For RPC methods which accept `options` parameters ((`importmulti`, `listunspent`, `fundrawtransaction`, `bumpfee`, `send`, `sendall`, `walletcreatefundedpsbt`, `simulaterawtransaction`), it is now possible to pass the options as named parameters without the need for a nested object. (#26485)
+
+This means it is possible make calls like:
+
+```sh
+src/bitcoin-cli -named bumpfee txid fee_rate=100
+```
+
+instead of
+
+```sh
+src/bitcoin-cli -named bumpfee txid options='{"fee_rate": 100}'
+```

src/rpc/client.cpp

@@ -101,6 +101,11 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "listunspent", 2, "addresses" },
     { "listunspent", 3, "include_unsafe" },
     { "listunspent", 4, "query_options" },
+    { "listunspent", 4, "minimumAmount" },
+    { "listunspent", 4, "maximumAmount" },
+    { "listunspent", 4, "maximumCount" },
+    { "listunspent", 4, "minimumSumAmount" },
+    { "listunspent", 4, "include_immature_coinbase" },
     { "getblock", 1, "verbosity" },
     { "getblock", 1, "verbose" },
     { "getblockheader", 1, "verbose" },
@@ -124,11 +129,38 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "submitpackage", 0, "package" },
     { "combinerawtransaction", 0, "txs" },
     { "fundrawtransaction", 1, "options" },
+    { "fundrawtransaction", 1, "add_inputs"},
+    { "fundrawtransaction", 1, "include_unsafe"},
+    { "fundrawtransaction", 1, "minconf"},
+    { "fundrawtransaction", 1, "maxconf"},
+    { "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"},
+    { "fundrawtransaction", 1, "replaceable"},
+    { "fundrawtransaction", 1, "solving_data"},
     { "fundrawtransaction", 2, "iswitness" },
     { "walletcreatefundedpsbt", 0, "inputs" },
     { "walletcreatefundedpsbt", 1, "outputs" },
     { "walletcreatefundedpsbt", 2, "locktime" },
     { "walletcreatefundedpsbt", 3, "options" },
+    { "walletcreatefundedpsbt", 3, "add_inputs"},
+    { "walletcreatefundedpsbt", 3, "include_unsafe"},
+    { "walletcreatefundedpsbt", 3, "minconf"},
+    { "walletcreatefundedpsbt", 3, "maxconf"},
+    { "walletcreatefundedpsbt", 3, "changePosition"},
+    { "walletcreatefundedpsbt", 3, "includeWatching"},
+    { "walletcreatefundedpsbt", 3, "lockUnspents"},
+    { "walletcreatefundedpsbt", 3, "fee_rate"},
+    { "walletcreatefundedpsbt", 3, "feeRate"},
+    { "walletcreatefundedpsbt", 3, "subtractFeeFromOutputs"},
+    { "walletcreatefundedpsbt", 3, "conf_target"},
+    { "walletcreatefundedpsbt", 3, "replaceable"},
+    { "walletcreatefundedpsbt", 3, "solving_data"},
     { "walletcreatefundedpsbt", 4, "bip32derivs" },
     { "walletprocesspsbt", 1, "sign" },
     { "walletprocesspsbt", 3, "bip32derivs" },
@@ -157,18 +189,49 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "send", 1, "conf_target" },
     { "send", 3, "fee_rate"},
     { "send", 4, "options" },
+    { "send", 4, "add_inputs"},
+    { "send", 4, "include_unsafe"},
+    { "send", 4, "minconf"},
+    { "send", 4, "maxconf"},
+    { "send", 4, "add_to_wallet"},
+    { "send", 4, "change_position"},
+    { "send", 4, "fee_rate"},
+    { "send", 4, "include_watching"},
+    { "send", 4, "inputs"},
+    { "send", 4, "locktime"},
+    { "send", 4, "lock_unspents"},
+    { "send", 4, "psbt"},
+    { "send", 4, "subtract_fee_from_outputs"},
+    { "send", 4, "conf_target"},
+    { "send", 4, "replaceable"},
+    { "send", 4, "solving_data"},
     { "sendall", 0, "recipients" },
     { "sendall", 1, "conf_target" },
     { "sendall", 3, "fee_rate"},
     { "sendall", 4, "options" },
+    { "sendall", 4, "add_to_wallet"},
+    { "sendall", 4, "fee_rate"},
+    { "sendall", 4, "include_watching"},
+    { "sendall", 4, "inputs"},
+    { "sendall", 4, "locktime"},
+    { "sendall", 4, "lock_unspents"},
+    { "sendall", 4, "psbt"},
+    { "sendall", 4, "send_max"},
+    { "sendall", 4, "minconf"},
+    { "sendall", 4, "maxconf"},
+    { "sendall", 4, "conf_target"},
+    { "sendall", 4, "replaceable"},
+    { "sendall", 4, "solving_data"},
     { "simulaterawtransaction", 0, "rawtxs" },
     { "simulaterawtransaction", 1, "options" },
+    { "simulaterawtransaction", 1, "include_watchonly"},
     { "importprivkey", 2, "rescan" },
     { "importaddress", 2, "rescan" },
     { "importaddress", 3, "p2sh" },
     { "importpubkey", 2, "rescan" },
     { "importmulti", 0, "requests" },
     { "importmulti", 1, "options" },
+    { "importmulti", 1, "rescan" },
     { "importdescriptors", 0, "requests" },
     { "listdescriptors", 0, "private" },
     { "verifychain", 0, "checklevel" },
@@ -192,7 +255,15 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "getmempooldescendants", 1, "verbose" },
     { "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" },
     { "logging", 1, "exclude" },
     { "disconnectnode", 1, "nodeid" },

src/rpc/server.cpp

@@ -392,7 +392,7 @@ std::string JSONRPCExecBatch(const JSONRPCRequest& jreq, const UniValue& vReq)
  * Process named arguments into a vector of positional arguments, based on the
  * passed-in specification for the RPC call's arguments.
  */
-static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, const std::vector<std::string>& argNames)
+static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, const std::vector<std::pair<std::string, bool>>& argNames)
 {
     JSONRPCRequest out = in;
     out.params = UniValue(UniValue::VARR);
@@ -417,7 +417,9 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
     // "args" parameter, if present.
     int hole = 0;
     int initial_hole_size = 0;
-    for (const std::string &argNamePattern: argNames) {
+    const std::string* initial_param = nullptr;
+    UniValue options{UniValue::VOBJ};
+    for (const auto& [argNamePattern, named_only]: argNames) {
         std::vector<std::string> vargNames = SplitString(argNamePattern, '|');
         auto fr = argsIn.end();
         for (const std::string & argName : vargNames) {
@@ -426,30 +428,58 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
                 break;
             }
         }
-        if (fr != argsIn.end()) {
+
+        // Handle named-only parameters by pushing them into a temporary options
+        // object, and then pushing the accumulated options as the next
+        // positional argument.
+        if (named_only) {
+            if (fr != argsIn.end()) {
+                if (options.exists(fr->first)) {
+                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + fr->first + " specified multiple times");
+                }
+                options.__pushKV(fr->first, *fr->second);
+                argsIn.erase(fr);
+            }
+            continue;
+        }
+
+        if (!options.empty() || fr != argsIn.end()) {
             for (int i = 0; i < hole; ++i) {
                 // Fill hole between specified parameters with JSON nulls,
                 // but not at the end (for backwards compatibility with calls
                 // that act based on number of specified parameters).
                 out.params.push_back(UniValue());
             }
             hole = 0;
-            out.params.push_back(*fr->second);
-            argsIn.erase(fr);
+            if (!initial_param) initial_param = &argNamePattern;
         } else {
             hole += 1;
             if (out.params.empty()) initial_hole_size = hole;
         }
+
+        // If named input parameter "fr" is present, push it onto out.params. If
+        // options are present, push them onto out.params. If both are present,
+        // throw an error.
+        if (fr != argsIn.end()) {
+            if (!options.empty()) {
+                throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + fr->first + " conflicts with parameter " + options.getKeys().front());
+            }
+            out.params.push_back(*fr->second);
+            argsIn.erase(fr);
+        }
+        if (!options.empty()) {
+            out.params.push_back(std::move(options));
+            options = UniValue{UniValue::VOBJ};
+        }
     }
     // If leftover "args" param was found, use it as a source of positional
     // arguments and add named arguments after. This is a convenience for
     // clients that want to pass a combination of named and positional
     // arguments as described in doc/JSON-RPC-interface.md#parameter-passing
     auto positional_args{argsIn.extract("args")};
     if (positional_args && positional_args.mapped()->isArray()) {
-        const bool has_named_arguments{initial_hole_size < (int)argNames.size()};
-        if (initial_hole_size < (int)positional_args.mapped()->size() && has_named_arguments) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + argNames[initial_hole_size] + " specified twice both as positional and named argument");
+        if (initial_hole_size < (int)positional_args.mapped()->size() && initial_param) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + *initial_param + " specified twice both as positional and named argument");
         }
         // Assign positional_args to out.params and append named_args after.
         UniValue named_args{std::move(out.params)};

src/rpc/server.h

@@ -95,7 +95,7 @@ class CRPCCommand
     using Actor = std::function<bool(const JSONRPCRequest& request, UniValue& result, bool last_handler)>;
 
     //! Constructor taking Actor callback supporting multiple handlers.
-    CRPCCommand(std::string category, std::string name, Actor actor, std::vector<std::string> args, intptr_t unique_id)
+    CRPCCommand(std::string category, std::string name, Actor actor, std::vector<std::pair<std::string, bool>> args, intptr_t unique_id)
         : category(std::move(category)), name(std::move(name)), actor(std::move(actor)), argNames(std::move(args)),
           unique_id(unique_id)
     {
@@ -115,7 +115,16 @@ class CRPCCommand
     std::string category;
     std::string name;
     Actor actor;
-    std::vector<std::string> argNames;
+    //! List of method arguments and whether they are named-only. Incoming RPC
+    //! requests contain a "params" field that can either be an array containing
+    //! unnamed arguments or an object containing named arguments. The
+    //! "argNames" vector is used in the latter case to transform the params
+    //! object into an array. Each argument in "argNames" gets mapped to a
+    //! unique position in the array, based on the order it is listed, unless
+    //! the argument is a named-only argument with argNames[x].second set to
+    //! true. Named-only arguments are combined into a JSON object that is
+    //! appended after other arguments, see transformNamedArguments for details.
+    std::vector<std::pair<std::string, bool>> argNames;
     intptr_t unique_id;
 };