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

pull jonasschnelli wants to merge 1 commits into bitcoin:master from jonasschnelli:2016/04/qt_console_nested changing 6 files +300 −71
  1. jonasschnelli commented at 3:15 pm on April 1, 2016: contributor

    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]

  2. jonasschnelli added the label GUI on Apr 1, 2016
  3. luke-jr commented at 6:34 pm on April 1, 2016: member
    Isn’t this going a bit overboard for debugging tools? (OTOH, it’s only about 150 LOC…)
  4. MarcoFalke commented at 6:44 pm on April 1, 2016: 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.
  5. jonasschnelli commented at 6:53 pm on April 1, 2016: contributor

    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.

  6. laanwj commented at 5:41 am on April 2, 2016: member

    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.

  7. paveljanik commented at 10:52 am on April 2, 2016: 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)

  8. luke-jr commented at 10:57 am on April 2, 2016: member
    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.
  9. sipa commented at 2:10 pm on June 2, 2016: member

    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.

  10. jonasschnelli commented at 2:14 pm on June 2, 2016: contributor

    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.

  11. sipa commented at 2:21 pm on June 2, 2016: member
    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.
  12. UniQredit commented at 5:08 am on July 19, 2016: none
    This would save a hell lot of time and copy/pasting
  13. jonasschnelli force-pushed on Jul 19, 2016
  14. jonasschnelli force-pushed on Jul 19, 2016
  15. jonasschnelli commented at 3:22 pm on July 19, 2016: contributor
    • 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.

  16. in src/test/rpc_tests.cpp: in b5ba8fb2ed outdated 
    352+    BOOST_CHECK_EQUAL(result.substr(0,1), "{");
353+
354+    BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo()"));
355+    BOOST_CHECK_EQUAL(result.substr(0,1), "{");
356+
357+    BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo")); //whitespace at the end will be tolerated
    MarcoFalke commented at 3:29 pm on July 19, 2016:
    There is no whitespace at the end?
  17. in src/test/rpc_tests.cpp: in b5ba8fb2ed outdated 
    357+    BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo")); //whitespace at the end will be tolerated
358+    BOOST_CHECK_EQUAL(result.substr(0,1), "{");
359+
360+    BOOST_CHECK_THROW(RPCExecuteCommandLine(result, "getblockchaininfo() .\n"), runtime_error); //invalid syntax
361+    BOOST_CHECK_THROW(RPCExecuteCommandLine(result, "getblockchaininfo() getblockchaininfo()"), runtime_error); //invalid syntax
362+    BOOST_CHECK_NO_THROW(RPCExecuteCommandLine(result, "getblockchaininfo(")); //tollerate non closing brackets if we have no arguments
    MarcoFalke commented at 3:30 pm on July 19, 2016:
    :%s/toller/toler/g
  18. jonasschnelli force-pushed on Jul 20, 2016
  19. jonasschnelli commented at 1:27 pm on July 20, 2016: contributor
    Fixed nits. Had to add client.cpp to libbitcoin_server_a. Should that be a problem @theuni?
  20. laanwj commented at 1:00 pm on August 12, 2016: member

    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)

  21. in src/Makefile.am: in 05a958e32d outdated 
    180@@ -181,6 +181,7 @@ libbitcoin_server_a_SOURCES = \
