rpc: Add support for JSON-RPC named arguments

laanwj commented at 7:29 pm on September 25, 2016: member

The JSON-RPC specification allows either passing parameters as an Array, for by-position arguments, or as an Object, for by-name arguments. Currently Bitcoin Core only supports by-position arguments.

This pull request makes the (minimal) changes to support by-name arguments, but preserves full backwards compatibility. APIs using by-name arguments are easier to extend - as arguments can be left out - and easier to use - no need to guess which argument goes where.

This is especially nice in languages such as Python, which have native support for named arguments. Examples from the test:

0h = node.help(command='getinfo')
1h = node.getblockhash(index=0)
2h = node.getblock(hash=h)

In this implementation named arguments are mapped to positions by a per-call structure, provided in the RPC command table. In the future calls could process the parameter object themselves, but this provides nice automatic interoperability with both worlds.
Missing arguments will be replaced by null (“holes”), except if at the end, then the argument is left out completely.
It is not possible to specify both named and positional arguments like in Python. JSON-RPC doesn’t allow for this, and I think it would be only confusing anyhow.
An error (RPC_INVALID_PARAMETER) will be return if an unknown named argument is specified.

Currently no calls have support for “holes” between arguments, but this should be implemented later on a per-call basis. These should be replaced with default values.

TODO

~~bitcoin-cli should grow support for providing named arguments~~ DONE see #8811 (comment)
~~Calls in mining, net, rawtransaction wallet still need argument names filled in. I’ll do this if reaction to this is positive.~~

Later

Same for the debug console. Syntax to be determined there. Not necessarily in this pull, my focus here is the core implementation.

laanwj added the label RPC/REST/ZMQ on Sep 25, 2016

laanwj force-pushed on Sep 25, 2016

laanwj commented at 3:35 pm on September 26, 2016: member

This is useful because a lot of features add new arguments to an existing call. For example #8751.

Currently the user of the API has to explicitly specify defaults for the intermediate arguments. With named arguments this would no longer be necessary.

It could also help clean up the API for calls that already have a lot of optional arguments such as sendtoaddress, or at least make it easier to use them.

in qa/rpc-tests/test_framework/authproxy.py: in a6b4276458 outdated

