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

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.

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?

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.

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.

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

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.

Simplified some of the changes.

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.

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.

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.

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

utACK

tested a few scenarios

Sounds like a tested ACK :eyes:

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.

utACK 22cb303cf099b430d602384bc92706ce01b4f98d