181   pow.cpp \
182   rest.cpp \
183   rpc/blockchain.cpp \
184+  rpc/client.cpp \
    laanwj commented at 1:06 pm on August 12, 2016:
    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.
  22. jonasschnelli commented at 1:08 pm on August 12, 2016: contributor
    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.
  23. laanwj commented at 1:12 pm on August 12, 2016: member

    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)

  24. theuni commented at 6:07 pm on August 12, 2016: member

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

    I really like this idea btw.

  25. jonasschnelli force-pushed on Aug 20, 2016
  26. jonasschnelli commented at 9:21 am on August 20, 2016: contributor
    Removed all changes from the core classes. It’s now a GUI only change. Added Qt unit tests for the nested commands.
  27. MarcoFalke commented at 12:02 pm on August 21, 2016: member
    qt-test fail on travis, apparently.
  28. jonasschnelli force-pushed on Aug 22, 2016
  29. jonasschnelli force-pushed on Aug 22, 2016
  30. jonasschnelli force-pushed on Aug 22, 2016
  31. jonasschnelli force-pushed on Aug 23, 2016
  32. jonasschnelli force-pushed on Aug 23, 2016
  33. jonasschnelli force-pushed on Aug 23, 2016
  34. jonasschnelli force-pushed on Aug 23, 2016
  35. jonasschnelli force-pushed on Aug 23, 2016
  36. jonasschnelli force-pushed on Aug 23, 2016
  37. jonasschnelli commented at 9:43 am on August 23, 2016: contributor
    Fixed the travis Qt-Test issue. This PR is looking for reviewers.
  38. in src/qt/rpcconsole.cpp: in e492505329 outdated 
    185+                            {
186+                                if (curarg.size())
187+                                {
188+                                    // if we have a value query, query arrays with index and objects with a string key
189+                                    UniValue subelement;
190+                                    if (curarg.size() && lastResult.isArray())
    UdjinM6 commented at 10:19 am on August 23, 2016:
    curarg.size() should already be ok since it’s checked in line 170
  39. in src/qt/rpcconsole.cpp: in e492505329 outdated 
    192+                                        for(char argch: curarg)
193+                                            if (!std::isdigit(argch))
194+                                                throw std::runtime_error("Invalid result query");
195+                                        subelement = lastResult[atoi(curarg.c_str())];
196+                                    }
197+                                    else if (curarg.size() && lastResult.isObject())
    UdjinM6 commented at 10:19 am on August 23, 2016:
    same here for curarg.size()
  40. in src/qt/rpcconsole.cpp: in e492505329 outdated 
    179-                    curarg.clear();
180+                    case '[': curarg.clear(); state = STATE_COMMAND_EXECUTED_INNER; break;
181+                    default:
182+                        if (state == STATE_COMMAND_EXECUTED_INNER)
183+                        {
184+                            if (ch == ']')
    UdjinM6 commented at 10:24 am on August 23, 2016:

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

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

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

  41. in src/qt/rpcconsole.cpp: in e492505329 outdated 
    230+                                stack.back().push_back(curarg);
231+                            else
232+                                strResult = curarg;
233+                        }
234+                        curarg.clear();
235+                        // assume easting space state
    MarcoFalke commented at 10:25 am on August 23, 2016:
    nit: typo
  42. in src/qt/rpcconsole.cpp: in e492505329 outdated 
    378-        else
379-            strPrint = result.write(2);
380-
381-        Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(strPrint));
382+        std::string result;
383+        std::string executableCommand = command.toStdString() + "\n";
    UdjinM6 commented at 10:27 am on August 23, 2016:
    nit: maybe change names to strResult and strExecutableCommand? Same for strings in rpcnestedtests.cpp
  43. in src/qt/test/rpcnestedtests.cpp: in e492505329 outdated 
    25+    // do some test setup
