aboutsummaryrefslogtreecommitdiff
path: root/src
diff options
context:
space:
mode:
authorAndrew Chow <github@achow101.com>2023-06-01 15:19:37 -0400
committerAndrew Chow <github@achow101.com>2023-06-01 15:30:31 -0400
commit34ac3f438a268e83af6cd11e2981e5bc07f699c9 (patch)
tree3e0eb5b017af1ec176b99ff635d39b38b4a9070e /src
parent9e54dde04ccd490c6719f5d975ed49a75242611b (diff)
parent2cd28e9fef5dd743bcd70025196ee311fcfdcae4 (diff)
downloadbitcoin-34ac3f438a268e83af6cd11e2981e5bc07f699c9.tar.xz
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
Diffstat (limited to 'src')
-rw-r--r--src/rpc/client.cpp71
-rw-r--r--src/rpc/server.cpp46
-rw-r--r--src/rpc/server.h13
-rw-r--r--src/rpc/util.cpp85
-rw-r--r--src/rpc/util.h23
-rw-r--r--src/test/rpc_tests.cpp75
-rw-r--r--src/wallet/rpc/backup.cpp2
-rw-r--r--src/wallet/rpc/coins.cpp2
-rw-r--r--src/wallet/rpc/spend.cpp18
-rw-r--r--src/wallet/rpc/wallet.cpp2
10 files changed, 294 insertions, 43 deletions
diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index d08e2d55d1..7278b57c76 100644
--- a/src/rpc/client.cpp
+++ b/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" },
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index 474b318c66..d39efcb245 100644
--- a/src/rpc/server.cpp
+++ b/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,7 +428,22 @@ 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
@@ -434,12 +451,26 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
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
@@ -447,9 +478,8 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
// 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)};
diff --git a/src/rpc/server.h b/src/rpc/server.h
index 01e8556050..24658ddb8b 100644
--- a/src/rpc/server.h
+++ b/src/rpc/server.h
@@ -95,7 +95,7 @@ public:
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 @@ public:
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;
};
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index d1ff3f3871..19e14f88df 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -389,7 +389,8 @@ struct Sections {
case RPCArg::Type::NUM:
case RPCArg::Type::AMOUNT:
case RPCArg::Type::RANGE:
- case RPCArg::Type::BOOL: {
+ case RPCArg::Type::BOOL:
+ case RPCArg::Type::OBJ_NAMED_PARAMS: {
if (is_top_level_arg) return; // Nothing more to do for non-recursive types on first recursion
auto left = indent;
if (arg.m_opts.type_str.size() != 0 && push_name) {
@@ -485,12 +486,32 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector<RP
m_results{std::move(results)},
m_examples{std::move(examples)}
{
- std::set<std::string> named_args;
+ // Map of parameter names and types just used to check whether the names are
+ // unique. Parameter names always need to be unique, with the exception that
+ // there can be pairs of POSITIONAL and NAMED parameters with the same name.
+ enum ParamType { POSITIONAL = 1, NAMED = 2, NAMED_ONLY = 4 };
+ std::map<std::string, int> param_names;
+
for (const auto& arg : m_args) {
std::vector<std::string> names = SplitString(arg.m_names, '|');
// Should have unique named arguments
for (const std::string& name : names) {
- CHECK_NONFATAL(named_args.insert(name).second);
+ auto& param_type = param_names[name];
+ CHECK_NONFATAL(!(param_type & POSITIONAL));
+ CHECK_NONFATAL(!(param_type & NAMED_ONLY));
+ param_type |= POSITIONAL;
+ }
+ 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) {
+ auto& param_type = param_names[inner_name];
+ CHECK_NONFATAL(!(param_type & POSITIONAL) || inner.m_opts.also_positional);
+ CHECK_NONFATAL(!(param_type & NAMED));
+ CHECK_NONFATAL(!(param_type & NAMED_ONLY));
+ param_type |= inner.m_opts.also_positional ? NAMED : NAMED_ONLY;
+ }
+ }
}
// Default value type should match argument type only when defined
if (arg.m_fallback.index() == 2) {
@@ -605,12 +626,17 @@ bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
return num_required_args <= num_args && num_args <= m_args.size();
}
-std::vector<std::string> RPCHelpMan::GetArgNames() const
+std::vector<std::pair<std::string, bool>> RPCHelpMan::GetArgNames() const
{
- std::vector<std::string> ret;
+ std::vector<std::pair<std::string, bool>> ret;
ret.reserve(m_args.size());
for (const auto& arg : m_args) {
- ret.emplace_back(arg.m_names);
+ if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
+ for (const auto& inner : arg.m_inner) {
+ ret.emplace_back(inner.m_names, /*named_only=*/true);
+ }
+ }
+ ret.emplace_back(arg.m_names, /*named_only=*/false);
}
return ret;
}
@@ -642,20 +668,31 @@ std::string RPCHelpMan::ToString() const
// Arguments
Sections sections;
+ Sections named_only_sections;
for (size_t i{0}; i < m_args.size(); ++i) {
const auto& arg = m_args.at(i);
if (arg.m_opts.hidden) break; // Any arg that follows is also hidden
- if (i == 0) ret += "\nArguments:\n";
-
// Push named argument name and description
sections.m_sections.emplace_back(::ToString(i + 1) + ". " + arg.GetFirstName(), arg.ToDescriptionString(/*is_named_arg=*/true));
sections.m_max_pad = std::max(sections.m_max_pad, sections.m_sections.back().m_left.size());
// Recursively push nested args
sections.Push(arg);
+
+ // Push named-only argument sections
+ if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
+ for (const auto& arg_inner : arg.m_inner) {
+ named_only_sections.PushSection({arg_inner.GetFirstName(), arg_inner.ToDescriptionString(/*is_named_arg=*/true)});
+ named_only_sections.Push(arg_inner);
+ }
+ }
}
+
+ if (!sections.m_sections.empty()) ret += "\nArguments:\n";
ret += sections.ToString();
+ if (!named_only_sections.m_sections.empty()) ret += "\nNamed Arguments:\n";
+ ret += named_only_sections.ToString();
// Result
ret += m_results.ToDescriptionString();
@@ -669,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, i, inner_name, inner.m_type);
+ }
+ }
+ }
}
}
return arr;
@@ -708,6 +758,7 @@ static std::optional<UniValue::VType> ExpectedType(RPCArg::Type type)
return UniValue::VBOOL;
}
case Type::OBJ:
+ case Type::OBJ_NAMED_PARAMS:
case Type::OBJ_USER_KEYS: {
return UniValue::VOBJ;
}
@@ -781,6 +832,7 @@ std::string RPCArg::ToDescriptionString(bool is_named_arg) const
break;
}
case Type::OBJ:
+ case Type::OBJ_NAMED_PARAMS:
case Type::OBJ_USER_KEYS: {
ret += "json object";
break;
@@ -809,6 +861,7 @@ std::string RPCArg::ToDescriptionString(bool is_named_arg) const
} // no default case, so the compiler can warn about missing cases
}
ret += ")";
+ if (m_type == Type::OBJ_NAMED_PARAMS) ret += " Options object that can be used to pass named arguments, listed below.";
ret += m_description.empty() ? "" : " " + m_description;
return ret;
}
@@ -1054,6 +1107,7 @@ std::string RPCArg::ToStringObj(const bool oneline) const
}
return res + "...]";
case Type::OBJ:
+ case Type::OBJ_NAMED_PARAMS:
case Type::OBJ_USER_KEYS:
// Currently unused, so avoid writing dead code
NONFATAL_UNREACHABLE();
@@ -1077,6 +1131,7 @@ std::string RPCArg::ToString(const bool oneline) const
return GetFirstName();
}
case Type::OBJ:
+ case Type::OBJ_NAMED_PARAMS:
case Type::OBJ_USER_KEYS: {
const std::string res = Join(m_inner, ",", [&](const RPCArg& i) { return i.ToStringObj(oneline); });
if (m_type == Type::OBJ) {
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;
diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp
index da0029c737..31c2010243 100644
--- a/src/test/rpc_tests.cpp
+++ b/src/test/rpc_tests.cpp
@@ -42,11 +42,11 @@ private:
class RPCTestingSetup : public TestingSetup
{
public:
- UniValue TransformParams(const UniValue& params, std::vector<std::string> arg_names) const;
+ UniValue TransformParams(const UniValue& params, std::vector<std::pair<std::string, bool>> arg_names) const;
UniValue CallRPC(std::string args);
};
-UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector<std::string> arg_names) const
+UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector<std::pair<std::string, bool>> arg_names) const
{
UniValue transformed_params;
CRPCTable table;
@@ -84,7 +84,7 @@ BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup)
BOOST_AUTO_TEST_CASE(rpc_namedparams)
{
- const std::vector<std::string> arg_names{"arg1", "arg2", "arg3", "arg4", "arg5"};
+ const std::vector<std::pair<std::string, bool>> arg_names{{"arg1", false}, {"arg2", false}, {"arg3", false}, {"arg4", false}, {"arg5", false}};
// Make sure named arguments are transformed into positional arguments in correct places separated by nulls
BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg4": 4})"), arg_names).write(), "[null,2,null,4]");
@@ -109,6 +109,28 @@ BOOST_AUTO_TEST_CASE(rpc_namedparams)
BOOST_CHECK_EQUAL(TransformParams(JSON(R"([1,2,3,4,5,6,7,8,9,10])"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]");
}
+BOOST_AUTO_TEST_CASE(rpc_namedonlyparams)
+{
+ const std::vector<std::pair<std::string, bool>> arg_names{{"arg1", false}, {"arg2", false}, {"opt1", true}, {"opt2", true}, {"options", false}};
+
+ // Make sure optional parameters are really optional.
+ BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg1": 1, "arg2": 2})"), arg_names).write(), "[1,2]");
+
+ // Make sure named-only parameters are passed as options.
+ BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg1": 1, "arg2": 2, "opt1": 10, "opt2": 20})"), arg_names).write(), R"([1,2,{"opt1":10,"opt2":20}])");
+
+ // Make sure options can be passed directly.
+ BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg1": 1, "arg2": 2, "options":{"opt1": 10, "opt2": 20}})"), arg_names).write(), R"([1,2,{"opt1":10,"opt2":20}])");
+
+ // Make sure options and named parameters conflict.
+ BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"arg1": 1, "arg2": 2, "opt1": 10, "options":{"opt1": 10}})"), arg_names), UniValue,
+ HasJSON(R"({"code":-8,"message":"Parameter options conflicts with parameter opt1"})"));
+
+ // Make sure options object specified through args array conflicts.
+ BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"args": [1, 2, {"opt1": 10}], "opt2": 20})"), arg_names), UniValue,
+ HasJSON(R"({"code":-8,"message":"Parameter options specified twice both as positional and named argument"})"));
+}
+
BOOST_AUTO_TEST_CASE(rpc_rawparams)
{
// Test raw transaction API argument handling
@@ -506,6 +528,53 @@ BOOST_AUTO_TEST_CASE(rpc_getblockstats_calculate_percentiles_by_weight)
}
}
+// Make sure errors are triggered appropriately if parameters have the same names.
+BOOST_AUTO_TEST_CASE(check_dup_param_names)
+{
+ enum ParamType { POSITIONAL, NAMED, NAMED_ONLY };
+ auto make_rpc = [](std::vector<std::tuple<std::string, ParamType>> param_names) {
+ std::vector<RPCArg> params;
+ std::vector<RPCArg> options;
+ auto push_options = [&] { if (!options.empty()) params.emplace_back(RPCArg{strprintf("options%i", params.size()), RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "", std::move(options)}); };
+ for (auto& [param_name, param_type] : param_names) {
+ if (param_type == POSITIONAL) {
+ push_options();
+ params.emplace_back(std::move(param_name), RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "description");
+ } else {
+ options.emplace_back(std::move(param_name), RPCArg::Type::NUM, RPCArg::Optional::OMITTED, "description", RPCArgOptions{.also_positional = param_type == NAMED});
+ }
+ }
+ push_options();
+ return RPCHelpMan{"method_name", "description", params, RPCResults{}, RPCExamples{""}};
+ };
+
+ // No errors if parameter names are unique.
+ make_rpc({{"p1", POSITIONAL}, {"p2", POSITIONAL}});
+ make_rpc({{"p1", POSITIONAL}, {"p2", NAMED}});
+ make_rpc({{"p1", POSITIONAL}, {"p2", NAMED_ONLY}});
+ make_rpc({{"p1", NAMED}, {"p2", POSITIONAL}});
+ make_rpc({{"p1", NAMED}, {"p2", NAMED}});
+ make_rpc({{"p1", NAMED}, {"p2", NAMED_ONLY}});
+ make_rpc({{"p1", NAMED_ONLY}, {"p2", POSITIONAL}});
+ make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED}});
+ make_rpc({{"p1", NAMED_ONLY}, {"p2", NAMED_ONLY}});
+
+ // Error if parameters names are duplicates, unless one parameter is
+ // positional and the other is named and .also_positional is true.
+ BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", POSITIONAL}}), NonFatalCheckError);
+ make_rpc({{"p1", POSITIONAL}, {"p1", NAMED}});
+ BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
+ make_rpc({{"p1", NAMED}, {"p1", POSITIONAL}});
+ BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED}}), NonFatalCheckError);
+ BOOST_CHECK_THROW(make_rpc({{"p1", NAMED}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
+ BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", POSITIONAL}}), NonFatalCheckError);
+ BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED}}), NonFatalCheckError);
+ BOOST_CHECK_THROW(make_rpc({{"p1", NAMED_ONLY}, {"p1", NAMED_ONLY}}), NonFatalCheckError);
+
+ // Make sure duplicate aliases are detected too.
+ BOOST_CHECK_THROW(make_rpc({{"p1", POSITIONAL}, {"p2|p1", NAMED_ONLY}}), NonFatalCheckError);
+}
+
BOOST_AUTO_TEST_CASE(help_example)
{
// test different argument types
diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index 43da15b36e..b93419292e 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -1298,7 +1298,7 @@ RPCHelpMan importmulti()
},
},
RPCArgOptions{.oneline_description="\"requests\""}},
- {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+ {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
{
{"rescan", RPCArg::Type::BOOL, RPCArg::Default{true}, "Scan the chain and mempool for wallet transactions after all imports."},
},
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index 5bdd3c9e72..22f0f0b83c 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -513,7 +513,7 @@ RPCHelpMan listunspent()
},
{"include_unsafe", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include outputs that are not safe to spend\n"
"See description of \"safe\" attribute below."},
- {"query_options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "JSON with query options",
+ {"query_options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
{
{"minimumAmount", RPCArg::Type::AMOUNT, RPCArg::Default{FormatMoney(0)}, "Minimum value of each UTXO in " + CURRENCY_UNIT + ""},
{"maximumAmount", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"unlimited"}, "Maximum value of each UTXO in " + CURRENCY_UNIT + ""},
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index a5b1f594bf..feee643f26 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -453,9 +453,9 @@ RPCHelpMan settxfee()
static std::vector<RPCArg> FundTxDoc(bool solving_data = true)
{
std::vector<RPCArg> args = {
- {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"},
+ {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks", RPCArgOptions{.also_positional = true}},
{"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
- "\"" + FeeModes("\"\n\"") + "\""},
+ "\"" + FeeModes("\"\n\"") + "\"", RPCArgOptions{.also_positional = true}},
{
"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Marks this transaction as BIP125-replaceable.\n"
"Allows this transaction to be replaced by a transaction with higher fees"
@@ -758,7 +758,7 @@ RPCHelpMan fundrawtransaction()
"Only pay-to-pubkey, multisig, and P2SH versions thereof are currently supported for watch-only\n",
{
{"hexstring", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The hex string of the raw transaction"},
- {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "for backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}",
+ {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "For backward compatibility: passing in a true instead of an object will result in {\"includeWatching\":true}",
Cat<std::vector<RPCArg>>(
{
{"add_inputs", RPCArg::Type::BOOL, RPCArg::Default{true}, "For a transaction with existing inputs, automatically include more if they are not enough."},
@@ -997,7 +997,7 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
"* WARNING: before version 0.21, fee_rate was in " + CURRENCY_UNIT + "/kvB. As of 0.21, fee_rate is in " + CURRENCY_ATOM + "/vB. *\n",
{
{"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The txid to be bumped"},
- {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+ {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
{
{"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks\n"},
{"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"},
@@ -1187,7 +1187,7 @@ RPCHelpMan send()
{"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, "The fee estimate mode, must be one of (case insensitive):\n"
"\"" + FeeModes("\"\n\"") + "\""},
{"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
- {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+ {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
Cat<std::vector<RPCArg>>(
{
{"add_inputs", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false when \"inputs\" are specified, true otherwise"},"Automatically include coins from the wallet to cover the target amount.\n"},
@@ -1200,7 +1200,7 @@ RPCHelpMan send()
{"change_address", RPCArg::Type::STR, RPCArg::DefaultHint{"automatic"}, "The bitcoin address to receive the change"},
{"change_position", RPCArg::Type::NUM, RPCArg::DefaultHint{"random"}, "The index of the change output"},
{"change_type", RPCArg::Type::STR, RPCArg::DefaultHint{"set by -changetype"}, "The output type to use. Only valid if change_address is not specified. Options are \"legacy\", \"p2sh-segwit\", \"bech32\" and \"bech32m\"."},
- {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
+ {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB.", RPCArgOptions{.also_positional = true}},
{"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch only.\n"
"Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n"
"e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."},
@@ -1302,11 +1302,11 @@ RPCHelpMan sendall()
"\"" + FeeModes("\"\n\"") + "\""},
{"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
{
- "options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+ "options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
Cat<std::vector<RPCArg>>(
{
{"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns the serialized transaction without broadcasting or adding it to the wallet"},
- {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB."},
+ {"fee_rate", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"not set, fall back to wallet fee estimation"}, "Specify a fee rate in " + CURRENCY_ATOM + "/vB.", RPCArgOptions{.also_positional = true}},
{"include_watching", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Also select inputs which are watch-only.\n"
"Only solvable inputs can be used. Watch-only destinations are solvable if the public key and/or output script was imported,\n"
"e.g. with 'importpubkey' or 'importmulti' with the 'pubkeys' or 'desc' field."},
@@ -1635,7 +1635,7 @@ RPCHelpMan walletcreatefundedpsbt()
OutputsDoc(),
RPCArgOptions{.skip_type_check = true}},
{"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"},
- {"options", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "",
+ {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
Cat<std::vector<RPCArg>>(
{
{"add_inputs", RPCArg::Type::BOOL, RPCArg::DefaultHint{"false when \"inputs\" are specified, true otherwise"}, "Automatically include coins from the wallet to cover the target amount.\n"},
diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp
index a22862bfa9..e0f246e2f2 100644
--- a/src/wallet/rpc/wallet.cpp
+++ b/src/wallet/rpc/wallet.cpp
@@ -645,7 +645,7 @@ RPCHelpMan simulaterawtransaction()
{"rawtx", RPCArg::Type::STR_HEX, RPCArg::Optional::OMITTED, ""},
},
},
- {"options", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "Options",
+ {"options", RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Optional::OMITTED, "",
{
{"include_watchonly", RPCArg::Type::BOOL, RPCArg::DefaultHint{"true for watch-only wallets, otherwise false"}, "Whether to include watch-only addresses (see RPC importaddress)"},
},