From 04d7864764e46cd8625b9a5a1ccd271512c6bb64 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Thu, 29 Jul 2021 03:54:17 +0000 Subject: [PATCH 1/4] RPC: Support specifying different types for param aliases --- src/rpc/util.cpp | 22 +++++++++++++++------- src/rpc/util.h | 20 ++++++++++++++++++++ 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index f7690f067ed..5906075c0f8 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -868,9 +868,15 @@ UniValue RPCHelpMan::GetArgMap() const 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, '|'); + RPCArg::Type argtype = arg.m_type; + size_t arg_num = 0; for (const auto& arg_name : arg_names) { - push_back_arg_info(m_name, i, arg_name, arg.m_type); - if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) { + if (!arg.m_type_per_name.empty()) { + argtype = arg.m_type_per_name.at(arg_num++); + } + + push_back_arg_info(m_name, i, arg_name, argtype); + if (argtype == 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) { @@ -921,13 +927,15 @@ UniValue RPCArg::MatchesType(const UniValue& request) const { if (m_opts.skip_type_check) return true; if (IsOptional() && request.isNull()) return true; - const auto exp_type{ExpectedType(m_type)}; - if (!exp_type) return true; // nothing to check + for (auto type : m_type_per_name.empty() ? std::vector{m_type} : m_type_per_name) { + const auto exp_type{ExpectedType(type)}; + if (!exp_type) return true; // nothing to check - if (*exp_type != request.getType()) { - return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type)); + if (*exp_type == request.getType()) { + return true; + } } - return true; + return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*ExpectedType(m_type))); } std::string RPCArg::GetFirstName() const diff --git a/src/rpc/util.h b/src/rpc/util.h index f7268f00012..175b395f843 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -221,6 +222,7 @@ struct RPCArg { const std::string m_names; //!< The name of the arg (can be empty for inner args, can contain multiple aliases separated by | for named request arguments) const Type m_type; + const std::vector m_type_per_name; const std::vector m_inner; //!< Only used for arrays or dicts const Fallback m_fallback; const std::string m_description; @@ -241,6 +243,24 @@ struct RPCArg { CHECK_NONFATAL(type != Type::ARR && type != Type::OBJ && type != Type::OBJ_NAMED_PARAMS && type != Type::OBJ_USER_KEYS); } + RPCArg( + std::string name, + std::vector types, + Fallback fallback, + std::string description, + std::vector inner = {}, + RPCArgOptions opts = {}) + : m_names{std::move(name)}, + m_type{types.at(0)}, + m_type_per_name{std::move(types)}, + m_inner{std::move(inner)}, + m_fallback{std::move(fallback)}, + m_description{std::move(description)}, + m_opts{std::move(opts)} + { + CHECK_NONFATAL(m_type_per_name.size() == size_t(std::count(m_names.begin(), m_names.end(), '|')) + 1); + } + RPCArg( std::string name, Type type, From 28023ff415ca1fa9c2b9c12e6bc525f8bc14aac5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 20 Jul 2021 21:24:56 -0400 Subject: [PATCH 2/4] RPC/Wallet: Convert walletprocesspsbt to use options parameter --- src/rpc/client.cpp | 3 ++ src/wallet/rpc/spend.cpp | 74 +++++++++++++++++++++++++++++-------- test/functional/rpc_help.py | 2 +- test/functional/rpc_psbt.py | 5 +++ 4 files changed, 67 insertions(+), 17 deletions(-) diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp index 43a7a218f66..b6000bcf8da 100644 --- a/src/rpc/client.cpp +++ b/src/rpc/client.cpp @@ -167,7 +167,10 @@ static const CRPCConvertParam vRPCConvertParams[] = { "walletcreatefundedpsbt", 3, "solving_data"}, { "walletcreatefundedpsbt", 3, "max_tx_weight"}, { "walletcreatefundedpsbt", 4, "bip32derivs" }, + { "walletprocesspsbt", 1, "options" }, { "walletprocesspsbt", 1, "sign" }, + { "walletprocesspsbt", 1, "bip32derivs" }, + { "walletprocesspsbt", 1, "finalize" }, { "walletprocesspsbt", 3, "bip32derivs" }, { "walletprocesspsbt", 4, "finalize" }, { "descriptorprocesspsbt", 1, "descriptors"}, diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp index bea9b2eec18..66da74fa54b 100644 --- a/src/wallet/rpc/spend.cpp +++ b/src/wallet/rpc/spend.cpp @@ -1580,17 +1580,25 @@ RPCHelpMan walletprocesspsbt() HELP_REQUIRING_PASSPHRASE, { {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction base64 string"}, - {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)"}, - {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n" - " \"DEFAULT\"\n" - " \"ALL\"\n" - " \"NONE\"\n" - " \"SINGLE\"\n" - " \"ALL|ANYONECANPAY\"\n" - " \"NONE|ANYONECANPAY\"\n" - " \"SINGLE|ANYONECANPAY\""}, - {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"}, - {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"}, + {"options|sign", {RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Type::BOOL}, RPCArg::Optional::OMITTED, "", + { + {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)", RPCArgOptions{.also_positional = true}}, + {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT for Taproot, ALL otherwise"}, "The signature hash type to sign with if not specified by the PSBT. Must be one of\n" + " \"DEFAULT\"\n" + " \"ALL\"\n" + " \"NONE\"\n" + " \"SINGLE\"\n" + " \"ALL|ANYONECANPAY\"\n" + " \"NONE|ANYONECANPAY\"\n" + " \"SINGLE|ANYONECANPAY\"", + RPCArgOptions{.also_positional = true}}, + {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them", RPCArgOptions{.also_positional = true}}, + {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible", RPCArgOptions{.also_positional = true}}, + }, + RPCArgOptions{.oneline_description="options"}}, + {"sighashtype", RPCArg::Type::STR, RPCArg::Default{"DEFAULT"}, "for backwards compatibility", RPCArgOptions{.hidden=true}}, + {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "for backwards compatibility", RPCArgOptions{.hidden=true}}, + {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "for backwards compatibility", RPCArgOptions{.hidden=true}}, }, RPCResult{ RPCResult::Type::OBJ, "", "", @@ -1620,13 +1628,47 @@ RPCHelpMan walletprocesspsbt() throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error)); } - // Get the sighash type - int nHashType = ParseSighashString(request.params[2]); + // Get options + bool sign = true; + bool bip32derivs = true; + bool finalize = true; + int nHashType = ParseSighashString(NullUniValue); // Use ParseSighashString default + if (request.params[1].isBool() || request.params[1].isNull()) { + // Old style positional parameters + sign = request.params[1].isNull() ? true : request.params[1].get_bool(); + nHashType = ParseSighashString(request.params[2]); + bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool(); + finalize = request.params[4].isNull() ? true : request.params[4].get_bool(); + } else { + // New style options are in an object + UniValue options = request.params[1]; + RPCTypeCheckObj(options, + { + {"sign", UniValueType(UniValue::VBOOL)}, + {"bip32derivs", UniValueType(UniValue::VBOOL)}, + {"finalize", UniValueType(UniValue::VBOOL)}, + {"sighashtype", UniValueType(UniValue::VSTR)}, + }, + true, true); + if (options.exists("sign")) { + sign = options["sign"].get_bool(); + } + if (options.exists("bip32derivs")) { + bip32derivs = options["bip32derivs"].get_bool(); + } + if (options.exists("finalize")) { + finalize = options["finalize"].get_bool(); + } + if (options.exists("sighashtype")) { + nHashType = ParseSighashString(options["sighashtype"]); + } + if (request.params.size() > 2) { + // Same behaviour as too many args passed normally + throw std::runtime_error(self.ToString()); + } + } // Fill transaction with our data and also sign - bool sign = request.params[1].isNull() ? true : request.params[1].get_bool(); - bool bip32derivs = request.params[3].isNull() ? true : request.params[3].get_bool(); - bool finalize = request.params[4].isNull() ? true : request.params[4].get_bool(); bool complete = true; if (sign) EnsureWalletIsUnlocked(*pwallet); diff --git a/test/functional/rpc_help.py b/test/functional/rpc_help.py index 4ce24ecb67d..41922b6c0e2 100755 --- a/test/functional/rpc_help.py +++ b/test/functional/rpc_help.py @@ -65,7 +65,7 @@ class HelpRpcTest(BitcoinTestFramework): mapping_server = self.nodes[0].help("dump_all_command_conversions") # Filter all RPCs whether they need conversion - mapping_server_conversion = [tuple(m[:3]) for m in mapping_server if not m[3]] + mapping_server_conversion = set(tuple(m[:3]) for m in mapping_server if not m[3]) # Only check if all RPC methods have been compiled (i.e. wallet is enabled) if self.is_wallet_compiled() and sorted(mapping_client) != sorted(mapping_server_conversion): diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index 8042bdf0715..f366aa6b305 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -281,6 +281,7 @@ class PSBTTest(BitcoinTestFramework): # Sign the transaction but don't finalize processed_psbt = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=False) + assert_equal(processed_psbt, self.nodes[0].walletprocesspsbt(psbtx, {"finalize": False})) assert "hex" not in processed_psbt signed_psbt = processed_psbt['psbt'] @@ -290,6 +291,7 @@ class PSBTTest(BitcoinTestFramework): # Alternative method: sign AND finalize in one command processed_finalized_psbt = self.nodes[0].walletprocesspsbt(psbt=psbtx, finalize=True) + assert_equal(processed_finalized_psbt, self.nodes[0].walletprocesspsbt(psbtx, {"finalize": True})) finalized_psbt = processed_finalized_psbt['psbt'] finalized_psbt_hex = processed_finalized_psbt['hex'] assert signed_psbt != finalized_psbt @@ -496,11 +498,13 @@ class PSBTTest(BitcoinTestFramework): # Update psbts, should only have data for one input and not the other psbt1 = self.nodes[1].walletprocesspsbt(psbt_orig, False, "ALL")['psbt'] + assert_equal(psbt1, self.nodes[1].walletprocesspsbt(psbt_orig, {"sign": False, "sighashtype": "ALL"})["psbt"]) psbt1_decoded = self.nodes[0].decodepsbt(psbt1) assert psbt1_decoded['inputs'][0] and not psbt1_decoded['inputs'][1] # Check that BIP32 path was added assert "bip32_derivs" in psbt1_decoded['inputs'][0] psbt2 = self.nodes[2].walletprocesspsbt(psbt_orig, False, "ALL", False)['psbt'] + assert_equal(psbt2, self.nodes[2].walletprocesspsbt(psbt_orig, {"sign": False, "sighashtype": "ALL", "bip32derivs": False})["psbt"]) psbt2_decoded = self.nodes[0].decodepsbt(psbt2) assert not psbt2_decoded['inputs'][0] and psbt2_decoded['inputs'][1] # Check that BIP32 paths were not added @@ -739,6 +743,7 @@ class PSBTTest(BitcoinTestFramework): # After update with wallet, only needs signing updated = self.nodes[1].walletprocesspsbt(psbt, False, 'ALL', True)['psbt'] + assert_equal(updated, self.nodes[1].walletprocesspsbt(psbt, {"sign": False, "sighashtype": 'ALL', "bip32derivs": True})["psbt"]) analyzed = self.nodes[0].analyzepsbt(updated) assert analyzed['inputs'][0]['has_utxo'] and not analyzed['inputs'][0]['is_final'] and analyzed['inputs'][0]['next'] == 'signer' and analyzed['next'] == 'signer' and analyzed['inputs'][0]['missing']['signatures'][0] == addrinfo['embedded']['witness_program'] From 7cd0315bc407375db11f3686f2dac2370df19f55 Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 12 Jan 2024 10:38:00 +0500 Subject: [PATCH 3/4] RPC: Strictly enforce the type of parameters passed by name --- src/rpc/request.h | 4 ++++ src/rpc/server.cpp | 4 ++++ src/rpc/util.cpp | 18 ++++++++++++------ src/rpc/util.h | 2 +- 4 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/rpc/request.h b/src/rpc/request.h index 24887e8691c..545f053c586 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include @@ -38,6 +39,9 @@ public: std::optional id = UniValue::VNULL; std::string strMethod; UniValue params; + //! List of original parameter names after transformNamedArguments is + //! called and params is changed from an object to an array. + std::vector> param_names; enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE; std::string URI; std::string authUser; diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 34f19df2564..df9dd1a9f40 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -397,9 +397,11 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c for (const auto& [argNamePattern, named_only]: argNames) { std::vector vargNames = SplitString(argNamePattern, '|'); auto fr = argsIn.end(); + std::string fr_name; for (const std::string & argName : vargNames) { fr = argsIn.find(argName); if (fr != argsIn.end()) { + fr_name = argName; break; } } @@ -424,6 +426,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c // but not at the end (for backwards compatibility with calls // that act based on number of specified parameters). out.params.push_back(UniValue()); + out.param_names.emplace_back(std::nullopt); } hole = 0; if (!initial_param) initial_param = &argNamePattern; @@ -440,6 +443,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + fr->first + " conflicts with parameter " + options.getKeys().front()); } out.params.push_back(*fr->second); + out.param_names.emplace_back(fr_name); argsIn.erase(fr); } if (!options.empty()) { diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 5906075c0f8..bd628a92e4b 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -669,7 +669,7 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const UniValue arg_mismatch{UniValue::VOBJ}; for (size_t i{0}; i < m_args.size(); ++i) { const auto& arg{m_args.at(i)}; - UniValue match{arg.MatchesType(request.params[i])}; + UniValue match{arg.MatchesType(request.params[i], i < request.param_names.size() ? request.param_names[i] : std::nullopt)}; if (!match.isTrue()) { arg_mismatch.pushKV(strprintf("Position %s (%s)", i + 1, arg.m_names), std::move(match)); } @@ -923,18 +923,24 @@ static std::optional ExpectedType(RPCArg::Type type) NONFATAL_UNREACHABLE(); } -UniValue RPCArg::MatchesType(const UniValue& request) const +UniValue RPCArg::MatchesType(const UniValue& request, const std::optional& param_name) const { if (m_opts.skip_type_check) return true; if (IsOptional() && request.isNull()) return true; - for (auto type : m_type_per_name.empty() ? std::vector{m_type} : m_type_per_name) { - const auto exp_type{ExpectedType(type)}; + const auto names = SplitString(m_names, '|'); + size_t i = 0; + do { + // If parameter was passed by name, only allow the specified type for + // that name. Otherwise allow any of the specified types. + if (param_name && i < names.size() && *param_name != names[i]) { + continue; + } + const auto exp_type{ExpectedType(i < m_type_per_name.size() ? m_type_per_name[i] : m_type)}; if (!exp_type) return true; // nothing to check - if (*exp_type == request.getType()) { return true; } - } + } while (++i < names.size()); return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*ExpectedType(m_type))); } diff --git a/src/rpc/util.h b/src/rpc/util.h index 175b395f843..5c38219af10 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -284,7 +284,7 @@ struct RPCArg { * Check whether the request JSON type matches. * Returns true if type matches, or object describing error(s) if not. */ - UniValue MatchesType(const UniValue& request) const; + UniValue MatchesType(const UniValue& request, const std::optional& param_name) const; /** Return the first of all aliases */ std::string GetFirstName() const; From 40143bafb52927eb40ecfde0ea722934213eb902 Mon Sep 17 00:00:00 2001 From: Luke Dashjr Date: Sun, 17 Nov 2024 04:34:22 +0000 Subject: [PATCH 4/4] QA: rpc_psbt: Test that the wrong type cannot be given to named params --- test/functional/rpc_psbt.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py index f366aa6b305..868d50d8a1b 100755 --- a/test/functional/rpc_psbt.py +++ b/test/functional/rpc_psbt.py @@ -950,6 +950,16 @@ class PSBTTest(BitcoinTestFramework): self.log.info("Test walletprocesspsbt raises if an invalid sighashtype is passed") assert_raises_rpc_error(-8, "'all' is not a valid sighash parameter.", self.nodes[0].walletprocesspsbt, psbt, sighashtype="all") + self.log.info("Test walletprocesspsbt raises RPC error with options=boolean") + for b in (True, False): + assert_raises_rpc_error(-3, "JSON value of type bool is not of expected type object", self.nodes[0].walletprocesspsbt, psbt, options=b) + + self.log.info("Test walletprocesspsbt raises RPC error with sign={options}") + assert_raises_rpc_error(-3, "JSON value of type object for field sign is not of expected type bool", self.nodes[0].walletprocesspsbt, psbt, sign={'sign': False}) + + self.log.info("Test walletprocesspsbt raises RPC help with both options and non-options sighashtype") + assert_raises_rpc_error(-1, "Arguments:", self.nodes[0].walletprocesspsbt, psbt, {'sign': False}, 'ALL') + self.log.info("Test decoding PSBT with per-input preimage types") # note that the decodepsbt RPC doesn't check whether preimages and hashes match hash_ripemd160, preimage_ripemd160 = randbytes(20), randbytes(50)