rpc: Allow named and positional arguments to be used together #19762

ryanofsky · 2020-08-18T15:49:43Z

It's nice to be able to use named options and positional arguments together.

Most shell tools accept both, and python functions combine options and arguments allowing them to be passed with even more flexibility. This change adds support for python's approach so as a motivating example:

bitcoin-cli -named createwallet wallet_name=mywallet load_on_startup=1

Can be shortened to:

bitcoin-cli -named createwallet mywallet load_on_startup=1

JSON-RPC standard doesn't have a convention for passing named and positional parameters together, so this implementation makes one up and interprets any unused "args" named parameter as a positional parameter array.

This change is backwards compatible. It doesn't change the interpretation of any previously valid calls, just treats some previously invalid calls as valid.

Another use case even if you only occasionally use named arguments is that you can define an alias:

alias bcli='bitcoin-cli -named'

And now use both named named and unnamed arguments from the same alias without having to manually add -named option for named arguments or see annoying error "No '=' in named argument... this needs to be present for every argument (even if it is empty)`" for unnamed arguments

jonatack · 2020-08-18T16:07:13Z

Concept ACK. As a frequent cli user, this seems quite handy.

practicalswift · 2020-08-18T17:45:34Z

Concept ACK: neat!

src/rpc/client.cpp

luke-jr · 2020-08-19T22:24:21Z

src/rpc/server.cpp

