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

2cd28e9fef5dd743bcd70025196ee311fcfdcae4 rpc: Add check for unintended option/parameter name clashes (Ryan Ofsky)
95d7de0964620a3f7386a4adc5707559868abf84 test: Update python tests to use named parameters instead of options objects (Ryan Ofsky)
96233146dd31c1d99fd1619be4449944623ef750 RPC: Allow RPC methods accepting options to take named parameters (Ryan Ofsky)
702b56d2a8ce48bc3b66a2867d09fa11dcf12fc5 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 2cd28e9fef5dd743bcd70025196ee311fcfdcae4

Tree-SHA512: 50f6e78fa622826dab3f810400d8c1a03a98a090b1f2fea79729c58ad8cff955554bd44c2a5975f62a526b900dda68981862fd7d7d05c17f94f5b5d847317436

1 files changed, 20 insertions, 3 deletions

diff --git a/src/rpc/util.h b/src/rpc/util.h
index 3ff02582a6..4cba5a9818 100644
--- a/src/rpc/util.h
+++ b/src/rpc/util.h
@@ -130,6 +130,15 @@ struct RPCArgOptions {
     std::string oneline_description{};   //!< Should be empty unless it is supposed to override the auto-generated summary line
     std::vector<std::string> type_str{}; //!< Should be empty unless it is supposed to override the auto-generated type strings. Vector length is either 0 or 2, m_opts.type_str.at(0) will override the type of the value in a key-value pair, m_opts.type_str.at(1) will override the type in the argument description.
     bool hidden{false};                  //!< For testing only
+    bool also_positional{false};         //!< If set allows a named-parameter field in an OBJ_NAMED_PARAM options object
+                                         //!< to have the same name as a top-level parameter. By default the RPC
+                                         //!< framework disallows this, because if an RPC request passes the value by
+                                         //!< name, it is assigned to top-level parameter position, not to the options
+                                         //!< position, defeating the purpose of using OBJ_NAMED_PARAMS instead OBJ for
+                                         //!< that option. But sometimes it makes sense to allow less-commonly used
+                                         //!< options to be passed by name only, and more commonly used options to be
+                                         //!< passed by name or position, so the RPC framework allows this as long as
+                                         //!< methods set the also_positional flag and read values from both positions.
 };
 
 struct RPCArg {
@@ -139,6 +148,13 @@ struct RPCArg {
         STR,
         NUM,
         BOOL,
+        OBJ_NAMED_PARAMS, //!< Special type that behaves almost exactly like
+                          //!< OBJ, defining an options object with a list of
+                          //!< pre-defined keys. The only difference between OBJ
+                          //!< and OBJ_NAMED_PARAMS is that OBJ_NAMED_PARMS
+                          //!< also allows the keys to be passed as top-level
+                          //!< named parameters, as a more convenient way to pass
+                          //!< options to the RPC method without nesting them.
         OBJ_USER_KEYS, //!< Special type where the user must set the keys e.g. to define multiple addresses; as opposed to e.g. an options object where the keys are predefined
         AMOUNT,        //!< Special type representing a floating point amount (can be either NUM or STR)
         STR_HEX,       //!< Special type that is a STR with only hex chars
@@ -183,7 +199,7 @@ struct RPCArg {
           m_description{std::move(description)},
           m_opts{std::move(opts)}
     {
-        CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ && type != Type::OBJ_USER_KEYS);
+        CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ && type != Type::OBJ_NAMED_PARAMS && type != Type::OBJ_USER_KEYS);
     }
 
     RPCArg(
@@ -200,7 +216,7 @@ struct RPCArg {
           m_description{std::move(description)},
           m_opts{std::move(opts)}
     {
-        CHECK_NONFATAL(type == Type::ARR || type == Type::OBJ || type == Type::OBJ_USER_KEYS);
+        CHECK_NONFATAL(type == Type::ARR || type == Type::OBJ || type == Type::OBJ_NAMED_PARAMS || type == Type::OBJ_USER_KEYS);
     }
 
     bool IsOptional() const;
@@ -369,7 +385,8 @@ public:
     UniValue GetArgMap() const;
     /** If the supplied number of args is neither too small nor too high */
     bool IsValidNumArgs(size_t num_args) const;
-    std::vector<std::string> GetArgNames() const;
+    //! Return list of arguments and whether they are named-only.
+    std::vector<std::pair<std::string, bool>> GetArgNames() const;
 
     const std::string m_name;