RPC/Wallet: Convert walletprocesspsbt to use options parameter

luke-jr commented at 2:03 am on April 25, 2022: member

walletprocesspsbt has a single logical positional argument and a bunch that only make sense as named options. Convert them to an actual options Object for a better positional API.

Retains backward compatibility for now.

DrahtBot added the label RPC/REST/ZMQ on Apr 25, 2022

DrahtBot added the label Wallet on Apr 25, 2022

bitcoin deleted a comment on Apr 25, 2022

vincenzopalazzo approved

vincenzopalazzo commented at 9:13 am on April 25, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/24963/commits/31ffd7782bf241ca09c7e192769ae9506bd2352f

DrahtBot commented at 6:11 pm on April 25, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/24963.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	achow101
Stale ACK	pk-b2, ryanofsky, vincenzopalazzo

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#21283 (Implement BIP 370 PSBTv2 by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pk-b2 approved

pk-b2 commented at 2:40 am on May 6, 2022: none

ACK 31ffd7782bf241ca09c7e192769ae9506bd2352f

Built branch locally and exercised various manual test using both versions of the RPC
Verified that passed in arguments get accepted
Throws error when passing in options argument + additional invalid arguments

maflcko assigned achow101 on May 12, 2022

achow101 commented at 7:45 pm on May 16, 2022: member

tbh I prefer #19762 instead of having options objects, but I don’t feel strongly either way about this PR.

luke-jr commented at 8:14 pm on November 9, 2022: member

tbh I prefer #19762 instead of having options objects, but I don’t feel strongly either way about this PR.

#19762 is complimentary to options objects, not an alternative to them.

achow101 commented at 8:32 pm on February 3, 2023: member

There’s a silent merge conflict with master.

 0wallet/rpc/spend.cpp: In function ‘RPCHelpMan wallet::walletprocesspsbt()’:
 1wallet/rpc/spend.cpp:1515:75: error: ‘OMITTED_NAMED_ARG’ is not a member of ‘RPCArg::Optional’
 2 1515 |                     {"options|sign", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
 3      |                                                                           ^~~~~~~~~~~~~~~~~
 4wallet/rpc/spend.cpp: In lambda function:
 5wallet/rpc/spend.cpp:1574:9: error: ‘RPCTypeCheckArgument’ was not declared in this scope
 6 1574 |         RPCTypeCheckArgument(options, UniValue::VOBJ);
 7      |         ^~~~~~~~~~~~~~~~~~~~
 8wallet/rpc/spend.cpp: In function ‘RPCHelpMan wallet::walletprocesspsbt()’:
 9wallet/rpc/spend.cpp:1619:5: error: no matching function for call to ‘RPCHelpMan::RPCHelpMan(<brace-enclosed initializer list>)’
10 1619 |     };
11      |     ^
12In file included from wallet/rpc/spend.cpp:10:
13./rpc/util.h:364:5: note: candidate: ‘RPCHelpMan::RPCHelpMan(std::string, std::string, std::vector<RPCArg>, RPCResults, RPCExamples, RPCMethodImpl)’
14  364 |     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
15      |     ^~~~~~~~~~
16./rpc/util.h:364:79: note:   no known conversion for argument 3 from ‘<brace-enclosed initializer list>’ to ‘std::vector<RPCArg>’
17  364 |     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
18      |                                                           ~~~~~~~~~~~~~~~~~~~~^~~~
19./rpc/util.h:362:5: note: candidate: ‘RPCHelpMan::RPCHelpMan(std::string, std::string, std::vector<RPCArg>, RPCResults, RPCExamples)’
20  362 |     RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples);
21      |     ^~~~~~~~~~
22./rpc/util.h:362:5: note:   candidate expects 5 arguments, 6 provided
23./rpc/util.h:359:7: note: candidate: ‘RPCHelpMan::RPCHelpMan(const RPCHelpMan&)’
24  359 | class RPCHelpMan
25      |       ^~~~~~~~~~
26./rpc/util.h:359:7: note:   candidate expects 1 argument, 6 provided
27./rpc/util.h:359:7: note: candidate: ‘RPCHelpMan::RPCHelpMan(RPCHelpMan&&)’
28./rpc/util.h:359:7: note:   candidate expects 1 argument, 6 provided

