Debug Console implementation of generate method #692

hernanmarino · 2022-12-28T16:47:21Z

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 bitcoin/bitcoin#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 · 2022-12-28T16:47:23Z

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.

pablomartin4btc

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.

src/qt/rpcconsole.cpp

pablomartin4btc

b7782a4 ACK.

src/qt/rpcconsole.cpp

RandyMcMillan · 2022-12-29T17:09:37Z

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 · 2022-12-30T19:01:38Z

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.

src/qt/rpcconsole.cpp

pablomartin4btc

Tested ACK 9b0b762.
Checked output with different params (amount of blocks, tries), compared with outputs from bitcoin-cli as well, all good!

pablomartin4btc

Is it possible for someone to link this PR to the issue #55 if appropriate please?

jarolrod

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:

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

furszy

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;
}

hernanmarino · 2023-01-14T01:00:44Z

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 · 2023-01-15T19:03:23Z

Concept ACK.

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

rserranon · 2023-01-16T23:08:41Z

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 · 2023-01-16T23:44:24Z

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 · 2023-01-25T00:16:45Z

tested on top of a62231b

Tested ConceptACK 9b0b762

macOS: Arm64

RandyMcMillan · 2023-01-25T00:33:10Z

tested on top of a62231b

Tested ConceptACK 9b0b762

macOS: x86_64

RandyMcMillan · 2023-01-25T01:07:15Z

@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 · 2023-01-25T01:27:54Z

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 · 2023-01-25T20:24:51Z

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

Sjors · 2023-10-23T05:27:48Z

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.

Sjors · 2023-10-23T05:30:00Z

src/qt/rpcconsole.cpp

+        return true;
+    }
+    // Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated
+    if (Params().GetChainType() == ChainType::MAIN) {


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.

Agree, I'll change it soon

To access the chain type, use the node interface. See 03d67301e081ecf3123372901b115ee5e29d7c79. So we don't violate the layers division.

hebasto · 2023-11-21T12:02:11Z

hernanmarino · 2024-01-31T04:11:17Z

Rebased on top of master to fix CI errors.

furszy

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.

furszy · 2024-01-31T02:19:37Z

src/qt/rpcconsole.cpp

+        return true;
+    }
+    // Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated
+    if (Params().GetChainType() == ChainType::MAIN) {


This should pass through the model.

furszy · 2024-01-31T15:25:40Z

src/qt/rpcconsole.cpp

 };

+/**
+ * Small and fast parser supporting console command syntax, with limited functionality.


The function iterates over the entire input command string twice. It isn't really "fast".
Could remove this line altogether.

furszy · 2024-01-31T15:31:18Z

src/qt/rpcconsole.cpp

+        if (!RPCExecutor::executeConsoleOnlyCommand(command.toStdString(), wallet_model)) {
+                // Send to the RPC command parser if not console-only
+                if (!RPCConsole::RPCExecuteCommandLine(m_node, result, executableCommand, nullptr, wallet_model)) {
+                    Q_EMIT reply(RPCConsole::CMD_ERROR, QString("Parse error: unbalanced ' or \""));
+                    return;
+                }
+                Q_EMIT reply(RPCConsole::CMD_REPLY, QString::fromStdString(result));


There is an indentation issue here.

furszy · 2024-01-31T15:35:33Z

src/qt/rpcconsole.cpp

+}
+
+/**
+ * Catches console-only command before a RPC call is executed


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.

furszy · 2024-01-31T15:46:58Z

src/qt/rpcconsole.cpp

+        return true;
+    }
+    // Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated
+    if (Params().GetChainType() == ChainType::MAIN) {


To access the chain type, use the node interface. See 03d67301e081ecf3123372901b115ee5e29d7c79. So we don't violate the layers division.

furszy · 2024-01-31T15:49:00Z

src/qt/rpcconsole.cpp

+        return true;
+    }
+
+    // Catch the console-only generate command with 2 or less parameters before RPC call is executed .


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.

furszy · 2024-01-31T15:55:17Z

src/qt/rpcconsole.cpp

+    bool executeConsoleHelpConsole(const std::vector<std::string>& parsed_command, const WalletModel* wallet_model, const bool exec_help = false);
+    bool executeConsoleOnlyCommand(const std::string& command, const WalletModel* wallet_model);
+    // std::map mapping strings to methods member of RPCExecutor class
+    // Keys must be strings with commands and (optionally) parameters in "canonical" form (separated by single space)
+    // Keys should match the beggining of user input commands (user commands can have more parameters than the key)
+    std::map<std::string, bool (RPCExecutor::*)(const std::vector<std::string>&, const WalletModel*, const bool)> m_method_map{
+        {"help-console", &RPCExecutor::executeConsoleHelpConsole}};


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.

DrahtBot · 2024-06-15T09:23:20Z

🚧 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}

DrahtBot · 2024-09-13T00:54:58Z

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

hernanmarino force-pushed the generate-gui-console branch from 4fd7c14 to 454fc48 Compare December 28, 2022 18:46

hernanmarino marked this pull request as ready for review December 28, 2022 19:53

hernanmarino mentioned this pull request Dec 28, 2022

Add generate method to debug console #55

Closed

pablomartin4btc reviewed Dec 28, 2022

View reviewed changes