Debug Console implementation of generate method

[hernanmarino]

This is an implementation of a (gui console only) generate method, and fixes https://github.com/bitcoin-core/gui/issues/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:

generate ( nblocks   ( maxtries) )
Mine a specified amount of blocks to own ( on the fly generated ) address.

Arguments:
1. nblocks    (numeric, optional, , default=1) How many blocks are generated.
2. maxtries   (numeric, optional, default=1000000) How many iterations to try.

Result:
{
  "address":          (address generated by this command)
  "blocks": [           (json array of block hashes)
     ....
]
}

The output looks similar to the following :

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

[DrahtBot]

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.

[RandyMcMillan]

ConceptACK

:)

Additional logic to detect whether `-generate` is or is not followed by an integer would be great.

![generate-gui-console](https://user-images.githubusercontent.com/152159/209985593-0fe468fc-038b-44d2-90fc-303ecc3944d4.png)

Slightly better syntax suggestion in the error message would be excellent. 

[hernanmarino]

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.

[hernanmarino]

Thanks for all the insights. In the first days of the next week I'll try to incorporate the feedback and squash the commits.

[hebasto]

Concept ACK.

@hernanmarino You could also link the initial issue in the PR description.

[rserranon]

Concept ACK

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

std::vector<std::string> split_command = SplitString(executableCommand, " ");
if (!split_command.empty() && split_command[0] == "generate") {
    // Remove last "\n"
    std::string last_param = split_command[split_command.size()-1];
    split_command[split_command.size()-1] = last_param.substr(0, last_param.size() - 1);

    // Generate address
    std::string address_result;
    if (!RPCConsole::RPCExecuteCommandLine(m_node, address_result, "getnewaddress\n", nullptr, wallet_model)) {
        Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Error: cannot get new address"));
         return;
    }

    // Craft command
    const std::string blocks_num = split_command.size() > 1 ? split_command[1] : "1";
    const std::string max_tries = split_command.size() > 2 ? split_command[2] : "";
    executableCommand = "generatetoaddress " + blocks_num + " \"" + address_result + "\" " + max_tries;
}

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?

[furszy]

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.

[RandyMcMillan]

tested on top of a62231bca629e945349255a1d331dd5c7a86ddd1

Tested ConceptACK 9b0b762fd774207d127213c1c9da354aef38e739

macOS: Arm64

[RandyMcMillan]

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

Tested ConceptACK 9b0b762fd774207d127213c1c9da354aef38e739

macOS: x86_64

[RandyMcMillan]

@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

[hernanmarino]

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

[hernanmarino]

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

[rserranon]

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

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.

[hernanmarino]

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:

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

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.

[hernanmarino]

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.

[rserranon]

testing ACK

[furszy]

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:

std::string cli_command;
if (IsCliOnlyCommand(executableCommand, cli_command)) {  // Check against a list of known cli-only commands and return the command inside "cli_command".
    // Craft Cli Command
    std::string res_cli_only_command;
    if (!CraftCliCommand(cli_command, executableCommand, res_cli_only_command)) {
         return; // errors could be emitted inside the function or here if it returns an error.
    }
    // Update command
    executableCommand = res_cli_only_command;
}

// Execute command
if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
    // etc...

Then could do something like:

bool CraftCliCommand(const std::string& cli_command, const std::string& executableCommand, std::string& command_ret)
{
    switch(cli_command) {
        case GENERATE_COMMAND:
            command_ret  = craftGenerateCommand(executableCommand);
            return true;
        case OTHER_CLI_COMMAND:
            command_ret  = craftOtherCliCommand(executableCommand);
            return true;
        // etc..
    }

    // command not implemented.
    return false;
}

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?

[hernanmarino]

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.

[hernanmarino]

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"

[hebasto]

@hernanmarino Are you still working on this PR?

[hernanmarino]

@hernanmarino Are you still working on this PR?

Yes, will update ASAP.

[hernanmarino]

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"

[luke-jr]

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

[hernanmarino]

Updated with all the changes suggested by @furszy in his latest review.

[pablomartin4btc]

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

[hernanmarino]

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 ( https://github.com/bitcoin-core/gui/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.

[hernanmarino]

@maflcko @jonatack @Sjors since you opened or ACK'ed the original issues I kindly request your review on this PR.

[Sjors]

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.

[hebasto]

Related:

• https://github.com/bitcoin/bitcoin/issues/28387
• https://bitcoin.stackexchange.com/questions/119491/generate-regtest-blocks-with-bitcoin-qt/119492

[hernanmarino]

Rebased on top of master to fix CI errors.