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

jonatack commented at 12:32 pm on June 1, 2020: member

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:

 0$ bitcoin-cli -generate
 1{
 2  "address": "bcrt1qn4aszr2y2xvpa70y675a76wsu70wlkwvdyyln6"
 3  "blocks": [
 4    "01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
 5  ]
 6}
 7
 8$ bitcoin-cli -rpcwallet=wallet-name -generate 3 100
 9{
10  "address": "bcrt1q4cunfw0gnsj7g7e6mk0v0uuvvau9mwr09dj45l",
11  "blocks": [
12    "7a6650ca5e0c614992ee64fb148a7e5e022af842e4b6003f81abd8baf1e75136",
13    "01d2ebcddf663da90b28da7f6805115e2ba7818f16fe747258836646a43a0bb5",
14    "3f8795ec40b1ad812b818c177680841be319a3f6753d4e32dc7dfb5bafe5d00e"
15  ]
16}

Help doc:

0$ bitcoin-cli -h | grep -A5 "\-generate"
1  -generate
2       Generate blocks immediately, equivalent to RPC generatenewaddress
3       followed by RPC generatetoaddress. Optional positional arguments
4       are number of blocks to generate (default: 1) and maximum
5       iterations to try (default: 1000000), equivalent to RPC
6       generatetoaddress nblocks and maxtries arguments. Example:
7       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.

fanquake added the label RPC/REST/ZMQ on Jun 1, 2020

fanquake added the label Feature on Jun 1, 2020

MarcoFalke commented at 12:52 pm on June 1, 2020: member

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 https://github.com/bitcoin/bitcoin/blob/9bc7751cadbd038faf8ac1d62cda23fcf00d4cc2/test/functional/test_framework/test_node.py#L296

Any thoughts on this?

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

jonatack commented at 1:22 pm on June 1, 2020: member

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.

MarcoFalke commented at 1:28 pm on June 1, 2020: member

Right now this happens on master:

0$ ./src/bitcoin-cli generate 
1error code: -32601
2error message:
3Method 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 commented at 4:14 pm on June 1, 2020: member

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 commented at 1:23 am on June 2, 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:

#19092 (cli: display multiwallet total balance in -getinfo by jonatack)
#19089 (cli, test, doc: bitcoin-cli -getinfo multiwallet balances follow-ups by jonatack)
#18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
#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.

rpc: make generatetoaddress locals const

and named the same as in generatetodescriptor just above.

9be7fd35c5

cli: create GetNewAddress() f7c65a3350

cli: create GenerateToAddressRequestHandler class f4185b26d9

cli: extract ParseResult() and ParseError()

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

ff41a36900

cli: create bitcoin-cli -generate command 4818124137

test: add tests for bitcoin-cli -generate 18f93545a1

cli: add multiwallet capability to GetNewAddress and -generate 4b859cfff9

test: add multiwallet tests for bitcoin-cli -generate bf53ebef06

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

jonatack force-pushed on Jun 2, 2020

jonatack commented at 9:09 am on June 2, 2020: member

Simplified some of the changes.

jonatack commented at 10:44 am on June 5, 2020: member

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 commented at 1:02 am on June 7, 2020: contributor

Tested ACK - tested using the following bitcoin-cli commands

rect976 rect817

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 commented at 11:32 am on June 8, 2020: member

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 commented at 7:09 pm on June 8, 2020: member

utACK 22cb303cf099b430d602384bc92706ce01b4f98d

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

MarcoFalke commented at 0:37 am on June 9, 2020: member

utACK

tested a few scenarios

Sounds like a tested ACK :eyes:

jonatack commented at 10:59 am on June 17, 2020: member

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 commented at 10:13 am on June 21, 2020: contributor

utACK 22cb303cf099b430d602384bc92706ce01b4f98d

meshcollider merged this on Jun 21, 2020

meshcollider closed this on Jun 21, 2020

jonatack deleted the branch on Jun 22, 2020

laanwj referenced this in commit 205b87d2f6 on Jun 24, 2020

sidhujag referenced this in commit 4868422bc1 on Jul 7, 2020

laanwj referenced this in commit dabab06a1a on Aug 14, 2020

sidhujag referenced this in commit ff1cb84f45 on Aug 16, 2020

PastaPastaPasta referenced this in commit f2cc9e4bdb on Jun 27, 2021

PastaPastaPasta referenced this in commit f0cbf074a7 on Jun 28, 2021

PastaPastaPasta referenced this in commit 8106002cbe on Jun 29, 2021

PastaPastaPasta referenced this in commit 5a5980ab81 on Jul 1, 2021

PastaPastaPasta referenced this in commit 7b9e479052 on Jul 1, 2021

PastaPastaPasta referenced this in commit ab260b949b on Jul 15, 2021

deadalnix referenced this in commit 1fed526a76 on Aug 26, 2021

deadalnix referenced this in commit bc48276efe on Aug 26, 2021

deadalnix referenced this in commit 237061547c on Aug 26, 2021

deadalnix referenced this in commit 8f6b1c9990 on Aug 26, 2021

deadalnix referenced this in commit 0edee2dcce on Aug 26, 2021

deadalnix referenced this in commit 40714b79a3 on Aug 26, 2021

deadalnix referenced this in commit b1c0b2cc98 on Aug 26, 2021

deadalnix referenced this in commit 27ebcb06bd on Aug 26, 2021

deadalnix referenced this in commit 055c2f96f8 on Aug 26, 2021

deadalnix referenced this in commit d6414fa557 on Aug 26, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts