cli: add -generate option #17700

Why is is needed to extend them with the defaults?

In generatetoaddress the first and third param are nblocks and maxtries. This is to make sure that all three args are there. But if there is a better way to handle it, please, give me a hint.

If the user does not give a value, you can just pass nothing along as well (or null). The server then checks this and fills in the current default: https://github.com/bitcoin/bitcoin/blob/a739d207a39fc34c2e5adf4e14bfbb3a6288ede1/src/rpc/mining.cpp#L209

But I need it to construct the "fake" RPC call, because I also have to generate an address first. Then I go to create blocks. So I have to "save" all those params and their positions, so I can find them again.

But of course, I could simply put a NULL there, no problem. Let me check it how it'll behave. :)

Hm, I'm now getting errors. No matter if I enter any args or none at all. Not sure if I should redo the logic? Is it really important that it doesn't use any default args?

So I tried this diff:

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index ef05c910d4..2fff98c05c 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -535,11 +535,11 @@ static int CommandLineRPC(int argc, char *argv[])
                 if (args.size() == 0) {
                     args.push_back(std::to_string(DEFAULT_GENERATED_NBLOCKS));
                     args.push_back(std::move(address));
-                    args.push_back(std::to_string(DEFAULT_GENERATED_MAXTRIES));
                 }
                 if (args.size() < 2) {
                     args.push_back(std::move(address));
-                    args.push_back(std::to_string(DEFAULT_GENERATED_MAXTRIES));
                 }
                 if (args.size() == 2) {
                     const std::string nblocks = args[0];

And I get

$ ./src/bitcoin-cli -generate 1 
error: Error parsing JSON:bcrt1qyqfgcaddym58h6py4rrsz4g428gl5tmw84rt36

Not sure why that is. I might take another look later.

This is surely because of the "ephemeral" calls in between. First, the address gets generated, then its JSON result flows into the second call, generatetoaddress. I am manipulating the args vector, which is being (re)used by the two APIs.

Oh, I forgot to replace the if with else if. This works:

diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index ef05c910d4..4179520cc1 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -229,7 +229,7 @@ class GetNewAddressRequestHandler: public BaseRequestHandler
 public:
    const int ID_GETNEWADDRESS = 0;
 
-   UniValue PrepareRequest(const std::string& method, const std::vector<std::string>& args) override
+   UniValue PrepareRequest(const std::string& method, const std::vector<std::string>& /* args */) override
     {
         const UniValue& request = JSONRPCRequestObj("getnewaddress", NullUniValue, ID_GETNEWADDRESS);
         return request;
@@ -248,11 +248,8 @@ public:
 
     UniValue PrepareRequest(const std::string& method, const std::vector<std::string>& args) override
     {
-        std::vector<std::string> params = {};
-        params.push_back(args[0]);
-        params.push_back(args[1]);
-        params.push_back(args[2]);
-        UniValue paramsObj = RPCConvertValues("generatetoaddress", params);
+        UniValue paramsObj = RPCConvertValues("generatetoaddress", args);
         return JSONRPCRequestObj("generatetoaddress", paramsObj, ID_GENERATETOADDRESS);
     }
 
@@ -535,13 +532,11 @@ static int CommandLineRPC(int argc, char *argv[])
                 if (args.size() == 0) {
                     args.push_back(std::to_string(DEFAULT_GENERATED_NBLOCKS));
                     args.push_back(std::move(address));
-                    args.push_back(std::to_string(DEFAULT_GENERATED_MAXTRIES));
                 }
-                if (args.size() < 2) {
+                else if (args.size() < 2) {
                     args.push_back(std::move(address));
-                    args.push_back(std::to_string(DEFAULT_GENERATED_MAXTRIES));
                 }
-                if (args.size() == 2) {
+                else if (args.size() == 2) {
                     const std::string nblocks = args[0];
                     const std::string maxtries = args[1];
                     args.clear();

Great! Many thanks!

• Is the equal sign wanted in -generate=...?
• I think it is fine to just mention that the default for maxtries is given by the default of the generatetoaddress::maxtries

I wasn't sure how to properly inform the user about the optional nature of these two flags. There is no requirement to keep the equal sign. I'll adapt this part to look like other commands.

Hm, I was wrong, the equal sign is needed. My tests are failing if I have no equal sign there.

But I will remove the [] around the params. Also, the call works with =, so "generate=1" returns a block.

But it doesn't process the arguments properly. So, if I put generate=10 it still generates only one block. And if I call it with generate=1 1000 it tries to generate 1000 blocks. This definitely is not good, so I need a way to get rid of = sign in the command description.

The next option, imo, is to extend the rpc/client.cpp with param names so they can be picked up by RPCConvertNamedValues in the handler functions. Will try it out.

Ok, the simplest solution is the best one. :)

I have removed the whole stuff =nblocks maxtries and only left -generate in command help info.

should be removed?

Yes, it should go now. Will adapt the code. Thanks!

            // 1) "getnewaddress" without any parameters

This is the default, but can be modified in the config settings

Yes, of course :)

style-nit: I prefer .at(0) and .at(1). Feel free to ignore.

at makes more sense, in every way. Will adapt.

    gArgs.AddArg("-generate", strprintf("[nblocks] [maxtries] Generate blocks immediately (default nblocks: %s, maxtries: Same default as the generatetoaddress RPC)", DEFAULT_GENERATED_NBLOCKS), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);

Many thanks!

This is now unused, right?

DEFAULT_GENERATED_MAXTRIES is unused. The other is still needed for args in CommandLineRPC

Maybe I messed it up and forgot to remove it in CommandLineRPC too?

And DEFAULT_GENERATED_NBLOCKS is still used in help info.

Have tested it without DEFAULT_GENERATED_NBLOCKS in CommandLIneRPC and it failed when used generate without any params.

sort, should be above -getinfo?

Ok, I can change it. However, as there are already a few other commits that I didn't squash because of some reasons I will keep this change and a few others you mentioned below in a separate commit. I can then squash them all together later.

Do you need a clean chain for the test or can it use the cached data?

Could place the tests in run_test() directly since they are all together anyway.

Could add a docstring and/or logging.

I think you don't need to do any of these 40 lines of setup as you're not batching. You can use the existing DefaultRequestHandler and put the 2 pushKV lines in your -generate code section below. See 06a1bf6c1e1de3e219d2f8eebe187c1ca2962e9d for a semi-related example.

Many thanks. A good idea. However, my code is based on a pretty "old" version that differs greatly from https://github.com/bitcoin/bitcoin/commit/06a1bf6c1e1de3e219d2f8eebe187c1ca2962e9d

Maybe before changing all that, I should rebase this code and try to re-implement it again.

ISTM the only (or main) change between the example in 06a1bf6c1e1de3e219d2f8eebe187c1ca2962e9d and your code is that you would use CallRPC instead of ConnectAndCallRPC (with the same arguments, just renaming).

Ok, then let's see how far this goes.

E.g.

CallRPC(rh.get(), "getnewaddress", args);
CallRPC(rh.get(), "generatetoaddress", args);

etc.

Sadly, it wouldn't make much sense to change it, because CallRPC isn't exactly the same as ConnectAndCallRPC. In my code there is a "default" CallRPC, which collides with my special "-generate" command that needs two of such calls. So, either I modify this implicit RPCCall thus making it even more complex, or I leave it as it is. I don't remember exactly, because it's been three months since I've worked on this code, but I wrote all that "ceremonial" stuff (extra request handlers etc.) because I didn't want to introduce even more if-then-else to find my way around the already complex structure there. But now, as the ConnectAndCallRPC has actually moved away this "default" CallRPC I could work on that basis. Hence my question, should I rebase the code.