26+    // could be moved to a more generic place when we add more tests on QT level
27+    const CChainParams& chainparams = Params();
28+    RegisterAllCoreRPCCommands(tableRPC);
29+    ClearDatadirCache();
30+    std::string path = QDir::tempPath().toStdString() + "/" + strprintf("test_bitcoin_%lu_%i", (unsigned long)GetTime(), (int)(GetRand(100000)));
    MarcoFalke commented at 10:31 am on August 23, 2016:
    Any reason for this name?
    MarcoFalke commented at 10:31 am on August 23, 2016:
    Also I feel like this folder should be cleaned up on exit?
  44. jonasschnelli force-pushed on Aug 23, 2016
  45. [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]`
    15860448d3
  46. jonasschnelli force-pushed on Aug 23, 2016
  47. jonasschnelli commented at 1:32 pm on August 23, 2016: contributor
    Fixed nits, added cleanup of Qt test data.
  48. dcousens commented at 6:54 am on September 14, 2016: contributor
    concept ACK
  49. laanwj commented at 11:27 am on September 20, 2016: member

    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’.

  50. jonasschnelli commented at 11:30 am on September 20, 2016: contributor

    I do think this needs documentation. […]

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

  51. laanwj commented at 11:32 am on September 20, 2016: member
    getwalletinfo()["walletversion"] doesn’t work - can’t it index into objects?
  52. jonasschnelli commented at 11:33 am on September 20, 2016: contributor
    @laanwj: I guess you need to use getwalletinfo()[walletversion] (without double-quotes).
  53. laanwj commented at 11:34 am on September 20, 2016: member
    @jonasschnelli awesome, that works. So that’s why we need documentation :)
  54. jonasschnelli commented at 11:36 am on September 20, 2016: contributor

    @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.

  55. laanwj commented at 11:52 am on September 20, 2016: member

    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:

     0diff --git a/src/rpc/client.cpp b/src/rpc/client.cpp
 1index c14d9d6..4e09249 100644
 2--- a/src/rpc/client.cpp
 3+++ b/src/rpc/client.cpp
 4@@ -109,6 +109,7 @@ static const CRPCConvertParam vRPCConvertParams[] =
 5     { "setban", 3 },
 6     { "getmempoolancestors", 1 },
 7     { "getmempooldescendants", 1 },
 8+    { "echon", 0}, { "echon", 1}, { "echon", 2}, { "echon", 3}, { "echon", 4}, { "echon", 5}, { "echon", 6}, { "echon", 7}, { "echon", 8}, { "echon", 9},
 9 };
10
11 class CRPCConvertTable
12diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
13index 5afcf63..e3b4550 100644
14--- a/src/rpc/misc.cpp
15+++ b/src/rpc/misc.cpp
16@@ -417,6 +417,17 @@ UniValue signmessagewithprivkey(const UniValue& params, bool fHelp)
17     return EncodeBase64(&vchSig[0], vchSig.size());
18 }
19
20+UniValue echo(const UniValue& params, bool fHelp)
21+{
22+    if (fHelp)
23+        throw runtime_error(
24+            "echo \"message\" ...\n"
25+            "\nSimply echo back the input arguments\n"
26+        );
27+
28+    return params;
29+}
30+
31 UniValue setmocktime(const UniValue& params, bool fHelp)
32 {
33     if (fHelp || params.size() != 1)
34@@ -454,6 +465,8 @@ static const CRPCCommand commands[] =
35 { //  category              name                      actor (function)         okSafeMode
36   //  --------------------- ------------------------  -----------------------  ----------
37     { "control",            "getinfo",                &getinfo,                true  }, /* uses wallet if enabled */
38+    { "control",            "echo",                   &echo,                   true  },
39+    { "control",            "echon",                  &echo,                   true  },
40     { "util",               "validateaddress",        &validateaddress,        true  }, /* uses wallet if enabled */
41     { "util",               "createmultisig",         &createmultisig,         true  },
42     { "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'.

  56. laanwj merged this on Sep 20, 2016
  57. laanwj closed this on Sep 20, 2016
  58. laanwj commented at 12:15 pm on September 20, 2016: member
    ACK 2ca6b9d
  59. laanwj referenced this in commit 4335d5a41b on Sep 20, 2016
  60. in src/qt/rpcconsole.cpp: in 15860448d3 
    126  *   - Outside quotes, any character can be escaped
127  *   - Within double quotes, only escape \c " and backslashes before a \c " or another backslash
128  *   - Within single quotes, no escaping is possible and no special interpretation takes place
129  *
130- * @param[out]   args        Parsed arguments will be appended to this list
131+ * @param[out]   result      stringified Result from the executed command(chain)
    MarcoFalke commented at 10:06 pm on September 20, 2016:
    Nit: strResult
  61. in src/qt/rpcconsole.cpp: in 15860448d3 
    378-        Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(strPrint));
379+        std::string result;
380+        std::string executableCommand = command.toStdString() + "\n";
381+        if(!RPCConsole::RPCExecuteCommandLine(result, executableCommand))
382+        {
383+            Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    MarcoFalke commented at 10:10 pm on September 20, 2016:
    Nit: [ can also be unbalanced
  62. in src/qt/test/rpcnestedtests.cpp: in 15860448d3 
    66+#if QT_VERSION >= 0x050300
67+    // do the QVERIFY_EXCEPTION_THROWN checks only with Qt5.3 and higher (QVERIFY_EXCEPTION_THROWN was introduced in Qt5.3)
68+    QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo() .\n"), std::runtime_error); //invalid syntax
69+    QVERIFY_EXCEPTION_THROWN(RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo() getblockchaininfo()"), std::runtime_error); //invalid syntax
70+    (RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo(")); //tolerate non closing brackets if we have no arguments
71+    (RPCConsole::RPCExecuteCommandLine(result, "getblockchaininfo()()()")); //tolerate non command brackts
    MarcoFalke commented at 10:14 pm on September 20, 2016:
    Nits: Can be moved out of the guard, typo brackts, no wrapping brackets needed.
  63. luke-jr referenced this in commit 9db5e22749 on Dec 21, 2016
  64. codablock referenced this in commit 89b7509a00 on Sep 19, 2017
  65. laanwj referenced this in commit 7293d06413 on Nov 19, 2017
  66. codablock referenced this in commit 387201712a on Jan 11, 2018
  67. andvgal referenced this in commit bf6bf4e902 on Jan 6, 2019
  68. MarcoFalke referenced this in commit 3077f11dad on Jun 26, 2019
  69. PastaPastaPasta referenced this in commit 7ffbf8143d on Jan 17, 2020
  70. PastaPastaPasta referenced this in commit 29ec248a03 on Jan 22, 2020
  71. PastaPastaPasta referenced this in commit 9733330f78 on Jan 22, 2020
  72. PastaPastaPasta referenced this in commit 97adc728da on Jan 29, 2020
  73. PastaPastaPasta referenced this in commit 8977118599 on Jan 29, 2020
  74. PastaPastaPasta referenced this in commit c7b8b2b35c on Jan 29, 2020
  75. ckti referenced this in commit 953ed59806 on Mar 28, 2021
  76. furszy referenced this in commit cb4e9396e5 on Apr 7, 2021
  77. Munkybooty referenced this in commit 2ae4f24a5c on Nov 2, 2021
  78. Munkybooty referenced this in commit de99f19b98 on Nov 2, 2021
  79. Munkybooty referenced this in commit 067f0a4605 on Nov 4, 2021
  80. malbit referenced this in commit b1d0da3148 on Nov 5, 2021
  81. Munkybooty referenced this in commit 3bc1982862 on Nov 16, 2021
  82. Munkybooty referenced this in commit 716f124b00 on Nov 18, 2021
  83. gades referenced this in commit 46c7363d9b on Jan 30, 2022
  84. DrahtBot locked this on Feb 15, 2022


jonasschnelli luke-jr MarcoFalke laanwj paveljanik sipa UniQredit theuni UdjinM6 dcousens

Labels
GUI

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me