rpc generate: print useful help and error message

jonatack commented at 6:31 am on July 6, 2020: member

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

0$ bitcoin-cli help generate
1help: unknown command: generate
2
3$ bitcoin-cli generate
4error code: -32601
5error message:
6Method not found

after

0$ bitcoin-cli help generate
1generate ( nblocks maxtries ) has been replaced by the -generate cli option. Refer to -help for more information.
2
3$ bitcoin-cli generate
4error code: -32601
5error message:
6generate ( 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.

0$ bitcoin-cli help
1
2== Generating ==
3generateblock "output" ["rawtx/txid",...]
4generatetoaddress nblocks "address" ( maxtries )
5generatetodescriptor num_blocks "descriptor" ( maxtries )

jonatack commented at 6:33 am on July 6, 2020: member

Tagging @adamjonas and @MarcoFalke who suggested this feature.

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

DrahtBot commented at 12:07 pm on July 6, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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 commented at 11:59 pm on July 6, 2020: member

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 force-pushed on Jul 7, 2020

jonatack commented at 4:00 am on July 7, 2020: member

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.

jonatack force-pushed on Jul 7, 2020

in src/rpc/mining.cpp:1208 in 68fe377d4b outdated

1204@@ -1194,6 +1205,7 @@ static const CRPCCommand commands[] =
1205     { "generating",         "generatetoaddress",      &generatetoaddress,      {"nblocks","address","maxtries"} },
1206     { "generating",         "generatetodescriptor",   &generatetodescriptor,   {"num_blocks","descriptor","maxtries"} },
1207     { "generating",         "generateblock",          &generateblock,          {"output","transactions"} },
1208+    { "generating",         "generate",               &generate,               {} },

laanwj commented at 2:21 pm on July 9, 2020:

Why not make this a hidden RPC? It’s a non-functional command only returning an error message, after all, there’s no need for discoverability.

jonatack commented at 3:30 pm on July 9, 2020:

Yes, that’s better. Done.

laanwj commented at 2:22 pm on July 9, 2020: member

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 b2f23c41538eaadd71c373ada75dd3a982eeb8bf. generate, though shorter ago, was removed more than a year ago (in 8bb3e4c487500a580e3e18791b1f4e7dcdd35442), so it seems to be a bit late to add this. But no strong opposition.

test: consider generate covered in _get_uncovered_rpc_commands() 8d32d2011d

jonatack force-pushed on Jul 9, 2020

rpc: print useful help and error message for generate 92d94ffb8d

test: add rpc_generate functional test f0aa8aeea5

jonatack force-pushed on Jul 11, 2020

jonatack commented at 5:14 pm on July 23, 2020: member

No ACKs. Closing.

jonatack closed this on Jul 23, 2020

adamjonas commented at 6:24 pm on August 3, 2020: member

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 f0aa8aeea5a183ea44a877255d12db7732f2e0a8

Tried help generate, which returns a helpful prompt:

0$ bitcoin-cli help generate
1generate ( 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.

0$ bitcoin-cli generate
1error code: -32601
2error message:
3generate ( 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 commented at 10:57 am on August 4, 2020: member

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

jonatack reopened this on Aug 4, 2020

kallewoof commented at 9:10 am on August 12, 2020: member

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 commented at 10:20 am on August 12, 2020: member

Thanks @adamjonas and @kallewoof :heart:

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.

in test/functional/test_runner.py:721 in f0aa8aeea5

718         coverage_filenames = set()
719         all_cmds = set()
720-        covered_cmds = set()
721+        # Consider RPC generate covered, because it is overloaded in
722+        # test_framework/test_node.py and not seen by the coverage check.
723+        covered_cmds = set({'generate'})

pinheadmz commented at 1:38 pm on August 12, 2020:

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:

https://github.com/bitcoin/bitcoin/blob/bd00d3b1f2036893419d1e8c514a8af2c4e4b1fb/test/functional/test_framework/coverage.py#L96

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

0Uncovered RPC commands:
1  - pruneblockchain

jonatack commented at 3:59 pm on August 12, 2020:

@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 commented at 1:40 pm on August 12, 2020: member

Concept ACK

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

0src/bitcoin-cli -regtest -generate
1src/bitcoin-cli -regtest generate
2src/bitcoin-cli -regtest help generate
3src/bitcoin-cli -regtest help -generate
4src/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 commented at 2:03 pm on August 12, 2020: member

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.

in test/functional/rpc_generate.py:20 in f0aa8aeea5

15+    def set_test_params(self):
16+        self.num_nodes = 1
17+
18+    def run_test(self):
19+        message = (
20+            "generate ( nblocks maxtries ) has been replaced by the -generate "

laanwj commented at 2:42 pm on August 12, 2020:

Isn’t a whole new test for what is essentially a stub maybe a bit overkill? :smile:

jonatack commented at 2:47 pm on August 12, 2020:

I usually write tests before code; it saved my deliverables so many times and habits die hard :D

pinheadmz approved

pinheadmz commented at 3:59 pm on August 12, 2020: member

ACK f0aa8aeea5a183ea44a877255d12db7732f2e0a8

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

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK f0aa8aeea5a183ea44a877255d12db7732f2e0a8
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAl80EaUACgkQ5+KYS2KJ
 7yTrkBw//dEwEnhDeko2ScTnzl/4mZSJLR0KG4D//BhWq+bhsSKKslWUQDMdJDYE/
 8HljPiuButxN01vJOVowqYV6ZGIPEVLiILu/R7OYkU81KLUPtZwsaouT9b0/8opmZ
 9/uXysTqvUa9UUfrk01vxkU63+viAQdpe5WBaf6dURdBjodRdoxL41r60GhMlacoe
10wS75MuzR+pojiyr/Wf9s+t2BKDVO79rhoClhdOo9xiud4kqomdpmyQh2/86T5moe
11LgTOjufZGAl/0haI1EAlWmtw0+AjMl6hRnrVUcgblElOJ201HZoE2Qj0L8bkykU1
12I890Cu28PFWFeeK6xX3ev/ja6v2iLzrq1VsKmH1WRkEYaOkL8GvH4MJHwuQSlsFS
13/h+dxMFjPvmigZMQTuaHDtDrsRGLqMTd6WEYoIC/NKRK+7UjgpGjzSiiRBS4g7iB
14pVTTpei9/zjvXwlvr427+ji1kbWt2YKQdqrDeMIHHc5gcsFY9E1vgSKDw94TjVyt
15YwOHlYG+ezhULsxNbW/D40ox1veFDkK/I0DkHN0AcUP7RuKTTdAiZ4B6+Fturk0f
163eFNIxf3qilCy5LPfRBcqtQ8GkpVgvcDpsO83cEeffyTIxrtRrPDULRc0upI81Jd
17mi6DrhAultLW1xFvAjB8EBTTEVrdg9doVD5vMqHM+CFGm+wnyvM=
18=wdk/
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on keybase

jonatack commented at 4:02 pm on August 12, 2020: member

Thanks @pinheadmz!

laanwj merged this on Aug 14, 2020

laanwj closed this on Aug 14, 2020

jonatack deleted the branch on Aug 14, 2020

sidhujag referenced this in commit ff1cb84f45 on Aug 16, 2020

deadalnix referenced this in commit 803956cc16 on Sep 14, 2021

DrahtBot locked this on Feb 15, 2022

rpc generate: print useful help and error message #19455

Conflicts