fanquake marked this as a draft on Feb 28, 2023

fanquake commented at 10:26 am on February 28, 2023: member

Drafted for now. Needs a PR description and rebasing to fix the merge conflict:

0wallet/rpc/spend.cpp:1541:75: error: no member named 'OMITTED_NAMED_ARG' in 'RPCArg::Optional'
1                    {"options|sign", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "",
2                                                        ~~~~~~~~~~~~~~~~~~^
3wallet/rpc/spend.cpp:1600:9: error: use of undeclared identifier 'RPCTypeCheckArgument'
4        RPCTypeCheckArgument(options, UniValue::VOBJ);
5        ^
62 errors generated.

luke-jr force-pushed on Jul 22, 2023

luke-jr marked this as ready for review on Jul 22, 2023

luke-jr commented at 3:38 am on July 22, 2023: member

Rebased

luke-jr requested review from pk-b2 on Jul 22, 2023

luke-jr requested review from vincenzopalazzo on Jul 22, 2023

in src/wallet/rpc/spend.cpp:1555 in baf99a9c78 outdated

1556-            "       \"ALL|ANYONECANPAY\"\n"
1557-            "       \"NONE|ANYONECANPAY\"\n"
1558-            "       \"SINGLE|ANYONECANPAY\""},
1559-                    {"bip32derivs", RPCArg::Type::BOOL, RPCArg::Default{true}, "Include BIP 32 derivation paths for public keys if we know them"},
1560-                    {"finalize", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also finalize inputs if possible"},
1561+                    {"options|sign", {RPCArg::Type::OBJ, RPCArg::Type::BOOL}, RPCArg::Optional::OMITTED, "",

ryanofsky commented at 4:29 pm on August 1, 2023:

In commit “RPC/Wallet: Convert walletprocesspsbt to use options parameter” (baf99a9c789779e89e7684a550e16cf1fb7ce862)

I think OBJ should be replaced OBJ_NAMED_PARAMS so generated documentation is consistent with other RPC methods that take options parameters.

Replacing OBJ with OBJ_NAMED_PARAMS will also allow sighashtype and bip32derivs and finalize options to be continue to be accepted as named parameters if the backwards compatibility code below accepting them as positional parameters is removed later (which it probably should be for safety).

luke-jr commented at 11:53 pm on December 27, 2023:

Done

in src/rpc/util.cpp:730 in 8a11b1525f outdated

726-            push_back_arg_info(m_name, i, arg_name, arg.m_type);
727+            if (!arg.m_type_per_name.empty()) {
728+                argtype = arg.m_type_per_name.at(arg_num++);
729+            }
730+            push_back_arg_info(m_name, i, arg_name, argtype);
731             if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {

ryanofsky commented at 4:38 pm on August 1, 2023:

In commit “RPC: Support specifying different types for param aliases” (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)

I think this arg.m_type reference on line 730 also needs to be replaced by argtype.

If we got rid of the RPCArg::m_type member (see other comment) it would avoid this type of bug.

luke-jr commented at 11:51 pm on December 27, 2023:

Fixed

in src/rpc/util.h:225 in 8a11b1525f outdated

182@@ -182,6 +183,7 @@ struct RPCArg {
183 
184     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)
185     const Type m_type;
186+    const std::vector<Type> m_type_per_name;

ryanofsky commented at 4:41 pm on August 1, 2023:

In commit “RPC: Support specifying different types for param aliases” (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)

I think just calling this new struct member m_types would be more consistent with the m_names member. Also I think a followup commit should remove the m_type member which is now confusing and redundant.

in src/rpc/util.cpp:855 in 8a11b1525f outdated

777@@ -773,13 +778,15 @@ UniValue RPCArg::MatchesType(const UniValue& request) const
778 {
779     if (m_opts.skip_type_check) return true;
780     if (IsOptional() && request.isNull()) return true;
781-    const auto exp_type{ExpectedType(m_type)};
782-    if (!exp_type) return true; // nothing to check
783+    for (auto type : m_type_per_name.empty() ? std::vector<RPCArg::Type>{m_type} : m_type_per_name) {

ryanofsky commented at 4:55 pm on August 1, 2023:

In commit “RPC: Support specifying different types for param aliases” (8a11b1525f2fa9f73715db46d228bda1eacdd2b2)

It would be good to have unit test coverage for this new code. Should be easy to add a test creating an RPCArg and calling this method in rpc_tests.cpp.

ryanofsky commented at 5:13 pm on August 1, 2023: contributor

Light code review ACK baf99a9c789779e89e7684a550e16cf1fb7ce862

DrahtBot removed review request from vincenzopalazzo on Aug 1, 2023

DrahtBot removed review request from pk-b2 on Aug 1, 2023

DrahtBot requested review from pk-b2 on Aug 1, 2023

DrahtBot requested review from vincenzopalazzo on Aug 1, 2023

DrahtBot removed review request from vincenzopalazzo on Aug 1, 2023

DrahtBot removed review request from pk-b2 on Aug 1, 2023

DrahtBot requested review from pk-b2 on Aug 1, 2023

DrahtBot requested review from vincenzopalazzo on Aug 1, 2023

DrahtBot added the label Needs rebase on Aug 24, 2023

achow101 removed review request from pk-b2 on Sep 20, 2023

achow101 removed review request from vincenzopalazzo on Sep 20, 2023

achow101 requested review from achow101 on Sep 20, 2023

achow101 unassigned achow101 on Sep 20, 2023

luke-jr force-pushed on Dec 27, 2023

DrahtBot removed the label Needs rebase on Dec 27, 2023

luke-jr force-pushed on Dec 27, 2023

DrahtBot added the label CI failed on Dec 28, 2023

fanquake commented at 11:53 am on December 28, 2023: member

0 2023-12-28T00:06:54.183352Z [scheduler] [util/thread.cpp:22] [TraceThread] scheduler thread exit
1unknown location:0: fatal error: in "psbt_wallet_tests/parse_hd_keypath": NonFatalCheckError: Internal bug detected: !(param_type & POSITIONAL) || inner.m_opts.also_positional
2rpc/util.cpp:536 (RPCHelpMan)

luke-jr force-pushed on Dec 28, 2023

vincenzopalazzo commented at 11:02 pm on December 28, 2023: none

I think the CI is complaing regarding the help command

 0stdout:
 12023-12-28T22:14:56.493000Z TestFramework (INFO): PRNG seed is: 8647031483739335376
 22023-12-28T22:14:56.493000Z TestFramework (INFO): Initializing test directory /ci_container_base/ci/scratch/test_runner/test_runner_₿_🏃_20231228_220750/rpc_help_5
 32023-12-28T22:14:56.823000Z TestFramework (ERROR): Assertion failed
 4Traceback (most recent call last):
 5  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 131, in main
 6    self.run_test()
 7  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_help.py", line 54, in run_test
 8    self.test_client_conversion_table()
 9  File "/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/test/functional/rpc_help.py", line 72, in test_client_conversion_table
10    raise AssertionError("RPC client conversion table ({}) and RPC server named arguments mismatch!\n{}".format(
11AssertionError: RPC client conversion table (/ci_container_base/ci/scratch/build/bitcoin-i686-pc-linux-gnu/src/rpc/client.cpp) and RPC server named arguments mismatch!
12{('walletprocesspsbt', 1, 'bip32derivs'), ('walletprocesspsbt', 1, 'finalize')}

luke-jr force-pushed on Dec 29, 2023

DrahtBot removed the label CI failed on Dec 29, 2023

luke-jr commented at 4:15 am on December 29, 2023: member

There we go, tests all happy now

vincenzopalazzo approved

vincenzopalazzo commented at 10:15 am on December 29, 2023: none

ACK https://github.com/bitcoin/bitcoin/pull/24963/commits/f43f992b7318086859f710b920b4427e6c657fe8

DrahtBot requested review from pk-b2 on Dec 29, 2023

DrahtBot requested review from ryanofsky on Dec 29, 2023

in src/rpc/util.cpp:871 in c2612273ef outdated

792@@ -793,9 +793,15 @@ UniValue RPCHelpMan::GetArgMap() const
793     for (int i{0}; i < int(m_args.size()); ++i) {
794         const auto& arg = m_args.at(i);
795         std::vector<std::string> arg_names = SplitString(arg.m_names, '|');
796+        RPCArg::Type argtype = arg.m_type;

ryanofsky commented at 2:21 pm on January 12, 2024:

In commit “RPC: Support specifying different types for param aliases” (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)

I’m pretty sure you can simplify this and revert all changes to this function. All this is doing is getting the sign parameter type from the outer parameter declaration instead of the inner option declaration, which doesn’t change anything because both types are boolean. The output of “bitcoin-cli -regtest help dump_all_command_conversions” is the same with or without this change.

The following simplification works for me:

 0--- a/src/rpc/util.cpp
 1+++ b/src/rpc/util.cpp
 2@@ -793,15 +793,9 @@ UniValue RPCHelpMan::GetArgMap() const
 3     for (int i{0}; i < int(m_args.size()); ++i) {
 4         const auto& arg = m_args.at(i);
 5         std::vector<std::string> arg_names = SplitString(arg.m_names, '|');
 6-        RPCArg::Type argtype = arg.m_type;
 7-        size_t arg_num = 0;
 8         for (const auto& arg_name : arg_names) {
 9-            if (!arg.m_type_per_name.empty()) {
10-                argtype = arg.m_type_per_name.at(arg_num++);
11-            }
12-
13-            push_back_arg_info(m_name, i, arg_name, argtype);
14-            if (argtype == RPCArg::Type::OBJ_NAMED_PARAMS) {
15+            push_back_arg_info(m_name, i, arg_name, arg.m_type);
16+            if (arg.m_type == RPCArg::Type::OBJ_NAMED_PARAMS) {
17                 for (const auto& inner : arg.m_inner) {
18                     std::vector<std::string> inner_names = SplitString(inner.m_names, '|');
19                     for (const std::string& inner_name : inner_names) {
20--- a/src/rpc/util.h
21+++ b/src/rpc/util.h
22@@ -239,7 +239,6 @@ struct RPCArg {
23           m_description{std::move(description)},
24           m_opts{std::move(opts)}
25     {
26-        CHECK_NONFATAL(m_type_per_name.size() == size_t(std::count(m_names.begin(), m_names.end(), '|')) + 1);
27     }
28 
29     RPCArg(
30--- a/src/wallet/rpc/spend.cpp
31+++ b/src/wallet/rpc/spend.cpp
32@@ -1552,7 +1552,7 @@ RPCHelpMan walletprocesspsbt()
33         HELP_REQUIRING_PASSPHRASE,
34                 {
35                     {"psbt", RPCArg::Type::STR, RPCArg::Optional::NO, "The transaction base64 string"},
36-                    {"options|sign", {RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Type::BOOL}, RPCArg::Optional::OMITTED, "",
37+                    {"options", {RPCArg::Type::OBJ_NAMED_PARAMS, RPCArg::Type::BOOL}, RPCArg::Optional::OMITTED, "",
38                         {
39                             {"sign", RPCArg::Type::BOOL, RPCArg::Default{true}, "Also sign the transaction when updating (requires wallet to be unlocked)", RPCArgOptions{.also_positional = true}},
40                             {"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"

EDIT: Could also drop “sign” .also_positional = true option with this change

luke-jr commented at 3:44 pm on January 26, 2024:

But if there’s only one name here, it’s unclear what name the bool type is for? Or we’re assuming it can only be used as a positional param at that point?

ryanofsky commented at 7:53 pm on January 29, 2024:

re: #24963 (review)

But if there’s only one name here, it’s unclear what name the bool type is for? Or we’re assuming it can only be used as a positional param at that point?

Yes, that’s a good point. The change I suggested to wallet/rpc/spend.cpp makes the code less clear, even if it is removes some code that is technically unnecessary. So your version is better.

In case anybody is following this though, the github comment this is responding to was mostly about RPCHelpMan::GetArgMap, which can still be simplified.

in src/rpc/util.cpp:940 in c2612273ef outdated

858+        const auto exp_type{ExpectedType(type)};
859+        if (!exp_type) return true; // nothing to check
860 
861-    if (*exp_type != request.getType()) {
862-        return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*exp_type));
863+        if (*exp_type == request.getType()) {

ryanofsky commented at 3:38 pm on January 12, 2024:

In commit “RPC: Support specifying different types for param aliases” (c2612273ef0b3ef127ba03c87f2a3303f0b9d299)

I don’t think this check is strict enough, because it isn’t actually verifying the parameter was passed with the corresponding name for the m_type_per_name value. It will accept a parameter with any type found in the vector, not just a parameter with the correct type for its name. This is not good because it means in the walletprocesspsbt case you could pass options=true and it will be interpreted as sign=true, even though the type of options is supposed to be an object, not a bool. I think the only way to fix this is to pass the parameter name to this function, which could be done as follows (untested):

  0--- a/src/rpc/request.h
  1+++ b/src/rpc/request.h
  2@@ -7,7 +7,9 @@
  3 #define BITCOIN_RPC_REQUEST_H
  4 
  5 #include <any>
  6+#include <optional>
  7 #include <string>
  8+#include <vector>
  9 
 10 #include <univalue.h>
 11 
 12@@ -31,6 +33,9 @@ public:
 13     UniValue id;
 14     std::string strMethod;
 15     UniValue params;
 16+    //! List of original parameter names after transformNamedArguments is
 17+    //! called and params is changed from an object to an array.
 18+    std::vector<std::optional<std::string>> param_names;
 19     enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;
 20     std::string URI;
 21     std::string authUser;
 22--- a/src/rpc/server.cpp
 23+++ b/src/rpc/server.cpp
 24@@ -422,9 +422,11 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
 25     for (const auto& [argNamePattern, named_only]: argNames) {
 26         std::vector<std::string> vargNames = SplitString(argNamePattern, '|');
 27         auto fr = argsIn.end();
 28+        std::string fr_name;
 29         for (const std::string & argName : vargNames) {
 30             fr = argsIn.find(argName);
 31             if (fr != argsIn.end()) {
 32+                fr_name = argName;
 33                 break;
 34             }
 35         }
 36@@ -449,6 +451,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
 37                 // but not at the end (for backwards compatibility with calls
 38                 // that act based on number of specified parameters).
 39                 out.params.push_back(UniValue());
 40+                out.param_names.emplace_back(std::nullopt);
 41             }
 42             hole = 0;
 43             if (!initial_param) initial_param = &argNamePattern;
 44@@ -465,6 +468,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
 45                 throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + fr->first + " conflicts with parameter " + options.getKeys().front());
 46             }
 47             out.params.push_back(*fr->second);
 48+            out.param_names.emplace_back(fr_name);
 49             argsIn.erase(fr);
 50         }
 51         if (!options.empty()) {
 52@@ -487,6 +491,7 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
 53         for (size_t i{out.params.size()}; i < named_args.size(); ++i) {
 54             out.params.push_back(named_args[i]);
 55         }
 56+        out.param_names.insert(out.param_names.begin(), positional_args.mapped()->size(), std::nullopt);
 57     }
 58     // If there are still arguments in the argsIn map, this is an error.
 59     if (!argsIn.empty()) {
 60--- a/src/rpc/util.cpp
 61+++ b/src/rpc/util.cpp
 62@@ -606,7 +606,7 @@ UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) const
 63     UniValue arg_mismatch{UniValue::VOBJ};
 64     for (size_t i{0}; i < m_args.size(); ++i) {
 65         const auto& arg{m_args.at(i)};
 66-        UniValue match{arg.MatchesType(request.params[i])};
 67+        UniValue match{arg.MatchesType(request.params[i], i < request.param_names.size() ? request.param_names[i] : std::nullopt)};
 68         if (!match.isTrue()) {
 69             arg_mismatch.pushKV(strprintf("Position %s (%s)", i + 1, arg.m_names), std::move(match));
 70         }
 71@@ -848,18 +848,24 @@ static std::optional<UniValue::VType> ExpectedType(RPCArg::Type type)
 72     NONFATAL_UNREACHABLE();
 73 }
 74 
 75-UniValue RPCArg::MatchesType(const UniValue& request) const
 76+UniValue RPCArg::MatchesType(const UniValue& request, const std::optional<std::string>& param_name) const
 77 {
 78     if (m_opts.skip_type_check) return true;
 79     if (IsOptional() && request.isNull()) return true;
 80-    for (auto type : m_type_per_name.empty() ? std::vector<RPCArg::Type>{m_type} : m_type_per_name) {
 81-        const auto exp_type{ExpectedType(type)};
 82+    const auto names = SplitString(m_names, '|');
 83+    int i = 0;
 84+    do {
 85+        // If parameter was passed by name, only allow the specified type for
 86+        // that name. Otherwise allow any of the specified types.
 87+        if (param_name && i < m_names.size() && *param_name != names[i]) {
 88+            continue;
 89+        }
 90+        const auto exp_type{ExpectedType(i < m_type_per_name.size() ? m_type_per_name[i] : m_type)};
 91         if (!exp_type) return true; // nothing to check
 92-
 93         if (*exp_type == request.getType()) {
 94             return true;
 95         }
 96-    }
 97+    } while (++i < m_names.size());
 98     return strprintf("JSON value of type %s is not of expected type %s", uvTypeName(request.getType()), uvTypeName(*ExpectedType(m_type)));
 99 }
100 
101--- a/src/rpc/util.h
102+++ b/src/rpc/util.h
103@@ -265,7 +265,7 @@ struct RPCArg {
104      * Check whether the request JSON type matches.
105      * Returns true if type matches, or object describing error(s) if not.
106      */
107-    UniValue MatchesType(const UniValue& request) const;
108+    UniValue MatchesType(const UniValue& request, const std::optional<std::string>& param_name) const;
109 
110     /** Return the first of all aliases */
111     std::string GetFirstName() const;

luke-jr commented at 4:35 am on November 17, 2024:

Fixed bug in this and added it, along with tests

ryanofsky commented at 4:18 pm on January 12, 2024: contributor

Code review f43f992b7318086859f710b920b4427e6c657fe8. I think this looks pretty close, and the approach taken in this PR to preserves backwards compatibility seems pretty clean. I would have thought prior to this PR that we might need to break backwards compatibility to convert existing named parameters to options, but this is not the case. Two things I think should be improved:

Lack of enforcement for m_type_per_name vector. Right now the MatchesType implementation just checks if the parameter has any of the specified types, not if the parameter has the right type corresponding to its name. For example you could pass options=true and it would be accepted even though options is supposed to be an object. I left a suggestion to fix this.
Changes to RPCHelpMan::GetArgMap() seem unnecessary and don’t actually affect dump_all_command_conversions output.

I think it could also be nice if this PR was expanded to make the same changes to createwallet as it is making walletprocesspsbt. This came up recently #29129#pullrequestreview-1804913032.

Some notes on how this PR works:

The first commit just loosens up RPCArg type checking so it can accept multiple types (e.g. object and bool) for the same positional parameter. Inadvertently it also allows it to accept multiple types for the named parameters, which is probably not intended and not good in this case, but I suggested a possible fix below.
The second commit takes advantage of less restrictive type checking in walletprocesspsbt to allow the second parameter to be either an options object or a bool, instead of just a bool, and updates the implementation to handle both these cases.
The second commit also moves parameter definitions 2-5 into a nested options object.
To retain backwards compatibility, the second commit adds the old parameters 3-5 as hidden parameters after the options object at position 2, so they can be passed by position. It also specifies .also_positional = true for each option to avoid errors about the outer parameters and inner options having duplicate names. And it raises a too many parameters error if these parameters are specified along with the options object.

DrahtBot removed review request from pk-b2 on Jan 12, 2024

DrahtBot requested review from pk-b2 on Jan 12, 2024

DrahtBot requested review from ryanofsky on Jan 12, 2024

DrahtBot removed review request from pk-b2 on Jan 12, 2024

DrahtBot requested review from pk-b2 on Jan 12, 2024

DrahtBot removed review request from pk-b2 on Jan 12, 2024

DrahtBot requested review from pk-b2 on Jan 12, 2024

DrahtBot removed review request from pk-b2 on Jan 12, 2024

DrahtBot requested review from pk-b2 on Jan 12, 2024

DrahtBot added the label CI failed on Jan 14, 2024

DrahtBot removed review request from pk-b2 on Jan 26, 2024

DrahtBot requested review from pk-b2 on Jan 26, 2024

DrahtBot removed review request from pk-b2 on Jan 29, 2024

DrahtBot requested review from pk-b2 on Jan 29, 2024

Retropex referenced this in commit e5160731d2 on Mar 28, 2024

achow101 commented at 3:19 pm on April 9, 2024: member

Are you still working on this?

achow101 commented at 9:27 pm on April 15, 2024: member

Concept ACK

The code looks fine, but I would like to see @ryanofsky’s comment be addressed.

achow101 removed review request from achow101 on Apr 15, 2024

achow101 removed review request from pk-b2 on Apr 15, 2024

DrahtBot marked this as a draft on Apr 23, 2024

DrahtBot commented at 6:46 am on April 23, 2024: contributor

Converted to draft due to failing CI and inactivity

luke-jr commented at 11:02 pm on November 16, 2024: member

CI failure appears to be a CI bug unrelated to this PR.

DrahtBot requested review from pk-b2 on Nov 16, 2024

luke-jr marked this as ready for review on Nov 16, 2024

RPC: Support specifying different types for param aliases 04d7864764

RPC/Wallet: Convert walletprocesspsbt to use options parameter 28023ff415

RPC: Strictly enforce the type of parameters passed by name 6df20d18f6

QA: rpc_psbt: Test that the wrong type cannot be given to named params 645d041839

luke-jr force-pushed on Nov 17, 2024

luke-jr commented at 4:38 am on November 17, 2024: member

Rebased to workaround CI bugs for now (is someone going to fix them?)

achow101 commented at 4:56 am on November 17, 2024: member

Rebased to workaround CI bugs for now (is someone going to fix them?)

Considering that CI is failing immediately after a walletprocesspsbt call, and that RPC is being changed in this PR, I’m inclined to think that it’s probably not a CI issue.

fixup! RPC: Strictly enforce the type of parameters passed by name ebbde4f791

luke-jr commented at 5:56 am on November 17, 2024: member

Considering that CI is failing immediately after a walletprocesspsbt call, and that RPC is being changed in this PR, I’m inclined to think that it’s probably not a CI issue.

Yes, there was a new issue introduced by the added commits too. But there was also a CI issue: it failed to merge into master before building.

DrahtBot removed the label CI failed on Nov 17, 2024

maflcko commented at 11:08 am on November 17, 2024: member

Considering that CI is failing immediately after a walletprocesspsbt call, and that RPC is being changed in this PR, I’m inclined to think that it’s probably not a CI issue.

Yes, there was a new issue introduced by the added commits too. But there was also a CI issue: it failed to merge into master before building.

Not sure what failure you are referring to, but looking at https://github.com/bitcoin/bitcoin/actions/runs/11875971339/job/33093790745, it looks like the problem was that the CI did merge the config into master.

The check-each-commit task is specifically written to compile the exact commits that are provided in the pull request, so that git bisect issues are detected early on.

GHA is certainly buggy and I am not sure if there is a way to ask for some tasks to merge config and code and for others to not merge config nor code.

Doing a rebase seems a simple enough workaround for now. If someone disagrees, pull requests with improvements are always welcome.

RPC/Wallet: Convert walletprocesspsbt to use options parameter #24963

Code Coverage & Benchmarks

Reviews

Conflicts