+        for (size_t i = 0; i < named_args.size(); ++i) {
+            if (i >= out.params.size()) {
+                out.params.push_back(named_args[i]);
+            } else if (!named_args[i].isNull()) {


Where do we overwrite the null with the named arg value??

re: #19762 (comment)

Where do we overwrite the null with the named arg value??

This code isn't overwriting the previous list of positional parameters, but building a new list from args and parts of the old list.

test/functional/interface_bitcoin_cli.py

test/functional/rpc_named_arguments.py

laanwj · 2020-08-20T12:54:53Z

Concept ACK on this functionality in the client. I wish it could be client-only, though and would not introduce a non-standard server side convention (and further complicate the RPC server's argument parsing logic).

luke-jr · 2020-08-20T18:50:48Z

Seems useful for non-CLI callers too?

DrahtBot · 2020-08-20T19:55:10Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	stickies-v, achow101, aureleoules
Concept ACK	practicalswift, laanwj, promag, meshcollider, MarcoFalke, adam2k
Stale ACK	jonatack

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26506 ([WIP] refactor: rpc: use convenience fn to auto convert non-RFC JSON values by stickies-v)

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.

This changes interpretation of bitcoin-cli -named arguments that begin with '='. Previously these were sent as JSON-RPC named parameters with empty string "" as parameter names. Now they are sent as unnamed arguments. For example, "bitcoin-cli -named =VALUE" previously sent named "" parameter containing "VALUE". Now it sends unnamed positional argument containing "VALUE". Motivation for this change is to provide a way to pass unnamed positional arguments containing '=' with bitcoin-cli -named. Luke-jr suggested this might be useful bitcoin#19762 (comment) This change is backwards compatible because bitcoind does not have any RPC methods accepting "" as a parameter name, so this is just making previously invalid bitcoin-cli calls valid, not changing interpretation of any already valid bitcoin-cli call.

ryanofsky

Updated f8cd575 -> f0bb56e (pr/named.1 -> pr/named.2, compare) with a few changes:

Added JSON-RPC documentation and release notes
Added two test cases suggested by Luke
Added stricter server error checking in the two test cases. Instead of allowing the same parameter to be specified twice (in positional and named forms if the positional value is null), the server always throws an error if a parameter is specified twice. This could be changed later, but for now being more strict seems better for preventing mistakes.

src/rpc/client.cpp

ryanofsky · 2020-08-21T00:38:03Z

src/rpc/server.cpp

+        for (size_t i = 0; i < named_args.size(); ++i) {
+            if (i >= out.params.size()) {
+                out.params.push_back(named_args[i]);
+            } else if (!named_args[i].isNull()) {


re: #19762 (comment)

Where do we overwrite the null with the named arg value??

This code isn't overwriting the previous list of positional parameters, but building a new list from args and parts of the old list.

test/functional/interface_bitcoin_cli.py

test/functional/rpc_named_arguments.py

luke-jr · 2020-08-22T01:53:32Z

Added stricter server error checking in the two test cases. Instead of allowing the same parameter to be specified twice (in positional and named forms if the positional value is null), the server always throws an error if a parameter is specified twice.

null is supposed to be equivalent to omitted, so I think it should work.

promag · 2020-08-23T23:32:55Z

Concept ACK.

promag · 2020-08-25T09:17:58Z

On the cli we could also support passing JSON arguments like query_options.minimumAmount=0.001 query_options.maximumCount=1.

jonatack

Tested almost-ACK. The example in the PR description works, but I wasn't able to make it work for me in some other cases. Perhaps I'm misunderstanding how it works, or the documentation can be improved.

These work:

$ ./src/bitcoin-cli -named getrawtransaction txid=abc verbose=true

$ ./src/bitcoin-cli -named getrawtransaction abc verbose=true

but

$ ./src/bitcoin-cli -named getrawtransaction txid=abc true
error code: -8
error message:
Parameter txid specified twice both as positional and named argument

$ ./src/bitcoin-cli -named getrawtransaction txid=abc 1
error code: -8
error message:
Parameter txid specified twice both as positional and named argument

src/rpc/server.cpp

doc/release-notes-19762.md

doc/JSON-RPC-interface.md

promag · 2020-09-01T16:35:28Z

src/rpc/server.cpp

+    }
+    // If leftover "args" param was found, use it as a source of positional
+    // arguments and add named arguments after.
+    auto positional_args = argsIn.find("args");


nit 1: call it _args to have some internal "feeling"?

nit 2: forbid registering arguments with this name.

re: #19762 (comment)

nit 1: call it _args to have some internal "feeling"?

I don't think it would be good have RPC methods accepting both "_args" and "args" parameters. Naming was chosen to discourage this (while not disallowing it). Also it isn't really meant to be internal. It's supported by two clients in this PR, explained in documentation, and meant to be something that can be used by other clients.

nit 2: forbid registering arguments with this name.

I think it's fine if RPC methods want to handle positional arguments with their own logic, and don't think it would be good to add extra code to detect and forbid it a priori.

I think it's fine if RPC methods want to handle positional arguments with their own logic, and don't think it would be good to add extra code to detect and forbid it a priori.

It makes this code needlessly complex. If in the future someone wants to add an "args" arg, they can modify the code.

re: #19762 (comment)

It makes this code needlessly complex. If in the future someone wants to add an "args" arg, they can modify the code.

It is true that this decision makes the implementation of the transformNamedArguments function slightly more complex. But it keeps the design as a whole simpler because:

It handles args parameter entirely within transformNamedArguments, and doesn't require adding special cases for parameters named args in other parts of the code.

It avoids adding a footgun where parameters are treated differently based on their names and choosing a different parameter name could cause errors or munged data.

I also think the transformNamedArguments function would be easier to simplify in the future with some changes to the UniValue class. The current UniValue API basically makes JSON objects append-only instead of read-write, and transformNamedArguments could be simpler and more efficient if this were changed,

src/rpc/server.cpp

ryanofsky

Updated f0bb56e -> 894c414 (pr/named.2 -> pr/named.3, compare) with suggested doc improvments

re: #19762 (review)

Tested almost-ACK. The example in the PR description works, but I wasn't able to make it work for me in some other cases. Perhaps I'm misunderstanding how it works, or the documentation can be improved.

I changed documentation to say "initial positional values" instead of "positional values" (and feel free to suggest other documentation updates that would help) but the examples cited wouldn't be accepted before or after this PR, and don't work in python either. The first positional argument is always the first positional argument. The second positional argument is always the second positional argument. You can't bump a positional argument into a different positions by adding named arguments. If you try, you see an explicit error about conflicting values for the positional argument.

re: #19762 (comment)

On the cli we could also support passing JSON arguments like query_options.minimumAmount=0.001 query_options.maximumCount=1.

Presumably this would be for another PR, but it seems like a nice idea

doc/JSON-RPC-interface.md

doc/release-notes-19762.md

src/rpc/server.cpp

ryanofsky · 2020-10-22T16:09:11Z

src/rpc/server.cpp

+    }
+    // If leftover "args" param was found, use it as a source of positional
+    // arguments and add named arguments after.
+    auto positional_args = argsIn.find("args");


re: #19762 (comment)

nit 1: call it _args to have some internal "feeling"?

I don't think it would be good have RPC methods accepting both "_args" and "args" parameters. Naming was chosen to discourage this (while not disallowing it). Also it isn't really meant to be internal. It's supported by two clients in this PR, explained in documentation, and meant to be something that can be used by other clients.

nit 2: forbid registering arguments with this name.

I think it's fine if RPC methods want to handle positional arguments with their own logic, and don't think it would be good to add extra code to detect and forbid it a priori.

src/rpc/server.cpp

ryanofsky · 2020-11-16T17:30:33Z

Looking over recent irc conversation, if you don't like the createwallet API, you probably won't like this PR. Even still, this PR should make the createwallet RPC easier to call in a backward compatible way, without getting in the way of future improvements. This PR can also nicely complement client side evaluation features like #20273

jonatack · 2020-11-16T18:50:13Z

Thanks for the comment; reminds me to re-test this PR.

jonatack

Tested ACK 894c414

Thanks for the updates. Not sure why I was confused the first time, but this seems much clearer.

jonatack · 2020-11-17T11:41:58Z

src/rpc/server.cpp

+        if (out.params.size() == 0) skipped += 1;
+    }
+    // If leftover "args" param was found, use it as a source of positional
+    // arguments and add named arguments after.


Outside of this PR changeset, it may not be clear where "args" comes from. It may be good to mention in the comment here that "args" is set in rpc/client.cpp::RPCConvertNamedValues().

re: #19762 (comment)

Outside of this PR changeset, it may not be clear where "args" comes from. It may be good to mention in the comment here that "args" is set in rpc/client.cpp::RPCConvertNamedValues().

Thanks, if I follow up I'll reference JSON-RPC-interface.md#parameter-passing here. Better to reference the documentation than the implementation details of one specific client, though. Referencing the RPCConvertNamedValues function where "args" is set in the CLI client would be equivalent to referencing AuthServiceProxy.get_request method where "args" is set in the python client. These would be potentially helpful comments, but also likely to become out of date.

re: #19762 (comment)

Thanks, if I follow up I'll reference JSON-RPC-interface.md#parameter-passing here.

Thanks again, added this comment now

jonatack · 2020-11-17T12:21:35Z

This PR would make it much nicer to use the fee_rate sat/vB param in #20305 with, for example, the sendtoaddress, sendmany, and send RPCs. Thumbs up.

…used to…

Current behavior isn't ideal and will be changed in upcoming commits, but it's useful to have test coverage regardless. MarcoFalke reported the case of positional arguments overwriting the named "args" parameter in bitcoin-cli bitcoin#19762 (comment)

…ferent ways MarcoFalke reported the case of positional arguments silently overwriting the named "args" parameter in bitcoin-cli bitcoin#19762 (comment) and this behavior is confusing and was not intended when support for support for "args" was added to bitcoin-cli. Instead of letting one "args" value overwrite the other in the client, just pass the values to the server verbatim, and let the error be handled server side.

Current behavior isn't ideal and will be changed in upcoming commits, but it's useful to have test coverage regardless. MarcoFalke reported the case of bitcoin-cli positional arguments overwriting the named "args" parameter in bitcoin#19762 (comment)

…ferent ways MarcoFalke reported the case of positional arguments silently overwriting the named "args" parameter in bitcoin-cli bitcoin#19762 (comment) and this behavior is confusing and was not intended when support for "args" parameters was added to bitcoin-cli in bitcoin#19762. Instead of letting one "args" value overwrite the other in the client, just pass the values to the server verbatim, and let the error be handled server side.

No changes in behavior, just implements review suggestions from bitcoin#19762 (comment) bitcoin#19762 (comment) bitcoin#26628 (comment)

Current behavior isn't ideal and will be changed in upcoming commits, but it's useful to have test coverage regardless. MarcoFalke reported the case of bitcoin-cli positional arguments overwriting the named "args" parameter in bitcoin/bitcoin#19762 (comment)

…ferent ways MarcoFalke reported the case of positional arguments silently overwriting the named "args" parameter in bitcoin-cli bitcoin/bitcoin#19762 (comment) and this behavior is confusing and was not intended when support for "args" parameters was added to bitcoin-cli in #19762. Instead of letting one "args" value overwrite the other in the client, just pass the values to the server verbatim, and let the error be handled server side.

No changes in behavior, just implements review suggestions from bitcoin/bitcoin#19762 (comment) bitcoin/bitcoin#19762 (comment) bitcoin/bitcoin#26628 (comment)

…together

backport: merge bitcoin#21391, bitcoin#19550, bitcoin#15946, bitcoin#21230, bitcoin#21252, bitcoin#21297, bitcoin#20605, bitcoin#21575, bitcoin#22309, bitcoin#19762 (assumeutxo backports: part 4)

DrahtBot added RPC/REST/ZMQ Tests labels Aug 18, 2020

luke-jr suggested changes Aug 19, 2020

View reviewed changes

DrahtBot mentioned this pull request Aug 21, 2020

cli/gui: support "@height" in place of blockhash for getblock on client side #16439

Closed

ryanofsky force-pushed the pr/named branch from f8cd575 to f0bb56e Compare August 21, 2020 22:18

ryanofsky commented Aug 21, 2020

View reviewed changes

maflcko removed RPC/REST/ZMQ Tests labels Aug 23, 2020

DrahtBot added Docs RPC/REST/ZMQ labels Aug 23, 2020

maflcko removed the Docs label Aug 23, 2020

jonatack reviewed Aug 31, 2020

View reviewed changes

src/rpc/server.cpp Outdated Show resolved Hide resolved

doc/release-notes-19762.md Outdated Show resolved Hide resolved

doc/JSON-RPC-interface.md Outdated Show resolved Hide resolved

promag reviewed Sep 1, 2020

View reviewed changes

ryanofsky force-pushed the pr/named branch from f0bb56e to c681ca9 Compare October 23, 2020 14:43

ryanofsky commented Oct 23, 2020

View reviewed changes

ryanofsky force-pushed the pr/named branch from c681ca9 to 894c414 Compare October 23, 2020 15:01

jonatack reviewed Nov 17, 2020

View reviewed changes

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 1, 2022

Merge bitcoin#19762: rpc: Allow named and positional arguments to be …

d179b00

…used to…

maflcko mentioned this pull request Dec 3, 2022

RPC: Reject RPC requests with same named parameter specified multiple times #26628

Merged

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Dec 9, 2022

test: Suggested cleanups for rpc_namedparams test

8c3ff7d

No changes in behavior, just implements review suggestions from bitcoin#19762 (comment) bitcoin#19762 (comment) bitcoin#26628 (comment)

kwvg added a commit to kwvg/dash that referenced this pull request Jul 18, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

e88969e

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 19, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

a17e57b

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 19, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

6f3799b

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 19, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

7c2eb36

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 23, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

6826dea

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 23, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

470924a

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 24, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

c88d924

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 24, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

2b189ef

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 24, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

8316f97

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 25, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

6ff956e

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 25, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

75ef114

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 25, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

f40a541

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 25, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

5e7086f

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 25, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

eb400a7

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 25, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

ca729a0

…together

UdjinM6 pushed a commit to kwvg/dash that referenced this pull request Jul 27, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

2bca58f

…together

kwvg added a commit to kwvg/dash that referenced this pull request Jul 27, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

bdd1b74

…together

PastaPastaPasta pushed a commit to kwvg/dash that referenced this pull request Jul 28, 2023

merge bitcoin#19762: Allow named and positional arguments to be used …

d40f28e

…together

bitcoin locked and limited conversation to collaborators Dec 2, 2023

rpc: Allow named and positional arguments to be used together #19762

rpc: Allow named and positional arguments to be used together #19762

Uh oh!

Conversation

ryanofsky commented Aug 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Aug 18, 2020

Uh oh!

practicalswift commented Aug 18, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laanwj commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Aug 20, 2020

Uh oh!

DrahtBot commented Aug 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

luke-jr commented Aug 22, 2020

Uh oh!

promag commented Aug 23, 2020

Uh oh!

promag commented Aug 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanofsky Oct 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky commented Nov 16, 2020

Uh oh!

jonatack commented Nov 16, 2020

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Aug 18, 2020 •

edited

Loading

laanwj commented Aug 20, 2020 •

edited

Loading

DrahtBot commented Aug 20, 2020 •

edited

Loading

promag commented Aug 25, 2020 •

edited

Loading

ryanofsky Oct 22, 2020 •

edited

Loading

ryanofsky left a comment •

edited

Loading

ryanofsky Oct 22, 2020 •

edited

Loading

jonatack left a comment •

edited

Loading