Debug Console implementation of generate method #692

pull hernanmarino wants to merge 2 commits into bitcoin-core:master from hernanmarino:generate-gui-console changing 1 files +157 −29
  1. hernanmarino commented at 4:47 pm on December 28, 2022: contributor

    This is an implementation of a (gui console only) generate method, and fixes #55 , as described in that issue. This changes the behaviour of bitcoin-qt while imitating the behaviour and return data from bitcoin-cli’s -generate argument as implemented in https://github.com/bitcoin/bitcoin/pull/19133.

    The generate comand takes two optional parameters:

     0generate ( nblocks   ( maxtries) )
     1Mine a specified amount of blocks to own ( on the fly generated ) address.
     2
     3Arguments:
     41. nblocks    (numeric, optional, , default=1) How many blocks are generated.
     52. maxtries   (numeric, optional, default=1000000) How many iterations to try.
     6
     7Result:
     8{
     9  "address":          (address generated by this command)
    10  "blocks": [           (json array of block hashes)
    11     ....
    12]
    13}
    

    The output looks similar to the following :

    image

    Note to reviewers / testers:

    • Consider that this is a bitcoin-qt only implementation . bitcoin-cli already had this functionality implemented in the referenced PR.
    • It is advised to run bitcoin-qt in regtest, to be able to actually generate some blocks and test the output properly
  2. DrahtBot commented at 4:47 pm on December 28, 2022: contributor

    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
    Concept ACK jarolrod, hebasto, rserranon, LarryRuane, furszy
    Approach NACK luke-jr
    Stale ACK RandyMcMillan, pablomartin4btc

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #841 (Decouple WalletModel from RPCExecutor by furszy)

    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.

  3. hernanmarino force-pushed on Dec 28, 2022
  4. in src/qt/rpcconsole.cpp:425 in 454fc48df2 outdated
    421+        std::smatch match;
    422+        if (std::regex_match(executableCommand, match, validRegex)) {
    423+            std::string nblocks=match[1];
    424+            std::string maxtries=match[2];
    425+            if (nblocks=="") nblocks = "1";
    426+            if (maxtries=="") maxtries = "1000000";
    


    pablomartin4btc commented at 7:26 pm on December 28, 2022:

    nit:

    0            const std::string nblocks{match.str(1) == "" ? "1" : match.str(1)};
    1            const std::string maxtries{match.str(2) == "" ? "1000000" : match.str(2)};
    
  5. hernanmarino marked this as ready for review on Dec 28, 2022
  6. pablomartin4btc commented at 9:09 pm on December 28, 2022: contributor
    Code Review ACK. It’s good to make the -generate command available at the rpc console level in bitcoin-qt as it’s already in bitcoin-cli.
  7. hernanmarino force-pushed on Dec 29, 2022
  8. in src/qt/rpcconsole.cpp:423 in b7782a4177 outdated
    419+        std::regex validRegex("generate\\s*[\\(]?\\s*(\\d+)?\\s*[,]?\\s*(\\d+)?\\s*[\\)]?\\s*");
    420+        std::regex invalidRegex("generate[\\s\\(]+.*");
    421+        std::smatch match;
    422+        if (std::regex_match(executableCommand, match, validRegex)) {
    423+            const std::string nblocks{match.str(1) == "" ? "1" : match.str(1)};
    424+            const std::string maxtries{match.str(2) == "" ? "1000000" : match.str(2)};
    


    pablomartin4btc commented at 4:19 pm on December 29, 2022:

    Sorry, just realised, perhaps you don’t need to set maxtries variable with the default value as in mining.h, DEFAULT_MAX_TRIES, it’s already set if maxtries is empty when you call generatetoaddress command implemented at mining.cpp, so:

    0            const std::string maxtries = match.str[2];
    

    hernanmarino commented at 4:01 pm on December 31, 2022:
    Thanks, I’ll test it and perhaps update the code in the next commit.
  9. pablomartin4btc commented at 4:21 pm on December 29, 2022: contributor
    b7782a4177977d62a2e2860c3a99b919b8e5b0a2 ACK.
  10. RandyMcMillan commented at 5:09 pm on December 29, 2022: contributor

    ConceptACK

    0
    1Additional logic to detect whether `-generate` is or is not followed by an integer would be great.
    2
    3![generate-gui-console](https://user-images.githubusercontent.com/152159/209985593-0fe468fc-038b-44d2-90fc-303ecc3944d4.png)
    4
    5Slightly better syntax suggestion in the error message would be excellent. 
    
  11. hernanmarino commented at 7:01 pm on December 30, 2022: contributor
    Hi @RandyMcMillan . Thank you for taking the time to look at my PR. The changes I implemented are actually on bitcoin-qt and not on bitcoin-cli . I updated the PR description to make it clearer. I believe the issues you describe are not present here, but you are welcome to test them, please let me know if you find anything.
  12. in src/qt/rpcconsole.cpp:418 in 9b0b762fd7 outdated
    414@@ -414,7 +415,32 @@ void RPCExecutor::request(const QString &command, const WalletModel* wallet_mode
    415     {
    416         std::string result;
    417         std::string executableCommand = command.toStdString() + "\n";
    418-
    419+        std::regex validRegex("generate\\s*[\\(]?\\s*(\\d+)?\\s*[,]?\\s*(\\d+)?\\s*[\\)]?\\s*");
    


    andrewtoth commented at 8:25 pm on January 2, 2023:
    This is pretty complicated regex. Would it be easier to split executableCommand into tokens separated by whitespace?


    hernanmarino commented at 3:04 am on January 19, 2023:
    At first I considered a similar approach, but since QT console’s commands are not simply separated by whitespace I decided to go with a regex. For reference you can take a look at the complexity of the parser with more than a couple of hundred lines of code at https://github.com/bitcoin-core/gui/blob/8ae2808a4354e8dcc697f76bacc5e2f2befe9220/src/qt/rpcconsole.cpp#L169-L409
  13. in src/qt/rpcconsole.cpp:423 in 9b0b762fd7 outdated
    419+        std::regex validRegex("generate\\s*[\\(]?\\s*(\\d+)?\\s*[,]?\\s*(\\d+)?\\s*[\\)]?\\s*");
    420+        std::regex invalidRegex("generate[\\s\\(]+.*");
    421+        std::smatch match;
    422+        if (std::regex_match(executableCommand, match, validRegex)) {
    423+            const std::string nblocks{match.str(1) == "" ? "1" : match.str(1)};
    424+            const std::string maxtries = match.str(2);
    


    andrewtoth commented at 8:26 pm on January 2, 2023:
    0            const std::string maxtries{match.str(2)};
    
  14. in src/qt/rpcconsole.cpp:428 in 9b0b762fd7 outdated
    424+            const std::string maxtries = match.str(2);
    425+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "getnewaddress\n", nullptr, wallet_model)) {
    426+                Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    427+            }
    428+            else {
    429+                std::string address = result;
    


    andrewtoth commented at 8:28 pm on January 2, 2023:
    0                const std::string address{result};
    
  15. in src/qt/rpcconsole.cpp:432 in 9b0b762fd7 outdated
    428+            else {
    429+                std::string address = result;
    430+                if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", nullptr, wallet_model)) {
    431+                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    432+                } else {
    433+                    std::string answer = "{\n  \"address\": \"" + address + "\",\n  \"blocks\": "+ result + "\n}";
    


    andrewtoth commented at 8:39 pm on January 2, 2023:
    Should we use UniValue to parse the result and response? See https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L686-L692

    hernanmarino commented at 6:22 pm on January 25, 2023:
    Hi @andrewtoth . I don’t think it’s really necesary , bitcoin-qt seems to take a different approach than bitcoin-cli, using strings and QStrings for communicating results. And although the RPC logic for actual commands use Univalue , these are commands intercepted and executed before that logic. Since the inputs (both address and block hashes) and the output needed are already both strings , using an Univalue as an intermediate object that is not used elsewhere, seems unnecesary. Unless you can observe something else that I’m missing, I prefer to leave this as a regular string.
  16. jarolrod added the label Feature on Jan 3, 2023
  17. jarolrod added the label UX on Jan 3, 2023
  18. pablomartin4btc commented at 7:02 pm on January 5, 2023: contributor
    Tested ACK 9b0b762fd774207d127213c1c9da354aef38e739. Checked output with different params (amount of blocks, tries), compared with outputs from bitcoin-cli as well, all good!
  19. pablomartin4btc commented at 7:07 pm on January 5, 2023: contributor
    Is it possible for someone to link this PR to the issue #55 if appropriate please?
  20. jarolrod commented at 2:44 am on January 13, 2023: member

    Concept ACK

    Did some light testing, seems to work as intended.

    Some window dressing, will review more deeply soon. First, please squash the commits so that you have a clean commit history; and give the commit a nice descriptive name like: qt: add generate command to gui console

    After you’ve squashed the commits, run the clang-format-diff script to clean up some of the code formatting issues here, for convenience you could just run the following:

    0git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v
    

    Additionally, the command could use a help generate, currently this results in a parse error as there is no help for it.

  21. furszy commented at 0:09 am on January 14, 2023: member

    Concept ACK

    Could re-write it, without the regex stuff, a bit simpler:

     0std::vector<std::string> split_command = SplitString(executableCommand, " ");
     1if (!split_command.empty() && split_command[0] == "generate") {
     2    // Remove last "\n"
     3    std::string last_param = split_command[split_command.size()-1];
     4    split_command[split_command.size()-1] = last_param.substr(0, last_param.size() - 1);
     5
     6    // Generate address
     7    std::string address_result;
     8    if (!RPCConsole::RPCExecuteCommandLine(m_node, address_result, "getnewaddress\n", nullptr, wallet_model)) {
     9        Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: cannot get new address"));
    10         return;
    11    }
    12
    13    // Craft command
    14    const std::string blocks_num = split_command.size() > 1 ? split_command[1] : "1";
    15    const std::string max_tries = split_command.size() > 2 ? split_command[2] : "";
    16    executableCommand = "generatetoaddress " + blocks_num + " \"" + address_result + "\" " + max_tries;
    17}
    
  22. hernanmarino commented at 1:00 am on January 14, 2023: contributor
    Thanks for all the insights. In the first days of the next week I’ll try to incorporate the feedback and squash the commits.
  23. hebasto renamed this:
    GUI: Debug Console implementation of generate method
    Debug Console implementation of generate method
    on Jan 15, 2023
  24. hebasto commented at 7:03 pm on January 15, 2023: member
    Concept ACK. @hernanmarino You could also link the initial issue in the PR description.
  25. rserranon commented at 11:08 pm on January 16, 2023: none

    Concept ACK

    Could re-write it, without the regex stuff, a bit simpler:

     0std::vector<std::string> split_command = SplitString(executableCommand, " ");
     1if (!split_command.empty() && split_command[0] == "generate") {
     2    // Remove last "\n"
     3    std::string last_param = split_command[split_command.size()-1];
     4    split_command[split_command.size()-1] = last_param.substr(0, last_param.size() - 1);
     5
     6    // Generate address
     7    std::string address_result;
     8    if (!RPCConsole::RPCExecuteCommandLine(m_node, address_result, "getnewaddress\n", nullptr, wallet_model)) {
     9        Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: cannot get new address"));
    10         return;
    11    }
    12
    13    // Craft command
    14    const std::string blocks_num = split_command.size() > 1 ? split_command[1] : "1";
    15    const std::string max_tries = split_command.size() > 2 ? split_command[2] : "";
    16    executableCommand = "generatetoaddress " + blocks_num + " \"" + address_result + "\" " + max_tries;
    17}
    

    Does it make sense to include parsing logic, generate a new address, and then craft the command to call the generatetoaddress, inside the RPCExecutor::request function, or it should be done on a separate function?

  26. furszy commented at 11:44 pm on January 16, 2023: member

    Does it make sense to include parsing logic, generate a new address, and then craft the command to call the generatetoaddress, inside the RPCExecutor::request function, or it should be done on a separate function?

    A separate function returning the command string would be better.

  27. RandyMcMillan commented at 0:16 am on January 25, 2023: contributor

    tested on top of a62231bca629e945349255a1d331dd5c7a86ddd1

    Tested ConceptACK 9b0b762fd774207d127213c1c9da354aef38e739

    macOS: Arm64

    Screen Shot 2023-01-24 at 7 12 54 PM

  28. RandyMcMillan commented at 0:33 am on January 25, 2023: contributor

    tested on top of https://github.com/bitcoin-core/gui/commit/a62231bca629e945349255a1d331dd5c7a86ddd1

    Tested ConceptACK 9b0b762fd774207d127213c1c9da354aef38e739

    macOS: x86_64

    Screen Shot 2023-01-24 at 7 31 04 PM

  29. RandyMcMillan commented at 1:07 am on January 25, 2023: contributor

    @hernanmarino - @rserranon ’s suggestion is compelling…

    https://github.com/RandyMcMillan/gui/tree/612a0c14173d681a04021581851422ba832236ee

    Applied as a patch on top of this PR: https://github.com/RandyMcMillan/gui/blob/612a0c14173d681a04021581851422ba832236ee/rserranon.patch

    I haven’t fully tested the patch but works on MacOS x86_64

  30. hernanmarino commented at 1:27 am on January 25, 2023: contributor

    Thanks for the feedback and testing. I ’ m already working on an update consider this and all feedback received, will update soon.

    @hernanmarino - @rserranon ’s suggestion is compelling…

    RandyMcMillan/gui@612a0c1

    Applied as a patch on top of this PR: RandyMcMillan/gui@612a0c1/rserranon.patch

    I haven’t fully tested the patch but works on MacOS x86_64

  31. hernanmarino force-pushed on Jan 25, 2023
  32. hernanmarino commented at 8:24 pm on January 25, 2023: contributor

    Summary of changes just pushed:

    • Changed the regex for a simpler string splitter, while still mantaining the syntax and several argument separators (beyond whitespace) supported by the console. Code now runs 180X faster (thanks @furszy and @andrewtoth for the suggestion)
    • Moved the implementation to a separate method that deals with these console-only commands (@rserranon)Also moved the already implemented “help-console” command handling to this method.
    • Added “help generate” code
    • Cleaned some code formatting issues according to clang-format ( thanks @jarolrod for the last two :) )
    • Squashed all changes on a single commit
  33. hernanmarino requested review from pablomartin4btc on Jan 25, 2023
  34. hernanmarino force-pushed on Jan 27, 2023
  35. hernanmarino removed review request from pablomartin4btc on Jan 30, 2023
  36. hernanmarino requested review from andrewtoth on Jan 30, 2023
  37. pablomartin4btc commented at 8:48 pm on January 31, 2023: contributor
    re ack 3f8fe6fd698380d659e5c88b0795f8d6ea3a2be0, cleaner than before.
  38. pablomartin4btc approved
  39. furszy commented at 8:11 pm on February 8, 2023: member

    Noticed that we are calling cli-only commands with the “-” prefix. Would be good to follow the same convention here too. The renaming will let us detect them in a simpler manner by checking the first character without requiring to split the string etc.

    Aside from that, now talking about the current changes: What do you think about structuring the changes a bit differently so your patch set is focused to the introduced feature, and we can continue expanding the cli-only options in the future:

     0if (IsCliOnlyCommand(executableCommand)) { // --> here we check if string starts with "-". 
     1    // Execute cli-only commands
     2    if (!ExecuteCliCommand(executableCommand, result, etc)) { // --> This function will dispatch cli-only commands ("-generate" should be placed into its own standalone function).
     3         // emit any error here or inside the function
     4         return;
     5    }
     6} else {
     7   // Execute RPC commands
     8   if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
     9        // etc...
    10}
    
  40. rserranon commented at 9:59 pm on February 10, 2023: none
     0if (IsCliOnlyCommand(executableCommand)) { // --> here we check if string starts with "-". 
     1    // Execute cli-only commands
     2    if (!ExecuteCliCommand(executableCommand, result, etc)) { // --> This function will dispatch cli-only commands ("-generate" should be placed into its own standalone function).
     3         // emit any error here or inside the function
     4         return;
     5    }
     6} else {
     7   // Execute RPC commands
     8   if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
     9        // etc...
    10}
    

    I agree with furszy’s idea of introducing the functions IsCliOnlyCommand and ExecuteCliCommand , to implement the generate cli-only command, for a cleaner implementation and with the advantage of leaving the structure ready for potential expansion of additional cli-only commands.

  41. pablomartin4btc commented at 9:36 pm on February 13, 2023: contributor

    I wouldn’t change the naming convention of the generate command in the “RPC Console” to -generate in order to match cli-commands. If we start to implement such commands into here maybe I’d think about it but this is the “RPC Console” it has a different purpose even the final objective is to communicate with the node.

    Also, as soon we do this, code-wise, we would need to remove the RPCHelpMan in src/rpc/mining.cpp:

    0static RPCHelpMan generate()
    1{
    2    return RPCHelpMan{"generate", "has been replaced by the -generate cli option. Refer to -help for more information.", {}, {}, RPCExamples{""}, [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue {
    3        throw JSONRPCError(RPC_METHOD_NOT_FOUND, self.ToString());
    4    }};
    5}
    

    Otherwise we will get this message error:

    image

    And if we do that, any other app trying to send the ‘generatecommand via RPC would receive themethod not found` error:

    image

    I believe the code is good as it is so far and as a first and safe approach to resolve the issue.

  42. hernanmarino commented at 10:43 pm on February 13, 2023: contributor

    Noticed that we are calling cli-only commands with the “-” prefix. Would be good to follow the same convention here too. The renaming will let us detect them in a simpler manner by checking the first character without requiring to split the string etc.

    This seems like something “nice to have” but there are other issues apart from what Pablo mentioned that makes me think that we shouldn´t do that:

    • There are other console-only commands that already existed, like help-console
    • This PR not only introduces the new behaviour for generate but also help generate as @jarolrod suggested
    • Future console-only commands that might be introduced later will also require their own help new_command

    None of this commands could be easily renamed to start with “-” without breaking “protocol” and backwards compatibility, and if we don’t do that, I believe the approach implemented in this PR is good enough. If in any case, if there’s further interest to discuss alternatives, perhaps we can take that discussion to a follow-up PR.

    Aside from that, now talking about the current changes: What do you think about structuring the changes a bit differently so your patch set is focused to the introduced feature, and we can continue expanding the cli-only options in the future:

     0if (IsCliOnlyCommand(executableCommand)) { // --> here we check if string starts with "-". 
     1    // Execute cli-only commands
     2    if (!ExecuteCliCommand(executableCommand, result, etc)) { // --> This function will dispatch cli-only commands ("-generate" should be placed into its own standalone function).
     3         // emit any error here or inside the function
     4         return;
     5    }
     6} else {
     7   // Execute RPC commands
     8   if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
     9        // etc...
    10}
    

    If we abandon the idea of renaming commands to start with “-” the suggested code is very similar to this PR’s code, only that the method ExecuteCliCommand is called executeConsoleOnlyCommand.

  43. hernanmarino commented at 10:50 pm on February 13, 2023: contributor

    I agree with furszy’s idea of introducing the functions IsCliOnlyCommand and ExecuteCliCommand , to implement the generate cli-only command, for a cleaner implementation and with the advantage of leaving the structure ready for potential expansion of additional cli-only commands.

    The provided method executeConsoleOnlyCommand is almost identical to the suggested ExecuteCliCommand and is also ready for potential future new console only commands, the only difference is that it detects ( like IsCliOnlyCommand ) and executes commands in a single method.

  44. rserranon commented at 11:22 pm on February 13, 2023: none
    testing ACK
  45. in src/qt/rpcconsole.cpp:461 in 3f8fe6fd69 outdated
    456+ */
    457+
    458+std::vector<std::string> RPCExecutor::parseHelper(const std::string& strCommand)
    459+{
    460+    std::vector<std::string> vec = SplitString(strCommand, " (),\n");
    461+    auto should_remove = [](const std::string& str) { return str.empty(); };
    


    LarryRuane commented at 4:08 pm on February 16, 2023:
    0    std::vector<std::string> vec{SplitString(strCommand, " (),\n")};
    1    auto should_remove{[](const std::string& str) { return str.empty(); }};
    
  46. in src/qt/rpcconsole.cpp:490 in 3f8fe6fd69 outdated
    485+        const std::string nblocks{parsedCommand.size() > 1 ? parsedCommand[1] : "1"};
    486+        // An empty maxtries parameters will be modified by generatetoaddress' default value
    487+        const std::string maxtries{parsedCommand.size() > 2 ? parsedCommand[2] : ""};
    488+
    489+        // Generate address
    490+        if (!RPCConsole::RPCExecuteCommandLine(m_node, address, "getnewaddress\n", nullptr, wallet_model)) {
    


    LarryRuane commented at 4:10 pm on February 16, 2023:
    0        if (!RPCConsole::RPCExecuteCommandLine(m_node, address, "getnewaddress\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    
  47. in src/qt/rpcconsole.cpp:494 in 3f8fe6fd69 outdated
    489+        // Generate address
    490+        if (!RPCConsole::RPCExecuteCommandLine(m_node, address, "getnewaddress\n", nullptr, wallet_model)) {
    491+            Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate new address"));
    492+        } else {
    493+            executableCommand = "generatetoaddress " + nblocks + " \"" + address + "\" " + maxtries;
    494+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", nullptr, wallet_model)) {
    


    LarryRuane commented at 4:16 pm on February 16, 2023:
    0            auto executableCommand = "generatetoaddress " + nblocks + " " + address + " " + maxtries;
    1            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand + "\n", nullptr, wallet_model)) {
    

    Also remove the declaration of executableCommand near the top of the function; better to declare variables only where and when needed. You could do similarly with result and address.


    hernanmarino commented at 6:37 pm on March 1, 2023:
    I ’ ll actually remove the variable executableCommand since it’s only used once, I prefer avoiding memory overhead and better speed execution (although minimal). Okey with the other 2 variables
  48. in src/qt/rpcconsole.cpp:514 in 3f8fe6fd69 outdated
    490+        if (!RPCConsole::RPCExecuteCommandLine(m_node, address, "getnewaddress\n", nullptr, wallet_model)) {
    491+            Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate new address"));
    492+        } else {
    493+            executableCommand = "generatetoaddress " + nblocks + " \"" + address + "\" " + maxtries;
    494+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", nullptr, wallet_model)) {
    495+                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate blocks"));
    


    LarryRuane commented at 4:20 pm on February 16, 2023:
    This line is indented one tab stop too much, and same for the two lines just below (after the else).

    hernanmarino commented at 6:40 pm on March 1, 2023:
    Indentation was introduced after running the script suggested by @ jarolrod and are apparently ok according to clang format standard.

    LarryRuane commented at 10:12 pm on March 15, 2023:

    That’s strange, when I run clang-format-diff, it moves this left, where they should be:

     0$ git show -U0 | ./contrib/devtools/clang-format-diff.py -p1 -i -v
     1Formatting src/qt/rpcconsole.cpp
     2$ git diff
     3diff --git a/src/qt/rpcconsole.cpp b/src/qt/rpcconsole.cpp
     4index 1408b289f..63820464b 100644
     5--- a/src/qt/rpcconsole.cpp
     6+++ b/src/qt/rpcconsole.cpp
     7@@ -419,12 +419,12 @@ void RPCExecutor::request(const QString &command, const WalletModel* wallet_mode
     8 
     9         // Attempt to execute console-only commands
    10         if (!RPCExecutor::executeConsoleOnlyCommand(executableCommand, wallet_model)) {
    11-                // Send to the RPC command parser if not console-only
    12-                if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
    13-                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    14-                    return;
    15-                }
    16-                Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(result));
    17+            // Send to the RPC command parser if not console-only
    18+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
    19+                Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    20+                return;
    21+            }
    22+            Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(result));
    23         }
    24     }
    25     catch (UniValue& objError)
    26@@ -492,10 +492,10 @@ bool RPCExecutor::executeConsoleOnlyCommand(const std::string& command, const Wa
    27             Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate new address"));
    28         } else {
    29             if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    30-                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate blocks"));
    31+                Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate blocks"));
    32             } else {
    33-                    std::string answer = "{\n  \"address\": \"" + address + "\",\n  \"blocks\": " + result + "\n}";
    34-                    Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString("\n" + answer + "\n\n"));
    35+                std::string answer = "{\n  \"address\": \"" + address + "\",\n  \"blocks\": " + result + "\n}";
    36+                Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString("\n" + answer + "\n\n"));
    37             }
    38         }
    39         return true;
    

    pablomartin4btc commented at 0:34 am on March 16, 2023:
    I’ll try from my side and give it a go later this week.
  49. in src/qt/rpcconsole.cpp:511 in 3f8fe6fd69 outdated
    506+                                                     "Generate blocks, equivalent to RPC getnewaddress followed by RPC generatetoaddress.\n"
    507+                                                     "Optional integer arguments are number of blocks to generate and maximum iterations to try.\n"
    508+                                                     "Equivalent to RPC generatetoaddress nblocks and maxtries arguments.\n"
    509+                                                     "   example:    generate\n"
    510+                                                     "   example:    generate 4\n"
    511+                                                     "   example:    generate 3 6000\n\n")));
    


    LarryRuane commented at 4:34 pm on February 16, 2023:

    Formatting-only suggestion (personal preference to avoid long lines), and the argument to QString() had extra parentheses. Could make a similar change below.

     0        if ((parsedCommand.size() > 1 && parsedCommand[0] == "help" && parsedCommand[1] == "generate") ||
     1            (parsedCommand.size() > 3 && parsedCommand[0] == "generate")) {
     2                auto message{
     3                    "\n"
     4                    "Generate blocks, equivalent to RPC getnewaddress followed by RPC generatetoaddress.\n"
     5                    "Optional integer arguments are number of blocks to generate and maximum iterations to try.\n"
     6                    "Equivalent to RPC generatetoaddress nblocks and maxtries arguments.\n"
     7                    "   example:    generate\n"
     8                    "   example:    generate 4\n"
     9                    "   example:    generate 3 6000\n\n"};
    10                Q_EMIT reply(RPCConsole::CMD_REPLY, QString(message));
    

    hernanmarino commented at 6:41 pm on March 1, 2023:
    Removed extra parenthesis, and I agree with extra long lines, but as I said before, indentation was introduced after running the script suggested by @ jarolrod and are apparently ok according to clang format standard.
  50. LarryRuane commented at 4:52 pm on February 16, 2023: contributor
    Concept, approach, and tested ACK, this is a nice improvement. It might be good to prepend “test:” to the title of this PR (and this might pick up the “test” label, which would be nice). I suggest making that change to the commit title too (“test: qt: add generate …”). If you’d like to make some or all of these suggested changes, I’ll re-review.
  51. furszy commented at 1:49 pm on February 23, 2023: member

    All good about the none usage of the “-” prefix on the GUI 👍🏼 .

    The provided method executeConsoleOnlyCommand is almost identical to the suggested ExecuteCliCommand and is also ready for potential future new console only commands, the only difference is that it detects ( like IsCliOnlyCommand ) and executes commands in a single method.

    I’m not fan of the current method mostly because the parsedCommand.size() > something and parsedCommand.size() < something_else checks. Those are specific checks that should be done inside each command function, and not at the top level dispatcher function.

    A slightly modified version of the code that shared before so we are in the same page:

     0std::string cli_command;
     1if (IsCliOnlyCommand(executableCommand, cli_command)) {  // Check against a list of known cli-only commands and return the command inside "cli_command".
     2    // Craft Cli Command
     3    std::string res_cli_only_command;
     4    if (!CraftCliCommand(cli_command, executableCommand, res_cli_only_command)) {
     5         return; // errors could be emitted inside the function or here if it returns an error.
     6    }
     7    // Update command
     8    executableCommand = res_cli_only_command;
     9}
    10
    11// Execute command
    12if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
    13    // etc...
    

    Then could do something like:

     0bool CraftCliCommand(const std::string& cli_command, const std::string& executableCommand, std::string& command_ret)
     1{
     2    switch(cli_command) {
     3        case GENERATE_COMMAND:
     4            command_ret  = craftGenerateCommand(executableCommand);
     5            return true;
     6        case OTHER_CLI_COMMAND:
     7            command_ret  = craftOtherCliCommand(executableCommand);
     8            return true;
     9        // etc..
    10    }
    11
    12    // command not implemented.
    13    return false;
    14}
    

    In this way, each command is contained within their own function and new checks/changes can be added or removed without requiring to modify the top level function. What do you think?

  52. hernanmarino force-pushed on Mar 1, 2023
  53. hernanmarino removed review request from andrewtoth on Mar 2, 2023
  54. hernanmarino requested review from pablomartin4btc on Mar 2, 2023
  55. in src/qt/rpcconsole.cpp:433 in 037e8b0138 outdated
    442-        }
    443-        if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
    444-            Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    445-            return;
    446+        // Attempt to execute console-only commands
    447+        if (!RPCExecutor::executeConsoleOnlyCommand(executableCommand, wallet_model)) {
    


    LarryRuane commented at 10:04 pm on March 15, 2023:
    0        if (RPCExecutor::executeConsoleOnlyCommand(executableCommand, wallet_model)) {
    1            return;
    2        }
    

    (or could put return on the same line)

  56. in src/qt/rpcconsole.cpp:478 in 037e8b0138 outdated
    473+ */
    474+
    475+bool RPCExecutor::executeConsoleOnlyCommand(const std::string& command, const WalletModel* wallet_model)
    476+{
    477+    std::string result;
    478+    std::string address;
    


    LarryRuane commented at 10:06 pm on March 15, 2023:
    these can be removed
  57. in src/qt/rpcconsole.cpp:479 in 037e8b0138 outdated
    474+
    475+bool RPCExecutor::executeConsoleOnlyCommand(const std::string& command, const WalletModel* wallet_model)
    476+{
    477+    std::string result;
    478+    std::string address;
    479+    std::vector<std::string> parsedCommand{parseHelper(command)};
    


    LarryRuane commented at 10:06 pm on March 15, 2023:
    0    const std::vector<std::string> parsedCommand{parseHelper(command)};
    
  58. LarryRuane commented at 10:15 pm on March 15, 2023: contributor
    tested 037e8b01386d5f30f295eec53eb939204b855cb4
  59. in src/qt/rpcconsole.cpp:99 in 037e8b0138 outdated
    95@@ -96,6 +96,8 @@ public Q_SLOTS:
    96 
    97 private:
    98     interfaces::Node& m_node;
    99+    std::vector<std::string> parseHelper(const std::string& strCommand);
    


    hebasto commented at 1:25 pm on March 27, 2023:

    Naming nit:

    0    std::vector<std::string> parseHelper(const std::string& str_command);
    
  60. hebasto commented at 1:26 pm on March 27, 2023: member

    Could this PR be separated in two commits:

    • refactoring and introducing the RPCExecutor::executeConsoleOnlyCommand() function
    • actually adding support for the generate command`

    ?

  61. hernanmarino commented at 2:56 pm on March 27, 2023: contributor

    Could this PR be separated in two commits:

    • refactoring and introducing the RPCExecutor::executeConsoleOnlyCommand() function
    • actually adding support for the generate command`

    ?

    Yes, will do ASAP.

  62. hernanmarino force-pushed on May 12, 2023
  63. hernanmarino force-pushed on May 12, 2023
  64. hernanmarino commented at 10:08 pm on May 16, 2023: contributor

    As requested by @hebasto i splitted this PR in 2 commits : ad5642ae91beb522b6ae806f28cb015a759d1d19 refactors the code to prepare for the new functionality implemented in the following commit, as well as for future console-only commands to be added.

    411b1da407f78f2f973f90d48676ce1ae26734a7 adds the new “generate” command, as well as the updated “help generate”

  65. hebasto requested review from furszy on May 18, 2023
  66. hebasto requested review from jarolrod on May 18, 2023
  67. pablomartin4btc approved
  68. pablomartin4btc commented at 9:51 pm on May 18, 2023: contributor
    Liked the refactoring and the 2 commits split as @hebasto requested. Tested ACK both commits separately, also checked also other RPC commands, it works as expected.
  69. in src/qt/rpcconsole.cpp:100 in ad5642ae91 outdated
     95@@ -96,6 +96,16 @@ public Q_SLOTS:
     96 
     97 private:
     98     interfaces::Node& m_node;
     99+    static std::vector<std::string> parseHelper(const std::string& strCommand);
    100+    static bool commandMatches(const std::vector<std::string>& parsed_command, const std::string& key);
    


    furszy commented at 2:45 pm on May 23, 2023:

    In ad5642ae:

    This two declarations can be removed. Can just make the definition static.


    hernanmarino commented at 5:05 pm on June 15, 2023:
    Done. Since the compilar doesn´t actually allow what you suggest, i removed the declarations from the class and implemented this 2 method as independent functions.
  70. in src/qt/rpcconsole.cpp:468 in ad5642ae91 outdated
    463+ * @return a vector of strings with command and parameters
    464+ */
    465+std::vector<std::string> RPCExecutor::parseHelper(const std::string& strCommand)
    466+{
    467+    // Split while recognizing the several characters that can be used as separators in the GUI console
    468+    std::vector<std::string> vec{SplitString(strCommand, " (),\n")};
    


    furszy commented at 2:50 pm on May 23, 2023:

    In ad5642ae:

    Can remove the “\n” separator by providing the command variable instead of the executableCommand string at line 428. The code adds the “\n” few lines before calling executeConsoleOnlyCommand.


    hernanmarino commented at 4:04 pm on June 15, 2023:
    Done
  71. in src/qt/rpcconsole.cpp:499 in ad5642ae91 outdated
    494+ * @return  True if the command was executed, false otherwise.
    495+ */
    496+bool RPCExecutor::executeConsoleHelpConsole(const std::vector<std::string>& parsed_command, const WalletModel* wallet_model)
    497+{
    498+    // Catch the console-only-help command before RPC call is executed and reply with help text as-if a RPC reply.
    499+    if (!parsed_command.empty() && parsed_command[0] == "help-console") {
    


    furszy commented at 2:52 pm on May 23, 2023:

    In ad5642ae:

    Can remove the conditional statement entirely and always return true.

    1. Functions are mapped in a “command name” -> function map structure. The code will never reach this point If the command wouldn’t be a help-console.

    2. The !parsed_command.empty() statement is not needed. It will never be empty at this point, the parent dispatcher function already checks for emptiness.


    hernanmarino commented at 5:29 pm on June 15, 2023:
    Modified
  72. in src/qt/rpcconsole.cpp:540 in ad5642ae91 outdated
    535+    } else {
    536+        // Iterate over m_method_map and execute the associated method if the beggining of parsed_command matches the key
    537+        for (auto const& [key, method] : m_method_map) {
    538+            if (commandMatches(parsed_command, key)) {
    539+                    return (this->*method)(parsed_command, wallet_model);
    540+            }
    


    furszy commented at 2:58 pm on May 23, 2023:

    In ad5642ae:

    Wrong Indentation here.

  73. in src/qt/rpcconsole.cpp:536 in ad5642ae91 outdated
    531+    // Parse command line into a vector of strings
    532+    const std::vector<std::string> parsed_command{parseHelper(command)};
    533+    if (parsed_command.empty()) {
    534+        return false;
    535+    } else {
    536+        // Iterate over m_method_map and execute the associated method if the beggining of parsed_command matches the key
    


    furszy commented at 3:00 pm on May 23, 2023:

    In ad5642ae:

    I think that would be cleaner to change the if/else to:

    0const std::vector<std::string> parsed_command{parseHelper(command)};
    1if (parsed_command.empty()) return false;
    2
    3// Iterate over m_method_map and execute the associated method if the beggining of parsed_command matches the key
    4etc..
    

    pablomartin4btc commented at 7:33 pm on June 13, 2023:
    I agree.

    hernanmarino commented at 5:33 pm on June 15, 2023:
    Agree ! Changed
  74. in src/qt/rpcconsole.cpp:503 in 411b1da407 outdated
    498+ * @return True if the command was executed, false otherwise.
    499+ */
    500+bool RPCExecutor::executeConsoleGenerate(const std::vector<std::string>& parsed_command, const WalletModel* wallet_model)
    501+{
    502+    // Catch the console-only generate command with 2 or less parameters before RPC call is executed .
    503+    if (!parsed_command.empty() && parsed_command[0] == "generate" && parsed_command.size() <= 3) {
    


    furszy commented at 3:05 pm on May 23, 2023:

    In 411b1da4:

    The !parsed_command.empty() is not needed. The dispatcher function already checks for emptiness. Also, it’s duplicated in both conditional branches.

    Same for the parsed_command[0] == "generate". The code wouldn’t reach this point if the command wouldn’t be a generate.


    pablomartin4btc commented at 7:46 pm on June 13, 2023:
    I agree, I thought before that could be a pseudo-implementation of a handler pattern but the evaluation is being done checking the key of the map is being the specific command in the command-line.

    hernanmarino commented at 1:35 am on June 16, 2023:
    Changed.
  75. in src/qt/rpcconsole.cpp:542 in 411b1da407 outdated
    537+ * @return  True if the command was executed, false otherwise.
    538+ */
    539+bool RPCExecutor::executeConsoleHelpGenerate(const std::vector<std::string>& parsed_command, const WalletModel* wallet_model)
    540+{
    541+    // Catch the console-only "help generate" command when requested, or when "generate is called with wrong parameters.
    542+    if ((parsed_command.size() >= 2 && parsed_command[0] == "help" && parsed_command[1] == "generate") || (parsed_command.size() > 3 && parsed_command[0] == "generate")) {
    


    furszy commented at 3:07 pm on May 23, 2023:

    In 411b1da4:

    Similar to the one above, The parsed_command[0] == "help" && parsed_command[1] == "generate" statement is not needed. The code wouldn’t reach this point if the command wouldn’t be a help generate command.


    pablomartin4btc commented at 7:51 pm on June 13, 2023:
    It would if -> Default to “help generate” when wrong number of parameters are given (line 524), but it’s the same case anyway, it’s the condition after theoroperator,

    hernanmarino commented at 1:36 am on June 16, 2023:
    Modified.
  76. in src/qt/rpcconsole.cpp:577 in ad5642ae91 outdated
    524+ * @param[in]    command  Command line to execute
    525+ * @param[in]    wallet_model  Wallet model to use
    526+ * @return  true if command was handled by this method (even on errors), false otherwise
    527+ *
    528+ */
    529+bool RPCExecutor::executeConsoleOnlyCommand(const std::string& command, const WalletModel* wallet_model)
    


    furszy commented at 3:10 pm on May 23, 2023:

    In ad5642a:

    nit: instead of passing the wallet model pointer, could pass by reference. Always safer than passing pointers around.


    pablomartin4btc commented at 7:34 pm on June 13, 2023:
    I agree.

    hernanmarino commented at 5:50 pm on June 15, 2023:
    I usually agree with you @furszy , but in this case I’m just passing around a pointer i didn´t create but received from a chain of method calls that already use pointers. Also, the pointer will be needed further down the chain of calls by other methods. I prefer to not modify this and keep it this way, if you don’t mind :slightly_smiling_face:
  77. furszy commented at 3:11 pm on May 23, 2023: member

    Looking much better, thanks for applying the suggestions 👌🏼.

    Left few comments, it’s getting closer.

  78. pablomartin4btc commented at 7:54 pm on June 13, 2023: contributor
    I think you’d need to check some comments.
  79. hebasto commented at 9:32 am on June 14, 2023: member
    @hernanmarino Are you still working on this PR?
  80. hernanmarino commented at 2:03 pm on June 14, 2023: contributor

    @hernanmarino Are you still working on this PR?

    Yes, will update ASAP.

  81. hernanmarino force-pushed on Jun 16, 2023
  82. hernanmarino commented at 2:03 am on June 16, 2023: contributor

    I changed the code to address @furszy’s suggestions. The new commit hashes are: aa898d6 refactors the code to prepare for the new functionality implemented in the following commit, as well as for future console-only commands to be added.

    1db3da2 adds the new “generate” command, as well as the updated “help generate”

  83. in src/qt/rpcconsole.cpp:589 in aa898d6a72 outdated
    530+    // Iterate over m_method_map and execute the associated method if the beggining of parsed_command matches the key
    531+    for (auto const& [key, method] : m_method_map) {
    532+        if (commandMatches(parsed_command, key)) {
    533+            return (this->*method)(parsed_command, wallet_model);
    534+        }
    535+    }
    


    furszy commented at 9:16 pm on June 21, 2023:

    This loop over all entries breaks the purpose of using a map, could have been a set<pair<string, method».

    Or.. instead of the loop, could:

    0std::string method = parsed_command[0];
    1bool exec_help = false;
    2if (method == "help") {
    3    exec_help = true;
    4    method = parsed_command[1];
    5}
    6auto it_method = m_method_map.find(method);
    7if (it_method == m_method_map.end()) return false; // method not found
    8return (*it_method.second)(parsed_command, wallet_model, exec_help);
    

    hernanmarino commented at 4:02 am on September 5, 2023:
    Implemented
  84. pablomartin4btc commented at 11:26 am on June 23, 2023: contributor
    re-ACK, verified all code changes, nice job. Tested the following commands in the RPC Console window in qt using regtest: generate (also with invalid arguments), help, help-console, help generate, getblockhash 0, getindexinfo.
  85. in src/qt/rpcconsole.cpp:122 in aa898d6a72 outdated
    114+ * @return a vector of strings with command and parameters
    115+ */
    116+std::vector<std::string> parseHelper(const std::string& strCommand)
    117+{
    118+    // Split while recognizing the several characters that can be used as separators in the GUI console
    119+    std::vector<std::string> vec{SplitString(strCommand, " (),")};
    


    furszy commented at 1:35 pm on July 1, 2023:
    Not really for this PR but this will not work with console-only commands that contain descriptors as they use () internally.

    hernanmarino commented at 9:28 pm on September 4, 2023:
    Yes, agree. This will have to be refactored if such thing is ever needed.
  86. in src/qt/rpcconsole.cpp:514 in 1db3da2f8b outdated
    509+        std::string address;
    510+        if (!RPCConsole::RPCExecuteCommandLine(m_node, address, "getnewaddress\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    511+            Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate new address"));
    512+        } else {
    513+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    514+                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate blocks"));
    


    furszy commented at 1:40 pm on July 1, 2023:

    nit: format:

    0                Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate blocks"));
    

    hernanmarino commented at 7:41 pm on August 28, 2023:
    Modified
  87. in src/qt/rpcconsole.cpp:517 in 1db3da2f8b outdated
    512+        } else {
    513+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    514+                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate blocks"));
    515+            } else {
    516+                    std::string answer = "{\n  \"address\": \"" + address + "\",\n  \"blocks\": " + result + "\n}";
    517+                    Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString("\n" + answer + "\n\n"));
    


    furszy commented at 1:41 pm on July 1, 2023:

    nit: format:

    0                std::string answer = "{\n  \"address\": \"" + address + "\",\n  \"blocks\": " + result + "\n}";
    1                Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString("\n" + answer + "\n\n"));
    

    hernanmarino commented at 7:40 pm on August 28, 2023:
    Modified
  88. in src/qt/rpcconsole.cpp:513 in 1db3da2f8b outdated
    508+        std::string result;
    509+        std::string address;
    510+        if (!RPCConsole::RPCExecuteCommandLine(m_node, address, "getnewaddress\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    511+            Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: could not generate new address"));
    512+        } else {
    513+            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, "generatetoaddress " + nblocks + " " + address + " " + maxtries + "\n", /*pstrFilteredOut=*/nullptr, wallet_model)) {
    


    furszy commented at 1:45 pm on July 1, 2023:

    nit:

    0            if (!RPCConsole::RPCExecuteCommandLine(m_node, result, strprintf("generatetoaddress %s %s %s\n", nblocks, address, maxtries), /*pstrFilteredOut=*/nullptr, wallet_model)) {
    
  89. in src/qt/rpcconsole.cpp:500 in 1db3da2f8b outdated
    495+ * @param wallet_model  WalletModel to use for the command
    496+ * @return True if the command was executed, false otherwise.
    497+ */
    498+bool RPCExecutor::executeConsoleGenerate(const std::vector<std::string>& parsed_command, const WalletModel* wallet_model)
    499+{
    500+    // Catch the console-only generate command with 2 or less parameters before RPC call is executed .
    


    furszy commented at 1:49 pm on July 1, 2023:
    Would be good to lock this for test networks only so users don’t end up generating addresses for no reason.

    hernanmarino commented at 1:09 am on August 29, 2023:
    Done
  90. hebasto requested review from furszy on Jul 3, 2023
  91. furszy commented at 2:31 pm on July 3, 2023: member
    Left a quick review, not really blocking. Mostly performance and code improvements. Will finish testing it later today and ACK it if all goes well.
  92. furszy commented at 2:50 pm on July 3, 2023: member

    Two small scenarios to fix:

    1. Should guard generate from using negative numbers. Currently, can call generate -20 and the process will return an empty blocks array instead of failing.
    2. The generatetoaddress result has a bracket without the correct indentation. See
  93. luke-jr commented at 9:39 pm on July 27, 2023: member
    Approach NACK. If we’re going to go this route, we can just add back the RPC method… :/
  94. hernanmarino force-pushed on Sep 5, 2023
  95. DrahtBot added the label CI failed on Sep 5, 2023
  96. hernanmarino force-pushed on Sep 5, 2023
  97. hernanmarino force-pushed on Sep 5, 2023
  98. hernanmarino force-pushed on Sep 5, 2023
  99. hernanmarino requested review from furszy on Sep 5, 2023
  100. hernanmarino requested review from pablomartin4btc on Sep 5, 2023
  101. DrahtBot removed the label CI failed on Sep 5, 2023
  102. hernanmarino commented at 0:53 am on September 6, 2023: contributor
    Updated with all the changes suggested by @furszy in his latest review.
  103. pablomartin4btc approved
  104. pablomartin4btc commented at 10:52 pm on September 17, 2023: contributor

    re tACK e6e0dacc92953f1a190a18bf64d18fd7572354d7

    Tested the 2 scenarios detected by @furszy above, both fixed.

  105. DrahtBot removed review request from furszy on Sep 17, 2023
  106. pablomartin4btc commented at 3:33 am on October 11, 2023: contributor

    Approach NACK. If we’re going to go this route, we can just add back the RPC method… :/

    I think we can’t bring the RPC command back as it was removed to decouple the wallet dependency from the node (#14468) simplifying also its interface (#10973).

    Also as generate is only used for testing, and it’s already implemented in cli, personally makes sense to have it in qt, as for me is even more practical to use it from the rpc console rather than configure qt to enable rpc (need to add "server": true, in settings.json) and send commands from a terminal with cli.

    Code-wise, the first commit, which refactors the current console-side commands (eg help-console) allows adding new console commands easily (if we ever need to).

  107. DrahtBot requested review from furszy on Oct 11, 2023
  108. DrahtBot requested review from LarryRuane on Oct 11, 2023
  109. DrahtBot requested review from hebasto on Oct 11, 2023
  110. hernanmarino commented at 5:29 pm on October 11, 2023: contributor

    Approach NACK. If we’re going to go this route, we can just add back the RPC method… :/

    I understand your point of view, but the reasons mentioned in the issues ( #55 https://github.com/bitcoin/bitcoin/issues/16000 ), some of them mentioned by Pablo, encouraged me to work on this. Also, it evolved to (a litlle bit) more than bringing back the RPC call.

  111. DrahtBot removed review request from LarryRuane on Oct 11, 2023
  112. DrahtBot requested review from LarryRuane on Oct 11, 2023
  113. hernanmarino commented at 9:20 pm on October 15, 2023: contributor
    @maflcko @jonatack @Sjors since you opened or ACK’ed the original issues I kindly request your review on this PR.
  114. DrahtBot removed review request from LarryRuane on Oct 15, 2023
  115. DrahtBot requested review from LarryRuane on Oct 15, 2023
  116. DrahtBot requested review from luke-jr on Oct 15, 2023
  117. Sjors commented at 5:27 am on October 23, 2023: member

    Approach NACK. If we’re going to go this route, we can just add back the RPC method… :/

    IIUC the reason the RPC method was removed (and replaced with bitcoin-cli -generate) is because it had a wallet dependency. The GUI has wallet dependencies all over the place, so I don’t see a problem adding this.

  118. DrahtBot removed review request from LarryRuane on Oct 23, 2023
  119. DrahtBot requested review from LarryRuane on Oct 23, 2023
  120. in src/qt/rpcconsole.cpp:494 in e6e0dacc92 outdated
    488+    if (parsed_command.size() > 3 || exec_help) {
    489+        executeConsoleHelpGenerate();
    490+        return true;
    491+    }
    492+    // Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated
    493+    if (Params().GetChainType() == ChainType::MAIN) {
    


    Sjors commented at 5:30 am on October 23, 2023:
    Better to fail on all networks except regtest. It might be possible to mine on a custom signet too, but anyone who knows how to make such a network also knows how to use the RPC.

    hernanmarino commented at 10:55 pm on January 8, 2024:
    Agree, I’ll change it soon

    furszy commented at 3:46 pm on January 31, 2024:
    To access the chain type, use the node interface. See 03d67301e081ecf3123372901b115ee5e29d7c79. So we don’t violate the layers division.
  121. DrahtBot removed review request from LarryRuane on Oct 23, 2023
  122. DrahtBot requested review from LarryRuane on Oct 23, 2023
  123. DrahtBot removed review request from LarryRuane on Nov 21, 2023
  124. DrahtBot requested review from LarryRuane on Nov 21, 2023
  125. DrahtBot removed review request from LarryRuane on Jan 8, 2024
  126. DrahtBot requested review from LarryRuane on Jan 8, 2024
  127. DrahtBot added the label CI failed on Jan 14, 2024
  128. qt: refactor console-only command parsing 1bf0c075c7
  129. qt: add generate command to gui console 2c9974742d
  130. hernanmarino force-pushed on Jan 31, 2024
  131. in src/qt/rpcconsole.cpp:494 in 2c9974742d
    489+    if (parsed_command.size() > 3 || exec_help) {
    490+        executeConsoleHelpGenerate();
    491+        return true;
    492+    }
    493+    // Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated
    494+    if (Params().GetChainType() == ChainType::MAIN) {
    


    furszy commented at 2:19 am on January 31, 2024:
    This should pass through the model.
  132. DrahtBot removed the label CI failed on Jan 31, 2024
  133. hernanmarino commented at 4:11 am on January 31, 2024: contributor
    Rebased on top of master to fix CI errors.
  134. in src/qt/rpcconsole.cpp:109 in 1bf0c075c7 outdated
    104+    std::map<std::string, bool (RPCExecutor::*)(const std::vector<std::string>&, const WalletModel*, const bool)> m_method_map{
    105+        {"help-console", &RPCExecutor::executeConsoleHelpConsole}};
    106 };
    107 
    108+/**
    109+ * Small and fast parser supporting console command syntax, with limited functionality.
    


    furszy commented at 3:25 pm on January 31, 2024:
    The function iterates over the entire input command string twice. It isn’t really “fast”. Could remove this line altogether.
  135. in src/qt/rpcconsole.cpp:450 in 1bf0c075c7 outdated
    471+                // Send to the RPC command parser if not console-only
    472+                if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
    473+                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
    474+                    return;
    475+                }
    476+                Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(result));
    


    furszy commented at 3:31 pm on January 31, 2024:
    There is an indentation issue here.
  136. in src/qt/rpcconsole.cpp:500 in 1bf0c075c7 outdated
    495+                         "   example:    getblock(getblockhash(0),1)[tx][0]\n\n"));
    496+    return true;
    497+}
    498+
    499+/**
    500+ * Catches console-only command before a RPC call is executed
    


    furszy commented at 3:35 pm on January 31, 2024:
    This shouldn’t be here. The function’s docstrings shouldn’t explain the caller workflow behavior. Something like “Executes gui-console-only commands” would be preferable.
  137. in src/qt/rpcconsole.cpp:506 in 2c9974742d
    501+    if (!nblocks_value || !maxtries_value || nblocks_value.value() <= 0 || maxtries_value.value() <= 0) {
    502+        Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: parameters must be positive integers"));
    503+        return true;
    504+    }
    505+
    506+    // Catch the console-only generate command with 2 or less parameters before RPC call is executed .
    


    furszy commented at 3:49 pm on January 31, 2024:
    Same as above, this shouldn’t be here. It is explaining the caller’s workflow inside executeConsoleGenerate. There is no other RPC calls at this point.
  138. in src/qt/rpcconsole.cpp:105 in 1bf0c075c7 outdated
    100+    bool executeConsoleOnlyCommand(const std::string& command, const WalletModel* wallet_model);
    101+    // std::map mapping strings to methods member of RPCExecutor class
    102+    // Keys must be strings with commands and (optionally) parameters in "canonical" form (separated by single space)
    103+    // Keys should match the beggining of user input commands (user commands can have more parameters than the key)
    104+    std::map<std::string, bool (RPCExecutor::*)(const std::vector<std::string>&, const WalletModel*, const bool)> m_method_map{
    105+        {"help-console", &RPCExecutor::executeConsoleHelpConsole}};
    


    furszy commented at 3:55 pm on January 31, 2024:
    In case you retouch this; these functions should return a util::Result<QString> instead of a boolean. This way, the returned string can be emitted at the caller side (same as it is done for the RPC commands). This will improve the existing structure that has all functions returning true on all code paths.
  139. furszy commented at 4:00 pm on January 31, 2024: member

    Executing generate 1 on a node without a wallet leads to a crash. And left some other review notes.

    Will read the historical context for the generate command deletion before proceed.

  140. DrahtBot added the label CI failed on Feb 24, 2024
  141. DrahtBot removed the label CI failed on Feb 29, 2024
  142. DrahtBot commented at 9:23 am on June 15, 2024: contributor

    🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

    Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    Leave a comment here, if you need help tracking down a confusing failure.

    Debug: https://github.com/bitcoin-core/gui/runs/21044663226

  143. DrahtBot added the label CI failed on Jun 15, 2024
  144. DrahtBot commented at 0:54 am on September 13, 2024: contributor

    🤔 There hasn’t been much activity lately and the CI seems to be failing.

    If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn’t been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin-core/gui. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-03 20:20 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me