rpc generate: print useful help and error message #19455

jonatack · 2020-07-06T06:31:47Z

This was a requested follow-up to #19133 and #17700 to alleviate confusion and head-scratching by people following tutorials that use generate. See #19455 (comment) below, #19133 (comment) and #17700 (comment).

before

$ bitcoin-cli help generate
help: unknown command: generate

$ bitcoin-cli generate
error code: -32601
error message:
Method not found

after

$ bitcoin-cli help generate
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.

$ bitcoin-cli generate
error code: -32601
error message:
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.

In the general help it remains hidden, as requested by laanwj.

$ bitcoin-cli help

== Generating ==
generateblock "output" ["rawtx/txid",...]
generatetoaddress nblocks "address" ( maxtries )
generatetodescriptor num_blocks "descriptor" ( maxtries )

jonatack · 2020-07-06T06:33:19Z

Tagging @adamjonas and @MarcoFalke who suggested this feature.

DrahtBot · 2020-07-06T12:07:06Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)

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.

adamjonas · 2020-07-06T23:59:04Z

Concept ACK. Thanks for circling back on this.

When looking at the help options (bitcoin-cli -regtest help), this explanation looks out of place. I'd suggest rewording to at least start with the word generate to keep it alphabetical and consistent.

jonatack · 2020-07-07T04:00:17Z

When looking at the help options (bitcoin-cli -regtest help), this explanation looks out of place. I'd suggest rewording to at least start with the word generate to keep it alphabetical and consistent.

Thanks Jonas! Good point -- updated.

EDIT: per review feedback below, changed it to a hidden RPC command, which won't shown up in the general help.

src/rpc/mining.cpp

laanwj · 2020-07-09T14:22:08Z

I'm not sure on this. On one hand this is useful, on the other hand we did remove the similar message for getinfo a long time ago in b2f23c4. generate, though shorter ago, was removed more than a year ago (in 8bb3e4c), so it seems to be a bit late to add this.
But no strong opposition.

jonatack · 2020-07-23T17:14:08Z

No ACKs. Closing.

adamjonas · 2020-08-03T18:24:22Z

This is my fault for the slow review, but I still think it's a useful addition to reduce confusion for all the outdated tutorials out there [1, 2, 3, 4].

tACK f0aa8ae

Tried help generate, which returns a helpful prompt:

$ bitcoin-cli help generate
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.

This is much more instructive than the method not found error.

$ bitcoin-cli generate
error code: -32601
error message:
generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.

And verified that it is a hidden RPC command

I'm still an advocate for including this for the poor, unfortunate souls out there just trying to get started on regtest.

jonatack · 2020-08-04T10:57:15Z

Thanks for the review @adamjonas. Re-opening to see if this gets traction.

kallewoof · 2020-08-12T09:10:20Z

Concept / code review ACK. LGTM

No strong opinion, but an alternative might be to PR an update on those outdated docs, and/or ping the respective authors. Perhaps both; ping and merge.

jonatack · 2020-08-12T10:20:58Z

Thanks @adamjonas and @kallewoof ❤️

Good idea; I notified on the bitcoin.org github a couple months ago that generate is back as a cli option, though following up on all the tuturials would be helpful for people.

If the maintainers agree on this change, ISTM the 2 ACKs and the test coverage here may enough to merge this no-risk user-facing aid.

pinheadmz · 2020-08-12T13:38:38Z

test/functional/test_runner.py

+        # Consider RPC generate covered, because it is overloaded in
+        # test_framework/test_node.py and not seen by the coverage check.
+        covered_cmds = set({'generate'})


I don't quite understand this. generate is not returned by help and so I don't think its even added to the coverage list:

bitcoin/test/functional/test_framework/coverage.py

Line 96 in bd00d3b

help_output = node.help().split('\n')

Both with and without this change, my coverage results were the same:

Uncovered RPC commands: - pruneblockchain

@pinheadmz Yes, it's a hidden command (not in the help, see the added test) as suggested by @laanwj. IIRC this was needed to appease a failing check in the Travis CI that despite the added test it thought there was no coverage.

pinheadmz · 2020-08-12T13:40:08Z

Concept ACK

Built and tested locally, all behavior is as expected trying different combinations:

src/bitcoin-cli -regtest -generate
src/bitcoin-cli -regtest generate
src/bitcoin-cli -regtest help generate
src/bitcoin-cli -regtest help -generate
src/bitcoin-cli -regtest -help -generate

Only thing that caught me as far as interface is the fact that there is help and -help. The extra help menu is a bit of an easter egg? But that's out of scope for this PR I think.

laanwj · 2020-08-12T14:03:57Z

Only thing that caught me as far as interface is the fact that there is help and -help. The extra help menu is a bit of an easter egg? But that's out of scope for this PR I think.

They are principally different on purpose. Like for any UNIX-ish application, -help or --help shows the help message for the local binary. This includes the different options how to connect to the server.

On the other hand, help is a command for the remote server. It shows the help for the specific RPC server it connects to, and fails if there is none.

