[Qt] RPC-Console: support nested commands and simple value queries #7783

Merged
merged 1 commit into from Sep 20, 2016

Projects

None yet

10 participants

@jonasschnelli
Member

this is currently limited to the QT RPC Console, but I'm happy to extend/refactor this to make it useable in bitcoin-cli

Commands can be executed with bracket syntax, example: getwalletinfo().
Commands can be nested, example: sendtoaddress(getnewaddress(), 10).
Simple queries are possible: listunspent()[0][txid]
Object values are accessed with a non-quoted string, example: [txid].

Fully backward compatible.
generate 101 is identical to generate(101)
Result value queries indicated with [] require the new brackets syntax.
Comma as argument separator is now also possible: sendtoaddress,<address>,<amount>
Space as argument separator works also with the bracket syntax, example: sendtoaddress(getnewaddress() 10)

No dept limitation, complex commands are possible:
decoderawtransaction(getrawtransaction(getblock(getbestblockhash())[tx][0]))[vout][0][value]

bildschirmfoto 2016-04-01 um 17 13 08

@jonasschnelli jonasschnelli added the GUI label Apr 1, 2016
@luke-jr
Member
luke-jr commented Apr 1, 2016

Isn't this going a bit overboard for debugging tools? (OTOH, it's only about 150 LOC...)

@MarcoFalke
Member

I think this is useful. An alternative proposed was to use variables a = getnewaddress sendtoaddress a 10, but this pull is fine as well.

@jonasschnelli
Member

Isn't this going a bit overboard for debugging tools? (OTOH, it's only about 150 LOC...)

It is a "luxury extension", right. But given the time some of us have spent in the console repeating and copy-pasting commands out- and input, I think it worth taking this in.
Also, I don't see critical risks for this.

IMO we should also extend bitcoin-cli to support nested commands. It simply increases productivity with that tool.

@laanwj
Member
laanwj commented Apr 2, 2016

I like this concept. I initially had @luke-jr 's concern as well. But only a bit of code added, and it's well-contained.

It's not just luxury: it's useful for cases like #7599 where someone wants to insert the output of a previous command into a new one, but it's too long for copy pasting.

IMO we should also extend bitcoin-cli to support nested commands. It simply increases productivity with that tool.

Not sure there. bitcoin-cli is simply an entry point and you can use whatever scripting you want in the shell that calls it.

For the GUI debug console, which is essentially it's own bash, this makes more sense.

I'm all for a fancy ncurses-based interactive client, but that should not be bitcoin-cli.

@paveljanik
Contributor

On the other hand, many advices read 'Run getsomething' and so. This will bring another "fork" - you have to also add that you have to run this in Debug console or via bitcoin-cli, because the syntax will "fork".

