-
Notifications
You must be signed in to change notification settings - Fork 271
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Debug Console implementation of generate method #692
base: master
Are you sure you want to change the base?
Debug Console implementation of generate method #692
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
4fd7c14
to
454fc48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
0031ef5
to
b7782a4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b7782a4 ACK.
ConceptACK :)
|
Hi @RandyMcMillan . Thank you for taking the time to look at my PR. The changes I implemented are actually on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested ACK 9b0b762.
Checked output with different params (amount of blocks, tries), compared with outputs from bitcoin-cli
as well, all good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible for someone to link this PR to the issue #55 if appropriate please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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;
}
Thanks for all the insights. In the first days of the next week I'll try to incorporate the feedback and squash the commits. |
Concept ACK. @hernanmarino You could also link the initial issue in the PR description. |
Does it make sense to include parsing logic, generate a new address, and then craft the command to call the |
A separate function returning the command string would be better. |
@hernanmarino - @rserranon 's suggestion is compelling... https://github.com/RandyMcMillan/gui/tree/612a0c14173d681a04021581851422ba832236ee Applied as a patch on top of this PR: I haven't fully tested the patch but works on MacOS x86_64 |
Thanks for the feedback and testing. I ' m already working on an update consider this and all feedback received, will update soon.
|
9b0b762
to
ad0dd07
Compare
Summary of changes just pushed:
|
IIUC the reason the RPC method was removed (and replaced with |
return true; | ||
} | ||
// Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated | ||
if (Params().GetChainType() == ChainType::MAIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree, I'll change it soon
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To access the chain type, use the node interface. See 03d67301e081ecf3123372901b115ee5e29d7c79. So we don't violate the layers division.
e6e0dac
to
2c99747
Compare
Rebased on top of master to fix CI errors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return true; | ||
} | ||
// Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated | ||
if (Params().GetChainType() == ChainType::MAIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should pass through the model.
}; | ||
|
||
/** | ||
* Small and fast parser supporting console command syntax, with limited functionality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function iterates over the entire input command string twice. It isn't really "fast".
Could remove this line altogether.
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 was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is an indentation issue here.
} | ||
|
||
/** | ||
* Catches console-only command before a RPC call is executed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return true; | ||
} | ||
// Fail if we are on mainnet, to avoid generating addresses for blocks that will not be generated | ||
if (Params().GetChainType() == ChainType::MAIN) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To access the chain type, use the node interface. See 03d67301e081ecf3123372901b115ee5e29d7c79. So we don't violate the layers division.
return true; | ||
} | ||
|
||
// Catch the console-only generate command with 2 or less parameters before RPC call is executed . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/qt/rpcconsole.cpp
Outdated
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}}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
🤔 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. |
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 frombitcoin-cli
's-generate
argument as implemented in bitcoin/bitcoin#19133.The
generate
comand takes two optional parameters:The output looks similar to the following :
Note to reviewers / testers:
bitcoin-qt
only implementation .bitcoin-cli
already had this functionality implemented in the referenced PR.bitcoin-qt
in regtest, to be able to actually generate some blocks and test the output properly