Merge 40143bafb5 into a50af6e4c4

src/rpc/client.cpp

@@ -170,7 +170,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"},

src/rpc/request.h

@@ -9,6 +9,7 @@
 #include <any>
 #include <optional>
 #include <string>
+#include <vector>
 
 #include <univalue.h>
 #include <util/fs.h>
@@ -38,6 +39,9 @@ public:
     std::optional<UniValue> 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<std::optional<std::string>> param_names;
     enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;
     std::string URI;
     std::string authUser;

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<std::string> 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()) {

src/rpc/util.cpp

@@ -671,7 +671,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));
         }
@@ -870,9 +870,15 @@ UniValue RPCHelpMan::GetArgMap() const
     for (int i{0}; i < int(m_args.size()); ++i) {
         const auto& arg = m_args.at(i);
         std::vector<std::string> arg_names = SplitString(arg.m_names, '|');
+        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_per_name.empty()) {
-            if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
+                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<std::string> inner_names = SplitString(inner.m_names, '|');
                     for (const std::string& inner_name : inner_names) {
@@ -919,17 +925,25 @@ static std::optional<UniValue::VType> ExpectedType(RPCArg::Type type)
     NONFATAL_UNREACHABLE();
 }
 
-UniValue RPCArg::MatchesType(const UniValue& request) const
+UniValue RPCArg::MatchesType(const UniValue& request, const std::optional<std::string>& param_name) const
 {
     if (m_opts.skip_type_check) return true;
     if (IsOptional() && request.isNull()) return true;
-    const auto exp_type{ExpectedType(m_type)};
+    const auto names = SplitString(m_names, '|');
-    if (!exp_type) return true; // nothing to check
+    size_t i = 0;
-
+    do {
-    if (*exp_type != request.getType()) {
+        // If parameter was passed by name, only allow the specified type for
-        return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type));
+        // that name. Otherwise allow any of the specified types.
-    }
+        if (param_name && i < names.size() && *param_name != names[i]) {
-    return true;
+            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)));
 }
 
 std::string RPCArg::GetFirstName() const

src/rpc/util.h

@@ -18,6 +18,7 @@
 #include <univalue.h>
 #include <util/check.h>
 
+#include <algorithm>
 #include <cstddef>
 #include <cstdint>
 #include <functional>
@@ -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<Type> m_type_per_name;
     const std::vector<RPCArg> 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<Type> types,
+        Fallback fallback,
+        std::string description,
+        std::vector<RPCArg> 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,
@@ -264,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<std::string>& param_name) const;
 
     /** Return the first of all aliases */
     std::string GetFirstName() const;

src/wallet/rpc/spend.cpp

@@ -1582,17 +1582,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)"},
+                    {"options|sign", {RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Type::BOOL}, RPCArg::Optional::OMITTED, "",
-                    {"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"
+                            {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)", RPCArgOptions{.also_positional = true}},
-            "       \"ALL\"\n"
+                            {"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"
-            "       \"NONE\"\n"
+                    "       \"DEFAULT\"\n"
-            "       \"SINGLE\"\n"
+                    "       \"ALL\"\n"
-            "       \"ALL|ANYONECANPAY\"\n"
+                    "       \"NONE\"\n"
-            "       \"NONE|ANYONECANPAY\"\n"
+                    "       \"SINGLE\"\n"
-            "       \"SINGLE|ANYONECANPAY\""},
+                    "       \"ALL|ANYONECANPAY\"\n"
-                    {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"},
+                    "       \"NONE|ANYONECANPAY\"\n"
-                    {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"},
+                    "       \"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, "", "",
@@ -1622,13 +1630,47 @@ RPCHelpMan walletprocesspsbt()
         throw JSONRPCError(RPC_DESERIALIZATION_ERROR, strprintf("TX decode failed %s", error));
     }
 
-    // Get the sighash type
+    // Get options
-    int nHashType = ParseSighashString(request.params[2]);
+    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);

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

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']
 
@@ -945,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)