From 702b56d2a8ce48bc3b66a2867d09fa11dcf12fc5 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 10 Nov 2022 11:54:11 -0500 Subject: [PATCH 1/4] RPC: Add add OBJ_NAMED_PARAMS type OBJ_NAMED_PARAMS type works the same as OBJ type except it registers the object keys to be accepted as top-level named-only RPC parameters. Generated documentation also lists the object keys seperately in a new "Named arguments" section of help text. Named-only RPC parameters have the same semantics as python keyword-only arguments (https://peps.python.org/pep-3102/). They are always required to be passed by name, so they don't affect interpretation of positional arguments, and aren't affected when positional arguments are added or removed. The new OBJ_NAMED_PARAMS type is used in the next commit to make it easier to pass options values to various RPC methods. Co-authored-by: Andrew Chow --- src/rpc/server.cpp | 46 +++++++++++++++++++++++++------ src/rpc/server.h | 13 +++++++-- src/rpc/util.cpp | 61 +++++++++++++++++++++++++++++++++--------- src/rpc/util.h | 14 +++++++--- src/test/rpc_tests.cpp | 28 ++++++++++++++++--- 5 files changed, 133 insertions(+), 29 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 354f9490024..bd684b7e86c 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& argNames) +static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, const std::vector>& 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 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 01e85560502..24658ddb8bc 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -95,7 +95,7 @@ public: using Actor = std::function; //! Constructor taking Actor callback supporting multiple handlers. - CRPCCommand(std::string category, std::string name, Actor actor, std::vector args, intptr_t unique_id) + CRPCCommand(std::string category, std::string name, Actor actor, std::vector> 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 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> argNames; intptr_t unique_id; }; diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 1f3f37d0a0f..2fa1732faaf 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) { @@ -605,12 +606,17 @@ bool RPCHelpMan::IsValidNumArgs(size_t num_args) const return num_required_args <= num_args && num_args <= m_args.size(); } -std::vector RPCHelpMan::GetArgNames() const +std::vector> RPCHelpMan::GetArgNames() const { - std::vector ret; + std::vector> 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 +648,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 +686,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 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 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 +738,7 @@ static std::optional 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 +812,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 +841,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 +1087,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 +1111,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 bb5c30a2f44..d017812e97a 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -139,6 +139,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 +190,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 +207,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 +376,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 GetArgNames() const; + //! Return list of arguments and whether they are named-only. + std::vector> GetArgNames() const; const std::string m_name; diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index 791c9ddf312..f8975dd9b29 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 arg_names) const; + UniValue TransformParams(const UniValue& params, std::vector> arg_names) const; UniValue CallRPC(std::string args); }; -UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector arg_names) const +UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector> 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 arg_names{"arg1", "arg2", "arg3", "arg4", "arg5"}; + const std::vector> 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> 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 From 96233146dd31c1d99fd1619be4449944623ef750 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 10 Nov 2022 12:04:07 -0500 Subject: [PATCH 2/4] RPC: Allow RPC methods accepting options to take named parameters Co-authored-by: Andrew Chow --- doc/release-notes-26485.md | 16 +++++++++ src/rpc/client.cpp | 71 +++++++++++++++++++++++++++++++++++++ src/wallet/rpc/backup.cpp | 2 +- src/wallet/rpc/coins.cpp | 2 +- src/wallet/rpc/spend.cpp | 10 +++--- src/wallet/rpc/wallet.cpp | 2 +- test/functional/rpc_help.py | 4 +-- 7 files changed, 97 insertions(+), 10 deletions(-) create mode 100644 doc/release-notes-26485.md diff --git a/doc/release-notes-26485.md b/doc/release-notes-26485.md new file mode 100644 index 00000000000..c8df3d22fb4 --- /dev/null +++ b/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}' +``` diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index f3c19003ff9..da28e3a4d19 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" }, @@ -154,18 +186,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" }, @@ -189,7 +252,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/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 553bbfb62fd..252c6580634 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 750ef69f6ea..3a9136f5e54 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 88ee6e96b05..3f98f93a90a 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -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>( { {"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>( { {"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"}, @@ -1302,7 +1302,7 @@ 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>( { {"add_to_wallet", RPCArg::Type::BOOL, RPCArg::Default{true}, "When false, returns the serialized transaction without broadcasting or adding it to the wallet"}, @@ -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>( { {"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 d728b2fb96b..62ce42072bc 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)"}, }, diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py index 7acc3cbbd51..53c5aa05e5e 100755 --- a/test/functional/rpc_help.py +++ b/test/functional/rpc_help.py @@ -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] From 95d7de0964620a3f7386a4adc5707559868abf84 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Thu, 10 Nov 2022 12:04:07 -0500 Subject: [PATCH 3/4] test: Update python tests to use named parameters instead of options objects --- test/functional/rpc_psbt.py | 12 +- test/functional/wallet_bumpfee.py | 50 +++--- test/functional/wallet_fundrawtransaction.py | 160 +++++++++--------- test/functional/wallet_import_rescan.py | 2 +- test/functional/wallet_keypool.py | 14 +- test/functional/wallet_migration.py | 2 +- .../wallet_multisig_descriptor_psbt.py | 4 +- .../wallet_resendwallettransactions.py | 6 +- test/functional/wallet_sendall.py | 18 +- test/functional/wallet_signer.py | 4 +- test/functional/wallet_taproot.py | 2 +- test/functional/wallet_timelock.py | 2 +- test/functional/wallet_watchonly.py | 4 +- 13 files changed, 140 insertions(+), 140 deletions(-) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index ef773463d81..35d6cc3568c 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -373,7 +373,7 @@ class PSBTTest(BitcoinTestFramework): self.log.info("Test various PSBT operations") # partially sign multisig things with node 1 - psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2wsh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2sh_p2wsh_pos}], outputs={self.nodes[1].getnewaddress():29.99}, options={'changeAddress': self.nodes[1].getrawchangeaddress()})['psbt'] + psbtx = wmulti.walletcreatefundedpsbt(inputs=[{"txid":txid,"vout":p2wsh_pos},{"txid":txid,"vout":p2sh_pos},{"txid":txid,"vout":p2sh_p2wsh_pos}], outputs={self.nodes[1].getnewaddress():29.99}, changeAddress=self.nodes[1].getrawchangeaddress())['psbt'] walletprocesspsbt_out = self.nodes[1].walletprocesspsbt(psbtx) psbtx = walletprocesspsbt_out['psbt'] assert_equal(walletprocesspsbt_out['complete'], False) @@ -778,7 +778,7 @@ class PSBTTest(BitcoinTestFramework): psbt = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) signed = wallet.walletprocesspsbt(psbt["psbt"]) signed = self.nodes[0].walletprocesspsbt(signed["psbt"]) @@ -788,21 +788,21 @@ class PSBTTest(BitcoinTestFramework): psbt2 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": low_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) assert_greater_than(psbt["fee"], psbt2["fee"]) # Increasing the weight should have a higher fee psbt2 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) assert_greater_than(psbt2["fee"], psbt["fee"]) # The provided weight should override the calculated weight when solving data is provided psbt3 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={'add_inputs': True, "solving_data":{"descriptors": [desc]}} + add_inputs=True, solving_data={"descriptors": [desc]}, ) assert_equal(psbt2["fee"], psbt3["fee"]) @@ -816,7 +816,7 @@ class PSBTTest(BitcoinTestFramework): psbt3 = wallet.walletcreatefundedpsbt( inputs=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}], outputs={self.nodes[0].getnewaddress(): 15}, - options={"add_inputs": True} + add_inputs=True, ) assert_equal(psbt2["fee"], psbt3["fee"]) diff --git a/test/functional/wallet_bumpfee.py b/test/functional/wallet_bumpfee.py index 19c8022600f..b9ebf64c22e 100755 --- a/test/functional/wallet_bumpfee.py +++ b/test/functional/wallet_bumpfee.py @@ -123,36 +123,36 @@ class BumpFeeTest(BitcoinTestFramework): assert_raises_rpc_error(-3, "Unexpected key {}".format(key), rbf_node.bumpfee, rbfid, {key: NORMAL}) # Bumping to just above minrelay should fail to increase the total fee enough. - assert_raises_rpc_error(-8, "Insufficient total fee 0.00000141", rbf_node.bumpfee, rbfid, {"fee_rate": INSUFFICIENT}) + assert_raises_rpc_error(-8, "Insufficient total fee 0.00000141", rbf_node.bumpfee, rbfid, fee_rate=INSUFFICIENT) self.log.info("Test invalid fee rate settings") assert_raises_rpc_error(-4, "Specified or calculated fee 0.141 is too high (cannot be higher than -maxtxfee 0.10", - rbf_node.bumpfee, rbfid, {"fee_rate": TOO_HIGH}) + rbf_node.bumpfee, rbfid, fee_rate=TOO_HIGH) # Test fee_rate with zero values. msg = "Insufficient total fee 0.00" for zero_value in [0, 0.000, 0.00000000, "0", "0.000", "0.00000000"]: - assert_raises_rpc_error(-8, msg, rbf_node.bumpfee, rbfid, {"fee_rate": zero_value}) + assert_raises_rpc_error(-8, msg, rbf_node.bumpfee, rbfid, fee_rate=zero_value) msg = "Invalid amount" # Test fee_rate values that don't pass fixed-point parsing checks. for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: - assert_raises_rpc_error(-3, msg, rbf_node.bumpfee, rbfid, {"fee_rate": invalid_value}) + assert_raises_rpc_error(-3, msg, rbf_node.bumpfee, rbfid, fee_rate=invalid_value) # Test fee_rate values that cannot be represented in sat/vB. for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: - assert_raises_rpc_error(-3, msg, rbf_node.bumpfee, rbfid, {"fee_rate": invalid_value}) + assert_raises_rpc_error(-3, msg, rbf_node.bumpfee, rbfid, fee_rate=invalid_value) # Test fee_rate out of range (negative number). - assert_raises_rpc_error(-3, "Amount out of range", rbf_node.bumpfee, rbfid, {"fee_rate": -1}) + assert_raises_rpc_error(-3, "Amount out of range", rbf_node.bumpfee, rbfid, fee_rate=-1) # Test type error. for value in [{"foo": "bar"}, True]: - assert_raises_rpc_error(-3, "Amount is not a number or string", rbf_node.bumpfee, rbfid, {"fee_rate": value}) + assert_raises_rpc_error(-3, "Amount is not a number or string", rbf_node.bumpfee, rbfid, fee_rate=value) self.log.info("Test explicit fee rate raises RPC error if both fee_rate and conf_target are passed") assert_raises_rpc_error(-8, "Cannot specify both conf_target and fee_rate. Please provide either a confirmation " "target in blocks for automatic fee estimation, or an explicit fee rate.", - rbf_node.bumpfee, rbfid, {"conf_target": NORMAL, "fee_rate": NORMAL}) + rbf_node.bumpfee, rbfid, conf_target=NORMAL, fee_rate=NORMAL) self.log.info("Test explicit fee rate raises RPC error if both fee_rate and estimate_mode are passed") assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and fee_rate", - rbf_node.bumpfee, rbfid, {"estimate_mode": "economical", "fee_rate": NORMAL}) + rbf_node.bumpfee, rbfid, estimate_mode="economical", fee_rate=NORMAL) self.log.info("Test invalid conf_target settings") assert_raises_rpc_error(-8, "confTarget and conf_target options should not both be set", @@ -161,10 +161,10 @@ class BumpFeeTest(BitcoinTestFramework): self.log.info("Test invalid estimate_mode settings") for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, f"JSON value of type {k} for field estimate_mode is not of expected type string", - rbf_node.bumpfee, rbfid, {"estimate_mode": v}) + rbf_node.bumpfee, rbfid, estimate_mode=v) for mode in ["foo", Decimal("3.1415"), "sat/B", "BTC/kB"]: assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', - rbf_node.bumpfee, rbfid, {"estimate_mode": mode}) + rbf_node.bumpfee, rbfid, estimate_mode=mode) self.log.info("Test invalid outputs values") assert_raises_rpc_error(-8, "Invalid parameter, output argument cannot be an empty array", @@ -232,12 +232,12 @@ def test_simple_bumpfee_succeeds(self, mode, rbf_node, peer_node, dest_address): self.sync_mempools((rbf_node, peer_node)) assert rbfid in rbf_node.getrawmempool() and rbfid in peer_node.getrawmempool() if mode == "fee_rate": - bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"fee_rate": str(NORMAL)}) - bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": NORMAL}) + bumped_psbt = rbf_node.psbtbumpfee(rbfid, fee_rate=str(NORMAL)) + bumped_tx = rbf_node.bumpfee(rbfid, fee_rate=NORMAL) elif mode == "new_outputs": new_address = peer_node.getnewaddress() - bumped_psbt = rbf_node.psbtbumpfee(rbfid, {"outputs": {new_address: 0.0003}}) - bumped_tx = rbf_node.bumpfee(rbfid, {"outputs": {new_address: 0.0003}}) + bumped_psbt = rbf_node.psbtbumpfee(rbfid, outputs={new_address: 0.0003}) + bumped_tx = rbf_node.bumpfee(rbfid, outputs={new_address: 0.0003}) else: bumped_psbt = rbf_node.psbtbumpfee(rbfid) bumped_tx = rbf_node.bumpfee(rbfid) @@ -305,7 +305,7 @@ def test_notmine_bumpfee(self, rbf_node, peer_node, dest_address): # Note that this test depends upon the RPC code checking input ownership prior to change outputs # (since it can't use fundrawtransaction, it lacks a proper change output) fee = Decimal("0.001") - utxos = [node.listunspent(query_options={'minimumAmount': fee})[-1] for node in (rbf_node, peer_node)] + utxos = [node.listunspent(minimumAmount=fee)[-1] for node in (rbf_node, peer_node)] inputs = [{ "txid": utxo["txid"], "vout": utxo["vout"], @@ -335,7 +335,7 @@ def test_notmine_bumpfee(self, rbf_node, peer_node, dest_address): psbt = rbf_node.psbtbumpfee(txid=rbfid) finish_psbtbumpfee(psbt["psbt"]) - psbt = rbf_node.psbtbumpfee(txid=rbfid, options={"fee_rate": old_feerate + 10}) + psbt = rbf_node.psbtbumpfee(txid=rbfid, fee_rate=old_feerate + 10) finish_psbtbumpfee(psbt["psbt"]) self.clear_mempool() @@ -445,7 +445,7 @@ def test_dust_to_fee(self, rbf_node, dest_address): # Expected fee is 141 vbytes * fee_rate 0.00350250 BTC / 1000 vbytes = 0.00049385 BTC. # or occasionally 140 vbytes * fee_rate 0.00350250 BTC / 1000 vbytes = 0.00049035 BTC. # Dust should be dropped to the fee, so actual bump fee is 0.00050000 BTC. - bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": 350.25}) + bumped_tx = rbf_node.bumpfee(rbfid, fee_rate=350.25) full_bumped_tx = rbf_node.getrawtransaction(bumped_tx["txid"], 1) assert_equal(bumped_tx["fee"], Decimal("0.00050000")) assert_equal(len(fulltx["vout"]), 2) @@ -569,7 +569,7 @@ def test_watchonly_psbt(self, peer_node, rbf_node, dest_address): assert_raises_rpc_error(-4, "bumpfee is not available with wallets that have private keys disabled. Use psbtbumpfee instead.", watcher.bumpfee, original_txid) # Bump fee, obnoxiously high to add additional watchonly input - bumped_psbt = watcher.psbtbumpfee(original_txid, {"fee_rate": HIGH}) + bumped_psbt = watcher.psbtbumpfee(original_txid, fee_rate=HIGH) assert_greater_than(len(watcher.decodepsbt(bumped_psbt['psbt'])["tx"]["vin"]), 1) assert "txid" not in bumped_psbt assert_equal(bumped_psbt["origfee"], -watcher.gettransaction(original_txid)["fee"]) @@ -593,17 +593,17 @@ def test_watchonly_psbt(self, peer_node, rbf_node, dest_address): def test_rebumping(self, rbf_node, dest_address): self.log.info('Test that re-bumping the original tx fails, but bumping successor works') rbfid = spend_one_input(rbf_node, dest_address) - bumped = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL}) + bumped = rbf_node.bumpfee(rbfid, fee_rate=ECONOMICAL) assert_raises_rpc_error(-4, f"Cannot bump transaction {rbfid} which was already bumped by transaction {bumped['txid']}", - rbf_node.bumpfee, rbfid, {"fee_rate": NORMAL}) - rbf_node.bumpfee(bumped["txid"], {"fee_rate": NORMAL}) + rbf_node.bumpfee, rbfid, fee_rate=NORMAL) + rbf_node.bumpfee(bumped["txid"], fee_rate=NORMAL) self.clear_mempool() def test_rebumping_not_replaceable(self, rbf_node, dest_address): self.log.info('Test that re-bumping non-replaceable fails') rbfid = spend_one_input(rbf_node, dest_address) - bumped = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL, "replaceable": False}) + bumped = rbf_node.bumpfee(rbfid, fee_rate=ECONOMICAL, replaceable=False) assert_raises_rpc_error(-4, "Transaction is not BIP 125 replaceable", rbf_node.bumpfee, bumped["txid"], {"fee_rate": NORMAL}) self.clear_mempool() @@ -615,7 +615,7 @@ def test_bumpfee_already_spent(self, rbf_node, dest_address): self.generate(rbf_node, 1) # spend coin simply by mining block with tx spent_input = rbf_node.gettransaction(txid=txid, verbose=True)['decoded']['vin'][0] assert_raises_rpc_error(-1, f"{spent_input['txid']}:{spent_input['vout']} is already spent", - rbf_node.bumpfee, txid, {"fee_rate": NORMAL}) + rbf_node.bumpfee, txid, fee_rate=NORMAL) def test_unconfirmed_not_spendable(self, rbf_node, rbf_node_address): @@ -694,7 +694,7 @@ def test_change_script_match(self, rbf_node, dest_address): assert_equal(len(change_addresses), 1) # Now find that address in each subsequent tx, and no other change - bumped_total_tx = rbf_node.bumpfee(rbfid, {"fee_rate": ECONOMICAL}) + bumped_total_tx = rbf_node.bumpfee(rbfid, fee_rate=ECONOMICAL) assert_equal(change_addresses, get_change_address(bumped_total_tx['txid'], rbf_node)) bumped_rate_tx = rbf_node.bumpfee(bumped_total_tx["txid"]) assert_equal(change_addresses, get_change_address(bumped_rate_tx['txid'], rbf_node)) diff --git a/test/functional/wallet_fundrawtransaction.py b/test/functional/wallet_fundrawtransaction.py index 29ddb77b416..c88e0b3f6e4 100755 --- a/test/functional/wallet_fundrawtransaction.py +++ b/test/functional/wallet_fundrawtransaction.py @@ -154,7 +154,7 @@ class RawTransactionsTest(BitcoinTestFramework): """Ensure setting changePosition in fundraw with an exact match is handled properly.""" self.log.info("Test fundrawtxn changePosition option") rawmatch = self.nodes[2].createrawtransaction([], {self.nodes[2].getnewaddress():50}) - rawmatch = self.nodes[2].fundrawtransaction(rawmatch, {"changePosition":1, "subtractFeeFromOutputs":[0]}) + rawmatch = self.nodes[2].fundrawtransaction(rawmatch, changePosition=1, subtractFeeFromOutputs=[0]) assert_equal(rawmatch["changepos"], -1) self.nodes[3].createwallet(wallet_name="wwatch", disable_private_keys=True) @@ -268,10 +268,10 @@ class RawTransactionsTest(BitcoinTestFramework): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - assert_raises_rpc_error(-3, "Unexpected key foo", self.nodes[2].fundrawtransaction, rawtx, {'foo':'bar'}) + assert_raises_rpc_error(-8, "Unknown named parameter foo", self.nodes[2].fundrawtransaction, rawtx, foo='bar') # reserveChangeKey was deprecated and is now removed - assert_raises_rpc_error(-3, "Unexpected key reserveChangeKey", lambda: self.nodes[2].fundrawtransaction(hexstring=rawtx, options={'reserveChangeKey': True})) + assert_raises_rpc_error(-8, "Unknown named parameter reserveChangeKey", lambda: self.nodes[2].fundrawtransaction(hexstring=rawtx, reserveChangeKey=True)) def test_invalid_change_address(self): self.log.info("Test fundrawtxn with an invalid change address") @@ -283,7 +283,7 @@ class RawTransactionsTest(BitcoinTestFramework): dec_tx = self.nodes[2].decoderawtransaction(rawtx) assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) - assert_raises_rpc_error(-5, "Change address must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':'foobar'}) + assert_raises_rpc_error(-5, "Change address must be a valid bitcoin address", self.nodes[2].fundrawtransaction, rawtx, changeAddress='foobar') def test_valid_change_address(self): self.log.info("Test fundrawtxn with a provided change address") @@ -296,8 +296,8 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) change = self.nodes[2].getnewaddress() - assert_raises_rpc_error(-8, "changePosition out of bounds", self.nodes[2].fundrawtransaction, rawtx, {'changeAddress':change, 'changePosition':2}) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {'changeAddress': change, 'changePosition': 0}) + assert_raises_rpc_error(-8, "changePosition out of bounds", self.nodes[2].fundrawtransaction, rawtx, changeAddress=change, changePosition=2) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, changeAddress=change, changePosition=0) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) out = dec_tx['vout'][0] assert_equal(change, out['scriptPubKey']['address']) @@ -309,9 +309,9 @@ class RawTransactionsTest(BitcoinTestFramework): inputs = [ {'txid' : utx['txid'], 'vout' : utx['vout']} ] outputs = { self.nodes[0].getnewaddress() : Decimal(4.0) } rawtx = self.nodes[2].createrawtransaction(inputs, outputs) - assert_raises_rpc_error(-3, "JSON value of type null is not of expected type string", self.nodes[2].fundrawtransaction, rawtx, {'change_type': None}) - assert_raises_rpc_error(-5, "Unknown change type ''", self.nodes[2].fundrawtransaction, rawtx, {'change_type': ''}) - rawtx = self.nodes[2].fundrawtransaction(rawtx, {'change_type': 'bech32'}) + assert_raises_rpc_error(-3, "JSON value of type null is not of expected type string", self.nodes[2].fundrawtransaction, rawtx, change_type=None) + assert_raises_rpc_error(-5, "Unknown change type ''", self.nodes[2].fundrawtransaction, rawtx, change_type='') + rawtx = self.nodes[2].fundrawtransaction(rawtx, change_type='bech32') dec_tx = self.nodes[2].decoderawtransaction(rawtx['hex']) assert_equal('witness_v0_keyhash', dec_tx['vout'][rawtx['changepos']]['scriptPubKey']['type']) @@ -331,7 +331,7 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal("00", dec_tx['vin'][0]['scriptSig']['hex']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, add_inputs=False) # add_inputs is enabled by default rawtxfund = self.nodes[2].fundrawtransaction(rawtx) @@ -363,8 +363,8 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, add_inputs=False) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, add_inputs=True) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) totalOut = 0 @@ -397,8 +397,8 @@ class RawTransactionsTest(BitcoinTestFramework): assert_equal(utx['txid'], dec_tx['vin'][0]['txid']) # Should fail without add_inputs: - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, {"add_inputs": False}) - rawtxfund = self.nodes[2].fundrawtransaction(rawtx, {"add_inputs": True}) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, self.nodes[2].fundrawtransaction, rawtx, add_inputs=False) + rawtxfund = self.nodes[2].fundrawtransaction(rawtx, add_inputs=True) dec_tx = self.nodes[2].decoderawtransaction(rawtxfund['hex']) totalOut = 0 @@ -567,7 +567,7 @@ class RawTransactionsTest(BitcoinTestFramework): oldBalance = self.nodes[1].getbalance() inputs = [] outputs = {self.nodes[1].getnewaddress():1.1} - funded_psbt = wmulti.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, options={'changeAddress': w2.getrawchangeaddress()})['psbt'] + funded_psbt = wmulti.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, changeAddress=w2.getrawchangeaddress())['psbt'] signed_psbt = w2.walletprocesspsbt(funded_psbt) final_psbt = w2.finalizepsbt(signed_psbt['psbt']) @@ -750,7 +750,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.nodes[3].loadwallet('wwatch') wwatch = self.nodes[3].get_wallet_rpc('wwatch') w3 = self.nodes[3].get_wallet_rpc(self.default_wallet_name) - result = wwatch.fundrawtransaction(rawtx, {'includeWatching': True, 'changeAddress': w3.getrawchangeaddress(), 'subtractFeeFromOutputs': [0]}) + result = wwatch.fundrawtransaction(rawtx, includeWatching=True, changeAddress=w3.getrawchangeaddress(), subtractFeeFromOutputs=[0]) res_dec = self.nodes[0].decoderawtransaction(result["hex"]) assert_equal(len(res_dec["vin"]), 1) assert res_dec["vin"][0]["txid"] == self.watchonly_txid @@ -779,10 +779,10 @@ class RawTransactionsTest(BitcoinTestFramework): result = node.fundrawtransaction(rawtx) # uses self.min_relay_tx_fee (set by settxfee) btc_kvb_to_sat_vb = 100000 # (1e5) - result1 = node.fundrawtransaction(rawtx, {"fee_rate": str(2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee)}) - result2 = node.fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}) - result3 = node.fundrawtransaction(rawtx, {"fee_rate": 10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}) - result4 = node.fundrawtransaction(rawtx, {"feeRate": str(10 * self.min_relay_tx_fee)}) + result1 = node.fundrawtransaction(rawtx, fee_rate=str(2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee)) + result2 = node.fundrawtransaction(rawtx, feeRate=2 * self.min_relay_tx_fee) + result3 = node.fundrawtransaction(rawtx, fee_rate=10 * btc_kvb_to_sat_vb * self.min_relay_tx_fee) + result4 = node.fundrawtransaction(rawtx, feeRate=str(10 * self.min_relay_tx_fee)) result_fee_rate = result['fee'] * 1000 / count_bytes(result['hex']) assert_fee_amount(result1['fee'], count_bytes(result1['hex']), 2 * result_fee_rate) @@ -797,54 +797,54 @@ class RawTransactionsTest(BitcoinTestFramework): # With no arguments passed, expect fee of 141 satoshis. assert_approx(node.fundrawtransaction(rawtx)["fee"], vexp=0.00000141, vspan=0.00000001) # Expect fee to be 10,000x higher when an explicit fee rate 10,000x greater is specified. - result = node.fundrawtransaction(rawtx, {"fee_rate": 10000}) + result = node.fundrawtransaction(rawtx, fee_rate=10000) assert_approx(result["fee"], vexp=0.0141, vspan=0.0001) self.log.info("Test fundrawtxn with invalid estimate_mode settings") for k, v in {"number": 42, "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, f"JSON value of type {k} for field estimate_mode is not of expected type string", - node.fundrawtransaction, rawtx, {"estimate_mode": v, "conf_target": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=v, conf_target=0.1, add_inputs=True) for mode in ["", "foo", Decimal("3.141592")]: assert_raises_rpc_error(-8, 'Invalid estimate_mode parameter, must be one of: "unset", "economical", "conservative"', - node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=mode, conf_target=0.1, add_inputs=True) self.log.info("Test fundrawtxn with invalid conf_target settings") for mode in ["unset", "economical", "conservative"]: self.log.debug("{}".format(mode)) for k, v in {"string": "", "object": {"foo": "bar"}}.items(): assert_raises_rpc_error(-3, f"JSON value of type {k} for field conf_target is not of expected type number", - node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": v, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=mode, conf_target=v, add_inputs=True) for n in [-1, 0, 1009]: assert_raises_rpc_error(-8, "Invalid conf_target, must be between 1 and 1008", # max value of 1008 per src/policy/fees.h - node.fundrawtransaction, rawtx, {"estimate_mode": mode, "conf_target": n, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode=mode, conf_target=n, add_inputs=True) self.log.info("Test invalid fee rate settings") for param, value in {("fee_rate", 100000), ("feeRate", 1.000)}: assert_raises_rpc_error(-4, "Fee exceeds maximum configured by user (e.g. -maxtxfee, maxfeerate)", - node.fundrawtransaction, rawtx, {param: value, "add_inputs": True}) + node.fundrawtransaction, rawtx, add_inputs=True, **{param: value}) assert_raises_rpc_error(-3, "Amount out of range", - node.fundrawtransaction, rawtx, {param: -1, "add_inputs": True}) + node.fundrawtransaction, rawtx, add_inputs=True, **{param: -1}) assert_raises_rpc_error(-3, "Amount is not a number or string", - node.fundrawtransaction, rawtx, {param: {"foo": "bar"}, "add_inputs": True}) + node.fundrawtransaction, rawtx, add_inputs=True, **{param: {"foo": "bar"}}) # Test fee rate values that don't pass fixed-point parsing checks. for invalid_value in ["", 0.000000001, 1e-09, 1.111111111, 1111111111111111, "31.999999999999999999999"]: - assert_raises_rpc_error(-3, "Invalid amount", node.fundrawtransaction, rawtx, {param: invalid_value, "add_inputs": True}) + assert_raises_rpc_error(-3, "Invalid amount", node.fundrawtransaction, rawtx, add_inputs=True, **{param: invalid_value}) # Test fee_rate values that cannot be represented in sat/vB. for invalid_value in [0.0001, 0.00000001, 0.00099999, 31.99999999, "0.0001", "0.00000001", "0.00099999", "31.99999999"]: assert_raises_rpc_error(-3, "Invalid amount", - node.fundrawtransaction, rawtx, {"fee_rate": invalid_value, "add_inputs": True}) + node.fundrawtransaction, rawtx, fee_rate=invalid_value, add_inputs=True) self.log.info("Test min fee rate checks are bypassed with fundrawtxn, e.g. a fee_rate under 1 sat/vB is allowed") - node.fundrawtransaction(rawtx, {"fee_rate": 0.999, "add_inputs": True}) - node.fundrawtransaction(rawtx, {"feeRate": 0.00000999, "add_inputs": True}) + node.fundrawtransaction(rawtx, fee_rate=0.999, add_inputs=True) + node.fundrawtransaction(rawtx, feeRate=0.00000999, add_inputs=True) self.log.info("- raises RPC error if both feeRate and fee_rate are passed") assert_raises_rpc_error(-8, "Cannot specify both fee_rate (sat/vB) and feeRate (BTC/kvB)", - node.fundrawtransaction, rawtx, {"fee_rate": 0.1, "feeRate": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, fee_rate=0.1, feeRate=0.1, add_inputs=True) self.log.info("- raises RPC error if both feeRate and estimate_mode passed") assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and feeRate", - node.fundrawtransaction, rawtx, {"estimate_mode": "economical", "feeRate": 0.1, "add_inputs": True}) + node.fundrawtransaction, rawtx, estimate_mode="economical", feeRate=0.1, add_inputs=True) for param in ["feeRate", "fee_rate"]: self.log.info("- raises RPC error if both {} and conf_target are passed".format(param)) @@ -854,7 +854,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.log.info("- raises RPC error if both fee_rate and estimate_mode are passed") assert_raises_rpc_error(-8, "Cannot specify both estimate_mode and fee_rate", - node.fundrawtransaction, rawtx, {"fee_rate": 1, "estimate_mode": "economical", "add_inputs": True}) + node.fundrawtransaction, rawtx, fee_rate=1, estimate_mode="economical", add_inputs=True) def test_address_reuse(self): """Test no address reuse occurs.""" @@ -884,10 +884,10 @@ class RawTransactionsTest(BitcoinTestFramework): # Test subtract fee from outputs with feeRate (BTC/kvB) result = [self.nodes[3].fundrawtransaction(rawtx), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee}), - self.nodes[3].fundrawtransaction(rawtx, {"feeRate": 2 * self.min_relay_tx_fee, "subtractFeeFromOutputs": [0]}),] + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[]), # empty subtraction list + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0]), # uses self.min_relay_tx_fee (set by settxfee) + self.nodes[3].fundrawtransaction(rawtx, feeRate=2 * self.min_relay_tx_fee), + self.nodes[3].fundrawtransaction(rawtx, feeRate=2 * self.min_relay_tx_fee, subtractFeeFromOutputs=[0]),] dec_tx = [self.nodes[3].decoderawtransaction(tx_['hex']) for tx_ in result] output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)] change = [d['vout'][r['changepos']]['value'] for d, r in zip(dec_tx, result)] @@ -904,10 +904,10 @@ class RawTransactionsTest(BitcoinTestFramework): # Test subtract fee from outputs with fee_rate (sat/vB) btc_kvb_to_sat_vb = 100000 # (1e5) result = [self.nodes[3].fundrawtransaction(rawtx), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": []}), # empty subtraction list - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0]}), # uses self.min_relay_tx_fee (set by settxfee) - self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee}), - self.nodes[3].fundrawtransaction(rawtx, {"fee_rate": 2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee, "subtractFeeFromOutputs": [0]}),] + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[]), # empty subtraction list + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0]), # uses self.min_relay_tx_fee (set by settxfee) + self.nodes[3].fundrawtransaction(rawtx, fee_rate=2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee), + self.nodes[3].fundrawtransaction(rawtx, fee_rate=2 * btc_kvb_to_sat_vb * self.min_relay_tx_fee, subtractFeeFromOutputs=[0]),] dec_tx = [self.nodes[3].decoderawtransaction(tx_['hex']) for tx_ in result] output = [d['vout'][1 - r['changepos']]['value'] for d, r in zip(dec_tx, result)] change = [d['vout'][r['changepos']]['value'] for d, r in zip(dec_tx, result)] @@ -927,7 +927,7 @@ class RawTransactionsTest(BitcoinTestFramework): result = [self.nodes[3].fundrawtransaction(rawtx), # Split the fee between outputs 0, 2, and 3, but not output 1. - self.nodes[3].fundrawtransaction(rawtx, {"subtractFeeFromOutputs": [0, 2, 3]})] + self.nodes[3].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0, 2, 3])] dec_tx = [self.nodes[3].decoderawtransaction(result[0]['hex']), self.nodes[3].decoderawtransaction(result[1]['hex'])] @@ -969,7 +969,7 @@ class RawTransactionsTest(BitcoinTestFramework): vout = find_vout_for_address(self.nodes[0], txid, addr) rawtx = self.nodes[0].createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(): 5}]) - fundedtx = self.nodes[0].fundrawtransaction(rawtx, {'subtractFeeFromOutputs': [0]}) + fundedtx = self.nodes[0].fundrawtransaction(rawtx, subtractFeeFromOutputs=[0]) signedtx = self.nodes[0].signrawtransactionwithwallet(fundedtx['hex']) self.nodes[0].sendrawtransaction(signedtx['hex']) @@ -1027,25 +1027,25 @@ class RawTransactionsTest(BitcoinTestFramework): assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.fundrawtransaction, raw_tx) # Error conditions - assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["not a pubkey"]}}) - assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["01234567890a0b0c0d0e0f"]}}) - assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"scripts":["not a script"]}}) - assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, {"solving_data": {"descriptors":["not a descriptor"]}}) - assert_raises_rpc_error(-8, "Invalid parameter, missing vout key", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"]}]}) - assert_raises_rpc_error(-8, "Invalid parameter, vout cannot be negative", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": -1}]}) - assert_raises_rpc_error(-8, "Invalid parameter, missing weight key", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"]}]}) - assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be less than 165", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 164}]}) - assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be less than 165", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": -1}]}) - assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be greater than", wallet.fundrawtransaction, raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 400001}]}) + assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["not a pubkey"]}) + assert_raises_rpc_error(-5, "'01234567890a0b0c0d0e0f' is not a valid public key", wallet.fundrawtransaction, raw_tx, solving_data={"pubkeys":["01234567890a0b0c0d0e0f"]}) + assert_raises_rpc_error(-5, "'not a script' is not hex", wallet.fundrawtransaction, raw_tx, solving_data={"scripts":["not a script"]}) + assert_raises_rpc_error(-8, "Unable to parse descriptor 'not a descriptor'", wallet.fundrawtransaction, raw_tx, solving_data={"descriptors":["not a descriptor"]}) + assert_raises_rpc_error(-8, "Invalid parameter, missing vout key", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"]}]) + assert_raises_rpc_error(-8, "Invalid parameter, vout cannot be negative", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": -1}]) + assert_raises_rpc_error(-8, "Invalid parameter, missing weight key", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"]}]) + assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be less than 165", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 164}]) + assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be less than 165", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": -1}]) + assert_raises_rpc_error(-8, "Invalid parameter, weight cannot be greater than", wallet.fundrawtransaction, raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 400001}]) # But funding should work when the solving data is provided - funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}}) + funded_tx = wallet.fundrawtransaction(raw_tx, solving_data={"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"]]}) signed_tx = wallet.signrawtransactionwithwallet(funded_tx['hex']) assert not signed_tx['complete'] signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx['hex']) assert signed_tx['complete'] - funded_tx = wallet.fundrawtransaction(raw_tx, {"solving_data": {"descriptors": [desc]}}) + funded_tx = wallet.fundrawtransaction(raw_tx, solving_data={"descriptors": [desc]}) signed_tx1 = wallet.signrawtransactionwithwallet(funded_tx['hex']) assert not signed_tx1['complete'] signed_tx2 = self.nodes[0].signrawtransactionwithwallet(signed_tx1['hex']) @@ -1060,30 +1060,30 @@ class RawTransactionsTest(BitcoinTestFramework): high_input_weight = input_weight * 2 # Funding should also work if the input weight is provided - funded_tx = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": input_weight}]}) + funded_tx = wallet.fundrawtransaction(raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": input_weight}]) signed_tx = wallet.signrawtransactionwithwallet(funded_tx["hex"]) signed_tx = self.nodes[0].signrawtransactionwithwallet(signed_tx["hex"]) assert_equal(self.nodes[0].testmempoolaccept([signed_tx["hex"]])[0]["allowed"], True) assert_equal(signed_tx["complete"], True) # Reducing the weight should have a lower fee - funded_tx2 = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": low_input_weight}]}) + funded_tx2 = wallet.fundrawtransaction(raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": low_input_weight}]) assert_greater_than(funded_tx["fee"], funded_tx2["fee"]) # Increasing the weight should have a higher fee - funded_tx2 = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}]}) + funded_tx2 = wallet.fundrawtransaction(raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}]) assert_greater_than(funded_tx2["fee"], funded_tx["fee"]) # The provided weight should override the calculated weight when solving data is provided - funded_tx3 = wallet.fundrawtransaction(raw_tx, {"solving_data": {"descriptors": [desc]}, "input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}]}) + funded_tx3 = wallet.fundrawtransaction(raw_tx, solving_data={"descriptors": [desc]}, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}]) assert_equal(funded_tx2["fee"], funded_tx3["fee"]) # The feerate should be met - funded_tx4 = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}], "fee_rate": 10}) + funded_tx4 = wallet.fundrawtransaction(raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": high_input_weight}], fee_rate=10) input_add_weight = high_input_weight - (41 * 4) tx4_weight = wallet.decoderawtransaction(funded_tx4["hex"])["weight"] + input_add_weight tx4_vsize = int(ceil(tx4_weight / 4)) assert_fee_amount(funded_tx4["fee"], tx4_vsize, Decimal(0.0001)) # Funding with weight at csuint boundaries should not cause problems - funded_tx = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 255}]}) - funded_tx = wallet.fundrawtransaction(raw_tx, {"input_weights": [{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 65539}]}) + funded_tx = wallet.fundrawtransaction(raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 255}]) + funded_tx = wallet.fundrawtransaction(raw_tx, input_weights=[{"txid": ext_utxo["txid"], "vout": ext_utxo["vout"], "weight": 65539}]) self.nodes[2].unloadwallet("extfund") @@ -1123,7 +1123,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Fund wallet with 2 outputs, 5 BTC each. addr2 = wallet.getnewaddress(address_type="bech32") - source_tx = self.nodes[0].send(outputs=[{addr1: 5}, {addr2: 5}], options={"change_position": 0}) + source_tx = self.nodes[0].send(outputs=[{addr1: 5}, {addr2: 5}], change_position=0) self.generate(self.nodes[0], 1) # Select only one input. @@ -1135,12 +1135,12 @@ class RawTransactionsTest(BitcoinTestFramework): } ] } - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 8}], options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 8}], **options) # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) options["add_inputs"] = True options["add_to_wallet"] = False - tx = wallet.send(outputs=[{addr1: 8}], options=options) + tx = wallet.send(outputs=[{addr1: 8}], **options) assert tx["complete"] # Case (4), Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount) @@ -1148,7 +1148,7 @@ class RawTransactionsTest(BitcoinTestFramework): "txid": source_tx["txid"], "vout": 2 # change position was hardcoded to index 0 }) - tx = wallet.send(outputs=[{addr1: 8}], options=options) + tx = wallet.send(outputs=[{addr1: 8}], **options) assert tx["complete"] # Check that only the preset inputs were added to the tx decoded_psbt_inputs = self.nodes[0].decodepsbt(tx["psbt"])['tx']['vin'] @@ -1158,12 +1158,12 @@ class RawTransactionsTest(BitcoinTestFramework): # Case (5), assert that inputs are added to the tx by explicitly setting add_inputs=true options = {"add_inputs": True, "add_to_wallet": True} - tx = wallet.send(outputs=[{addr1: 8}], options=options) + tx = wallet.send(outputs=[{addr1: 8}], **options) assert tx["complete"] # 6. Explicit add_inputs=false, no preset inputs: options = {"add_inputs": False} - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 3}], options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.send, outputs=[{addr1: 3}], **options) ################################################ @@ -1184,14 +1184,14 @@ class RawTransactionsTest(BitcoinTestFramework): # Case (3), Explicit add_inputs=true and preset inputs (with preset inputs not-covering the target amount) options["add_inputs"] = True - assert "psbt" in wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, options=options) + assert "psbt" in wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, **options) # Case (4), Explicit add_inputs=true and preset inputs (with preset inputs covering the target amount) inputs.append({ "txid": source_tx["txid"], "vout": 2 # change position was hardcoded to index 0 }) - psbt_tx = wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, options=options) + psbt_tx = wallet.walletcreatefundedpsbt(outputs=[{addr1: 8}], inputs=inputs, **options) # Check that only the preset inputs were added to the tx decoded_psbt_inputs = self.nodes[0].decodepsbt(psbt_tx["psbt"])['tx']['vin'] assert_equal(len(decoded_psbt_inputs), 2) @@ -1203,11 +1203,11 @@ class RawTransactionsTest(BitcoinTestFramework): options = { "add_inputs": True } - assert "psbt" in wallet.walletcreatefundedpsbt(inputs=[], outputs=outputs, options=options) + assert "psbt" in wallet.walletcreatefundedpsbt(inputs=[], outputs=outputs, **options) # Case (6). Explicit add_inputs=false, no preset inputs: options = {"add_inputs": False} - assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, options=options) + assert_raises_rpc_error(-4, ERR_NOT_ENOUGH_PRESET_INPUTS, wallet.walletcreatefundedpsbt, inputs=[], outputs=outputs, **options) self.nodes[2].unloadwallet("test_preset_inputs") @@ -1271,7 +1271,7 @@ class RawTransactionsTest(BitcoinTestFramework): self.generate(self.nodes[0], 1) rawtx = wallet.createrawtransaction([{'txid': txid, 'vout': vout}], [{self.nodes[0].getnewaddress(address_type="bech32"): 8}]) - fundedtx = wallet.fundrawtransaction(rawtx, {'fee_rate': 10, "change_type": "bech32"}) + fundedtx = wallet.fundrawtransaction(rawtx, fee_rate=10, change_type="bech32") # with 71-byte signatures we should expect following tx size # tx overhead (10) + 2 inputs (41 each) + 2 p2wpkh (31 each) + (segwit marker and flag (2) + 2 p2wpkh 71 byte sig witnesses (107 each)) / witness scaling factor (4) tx_size = ceil(10 + 41*2 + 31*2 + (2 + 107*2)/4) @@ -1280,7 +1280,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Using the other output should have 72 byte sigs rawtx = wallet.createrawtransaction([{'txid': txid, 'vout': ext_vout}], [{self.nodes[0].getnewaddress(): 13}]) ext_desc = self.nodes[0].getaddressinfo(ext_addr)["desc"] - fundedtx = wallet.fundrawtransaction(rawtx, {'fee_rate': 10, "change_type": "bech32", "solving_data": {"descriptors": [ext_desc]}}) + fundedtx = wallet.fundrawtransaction(rawtx, fee_rate=10, change_type="bech32", solving_data={"descriptors": [ext_desc]}) # tx overhead (10) + 3 inputs (41 each) + 2 p2wpkh(31 each) + (segwit marker and flag (2) + 2 p2wpkh 71 bytes sig witnesses (107 each) + p2wpkh 72 byte sig witness (108)) / witness scaling factor (4) tx_size = ceil(10 + 41*3 + 31*2 + (2 + 107*2 + 108)/4) assert_equal(fundedtx['fee'] * COIN, tx_size * 10) @@ -1307,7 +1307,7 @@ class RawTransactionsTest(BitcoinTestFramework): assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, rawtx) # But we can opt-in to use them for funding. - fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": True}) + fundedtx = wallet.fundrawtransaction(rawtx, include_unsafe=True) tx_dec = wallet.decoderawtransaction(fundedtx['hex']) assert all((txin["txid"], txin["vout"]) in inputs for txin in tx_dec["vin"]) signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) @@ -1315,7 +1315,7 @@ class RawTransactionsTest(BitcoinTestFramework): # And we can also use them once they're confirmed. self.generate(self.nodes[0], 1) - fundedtx = wallet.fundrawtransaction(rawtx, {"include_unsafe": False}) + fundedtx = wallet.fundrawtransaction(rawtx, include_unsafe=False) tx_dec = wallet.decoderawtransaction(fundedtx['hex']) assert all((txin["txid"], txin["vout"]) in inputs for txin in tx_dec["vin"]) signedtx = wallet.signrawtransactionwithwallet(fundedtx['hex']) @@ -1350,7 +1350,7 @@ class RawTransactionsTest(BitcoinTestFramework): # Create transactions in order to calculate fees for the target bounds that can trigger this bug change_tx = tester.fundrawtransaction(tester.createrawtransaction([], [{funds.getnewaddress(): 1.5}])) tx = tester.createrawtransaction([], [{funds.getnewaddress(): 2}]) - no_change_tx = tester.fundrawtransaction(tx, {"subtractFeeFromOutputs": [0]}) + no_change_tx = tester.fundrawtransaction(tx, subtractFeeFromOutputs=[0]) overhead_fees = feerate * len(tx) / 2 / 1000 cost_of_change = change_tx["fee"] - no_change_tx["fee"] @@ -1402,7 +1402,7 @@ class RawTransactionsTest(BitcoinTestFramework): # To test this does not happen, we subtract 202 sats from the input value. If working correctly, this should # fail with insufficient funds rather than bitcoind asserting. rawtx = w.createrawtransaction(inputs=[], outputs=[{self.nodes[0].getnewaddress(address_type="bech32"): 1 - 0.00000202}]) - assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, {"fee_rate": 1.85}) + assert_raises_rpc_error(-4, "Insufficient funds", w.fundrawtransaction, rawtx, fee_rate=1.85) def test_input_confs_control(self): self.nodes[0].createwallet("minconf") diff --git a/test/functional/wallet_import_rescan.py b/test/functional/wallet_import_rescan.py index 211e939a394..0ac67607e19 100755 --- a/test/functional/wallet_import_rescan.py +++ b/test/functional/wallet_import_rescan.py @@ -75,7 +75,7 @@ class Variant(collections.namedtuple("Variant", "call data address_type rescan p request.update({"redeemscript": self.address['embedded']['scriptPubKey']}) response = self.node.importmulti( requests=[request], - options={"rescan": self.rescan in (Rescan.yes, Rescan.late_timestamp)}, + rescan=self.rescan in (Rescan.yes, Rescan.late_timestamp), ) assert_equal(response, [{"success": True}]) diff --git a/test/functional/wallet_keypool.py b/test/functional/wallet_keypool.py index bd978511536..a39db3bfb8e 100755 --- a/test/functional/wallet_keypool.py +++ b/test/functional/wallet_keypool.py @@ -178,30 +178,30 @@ class KeyPoolTest(BitcoinTestFramework): # Using a fee rate (10 sat / byte) well above the minimum relay rate # creating a 5,000 sat transaction with change should not be possible - assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010}) + assert_raises_rpc_error(-4, "Transaction needs a change address, but we can't generate it.", w2.walletcreatefundedpsbt, inputs=[], outputs=[{addr.pop(): 0.00005000}], subtractFeeFromOutputs=[0], feeRate=0.00010) # creating a 10,000 sat transaction without change, with a manual input, should still be possible - res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010}) + res = w2.walletcreatefundedpsbt(inputs=w2.listunspent(), outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.00010) assert_equal("psbt" in res, True) # creating a 10,000 sat transaction without change should still be possible - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.00010) assert_equal("psbt" in res, True) # should work without subtractFeeFromOutputs if the exact fee is subtracted from the amount - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008900}], options={"feeRate": 0.00010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008900}], feeRate=0.00010) assert_equal("psbt" in res, True) # dust change should be removed - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008800}], options={"feeRate": 0.00010}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00008800}], feeRate=0.00010) assert_equal("psbt" in res, True) # create a transaction without change at the maximum fee rate, such that the output is still spendable: - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.0008823}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.0008823) assert_equal("psbt" in res, True) assert_equal(res["fee"], Decimal("0.00009706")) # creating a 10,000 sat transaction with a manual change address should be possible - res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], options={"subtractFeeFromOutputs": [0], "feeRate": 0.00010, "changeAddress": addr.pop()}) + res = w2.walletcreatefundedpsbt(inputs=[], outputs=[{destination: 0.00010000}], subtractFeeFromOutputs=[0], feeRate=0.00010, changeAddress=addr.pop()) assert_equal("psbt" in res, True) if not self.options.descriptors: diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 7c2959bb895..d9d4d542297 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -281,7 +281,7 @@ class WalletMigrationTest(BitcoinTestFramework): imports0.importaddress(import_sent_addr) received_sent_watchonly_txid = default.sendtoaddress(import_sent_addr, 10) received_sent_watchonly_vout = find_vout_for_address(self.nodes[0], received_sent_watchonly_txid, import_sent_addr) - send = default.sendall(recipients=[default.getnewaddress()], options={"inputs": [{"txid": received_sent_watchonly_txid, "vout": received_sent_watchonly_vout}]}) + send = default.sendall(recipients=[default.getnewaddress()], inputs=[{"txid": received_sent_watchonly_txid, "vout": received_sent_watchonly_vout}]) sent_watchonly_txid = send["txid"] self.generate(self.nodes[0], 1) diff --git a/test/functional/wallet_multisig_descriptor_psbt.py b/test/functional/wallet_multisig_descriptor_psbt.py index 12069fb00df..28bee1911e0 100755 --- a/test/functional/wallet_multisig_descriptor_psbt.py +++ b/test/functional/wallet_multisig_descriptor_psbt.py @@ -121,7 +121,7 @@ class WalletMultisigDescriptorPSBTTest(BitcoinTestFramework): to = participants["signers"][self.N - 1].getnewaddress() value = 1 self.log.info("First, make a sending transaction, created using `walletcreatefundedpsbt` (anyone can initiate this)...") - psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, options={"feeRate": 0.00010}) + psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010) psbts = [] self.log.info("Now at least M users check the psbt with decodepsbt and (if OK) signs it with walletprocesspsbt...") @@ -143,7 +143,7 @@ class WalletMultisigDescriptorPSBTTest(BitcoinTestFramework): assert_equal(participants["signers"][self.N - 1].getbalance(), value) self.log.info("Send another transaction from the multisig, this time with a daisy chained signing flow (one after another in series)!") - psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, options={"feeRate": 0.00010}) + psbt = participants["multisigs"][0].walletcreatefundedpsbt(inputs=[], outputs={to: value}, feeRate=0.00010) for m in range(self.M): signers_multisig = participants["multisigs"][m] self._check_psbt(psbt["psbt"], to, value, signers_multisig) diff --git a/test/functional/wallet_resendwallettransactions.py b/test/functional/wallet_resendwallettransactions.py index 7e4a4002b2c..7bdb6f5e3a1 100755 --- a/test/functional/wallet_resendwallettransactions.py +++ b/test/functional/wallet_resendwallettransactions.py @@ -35,7 +35,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): self.log.info("Create a new transaction and wait until it's broadcast") parent_utxo, indep_utxo = node.listunspent()[:2] addr = node.getnewaddress() - txid = node.send(outputs=[{addr: 1}], options={"inputs": [parent_utxo]})["txid"] + txid = node.send(outputs=[{addr: 1}], inputs=[parent_utxo])["txid"] # Can take a few seconds due to transaction trickling peer_first.wait_for_broadcast([txid]) @@ -86,7 +86,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): # ordering of mapWallet is, if the child is not before the parent, we will create a new # child (via bumpfee) and remove the old child (via removeprunedfunds) until we get the # ordering of child before parent. - child_txid = node.send(outputs=[{addr: 0.5}], options={"inputs": [{"txid":txid, "vout":0}]})["txid"] + child_txid = node.send(outputs=[{addr: 0.5}], inputs=[{"txid":txid, "vout":0}])["txid"] while True: txids = node.listreceivedbyaddress(minconf=0, address_filter=addr)[0]["txids"] if txids == [child_txid, txid]: @@ -111,7 +111,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework): # Evict these txs from the mempool evict_time = block_time + 60 * 60 * DEFAULT_MEMPOOL_EXPIRY_HOURS + 5 node.setmocktime(evict_time) - indep_send = node.send(outputs=[{node.getnewaddress(): 1}], options={"inputs": [indep_utxo]}) + indep_send = node.send(outputs=[{node.getnewaddress(): 1}], inputs=[indep_utxo]) node.getmempoolentry(indep_send["txid"]) assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, txid) assert_raises_rpc_error(-5, "Transaction not in mempool", node.getmempoolentry, child_txid) diff --git a/test/functional/wallet_sendall.py b/test/functional/wallet_sendall.py index f6440f07d75..c2b800df218 100755 --- a/test/functional/wallet_sendall.py +++ b/test/functional/wallet_sendall.py @@ -151,7 +151,7 @@ class SendallTest(BitcoinTestFramework): self.log.info("Test sending more than balance") pre_sendall_balance = self.add_utxos([7, 14]) - expected_tx = self.wallet.sendall(recipients=[{self.recipient: 5}, self.remainder_target], options={"add_to_wallet": False}) + expected_tx = self.wallet.sendall(recipients=[{self.recipient: 5}, self.remainder_target], add_to_wallet=False) tx = self.wallet.decoderawtransaction(expected_tx['hex']) fee = 21 - sum([o["value"] for o in tx["vout"]]) @@ -190,7 +190,7 @@ class SendallTest(BitcoinTestFramework): self.add_utxos([0.00000400, 0.00000300, 1]) # sendall with send_max - sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], fee_rate=300, options={"send_max": True}) + sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], fee_rate=300, send_max=True) tx_from_wallet = self.wallet.gettransaction(txid = sendall_tx_receipt["txid"], verbose = True) assert_equal(len(tx_from_wallet["decoded"]["vin"]), 1) @@ -206,7 +206,7 @@ class SendallTest(BitcoinTestFramework): self.add_utxos([17, 4]) utxo = self.wallet.listunspent()[0] - sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], options={"inputs": [utxo]}) + sendall_tx_receipt = self.wallet.sendall(recipients=[self.remainder_target], inputs=[utxo]) tx_from_wallet = self.wallet.gettransaction(txid = sendall_tx_receipt["txid"], verbose = True) assert_equal(len(tx_from_wallet["decoded"]["vin"]), 1) assert_equal(len(tx_from_wallet["decoded"]["vout"]), 1) @@ -227,26 +227,26 @@ class SendallTest(BitcoinTestFramework): # fails on out of bounds vout assert_raises_rpc_error(-8, "Input not found. UTXO ({}:{}) is not part of wallet.".format(spent_utxo["txid"], 1000), - self.wallet.sendall, recipients=[self.remainder_target], options={"inputs": [{"txid": spent_utxo["txid"], "vout": 1000}]}) + self.wallet.sendall, recipients=[self.remainder_target], inputs=[{"txid": spent_utxo["txid"], "vout": 1000}]) # fails on unconfirmed spent UTXO self.wallet.sendall(recipients=[self.remainder_target]) assert_raises_rpc_error(-8, "Input not available. UTXO ({}:{}) was already spent.".format(spent_utxo["txid"], spent_utxo["vout"]), - self.wallet.sendall, recipients=[self.remainder_target], options={"inputs": [spent_utxo]}) + self.wallet.sendall, recipients=[self.remainder_target], inputs=[spent_utxo]) # fails on specific previously spent UTXO, while other UTXOs exist self.generate(self.nodes[0], 1) self.add_utxos([19, 2]) assert_raises_rpc_error(-8, "Input not available. UTXO ({}:{}) was already spent.".format(spent_utxo["txid"], spent_utxo["vout"]), - self.wallet.sendall, recipients=[self.remainder_target], options={"inputs": [spent_utxo]}) + self.wallet.sendall, recipients=[self.remainder_target], inputs=[spent_utxo]) # fails because UTXO is unknown, while other UTXOs exist foreign_utxo = self.def_wallet.listunspent()[0] assert_raises_rpc_error(-8, "Input not found. UTXO ({}:{}) is not part of wallet.".format(foreign_utxo["txid"], foreign_utxo["vout"]), self.wallet.sendall, recipients=[self.remainder_target], - options={"inputs": [foreign_utxo]}) + inputs=[foreign_utxo]) @cleanup def sendall_fails_on_no_address(self): @@ -270,7 +270,7 @@ class SendallTest(BitcoinTestFramework): "Cannot combine send_max with specific inputs.", self.wallet.sendall, recipients=[self.remainder_target], - options={"inputs": [utxo], "send_max": True}) + inputs=[utxo], send_max=True) @cleanup def sendall_fails_on_high_fee(self): @@ -308,7 +308,7 @@ class SendallTest(BitcoinTestFramework): else: watchonly.importmulti(import_req) - sendall_tx_receipt = watchonly.sendall(recipients=[self.remainder_target], options={"inputs": [utxo]}) + sendall_tx_receipt = watchonly.sendall(recipients=[self.remainder_target], inputs=[utxo]) psbt = sendall_tx_receipt["psbt"] decoded = self.nodes[0].decodepsbt(psbt) assert_equal(len(decoded["inputs"]), 1) diff --git a/test/functional/wallet_signer.py b/test/functional/wallet_signer.py index 8d25044e433..7a41b646b52 100755 --- a/test/functional/wallet_signer.py +++ b/test/functional/wallet_signer.py @@ -211,13 +211,13 @@ class WalletSignerTest(BitcoinTestFramework): self.log.info('Test send using hww1') # Don't broadcast transaction yet so the RPC returns the raw hex - res = hww.send(outputs={dest:0.5},options={"add_to_wallet": False}) + res = hww.send(outputs={dest:0.5},add_to_wallet=False) assert res["complete"] assert_equal(res["hex"], mock_tx) self.log.info('Test sendall using hww1') - res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()],options={"add_to_wallet": False}) + res = hww.sendall(recipients=[{dest:0.5}, hww.getrawchangeaddress()], add_to_wallet=False) assert res["complete"] assert_equal(res["hex"], mock_tx) # Broadcast transaction so we can bump the fee diff --git a/test/functional/wallet_taproot.py b/test/functional/wallet_taproot.py index b52892704f2..a5d7445ce8d 100755 --- a/test/functional/wallet_taproot.py +++ b/test/functional/wallet_taproot.py @@ -378,7 +378,7 @@ class WalletTaprootTest(BitcoinTestFramework): assert psbt_online.gettransaction(txid)['confirmations'] > 0 # Cleanup - psbt = psbt_online.sendall(recipients=[self.boring.getnewaddress()], options={"psbt": True})["psbt"] + psbt = psbt_online.sendall(recipients=[self.boring.getnewaddress()], psbt=True)["psbt"] res = psbt_offline.walletprocesspsbt(psbt=psbt, finalize=False) rawtx = self.nodes[0].finalizepsbt(res['psbt'])['hex'] txid = self.nodes[0].sendrawtransaction(rawtx) diff --git a/test/functional/wallet_timelock.py b/test/functional/wallet_timelock.py index 57a7c3907ad..0a622979a49 100755 --- a/test/functional/wallet_timelock.py +++ b/test/functional/wallet_timelock.py @@ -29,7 +29,7 @@ class WalletLocktimeTest(BitcoinTestFramework): self.log.info("Send to new address with locktime") node.send( outputs={address: 5}, - options={"locktime": mtp_tip - 1}, + locktime=mtp_tip - 1, ) self.generate(node, 1) diff --git a/test/functional/wallet_watchonly.py b/test/functional/wallet_watchonly.py index dd4514318cc..b473f5d65c1 100755 --- a/test/functional/wallet_watchonly.py +++ b/test/functional/wallet_watchonly.py @@ -98,13 +98,13 @@ class CreateWalletWatchonlyTest(BitcoinTestFramework): options = {'changeAddress': wo_change} no_wo_options = {'changeAddress': wo_change, 'includeWatching': False} - result = wo_wallet.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, options=options) + result = wo_wallet.walletcreatefundedpsbt(inputs=inputs, outputs=outputs, **options) assert_equal("psbt" in result, True) assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.walletcreatefundedpsbt, inputs, outputs, 0, no_wo_options) self.log.info('Testing fundrawtransaction watch-only defaults') rawtx = wo_wallet.createrawtransaction(inputs=inputs, outputs=outputs) - result = wo_wallet.fundrawtransaction(hexstring=rawtx, options=options) + result = wo_wallet.fundrawtransaction(hexstring=rawtx, **options) assert_equal("hex" in result, True) assert_raises_rpc_error(-4, "Insufficient funds", wo_wallet.fundrawtransaction, rawtx, no_wo_options) From 2cd28e9fef5dd743bcd70025196ee311fcfdcae4 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Wed, 19 Apr 2023 15:02:38 -0400 Subject: [PATCH 4/4] rpc: Add check for unintended option/parameter name clashes Also add flag to allow RPC methods that intendionally accept options and parameters with the same name bypass the check. Check and flag were suggested by ajtowns https://github.com/bitcoin/bitcoin/pull/26485#issuecomment-1507916357 Co-authored-by: Anthony Towns --- src/rpc/util.cpp | 24 ++++++++++++++++++-- src/rpc/util.h | 9 ++++++++ src/test/rpc_tests.cpp | 47 ++++++++++++++++++++++++++++++++++++++++ src/wallet/rpc/spend.cpp | 8 +++---- 4 files changed, 82 insertions(+), 6 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 2fa1732faaf..dd6b25fba5f 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -486,12 +486,32 @@ RPCHelpMan::RPCHelpMan(std::string name, std::string description, std::vector 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 param_names; + for (const auto& arg : m_args) { std::vector 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 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) { diff --git a/src/rpc/util.h b/src/rpc/util.h index d017812e97a..b10307e3141 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 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 { diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index f8975dd9b29..7610d4fa399 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -528,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> param_names) { + std::vector params; + std::vector 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/spend.cpp b/src/wallet/rpc/spend.cpp index 3f98f93a90a..5945c660007 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -453,9 +453,9 @@ RPCHelpMan settxfee() static std::vector FundTxDoc(bool solving_data = true) { std::vector 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" @@ -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."}, @@ -1306,7 +1306,7 @@ RPCHelpMan sendall() Cat>( { {"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."},