test/functional/rpc_generate.py

pinheadmz

ACK f0aa8ae

Thanks for your answers to my questions @jonatack and @laanwj

Show Signature

pinheadmz's public key is on keybase

jonatack · 2020-08-12T16:02:52Z

Thanks @pinheadmz!

f0aa8ae test: add rpc_generate functional test (Jon Atack) 92d94ff rpc: print useful help and error message for generate (Jon Atack) 8d32d20 test: consider generate covered in _get_uncovered_rpc_commands() (Jon Atack) Pull request description: This was a requested follow-up to bitcoin#19133 and bitcoin#17700 to alleviate confusion and head-scratching by people following tutorials that use `generate`. See bitcoin#19455 (comment) below, bitcoin#19133 (comment) and bitcoin#17700 (comment). before ``` $ bitcoin-cli help generate help: unknown command: generate $ bitcoin-cli generate error code: -32601 error message: Method not found ``` after ``` $ bitcoin-cli help generate generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information. $ bitcoin-cli generate error code: -32601 error message: generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information. ``` In the general help it remains hidden, as requested by laanwj. ``` $ bitcoin-cli help == Generating == generateblock "output" ["rawtx/txid",...] generatetoaddress nblocks "address" ( maxtries ) generatetodescriptor num_blocks "descriptor" ( maxtries ) ``` ACKs for top commit: adamjonas: utACK f0aa8ae pinheadmz: ACK f0aa8ae Tree-SHA512: d083652589ad3e8228c733455245001db22397559c3946e7e573cf9bd01c46e9e88b72d934728ec7f4361436ae4c74adb8f579670b09f479011924357e729af5

Summary: PR rationale: > This was a requested follow-up to #19133 and #17700 to alleviate confusion and head-scratching by people following tutorials that use `generate`. > test: consider generate covered in _get_uncovered_rpc_commands() > rpc: print useful help and error message for generate > test: add rpc_generate functional test This is a backport of [[ bitcoin/bitcoin#19455 | core#19455 ]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10091

fanquake added the RPC/REST/ZMQ label Jul 6, 2020

DrahtBot mentioned this pull request Jul 6, 2020

rpc: remove deprecated CRPCCommand constructor #18531

Merged

jonatack force-pushed the rpc-generate-helpful-error_message branch from 260b7c2 to 210873d Compare July 7, 2020 03:53

jonatack force-pushed the rpc-generate-helpful-error_message branch 5 times, most recently from 8804531 to 68fe377 Compare July 7, 2020 11:04

laanwj reviewed Jul 9, 2020

View reviewed changes

src/rpc/mining.cpp Outdated Show resolved Hide resolved

test: consider generate covered in _get_uncovered_rpc_commands()

8d32d20

jonatack force-pushed the rpc-generate-helpful-error_message branch from 68fe377 to 14e2a82 Compare July 9, 2020 15:30

DrahtBot mentioned this pull request Jul 10, 2020

doc: Merge release notes #19483

Merged

jonatack added 2 commits July 11, 2020 07:47

rpc: print useful help and error message for generate

92d94ff

test: add rpc_generate functional test

f0aa8ae

jonatack force-pushed the rpc-generate-helpful-error_message branch from 14e2a82 to f0aa8ae Compare July 11, 2020 05:49

DrahtBot mentioned this pull request Jul 13, 2020

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

jonatack closed this Jul 23, 2020

jonatack reopened this Aug 4, 2020

pinheadmz reviewed Aug 12, 2020

View reviewed changes

laanwj reviewed Aug 12, 2020

View reviewed changes

test/functional/rpc_generate.py Show resolved Hide resolved

pinheadmz approved these changes Aug 12, 2020

View reviewed changes

laanwj merged commit dabab06 into bitcoin:master Aug 14, 2020

jonatack deleted the rpc-generate-helpful-error_message branch August 14, 2020 09:32

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

rpc generate: print useful help and error message #19455

rpc generate: print useful help and error message #19455

Uh oh!

Conversation

jonatack commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Jul 6, 2020

Uh oh!

DrahtBot commented Jul 6, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

adamjonas commented Jul 6, 2020

Uh oh!

jonatack commented Jul 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

laanwj commented Jul 9, 2020

Uh oh!

jonatack commented Jul 23, 2020

Uh oh!

adamjonas commented Aug 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Aug 4, 2020

Uh oh!

kallewoof commented Aug 12, 2020

Uh oh!

jonatack commented Aug 12, 2020

Uh oh!

pinheadmz Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

jonatack Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

pinheadmz commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Aug 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack commented Aug 12, 2020

Uh oh!

Uh oh!

jonatack commented Jul 6, 2020 •

edited

Loading

DrahtBot commented Jul 6, 2020 •

edited

Loading

jonatack commented Jul 7, 2020 •

edited

Loading

adamjonas commented Aug 3, 2020 •

edited

Loading

pinheadmz commented Aug 12, 2020 •

edited

Loading

laanwj commented Aug 12, 2020 •

edited

Loading