Concept ACK (I'd also like to see this in bitcoin-cli)

@luke-jr
Member
luke-jr commented Apr 2, 2016

Almost tempting to make it server-side, if we're using long output-inputs... but this seems fine (Concept ACK) as-is; further improvement can wait for another PR.

@sipa
Member
sipa commented Jun 2, 2016

Would it be possible to abstract out this functionality in a separate commit (including the existing RPC parsing logic from the Qt console) and move it to rpc/server.cpp, as an actual RPC call that just takes a string argument with a command to parse?

That would make it both more usable (by exposing it as RPC, bitcoin-cli and other tools can use it too), and more testable (we can have RPC tests for it), without complicating the change much.

@jonasschnelli
Member

I have though about that but I wasn't sure if we should delegate the parsing/executing of nested command to the server. This PR would do the parsing "client side". We could also factor out the parsing and use it client-side (Qt / bitcoin-cli).

But I agree, it could be useful server-side.

@sipa
Member
sipa commented Jun 2, 2016 edited

Yes, I agree having it client side is useful. My main reason for suggesting abstracting it out it because I don't think it's very hard, and would make the parsing logic much easier to test.

@UniQredit

This would save a hell lot of time and copy/pasting

@jonasschnelli
Member
  • Finally rebased this great PR and refactored the parsing logic into rpc/server.cpp
  • Added some unit tests

This could now be simply extended to the RPC server, although, nested commands could be resource and time hungry.

@MarcoFalke MarcoFalke commented on an outdated diff Jul 19, 2016
src/test/rpc_tests.cpp
+ std::string result2;
+ RPCExecuteCommandLine(result, "getblockchaininfo()[chain]"); //simple result filtering with path
+ BOOST_CHECK_EQUAL(result, "main");
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblock(getbestblockhash())")); //simple 2 level nesting
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblock(getblock(getbestblockhash())[hash], true)"));
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblock( getblock( getblock(getbestblockhash())[hash] )[hash], true)")); //4 level nesting with whitespace, filtering path and boolean parameter
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo"));
+ BOOST_CHECK_EQUAL(result.substr(0,1), "{");
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo()"));
+ BOOST_CHECK_EQUAL(result.substr(0,1), "{");
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo")); //whitespace at the end will be tolerated
@MarcoFalke
MarcoFalke Jul 19, 2016 Member

There is no whitespace at the end?

@MarcoFalke MarcoFalke commented on an outdated diff Jul 19, 2016
src/test/rpc_tests.cpp
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblock(getblock(getbestblockhash())[hash], true)"));
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblock( getblock( getblock(getbestblockhash())[hash] )[hash], true)")); //4 level nesting with whitespace, filtering path and boolean parameter
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo"));
+ BOOST_CHECK_EQUAL(result.substr(0,1), "{");
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo()"));
+ BOOST_CHECK_EQUAL(result.substr(0,1), "{");
+
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo")); //whitespace at the end will be tolerated
+ BOOST_CHECK_EQUAL(result.substr(0,1), "{");
+
+ BOOST_CHECK_THROW(RPCExecuteCommandLine(result, "getblockchaininfo() .\n"), runtime_error); //invalid syntax
+ BOOST_CHECK_THROW(RPCExecuteCommandLine(result, "getblockchaininfo() getblockchaininfo()"), runtime_error); //invalid syntax
+ BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo(")); //tollerate non closing brackets if we have no arguments
@MarcoFalke
MarcoFalke Jul 19, 2016 Member

:%s/toller/toler/g

@jonasschnelli
Member

Fixed nits. Had to add client.cpp to libbitcoin_server_a. Should that be a problem @theuni?

@laanwj
Member
laanwj commented Aug 12, 2016 edited

Would it be possible to abstract out this functionality in a separate commit (including the existing RPC parsing logic from the Qt console) and move it to rpc/server.cpp,

Sorry to be contrary, but IMO, functionality related to parsing and not dispatching should be in rpc/client.cpp instead of rpc/server.cpp. Note that bitcoin-cli links the client library, not the server one.

(another reason that it doesn't belong server-side is that it doesn't act on univalue/JSON objects, but on command line strings. Wouldn't want string parsing on the server side, as this makes the JSON-RPC API unclear, and introduces potential vulnerabilities and DoS possibilities etc)

@laanwj laanwj commented on an outdated diff Aug 12, 2016
src/Makefile.am
@@ -181,6 +181,7 @@ libbitcoin_server_a_SOURCES = \
pow.cpp \
rest.cpp \
rpc/blockchain.cpp \
+ rpc/client.cpp \
@laanwj
laanwj Aug 12, 2016 Member

client.cpp is part of libbitcoin_cli, which is "cli: shared between bitcoin-cli and bitcoin-qt" if you need access to that then link that library. Don't include the compilation unit in two libraries.

@jonasschnelli
Member

Having it in rpc/client.cpp instead of server.cpp would be good I guess. The only restriction would then be, that we cannot allow (later) server side nested commands (which would probably perform slightly faster). But I'm not sure if we really want server side nested commands with the current locking behavior.

@laanwj
Member
laanwj commented Aug 12, 2016 edited

Server-side nested commands are not part of the JSON-RPC standard. It is an interesting thought but that would be a completely different proposal, and I don't think it would share any code with this. I'd imagine it would work something akin to batching (but w/ nested structures), not by parsing/formatting expression strings. Seeing how little even simple batching is used, I'm also not sure there is enough demand for that kind of advanced behavior, but that aside.

Edit: Looked it up a bit, 'nested remote function call' in RPC protocols is commonly implemented in the form of 'promise pipelining', a strategy to reduce round-trips. A call can return a handle, which is essentially a temporary variable, which can be passed as argument to other calls before the result is known. This allows more versatile manipulation than just nesting (e.g. a DAG instead of a tree). In any case this is something to be found in the more advanced RPC frameworks, I couldn't find anyone having bolted it into JSON-RPC. As said, an issue for another time :)

Edit.2: Had a try at a proposal here: #8457 (comment)

@theuni
Member
theuni commented Aug 12, 2016

Agreed with keeping parsing client-side. Let's not tangle up the dependencies.

I really like this idea btw.

@jonasschnelli
Member

Removed all changes from the core classes.
It's now a GUI only change.
Added Qt unit tests for the nested commands.

@MarcoFalke
Member

qt-test fail on travis, apparently.

@jonasschnelli
Member

Fixed the travis Qt-Test issue.
This PR is looking for reviewers.

@UdjinM6 UdjinM6 commented on an outdated diff Aug 23, 2016
src/qt/rpcconsole.cpp
{
- args.push_back(curarg);
- curarg.clear();
+ case '[': curarg.clear(); state = STATE_COMMAND_EXECUTED_INNER; break;
+ default:
+ if (state == STATE_COMMAND_EXECUTED_INNER)
+ {
+ if (ch == ']')
+ {
+ if (curarg.size())
+ {
+ // if we have a value query, query arrays with index and objects with a string key
+ UniValue subelement;
+ if (curarg.size() && lastResult.isArray())
@UdjinM6
UdjinM6 Aug 23, 2016 Contributor

curarg.size() should already be ok since it's checked in line 170

@UdjinM6 UdjinM6 commented on an outdated diff Aug 23, 2016
src/qt/rpcconsole.cpp
+ if (state == STATE_COMMAND_EXECUTED_INNER)
+ {
+ if (ch == ']')
+ {
+ if (curarg.size())
+ {
+ // if we have a value query, query arrays with index and objects with a string key
+ UniValue subelement;
+ if (curarg.size() && lastResult.isArray())
+ {
+ for(char argch: curarg)
+ if (!std::isdigit(argch))
+ throw std::runtime_error("Invalid result query");
+ subelement = lastResult[atoi(curarg.c_str())];
+ }
+ else if (curarg.size() && lastResult.isObject())
@UdjinM6
UdjinM6 Aug 23, 2016 Contributor

same here for curarg.size()

@UdjinM6 UdjinM6 commented on an outdated diff Aug 23, 2016
src/qt/rpcconsole.cpp
{
- args.push_back(curarg);
- curarg.clear();
+ case '[': curarg.clear(); state = STATE_COMMAND_EXECUTED_INNER; break;
+ default:
+ if (state == STATE_COMMAND_EXECUTED_INNER)
+ {
+ if (ch == ']')
@UdjinM6
UdjinM6 Aug 23, 2016 Contributor

nit: since both branches for that if end with break, nesting level here can be reduced by smth like:

                            if (ch != ']')
                            {
                                // append char to the current argument (which is also used for the query command)
                                curarg += ch;
                                break;
                            }

and then all the code in the scope below but up one level of nesting.

@MarcoFalke MarcoFalke commented on an outdated diff Aug 23, 2016
src/qt/rpcconsole.cpp
+ // don't stringify the json in case of a string to avoid doublequotes
+ if (lastResult.isStr())
+ curarg = lastResult.get_str();
+ else
+ curarg = lastResult.write(2);
+
+ // if we have a non empty result, use it as stack argument otherwise as general result
+ if (curarg.size())
+ {
+ if (stack.size())
+ stack.back().push_back(curarg);
+ else
+ strResult = curarg;
+ }
+ curarg.clear();
+ // assume easting space state
@MarcoFalke
MarcoFalke Aug 23, 2016 Member

nit: typo

@UdjinM6 UdjinM6 commented on the diff Aug 23, 2016
src/qt/rpcconsole.cpp
- // and pass it along with the method name to the dispatcher.
- UniValue result = tableRPC.execute(
- args[0],
- RPCConvertValues(args[0], std::vector<std::string>(args.begin() + 1, args.end())));
-
- // Format result reply
- if (result.isNull())
- strPrint = "";
- else if (result.isStr())
- strPrint = result.get_str();
- else
- strPrint = result.write(2);
-
- Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(strPrint));
+ std::string result;
+ std::string executableCommand = command.toStdString() + "\n";
@UdjinM6
UdjinM6 Aug 23, 2016 Contributor

nit: maybe change names to strResult and strExecutableCommand? Same for strings in rpcnestedtests.cpp

@MarcoFalke MarcoFalke commented on an outdated diff Aug 23, 2016
src/qt/test/rpcnestedtests.cpp
+#include "util.h"
+
+#include <QDir>
+
+#include <boost/filesystem.hpp>
+
+void RPCNestedTests::rpcNestedTests()
+{
+ UniValue jsonRPCError;
+
+ // do some test setup
+ // could be moved to a more generic place when we add more tests on QT level
+ const CChainParams& chainparams = Params();
+ RegisterAllCoreRPCCommands(tableRPC);
+ ClearDatadirCache();
+ std::string path = QDir::tempPath().toStdString() + "/" + strprintf("test_bitcoin_%lu_%i", (unsigned long)GetTime(), (int)(GetRand(100000)));
@MarcoFalke
MarcoFalke Aug 23, 2016 Member

Any reason for this name?

@MarcoFalke
MarcoFalke Aug 23, 2016 edited Member

Also I feel like this folder should be cleaned up on exit?

@jonasschnelli jonasschnelli [Qt] RPC-Console: support nested commands and simple value queries
Commands can be executed with bracket syntax, example: `getwalletinfo()`.
Commands can be nested, example: `sendtoaddress(getnewaddress(), 10)`.
Simple queries are possible: `listunspent()[0][txid]`
Object values are accessed with a non-quoted string, example: [txid].

Fully backward compatible.
`generate 101` is identical to `generate(101)`
Result value queries indicated with `[]` require the new brackets syntax.
Comma as argument separator is now also possible: `sendtoaddress,<address>,<amount>`
Space as argument separator works also with the bracket syntax, example: `sendtoaddress(getnewaddress() 10)

No dept limitation, complex commands are possible:
`decoderawtransaction(getrawtransaction(getblock(getbestblockhash())[tx][0]))[vout][0][value]`
1586044
@jonasschnelli
Member

Fixed nits, added cleanup of Qt test data.

@dcousens
Contributor

concept ACK

@laanwj
Member
laanwj commented Sep 20, 2016

utACK.

I do think this needs documentation. Not necessarily in this pull, but currently the debug console help consists of two lines "Use up and down arrows to navigate history, and Ctrl-L to clear screen. Type help for an overview of available commands.".

Maybe add a debug-console-only command like help that shows how to use nested commands and potentially other advanced tricks, and add 'for more information on using this console type XXX'.

@jonasschnelli
Member

I do think this needs documentation. [...]

Good point. I try something. Maybe not in this PR.
Could – maybe – be combined with this #8544 (comment)

@laanwj
Member
laanwj commented Sep 20, 2016

getwalletinfo()["walletversion"] doesn't work - can't it index into objects?

@jonasschnelli
Member

@laanwj: I guess you need to use getwalletinfo()[walletversion] (without double-quotes).

@laanwj
Member
laanwj commented Sep 20, 2016

@jonasschnelli awesome, that works. So that's why we need documentation :)

@jonasschnelli
Member

@laanwj: Agree on the documentation. The dropped "(double quotes) for an index access is quite uncommon, but can make sense because all our JSON properties are pure ASCII without whitespace.

Allowing the double-quotes (ignore them while parsing) could be a useful addition.

@laanwj
Member
laanwj commented Sep 20, 2016 edited

For testing this it's useful to add an echo command that simply returns what is passed to it, and echon which does the same but is marked to receive numbers/booleans/objects in vRPCConvertParams:

diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
index c14d9d6..4e09249 100644
--- a/src/rpc/client.cpp
+++ b/src/rpc/client.cpp
@@ -109,6 +109,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
     { "setban", 3 },
     { "getmempoolancestors", 1 },
     { "getmempooldescendants", 1 },
+    { "echon", 0}, { "echon", 1}, { "echon", 2}, { "echon", 3}, { "echon", 4}, { "echon", 5}, { "echon", 6}, { "echon", 7}, { "echon", 8}, { "echon", 9},
 };

 class CRPCConvertTable
diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index 5afcf63..e3b4550 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -417,6 +417,17 @@ UniValue signmessagewithprivkey(const UniValue& params, bool fHelp)
     return EncodeBase64(&vchSig[0], vchSig.size());
 }

+UniValue echo(const UniValue& params, bool fHelp)
+{
+    if (fHelp)
+        throw runtime_error(
+            "echo \"message\" ...\n"
+            "\nSimply echo back the input arguments\n"
+        );
+
+    return params;
+}
+
 UniValue setmocktime(const UniValue& params, bool fHelp)
 {
     if (fHelp || params.size() != 1)
@@ -454,6 +465,8 @@ static const CRPCCommand commands[] =
 { //  category              name                      actor (function)         okSafeMode
   //  --------------------- ------------------------  -----------------------  ----------
     { "control",            "getinfo",                &getinfo,                true  }, /* uses wallet if enabled */
+    { "control",            "echo",                   &echo,                   true  },
+    { "control",            "echon",                  &echo,                   true  },
     { "util",               "validateaddress",        &validateaddress,        true  }, /* uses wallet if enabled */
     { "util",               "createmultisig",         &createmultisig,         true  },
     { "util",               "verifymessage",          &verifymessage,          true  },

Allowing the double-quotes (ignore them while parsing) could be a useful addition.

Yes, would be useful to add, maybe in a later pull. Somehow I keep typing [] accidentally, and being surprised I get 'null'.

@laanwj laanwj merged commit 1586044 into bitcoin:master Sep 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@laanwj
Member
laanwj commented Sep 20, 2016

ACK 2ca6b9d

@laanwj laanwj added a commit that referenced this pull request Sep 20, 2016
@laanwj laanwj Merge #7783: [Qt] RPC-Console: support nested commands and simple val…
…ue queries


1586044 [Qt] RPC-Console: support nested commands and simple value queries (Jonas Schnelli)
4335d5a
* - Extra whitespace at the beginning and end and between arguments will be ignored
* - Text can be "double" or 'single' quoted
* - The backslash \c \ is used as escape character
* - Outside quotes, any character can be escaped
* - Within double quotes, only escape \c " and backslashes before a \c " or another backslash
* - Within single quotes, no escaping is possible and no special interpretation takes place
*
- * @param[out] args Parsed arguments will be appended to this list
+ * @param[out] result stringified Result from the executed command(chain)
@MarcoFalke
MarcoFalke Sep 20, 2016 Member

Nit: strResult

+ std::string executableCommand = command.toStdString() + "\n";
+ if(!RPCConsole::RPCExecuteCommandLine(result, executableCommand))
+ {
+ Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
@MarcoFalke
MarcoFalke Sep 20, 2016 Member

Nit: [ can also be unbalanced

+ QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo() .\n"), std::runtime_error); //invalid syntax
+ QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo() getblockchaininfo()"), std::runtime_error); //invalid syntax
+ (RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo(")); //tolerate non closing brackets if we have no arguments
+ (RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo()()()")); //tolerate non command brackts
@MarcoFalke
MarcoFalke Sep 20, 2016 Member

Nits: Can be moved out of the guard, typo brackts, no wrapping brackets needed.

@luke-jr luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016
@jonasschnelli @luke-jr jonasschnelli + luke-jr [Qt] RPC-Console: support nested commands and simple value queries
Commands can be executed with bracket syntax, example: `getwalletinfo()`.
Commands can be nested, example: `sendtoaddress(getnewaddress(), 10)`.
Simple queries are possible: `listunspent()[0][txid]`
Object values are accessed with a non-quoted string, example: [txid].

Fully backward compatible.
`generate 101` is identical to `generate(101)`
Result value queries indicated with `[]` require the new brackets syntax.
Comma as argument separator is now also possible: `sendtoaddress,<address>,<amount>`
Space as argument separator works also with the bracket syntax, example: `sendtoaddress(getnewaddress() 10)

No dept limitation, complex commands are possible:
`decoderawtransaction(getrawtransaction(getblock(getbestblockhash())[tx][0]))[vout][0][value]`

Github-Pull: #7783
Rebased-From: 1586044
9db5e22
@jonasschnelli jonasschnelli referenced this pull request Jan 10, 2017
Open

TODO for release notes 0.14.0 #8455

1 of 18 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment