rpc, cli, test: add bitcoin-cli -generate command #19133

jonatack · 2020-06-01T12:32:47Z

This PR continues and completes the work begun in #17700 working on issue #16000 to create a client-side version of RPC generate.

Basically, bitcoin-cli -generate wraps calling generatenewaddress followed by generatetoaddress [nblocks] [maxtries] and prints the following:

$ bitcoin-cli -generate
{
  "address": "bcrt1qn4aszr2y2xvpa70y675a76wsu70wlkwvdyyln6"
  "blocks": [
    "01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
  ]
}

$ bitcoin-cli -rpcwallet=wallet-name -generate 3 100
{
  "address": "bcrt1q4cunfw0gnsj7g7e6mk0v0uuvvau9mwr09dj45l",
  "blocks": [
    "7a6650ca5e0c614992ee64fb148a7e5e022af842e4b6003f81abd8baf1e75136",
    "01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
    "3f8795ec40b1ad812b818c177680841be319a3f6753d4e32dc7dfb5bafe5d00e"
  ]
}

Help doc:

$ bitcoin-cli -h | grep -A5 "\-generate"
  -generate
       Generate blocks immediately, equivalent to RPC generatenewaddress
       followed by RPC generatetoaddress. Optional positional arguments
       are number of blocks to generate (default: 1) and maximum
       iterations to try (default: 1000000), equivalent to RPC
       generatetoaddress nblocks and maxtries arguments. Example:
       bitcoin-cli -generate 4 1000

Quite a bit of test coverage turned out to be needed to cover the change and the different cases (arguments, multiwallet mode) and error-handling.

This PR also improves some things that working on these changes brought to light.

Credit to Harris Brakmić for the initial work in #17700.

maflcko · 2020-06-01T12:52:25Z

In #17700 (comment) @adamjonas mentions that some documents on the internet refer to generate (without the dash prefix), so I think it would be preferable to leave out the dash.

We do the same in the python test suite

bitcoin/test/functional/test_framework/test_node.py

Line 296 in 9bc7751

    
           self.log.debug("TestNode.generate() dispatches `generate` call to `generatetoaddress`")

Any thoughts on this?

jonatack · 2020-06-01T13:22:27Z

Thanks -- as the CLI commands/args are preceded with a dash, I read @adamjonas' comment as being for a follow-up to provide a help (and/or possibly a redirect).

catching generate client side and providing user feedback, even if it refers to adding the dash or gives fuller instructions on how to use generatetoaddress, would reduce a lot of head scratching for first-time users

If there is rough consensus to leave off the dash completely, I don't mind looking into it. I just didn't have the context to be confident about starting with it.

maflcko · 2020-06-01T13:28:14Z

Right now this happens on master:

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

I think if this pull request is agreed on as a concept, the error should be changed to read "generate has been replaced by the -generate cli option. Refer to -help for more info." or the method should be changed silently dispatch to generatetoaddress in the background.

But as you say it might be good to wait for some more opinions.

adamjonas · 2020-06-01T16:14:46Z

Leaving off the dash is preferable so tutorials will work again, but I understand the reason to be consistent with the - precedent so adding help/redirect was a reasonable fallback.

DrahtBot · 2020-06-02T01:23:39Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

cli: display multiwallet total balance in -getinfo #19092 (cli: display multiwallet total balance in -getinfo by jonatack)
cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups #19089 (cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups by jonatack)
rpc: remove deprecated CRPCCommand constructor #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
cli/gui: support "@height" in place of blockhash for getblock on client side #16439 (cli/gui: support "@height" in place of blockhash for getblock on client side by ajtowns)

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.

and named the same as in generatetodescriptor just above.

and make callable higher up with (nRet == 0) check.

jonatack · 2020-06-02T09:09:46Z

Simplified some of the changes.

jonatack · 2020-06-05T10:44:06Z

Thanks for the feedback @MarcoFalke and @adamjonas. I'd be happy to look into removing the dash, redirecting, or improving the help as a follow-up if this command is merged. CLI commands aren't encumbered by RPC API constraints, and we are early in the release cycle, so the hurdle here seems to just be review and acks.

za-kk · 2020-06-07T01:02:57Z

Tested ACK - tested using the following bitcoin-cli commands

All worked as expected, generating a new address and then generating blocks to that address. Ran test/functional/interface_bitcoin_cli.py in which all tests were successful.

Looked over code for the other improvements/clean up in this pr and all looks okay to me.

I agree that the - should stay in order to keep consistency with the other commands and changing the error message on bitcoin-cli generate to point towards -generate as a fallback seems sensible to me.

Sjors · 2020-06-08T11:32:53Z

Concept ACK. I also prefer to keep the dash, as we do with -getinfo. A helpful message for generate is fine. Would be nice if this worked with -nowallet too, e.g. by generating to an OP_RETURN.

adamjonas · 2020-06-08T19:09:50Z

utACK 22cb303

Ran tests, tested a few scenarios including -generate with no args, -generate with nblocks specified > 1 and 0 (nicely caught), usage with max iterations specified, and passing in too many arguments. Also verified help message looks good.

Stepped through commits. I think these are useful for review but would advocate squashing before merge. Also, as commented above, would still like a fall back for generate with no -.

maflcko · 2020-06-09T00:37:51Z

utACK

tested a few scenarios

Sounds like a tested ACK 👀

jonatack · 2020-06-17T10:59:26Z

Thanks, everyone. With 2 tested ACKs and a Concept ACK, it seems best to not invalidate existing review, get this merged, and to handle the suggestions for a fallback message on the former generate command and possibly allowing -generate to work with -nowallet by generating to an OP_RETURN via a follow-up, which can also add a release note.

meshcollider

utACK 22cb303

9886c7d doc: add release note for bitcoin-cli -generate (Jon Atack) Pull request description: Adds a release note for #19133. ACKs for top commit: laanwj: ACK 9886c7d Tree-SHA512: 1354e5db0098447788c9ded6f2cd868fd6ea4a1443c99bec8026881b962c92b7257bfea45757769071605faeb989e9db48eaa2fbe9d273513aa2905ee479a8ec

22cb303 rpc: add missing space in JSON parsing error message, update test (Jon Atack) bf53ebe test: add multiwallet tests for bitcoin-cli -generate (Jon Atack) 4b859cf cli: add multiwallet capability to GetNewAddress and -generate (Jon Atack) 18f9354 test: add tests for bitcoin-cli -generate (Jon Atack) 4818124 cli: create bitcoin-cli -generate command (Jon Atack) ff41a36 cli: extract ParseResult() and ParseError() (Jon Atack) f4185b2 cli: create GenerateToAddressRequestHandler class (Harris) f7c65a3 cli: create GetNewAddress() (Jon Atack) 9be7fd3 rpc: make generatetoaddress locals const (Jon Atack) cb00510 rpc: create rpc/mining.h, hoist default max tries values to constant (Jon Atack) Pull request description: This PR continues and completes the work begun in bitcoin#17700 working on issue bitcoin#16000 to create a client-side version of RPC `generate`. Basically, `bitcoin-cli -generate` wraps calling `generatenewaddress` followed by `generatetoaddress [nblocks] [maxtries]` and prints the following: ``` $ bitcoin-cli -generate { "address": "bcrt1qn4aszr2y2xvpa70y675a76wsu70wlkwvdyyln6" "blocks": [ "01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5", ] } $ bitcoin-cli -rpcwallet=wallet-name -generate 3 100 { "address": "bcrt1q4cunfw0gnsj7g7e6mk0v0uuvvau9mwr09dj45l", "blocks": [ "7a6650ca5e0c614992ee64fb148a7e5e022af842e4b6003f81abd8baf1e75136", "01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5", "3f8795ec40b1ad812b818c177680841be319a3f6753d4e32dc7dfb5bafe5d00e" ] } ``` Help doc: ``` $ bitcoin-cli -h | grep -A5 "\-generate" -generate Generate blocks immediately, equivalent to RPC generatenewaddress followed by RPC generatetoaddress. Optional positional arguments are number of blocks to generate (default: 1) and maximum iterations to try (default: 1000000), equivalent to RPC generatetoaddress nblocks and maxtries arguments. Example: bitcoin-cli -generate 4 1000 ``` Quite a bit of test coverage turned out to be needed to cover the change and the different cases (arguments, multiwallet mode) and error-handling. This PR also improves some things that working on these changes brought to light. Credit to Harris Brakmić for the initial work in bitcoin#17700. ACKs for top commit: adamjonas: utACK 22cb303 meshcollider: utACK 22cb303 Tree-SHA512: 94f67f632fe093d076f614e0ecff09ce7342ac6e424579200d5211a6615260e438d857861767fb788950ec6da0b26ef56dc8268c430012a3b3d4822b24ca6fbf

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 #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 ) ``` ACKs for top commit: adamjonas: utACK f0aa8ae pinheadmz: ACK f0aa8ae Tree-SHA512: d083652589ad3e8228c733455245001db22397559c3946e7e573cf9bd01c46e9e88b72d934728ec7f4361436ae4c74adb8f579670b09f479011924357e729af5

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

9886c7d doc: add release note for bitcoin-cli -generate (Jon Atack) Pull request description: Adds a release note for bitcoin#19133. ACKs for top commit: laanwj: ACK 9886c7d Tree-SHA512: 1354e5db0098447788c9ded6f2cd868fd6ea4a1443c99bec8026881b962c92b7257bfea45757769071605faeb989e9db48eaa2fbe9d273513aa2905ee479a8ec

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [1/10] bitcoin/bitcoin@cb00510 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9941

Summary: and named the same as in generatetodescriptor just above. This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [2/10] bitcoin/bitcoin@9be7fd3 Backport note: we use `get_int64` instead of `get_int` for `max_tries` because of D7138 Depends on D9941 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9942

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [3/10] bitcoin/bitcoin@f7c65a3 Depends on D9942 Test Plan: `ninja` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9943

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [4/10] bitcoin/bitcoin@f4185b2 Depends on D9943 Test Plan: `ninja` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9944

Summary: and make callable higher up with (nRet == 0) check. This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [5/10] bitcoin/bitcoin@ff41a36 Depends on D9944 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9945

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [6/10] bitcoin/bitcoin@4818124 Depends on D9945 Test Plan: ``` src/qt/bitcoin-qt -regtest -server src/bitcoin-cli -regtest -generate 8 ``` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9946

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [7/10] bitcoin/bitcoin@18f9354 Depends on D9946 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9947

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [8/10] bitcoin/bitcoin@4b859cf Depends on D9947 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9948

Summary: This is a backport of [[bitcoin/bitcoin#19133 | core#19133]] [9/10] bitcoin/bitcoin@bf53ebe Depends on D9948 Test Plan: `ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9949

Summary: This concludes backport of [[bitcoin/bitcoin#19133 | core#19133]] [10/10] bitcoin/bitcoin@22cb303 Depends on D9949 Test Plan: `ninja && ninja check-functional` Reviewers: #bitcoin_abc, majcosta Reviewed By: #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D9950

fanquake added RPC/REST/ZMQ Feature labels Jun 1, 2020

jonatack mentioned this pull request Jun 1, 2020

cli: add -generate option #17700

Closed

rpc: create rpc/mining.h, hoist default max tries values to constant

cb00510

This was referenced Jun 2, 2020

cli: display multiwallet total balance in -getinfo #19092

Closed

cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups #19089

Closed

jonatack and others added 9 commits June 2, 2020 08:23

rpc: make generatetoaddress locals const

9be7fd3

and named the same as in generatetodescriptor just above.

cli: create GetNewAddress()

f7c65a3

cli: create GenerateToAddressRequestHandler class

f4185b2

cli: extract ParseResult() and ParseError()

ff41a36

and make callable higher up with (nRet == 0) check.

cli: create bitcoin-cli -generate command

4818124

test: add tests for bitcoin-cli -generate

18f9354

cli: add multiwallet capability to GetNewAddress and -generate

4b859cf

test: add multiwallet tests for bitcoin-cli -generate

bf53ebe

rpc: add missing space in JSON parsing error message, update test

22cb303

jonatack force-pushed the cli-generate-command branch from 55bdd0b to 22cb303 Compare June 2, 2020 06:54

This was referenced Jun 2, 2020

cli/gui: support "@height" in place of blockhash for getblock on client side #16439

Closed

rpc: remove deprecated CRPCCommand constructor #18531

Merged

meshcollider reviewed Jun 21, 2020

View reviewed changes

meshcollider merged commit 47a30ef into bitcoin:master Jun 21, 2020

jonatack deleted the cli-generate-command branch June 22, 2020 07:45

This was referenced Jun 23, 2020

doc: add release note for bitcoin-cli -generate #19357

Merged

Update regtest block generation command bitcoin-dot-org/Bitcoin.org#3077

Merged

jonatack mentioned this pull request Jul 6, 2020

rpc generate: print useful help and error message #19455

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

rpc, cli, test: add bitcoin-cli -generate command #19133

rpc, cli, test: add bitcoin-cli -generate command #19133

Uh oh!

jonatack commented Jun 1, 2020

Uh oh!

maflcko commented Jun 1, 2020

Uh oh!

jonatack commented Jun 1, 2020

Uh oh!

maflcko commented Jun 1, 2020

Uh oh!

adamjonas commented Jun 1, 2020

Uh oh!

DrahtBot commented Jun 2, 2020 •

edited

Loading

Uh oh!

jonatack commented Jun 2, 2020

Uh oh!

jonatack commented Jun 5, 2020

Uh oh!

za-kk commented Jun 7, 2020

Uh oh!

Sjors commented Jun 8, 2020

Uh oh!

adamjonas commented Jun 8, 2020

Uh oh!

maflcko commented Jun 9, 2020

Uh oh!

jonatack commented Jun 17, 2020

Uh oh!

meshcollider left a comment

Uh oh!

Uh oh!

rpc, cli, test: add bitcoin-cli -generate command #19133

rpc, cli, test: add bitcoin-cli -generate command #19133

Uh oh!

Conversation

jonatack commented Jun 1, 2020

Uh oh!

maflcko commented Jun 1, 2020

Uh oh!

jonatack commented Jun 1, 2020

Uh oh!

maflcko commented Jun 1, 2020

Uh oh!

adamjonas commented Jun 1, 2020

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

jonatack commented Jun 2, 2020

Uh oh!

jonatack commented Jun 5, 2020

Uh oh!

za-kk commented Jun 7, 2020

Uh oh!

Sjors commented Jun 8, 2020

Uh oh!

adamjonas commented Jun 8, 2020

Uh oh!

maflcko commented Jun 9, 2020

Uh oh!

jonatack commented Jun 17, 2020

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Jun 2, 2020 •

edited

Loading