142 
143         log.debug("-%s-> %s %s"%(AuthServiceProxy.__id_count, self._service_name,
144                                  json.dumps(args, default=EncodeDecimal, ensure_ascii=self.ensure_ascii)))
145+        if args and argsn:
146+            raise ValueError('Cannot handle both named and positional arguments')
147         postdata = json.dumps({'version': '1.1',

MarcoFalke commented at 11:06 am on October 2, 2016:

In the OP you are linking to the “JSON-RPC 2.0 Specification”, which states

jsonrpc A String specifying the version of the JSON-RPC protocol. MUST be exactly “2.0”.

Can you explain why you keep this at 1.1?

laanwj commented at 11:20 am on October 2, 2016:

May make sense to change it to that (even irrespective of this pull) however I’m not sure that we support the entire 2.0 standard, so that may involve other changes too.

MarcoFalke commented at 11:16 am on October 2, 2016: member

The only alternative I see would be to only accept one object as first positional argument for all methods, but this will likely never happen, so Concept ACK.

jonasschnelli commented at 10:32 am on October 10, 2016: contributor

Concept ACK. I think this is very useful. ACK to extend it to bitcoin-cli, to the other rpc tables and to the RPC value conversions table.

After this, we should update the documents (and RPC help examples) to slowly move away from recommending fixed position arguments.

laanwj force-pushed on Oct 10, 2016

laanwj commented at 2:02 pm on October 10, 2016: member

Rebased to get rid of the univalue change

laanwj force-pushed on Oct 25, 2016

laanwj commented at 2:58 pm on October 25, 2016: member

Needed rebase, rebased

in src/rpc/server.cpp: in 0e2825c377 outdated

438+    // Process expected parameters.
439+    int hole = 0;
440+    for (const std::string &argName: argNames) {
441+        auto fr = argsIn.find(argName);
442+        if (fr != argsIn.end()) {
443+            for (int i = 0; i < hole; ++i) {

instagibbs commented at 2:50 pm on November 1, 2016:

after this loop hole should be reset to 0, otherwise holes will be repeated for every subsequent entry?

laanwj commented at 9:31 am on November 15, 2016:

Yes, good catch, thanks!

in src/rpc/server.cpp: in 0e2825c377 outdated

436+        argsIn[keys[i]] = &values[i];
437+    }
438+    // Process expected parameters.
439+    int hole = 0;
440+    for (const std::string &argName: argNames) {
441+        auto fr = argsIn.find(argName);

instagibbs commented at 2:52 pm on November 1, 2016:

what does fr stand for?

mrbandrews commented at 5:05 pm on November 14, 2016:

find returnvalue? Just a guess.

in qa/rpc-tests/rpcnamedargs.py: in e4aa9b3bf1 outdated

41+
42+        h = node.getblockhash(index=0)
43+        node.getblock(hash=h)
44+
45+        assert_equal(node.echo(), [])
46+        assert_equal(node.echo(arg0=0,arg9=9), [0] + [None]*8 + [9])

instagibbs commented at 2:55 pm on November 1, 2016:

may want to add test of [exist, hole, exist, exist] to catch gap repeat error

mrbandrews commented at 5:07 pm on November 14, 2016:

I added the test: assert_equal(node.echo(arg0=0,arg3=3,arg9=9), [0] + [None]_2 + [3] + [None]_5 + [9]) Verified that with the code as is it fails, but with instagibbs fix of hole=0 it succeeds.

instagibbs changes_requested

instagibbs commented at 2:56 pm on November 1, 2016: member

concept ACK, and aside from the error mentioned, looks like good minimal support

afk11 commented at 1:37 pm on November 8, 2016: contributor

Concept ACK, needs rebase

laanwj force-pushed on Nov 15, 2016

laanwj commented at 9:31 am on November 15, 2016: member

Rebased, added @instagibbs’ test and fix.

mrbandrews commented at 2:03 pm on November 15, 2016: contributor

ACK

sipa commented at 6:12 pm on November 15, 2016: member

Concept ACK!

Do you intend to include the echo command in the final merge?

laanwj commented at 6:54 am on November 16, 2016: member

@sipa I’ve had use for it two times in testing - here and in #7783. Also I fathom it’s somewhat useful for users when testing JSON RPC libraries. So my preference would be to keep it.

instagibbs commented at 0:06 am on November 18, 2016: member

utACK https://github.com/bitcoin/bitcoin/pull/8811/commits/3c7647ea7814f7d3f0304bb4b39274c748a88900

laanwj force-pushed on Nov 21, 2016

laanwj commented at 1:52 pm on November 21, 2016: member

As there is interest in this I’m continuing work on the issues that are still open in the OP. I’ve pushed argument naming for mining, net, rawtransaction and wallet, which completes it for all calls. I’ll also make it possible to use named arguments with bitcoin-cli. I’ll leave the GUI change for another time though.

laanwj force-pushed on Nov 22, 2016

laanwj commented at 2:22 pm on November 22, 2016: member

Added support to bitcoin-cli. This should complete the feature set for this pull. I’d appreciate testing and review.

Especially important to review are the parameter names in the RPC tables, as they will become part of the API:

More examples

$ src/bitcoin-cli -testnet -named getbalance account="*" minconf="1" include-watchonly=false
413.44052133

Note right now, getbalance doesn’t allow leaving intermediate arguments out right now, so this won’t work yet.

$ src/bitcoin-cli -testnet -named getbalance minconf="1" include-watchonly=false
error code: -1
error message:
JSON value is not a string as expected

It doesn’t know how to handle the univalue null for the account parameter. It should interpret it as the default, "*". This should be addressed in later changes for specific methods.

in src/rpc/misc.cpp: in 966d33c9c8 outdated

512-    { "util",               "createmultisig",         &createmultisig,         true  },
513-    { "util",               "verifymessage",          &verifymessage,          true  },
514-    { "util",               "signmessagewithprivkey", &signmessagewithprivkey, true  },
515+    { "control",            "getinfo",                &getinfo,                true,  {} }, /* uses wallet if enabled */
516+    { "control",            "getmemoryinfo",          &getmemoryinfo,          true,  {} },
517+    { "util",               "validateaddress",        &validateaddress,        true,  {"bitcoinaddress"} }, /* uses wallet if enabled */

theuni commented at 9:12 pm on November 25, 2016:

This is referred to as “address” in the help

laanwj commented at 5:39 am on November 26, 2016:

address/privkey/pubkey usage is made consistent in a later commit: https://github.com/bitcoin/bitcoin/pull/8811/commits/8b6fd17692b0a402858ef313d6b15e0d086db2c2

in src/rpc/misc.cpp: in 966d33c9c8 outdated

514-    { "util",               "signmessagewithprivkey", &signmessagewithprivkey, true  },
515+    { "control",            "getinfo",                &getinfo,                true,  {} }, /* uses wallet if enabled */
516+    { "control",            "getmemoryinfo",          &getmemoryinfo,          true,  {} },
517+    { "util",               "validateaddress",        &validateaddress,        true,  {"bitcoinaddress"} }, /* uses wallet if enabled */
518+    { "util",               "createmultisig",         &createmultisig,         true,  {"nrequired","keys"} },
519+    { "util",               "verifymessage",          &verifymessage,          true,  {"bitcoinaddress","signature","message"} },

theuni commented at 9:12 pm on November 25, 2016:

same

in src/rpc/net.cpp: in 9e73f62fe3 outdated

610-    { "network",            "clearbanned",            &clearbanned,            true  },
611-    { "network",            "setnetworkactive",       &setnetworkactive,       true, },
612+    { "network",            "getconnectioncount",     &getconnectioncount,     true,  {} },
613+    { "network",            "ping",                   &ping,                   true,  {} },
614+    { "network",            "getpeerinfo",            &getpeerinfo,            true,  {} },
615+    { "network",            "addnode",                &addnode,                true,  {"node","command"} },

theuni commented at 9:18 pm on November 25, 2016:

maybe these should be “address” rather than “node”, so that in the future we can differentiate? (nodeid, subnet, etc)

laanwj commented at 5:37 am on November 26, 2016:

‘address’ is already used for bitcoin addresses, so I’d prefer not overloading that for network addresses. Other suggestions are welcome, though

in src/rpc/net.cpp: in 9e73f62fe3 outdated

615+    { "network",            "addnode",                &addnode,                true,  {"node","command"} },
616+    { "network",            "disconnectnode",         &disconnectnode,         true,  {"node"} },
617+    { "network",            "getaddednodeinfo",       &getaddednodeinfo,       true,  {"node"} },
618+    { "network",            "getnettotals",           &getnettotals,           true,  {} },
619+    { "network",            "getnetworkinfo",         &getnetworkinfo,         true,  {} },
620+    { "network",            "setban",                 &setban,                 true,  {"ip", "command", "bantime", "absolute"} },

theuni commented at 9:25 pm on November 25, 2016:

probably better to call this subnet

laanwj commented at 5:39 am on November 26, 2016:

Will do

in src/rpc/mining.cpp: in 67fee407a8 outdated

921+    { "mining",             "getblocktemplate",       &getblocktemplate,       true,  {"template_request"} },
922+    { "mining",             "submitblock",            &submitblock,            true,  {"hexdata","parameters"} },
923+
924+    { "generating",         "generate",               &generate,               true,  {"numblocks","maxtries"} },
925+    { "generating",         "generatetoaddress",      &generatetoaddress,      true,  {"numblocks","address","maxtries"} },
926+

theuni commented at 9:36 pm on November 25, 2016:

unify nblocks/numblocks?

laanwj commented at 5:39 am on November 26, 2016:

Will do

in src/rpc/client.cpp: in 76973f42f8 outdated

207+        size_t pos = s.find("=", ptr);
208+        if (pos == std::string::npos) {
209+            throw(std::runtime_error("No '=' in named argument, a value needs to be provided for every argument"));
210+        }
211+        std::string name = s.substr(ptr, pos-ptr);
212+        std::string value = s.substr(pos+1);

theuni commented at 10:18 pm on November 25, 2016:

should probably make sure that !name.empty() && !value.empty() for good measure.

laanwj commented at 5:34 am on November 26, 2016:

In that case it should do the expected thing: send an empty name or value. An empty value is perfectly acceptable if you want to send an empty string. An empty name is a bit strange, agreed, but it gets caught server-side as an unknown argument.

in src/bitcoin-cli.cpp: in 76973f42f8 outdated

303+
304+        UniValue params;
305+        if (haveNamed && havePositional) {
306+            throw runtime_error("cannot have both named and positional arguments, did you mean to pass arguments before the command?");
307+        } else if(haveNamed) {
308+            params = RPCConvertNamedValues(strMethod, args);

theuni commented at 10:58 pm on November 25, 2016:

Mixing args here with GetBoolArg/GetArg is a little scary. If some command took a (for example) “-version=foo” argument, I believe it would be swallowed by line 81 instead?

laanwj commented at 5:32 am on November 26, 2016:

ParseParameters stops at the first argument that doesn’t start with -, which happens to be the command. This starts after the command. So unless I miss something they will never interfere.

theuni commented at 6:45 am on November 26, 2016:

Ah, that’s the part I was missing, thanks. Maybe a comment to that effect to remind me next time I have to ask? :)

laanwj commented at 9:09 am on November 26, 2016:

Added a comment :)

theuni commented at 11:04 pm on November 25, 2016: member

Concept ACK, and code generally looks good. I added a few nits, most of which have actually already been addressed in subsequent commits.

At a high level, I’m worried about introducing the “txid” param name for a bunch of calls that will likely need to make the distinction between txid/wtxid soon. Wallet especially. Maybe some convention like “txhash” should be preferred instead?

luke-jr commented at 5:28 am on November 26, 2016: member

Why is there a “wtxid” ever needed as a param? “txhash” and “txid” are all I see existing at all (hash != txid).

luke-jr commented at 5:32 am on November 26, 2016: member

I think a lot of the options params ought to be flattened?

laanwj commented at 5:38 am on November 26, 2016: member

I think a lot of the options params ought to be flattened?

Possibly. Not in this pull, though. JSON can perfectly deal with nested structures. Also this would break backwards compatibility, unlike anything in this pull.

theuni commented at 6:49 am on November 26, 2016: member

@luke-jr Sure, txhash/txid. I was only making the point that going forward there will be a distinction between those, so it’d be good to make sure that all “txid” params here really mean txid.

luke-jr commented at 7:14 am on November 26, 2016: member

@laanwj If this pull is merged as-is, then flattening the options will involve breaking backward compatibility with this, no? ie, it may be better to just not allow for already-named options at all right now.

laanwj commented at 8:57 am on November 26, 2016: member

I pushed cfields’ rename suggestions, and compiled statistics on what argument names are used in which calls. This could be helpful in determining further unifications:

 0"absolute",          // setban
 1"account",           // addmultisigaddress getaccountaddress getaddressesbyaccount getbalance getnewaddress getreceivedbyaccount listtransactions setaccount
 2"address",           // generatetoaddress validateaddress verifymessage addwitnessaddress dumpprivkey getaccount getreceivedbyaddress importaddress sendtoaddress setaccount signmessage
 3"addresses",         // listunspent
 4"allowhighfees",     // sendrawtransaction
 5"amount",            // move sendfrom sendtoaddress settxfee
 6"amounts",           // sendmany
 7"arg0",              // echo echojson
 8"arg1",              // echo echojson
 9"arg2",              // echo echojson
10"arg3",              // echo echojson
11"arg4",              // echo echojson
12"arg5",              // echo echojson
13"arg6",              // echo echojson
14"arg7",              // echo echojson
15"arg8",              // echo echojson
16"arg9",              // echo echojson
17"bantime",           // setban
18"blockhash",         // getblock getblockheader preciousblock invalidateblock reconsiderblock waitforblock gettxoutproof listsinceblock
19"checklevel",        // verifychain
20"command",           // help addnode setban
21"comment",           // move sendfrom sendmany sendtoaddress
22"comment_to",        // sendfrom sendtoaddress
23"count",             // listtransactions
24"destination",       // backupwallet
25"fee_delta",         // prioritisetransaction
26"filename",          // dumpwallet importwallet
27"fromaccount",       // move sendfrom sendmany
28"height",            // getblockhash waitforblockheight getnetworkhashps
29"hexdata",           // submitblock
30"hexstring",         // decoderawtransaction decodescript sendrawtransaction signrawtransaction fundrawtransaction
31"include_empty",     // listreceivedbyaccount listreceivedbyaddress
32"include_mempool",   // gettxout
33"include_watchonly", // getbalance gettransaction listaccounts listreceivedbyaccount listreceivedbyaddress listsinceblock listtransactions
34"keys",              // createmultisig addmultisigaddress
35"label",             // importprivkey importaddress importpubkey
36"locktime",          // createrawtransaction
37"maxconf",           // listunspent
38"maxtries",          // generate generatetoaddress
39"message",           // verifymessage signmessagewithprivkey signmessage
40"minconf",           // getbalance getreceivedbyaccount getreceivedbyaddress listaccounts listreceivedbyaccount listreceivedbyaddress listunspent move sendfrom sendmany
41"n",                 // gettxout
42"nblocks",           // verifychain getnetworkhashps generate generatetoaddress estimatefee estimatepriority estimatesmartfee estimatesmartpriority
43"newpassphrase",     // walletpassphrasechange
44"newsize",           // keypoolrefill
45"node",              // addnode disconnectnode getaddednodeinfo
46"nrequired",         // createmultisig addmultisigaddress
47"oldpassphrase",     // walletpassphrasechange
48"options",           // fundrawtransaction importmulti
49"outputs",           // createrawtransaction
50"p2sh",              // importaddress
51"parameters",        // submitblock
52"passphrase",        // encryptwallet walletpassphrase
53"prevtxs",           // signrawtransaction
54"priority_delta",    // prioritisetransaction
55"privkey",           // signmessagewithprivkey importprivkey
56"privkeys",          // signrawtransaction
57"proof",             // verifytxoutproof
58"pubkey",            // importpubkey
59"rawtransaction",    // importprunedfunds
60"requests",          // importmulti
61"rescan",            // importprivkey importaddress importpubkey
62"sighashtype",       // signrawtransaction
63"signature",         // verifymessage
64"skip",              // listtransactions
65"state",             // setnetworkactive
66"subnet",            // setban
67"subtractfeefrom",   // sendmany
68"subtractfeefromamount", // sendtoaddress
69"target_confirmations", // listsinceblock
70"template_request",  // getblocktemplate
71"timeout",           // waitfornewblock waitforblock waitforblockheight walletpassphrase
72"timestamp",         // setmocktime
73"toaccount",         // move
74"toaddress",         // sendfrom
75"transactions",      // createrawtransaction lockunspent
76"txid",              // getmempoolancestors getmempooldescendants getmempoolentry gettxout prioritisetransaction getrawtransaction abandontransaction gettransaction removeprunedfunds
77"txids",             // gettxoutproof
78"txoutproof",        // importprunedfunds
79"unlock",            // lockunspent
80"verbose",           // getblock getblockheader getmempoolancestors getmempooldescendants getrawmempool getrawtransaction

@laanwj If this pull is merged as-is, then flattening the options will involve breaking backward compatibility with this, no? ie, it may be better to just not allow for already-named options at all right now.

If a RPC call has a designated options argument now, then a future pull - that has functionality to pass the object down to the RPC call as-is - could allow specifying those options on the top level instead. If we decide that flattening is desirable at all. I don’t want to handle any RPC-call-specific changes here. Mind that I don’t add any “null” argument handling in RPC calls here either. This just lays the ground work.

laanwj force-pushed on Nov 26, 2016

laanwj commented at 11:08 am on November 26, 2016: member

This looks like a good candidate for unification. Any opinions on “hexdata” versus “hexstring”?

0"hexdata",           // submitblock
1"hexstring",         // decoderawtransaction decodescript sendrawtransaction signrawtransaction fundrawtransaction

luke-jr commented at 11:40 am on November 26, 2016: member

@laanwj GBT/submitblock probably shouldn’t accept named parameters at all, since they are according to BIPs…?

laanwj commented at 12:10 pm on November 26, 2016: member

You don’t have to use named arguments. For consistency I prefer all calls to have the possibility to have their arguments named, though.

laanwj force-pushed on Dec 2, 2016

laanwj commented at 4:24 am on December 2, 2016: member

Rebased for #9144, squashed (into logically separate commits)

MarcoFalke commented at 5:38 pm on December 2, 2016: member

I think travis wants you to include stdexcept

in src/bitcoin-cli.cpp: in dd9b157f03 outdated

303+                havePositional = true; // Have positional arguments
304+        }
305+
306+        UniValue params;
307+        if (haveNamed && havePositional) {
308+            throw runtime_error("cannot have both named and positional arguments, did you mean to pass arguments before the command?");

jnewbery commented at 3:48 pm on December 5, 2016:

needs to be std::runtime_error since we’re not using namespace std

laanwj commented at 6:41 am on December 6, 2016:

no longer, no

in src/bitcoin-cli.cpp: in dd9b157f03 outdated

295+        // starts scanning after the method, so arguments before and after the
296+        // command will never interfere.
297+        bool haveNamed = false;
298+        bool havePositional = false;
299+        for (const std::string &s: args) {
300+            if (s.size() >= 1 && IsSwitchChar(s[0]))

jnewbery commented at 4:00 pm on December 5, 2016:

I believe this breaks passing in negative numbers as a positional argument. For example, the following cli command is valid prior to this commit:

bitcoin-cli getnetworkhashps -1

and returns the estimated network hashes per second in the current difficulty window. After this commit, the method fails:

0vagrant@vagrant-ubuntu-trusty-64:~$ bitcoin-cli getnetworkhashps -1
1error: No '=' in named argument, a value needs to be provided for every argument

I haven’t checked whether there are arguments that accept negative numbers in other RPC methods.

laanwj commented at 6:29 pm on December 5, 2016: member

Good point about the negative values. Hadn’t thought about that. Same will hold for string values that start with ‘-’, such as labels. Any argument could have been meant as literal value.

Thinking of it this could even be a security issue. Say someone controls the first argument to a call, they could now instead provide the second by using, say –amount=123.

It should be made explicit instead of auto-detected. E.g.

bitcoin-cli {method} val1 val2. ..
bitcoin-cli -named {method} -arg1=val1 -arg2=val2 ...

in src/rpc/client.cpp: in dd9b157f03 outdated

188+    { "importaddress", 3, "p2sh" },
189+    { "importpubkey", 2, "rescan" },
190+    { "importmulti", 0, "requests" },
191+    { "importmulti", 1, "options" },
192+    { "verifychain", 0, "checklevel" },
193+    { "verifychain", 1, "blocks" },

jnewbery commented at 11:29 pm on December 5, 2016:

should be nblocks

laanwj commented at 6:40 am on December 6, 2016:

will rename it

theuni commented at 6:09 am on December 6, 2016: member

@laanwj +1 for explicit

sipa commented at 6:34 am on December 6, 2016: member

The issue only exists for string arguments, as anything else would be invalid JSON anyway.

Also, I have a slight preference for bitcoin-cli getblock hash=X over bitcoin-cli getblock -hash=X… it feels like the latter is an option to bitcoin-cli rather than getblock (but feel free to ignore this if I’m alone with that opinion).

laanwj commented at 6:40 am on December 6, 2016: member

The issue only exists for string arguments, as anything else would be invalid JSON anyway.

Yes, only a problem for string arguments and negative numbers. But unfortunately that’s enough to make it irresponsible to ‘guess’.

Also, I have a slight preference for bitcoin-cli getblock hash=X over bitcoin-cli getblock -hash=X… it feels like the latter is an option to bitcoin-cli rather than getblock (but feel free to ignore this if I’m alone with that opinion).

I have thought about that, but didn’t propose it because I think some shells interpret A=B tuples after the command as environment settings - or if not - that is a convention people assume to be so because of autoconf/make.

theuni commented at 6:45 am on December 6, 2016: member

Worth mentioning how subarg syntax is handled by other familiar progs:

git:./bitcoin-cli -testnet -- --hash=X or gdb: ./bitcoin-cli -testnet --args --hash=X

sipa commented at 6:57 am on December 6, 2016: member

Another idea: named arguments have to precede the command (so bitcoin-cli -hash=X getblock), that’s never ambiguous.

laanwj commented at 7:51 am on December 6, 2016: member

./bitcoin-cli -testnet – –hash=X ./bitcoin-cli -testnet –args –hash=X

I like supporting at least one of those. Mind that the --/--args delimiter has to be before the command for it to be nonambigious, otherwise it may be a legitimate string argument.

Another idea: named arguments have to precede the command (so bitcoin-cli -hash=X getblock), that’s never ambiguous.

Sure, but then they can collide with bitcoin-cli’s own arguments, which we were trying to avoid at the same time. It will be tricky to figure out what arguments to send to the server and which ones to keep for ourselves, and may change per version.

But even if we manage to get that sorted out, it looks like “putting the cart before the horse” syntax-wise.

sipa commented at 7:54 am on December 6, 2016: member

Sure, but then they can collide with bitcoin-cli’s own arguments, which we were trying to avoid at the same time. It will be tricky to figure out what arguments to send to the server and which ones to keep for ourselves, and may change per version.

That can be avoided by using hash=X style naming ;)

But even if we manage to get that sorted out, it looks like “putting the cart before the horse” syntax-wise.

Yes, I agree there.

jnewbery commented at 10:13 am on December 6, 2016: member

I agree with the explicit bitcoin-cli -named {method} -arg1=val1 -arg2=val2 ... syntax. It guarantees backwards compatibility for any applications that are calling into bitcoin-cli with the current positional argument syntax.

If we’re explicitly specifying that the arguments are named, then we could even drop the dashes before the argument names, ie bitcoin-cli -named {method} arg1=val1 arg2=val2 ...

laanwj commented at 11:30 am on December 6, 2016: member

If we’re explicitly specifying that the arguments are named, then we could even drop the dashes before the argument names, ie bitcoin-cli -named {method} arg1=val1 arg2=val2 …

That’s what @sipa is suggesting too. Seems fine to me if we can be sure that there are no shells that eat these. I’ve only seen the syntax used for passing arguments to make/configure and always assumed it was setting environment variables .

theuni commented at 12:24 pm on December 6, 2016: member

@laanwj Not sure if you were just using configure/make as an example, but in those cases anyway, those assignments are separate from environment vars.

That’s why, for ex, CC=clang ./configure != ./configure CC=clang. The first sets env vars that aren’t necessarily persistent to the resulting builds unless the user persists them explicitly in the shell. The second allows configure to forward the vars without polluting the env. If you pass a var using the 2nd syntax, and it’s not understood (“precious” in autotools-speak), it will not be forwarded. afaik, the same goes for make.

Edit: Just to be explicit, it’s configure/make swallowing the args in those cases, not the shell itself.

sipa commented at 7:01 pm on December 6, 2016: member

@laanwj dd and bitcoin-tx also use key=value.

morcos commented at 8:54 pm on December 6, 2016: member

concept ACK I will review more thoroughly after update

I think I have a slight preference for eliminating the dashes

laanwj force-pushed on Dec 8, 2016

laanwj commented at 11:04 am on December 8, 2016: member

Dashes have been successfully eliminated. New syntax:

0     $ src/bitcoin-cli -testnet -named echo arg0="dfdf"
1     [
2     "dfdf"
3     ]

Argument conversion also works, for arguments thus flagged in the table in src/rpc/client.cpp.

0    $ src/bitcoin-cli -testnet -named echojson arg0="[1,2,3]"
1    [
2      [
3        1,
4        2,
5        3
6      ]
7    ]

Unknown parameter (detected server-side):

0    $ src/bitcoin-cli -testnet -named getinfo arg0="dfdf"
1    error code: -8
2    error message:
3    Unknown named parameter arg0

Also updated “more examples” above.

Renamed the remaining instances of nblocks and made sure the naming is consistent between client.cpp and the appropriate dispatch table (I have a script for this, which we could make part of the travis checks at some point).
Rebased and squashed

jnewbery commented at 5:08 pm on December 9, 2016: member

I’ve spent some time looking through the RPC arguments, and my view is that the following argument names should be changed to be more descriptive:

getblock/index becomes getblock/height to conform with waitforblockheight and getnetworkhashps
createrawtransaction/transactions becomes createrawtransaction/inputs to conform with createrawtransaction/outputs
listtransactions/from becomes listtransactions/skip to avoid confusion with ‘from’ as in ‘from address…’
getblock/hash becomes getblock/blockhash …
getblockheader/hash becomes getblockheader/blockhash ..
preciousblock/hash becomes preciousblock/blockhash …
invalidateblock/hash becomes invalidateblock/blockhash …
reconsiderblock/hash becomes reconsiderblock/blockhash …
… and waitforblock/hash becomes reconsiderblock/blockhash to confirm with gettxoutproof listsinceblock and to avoid confusion with other hash types

There’s a patch set which makes these changes (as well as generally cleaning up RPC comments) at https://github.com/jnewbery/bitcoin/commit/f08b91c21b9e715ec273cf431d2ec2bb8916def7

laanwj commented at 12:15 pm on December 12, 2016: member

@jnewbery Sounds good to me, thanks, will cherry-pick that.

jnewbery commented at 2:59 pm on December 12, 2016: member

Oops - I didn’t change the named arguments in rpcnamedargs.py to match the new argument names.

Fix is here: https://github.com/jnewbery/bitcoin/commit/d45ef7c02cf682f59f129b2af5df064270b20e03

laanwj force-pushed on Dec 14, 2016

laanwj commented at 9:16 am on December 14, 2016: member

@jnewbery Thanks - cherry-picked and squashed into the last commit.

laanwj force-pushed on Dec 19, 2016

laanwj commented at 9:15 am on December 19, 2016: member

Rebased for a small add/add conflict in rpc-tests.py.

authproxy: Add support for RPC named arguments 5865d41f88

rpc: Support named arguments

The [JSON-RPC specification](http://www.jsonrpc.org/specification) allows passing parameters as an Array, for by-position
arguments, or an Object, for by-name arguments.

This implements by-name arguments, but preserves full backwards compatibility. API using by-name arguments are
easier to extend, and easier to use (no need to guess which argument goes where).

Named are mapped to positions by a per-call structure, provided through the RPC command table.

Missing arguments will be replaced by null, except if at the end, then the argument is left out completely.

Currently calls fail (though not crash) on intermediate nulls, but this should be improved on a per-call basis later.

6f1c76ae14

rpc: Named arguments for blockchain calls 495eb44a4f

rpc: Add 'echo' call for testing

This hidden call simply returns what is passed in.

286ec08cb0

rpc: Named arguments for misc calls fba1a6150c

test: Add test for RPC named arguments

Add RPC testcase for RPC named arguments.

2ca9dcd5b9

rpc: Named arguments for net calls

Also add a more descriptive message for `setnetworkactive`.

b8ebc595bb

rpc: Named arguments for mining calls 78b684f2ac

rpc: Named arguments for wallet calls 37a166f146

rpc: Named arguments for rawtransaction calls 8d713f761b

rpc: Argument name consistency

The meaning is clear from the context, and we're inconsistent here.
Also save typing when using named arguments.

- `bitcoinaddress` -> `address`
- `bitcoinprivkey` -> `privkey`
- `bitcoinpubkey` -> `pubkey`

9adb4e1a59

laanwj force-pushed on Jan 5, 2017

laanwj commented at 10:34 am on January 5, 2017: member

Rebased for minor argument documentation conflict.

laanwj added this to the milestone 0.14.0 on Jan 5, 2017

sipa commented at 11:28 pm on January 7, 2017: member

utACK 65bf5d27c578e7560128a28af08fef57135ec841.

I have not read through the test code in detail, nor verified whether the RPC help names match the named arguments in the server and client tables. I have also not checked the consistency of the chosen names (but I don’t care strongly). I have played around briefly with the new echo and echojson commands and they seem to work as expected.

in src/rpc/server.cpp: in 6f1c76ae14 outdated

433+    // there is an unknown one.
434+    const std::vector<std::string>& keys = in.params.getKeys();
435+    const std::vector<UniValue>& values = in.params.getValues();
436+    std::unordered_map<std::string, const UniValue*> argsIn;
437+    for (size_t i=0; i<keys.size(); ++i) {
438+        argsIn[keys[i]] = &values[i];

TheBlueMatt commented at 1:27 am on January 8, 2017:

Because univalue does not check for duplicate keys, things are a bit weird. I’m not sure if there’s much that this PR could/should do about this, but as long as we’re adding more user-provided-json-parsing stuff I thought I’d mention it.

laanwj commented at 6:57 am on January 9, 2017:

Almost no JSON implementation checks for duplicate keys. The usual behavior is that the last value assigned to the key takes precedence. I think that will happen here?

TheBlueMatt commented at 5:02 pm on January 9, 2017:

Yes, I believe the last one will take precedence. I’m ok with not fixing this, but it seems super strange.

laanwj commented at 11:00 am on January 10, 2017:

Well if it’s any guide, python’s JSON behaves exactly the same:

0>>> json.loads('{"a":"b","a":"C"}')
1{'a': 'C'}

What I would not mind is checking at the client side for duplicate arguments, as a user friendly feature, but I’m not going to do it in this pull.

in src/rpc/blockchain.cpp: in 65bf5d27c5 outdated

278@@ -279,7 +279,7 @@ UniValue waitforblockheight(const JSONRPCRequest& request)
279             "of the current tip.\n"
280             "\nReturns the current block on timeout or exit.\n"
281             "\nArguments:\n"
282-            "1. block height to wait for (int)\n"
283+            "1. height  (required, int) Block height to wait for (int)\n"

TheBlueMatt commented at 5:36 pm on January 8, 2017:

Docs here still say “blockheight” for one-line help but height for multi-line.

laanwj commented at 6:58 am on January 9, 2017:

OK, will make that consistent.

in src/rpc/client.cpp: in 65bf5d27c5 outdated

203+        }
204+
205+        std::string name = s.substr(0, pos);
206+        std::string value = s.substr(pos+1);
207+
208+        // allow include-mempool=true as well as include_mempool=true

TheBlueMatt commented at 6:17 pm on January 8, 2017:

Why?

laanwj commented at 6:58 am on January 9, 2017:

This was useful when the syntax was -arg=value. I guess it no longer really is now that plain name=value pairs are used.

TheBlueMatt commented at 6:21 pm on January 8, 2017: member

Didn’t bother to audit that help text matches the server and client tables, but glanced at a few and they look good. None of the comments really matter all that much.

utACK 65bf5d27c578e7560128a28af08fef57135ec841

rpc: Named argument support for bitcoin-cli

Usage e.g.:

    $ src/bitcoin-cli -testnet -named echo arg0="dfdf"
    [
    "dfdf"
    ]

Argument conversion also works, for arguments thus flagged in the table in
`src/rpc/client.cpp`.

    $ src/bitcoin-cli -testnet -named echojson arg0="[1,2,3]"
    [
      [
        1,
        2,
        3
      ]
    ]

Unknown parameter (detected server-side):

    $ src/bitcoin-cli -testnet -named getinfo arg0="dfdf"
    error code: -8
    error message:
    Unknown named parameter arg0

481f289765

Update RPC argument names 4e7e2e16e4

laanwj force-pushed on Jan 10, 2017

laanwj commented at 11:06 am on January 10, 2017: member

Fixed @bluematt’s documentation nit and removed the _/- interchangeability for option names, and squashed. Should be ready for merge now if travis passes.

laanwj merged this on Jan 10, 2017

laanwj closed this on Jan 10, 2017

laanwj referenced this in commit 5754e0341b on Jan 10, 2017

jtimon commented at 5:46 pm on January 10, 2017: contributor

Concept ACK (I’m surprised I didn’t do it before).

jnewbery referenced this in commit cb546cf937 on Jan 11, 2017

jnewbery referenced this in commit 2dbbd0400c on Jan 19, 2017

codablock referenced this in commit eb7a6b08f1 on Jan 18, 2018

andvgal referenced this in commit cc86a34fa2 on Jan 6, 2019

CryptoCentric referenced this in commit e2f166ece5 on Feb 26, 2019

furszy referenced this in commit 29650e00cf on Jun 2, 2021

MarcoFalke locked this on Sep 8, 2021

rpc: Add support for JSON-RPC named arguments #8811

TODO

Later

More examples