Merge #19528: rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc)

fa77de2baa40ee828c850ef4068c76cc3619e87b rpc: Assert that RPCArg names are equal to CRPCCommand ones (misc) (MarcoFalke)
fa50bdc755489b2e291ea5ba0e39e44a20c6c6de rpc: Limit echo to 10 args (MarcoFalke)
fa89ca9b5bd334813fd7e7edb202c56b35076e8d refactor: Use C++11 range based for loops to simplify rpc code (MarcoFalke)
fa459bdc87bbb050ca1c8d469023a96ed798540e rpc: Treat all args after a hidden arg as hidden as well (MarcoFalke)

Pull request description:

  This is split out from #18531 to just touch the RPC methods in misc. Description from the main pr:

  ### Motivation

  RPCArg names in the rpc help are currently only used for documentation. However, in the future they could be used to teach the server the named arguments. Named arguments are currently registered by the `CRPCCommand`s and duplicate the RPCArg names from the documentation. This redundancy is fragile, and has lead to errors in the past (despite having linters to catch those kind of errors). See section "bugs found" for a list of bugs that have been found as a result of the changes here.

  ### Changes

  The changes here add an assert in the `CRPCCommand` constructor that the RPCArg names are identical to the ones in the `CRPCCommand`.

  ### Future work

  > Here or follow up, makes sense to also assert type of returned UniValue?

  Sure, but let's not get ahead of ourselves. I am going to submit any further works as follow-ups, including:

  * Removing the CRPCCommand arguments, now that they are asserted to be equal and thus redundant
  * Removing all python regex linters on the args, now that RPCMan can be used to generate any output, including the cli.cpp table
  * Auto-formatting and sanity checking the RPCExamples with RPCMan
  * Checking passed-in json in self-check. Removing redundant checks
  * Checking returned json against documentation to avoid regressions or false documentation
  * Compile the RPC documentation at compile-time to ensure it doesn't change at runtime and is completely static

  ### Bugs found

  * The assert identified issue #18607
  * The changes itself fixed bug #19250

ACKs for top commit:
  laanwj:
    Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b
  fjahr:
    tested ACK fa77de2baa40ee828c850ef4068c76cc3619e87b
  theStack:
    ACK https://github.com/bitcoin/bitcoin/pull/19528/commits/fa77de2baa40ee828c850ef4068c76cc3619e87b
  ryanofsky:
    Code review ACK fa77de2baa40ee828c850ef4068c76cc3619e87b. Pretty straightfoward changes

Tree-SHA512: badae1606518c0b55ce2c0bb9025d14f05556532375eb20fd6f3bfadae1e5e6568860bff8599d037e655bf1d23f1f464ca17f4db10a6ab3d502b6e9e61c7b3d3

8 files changed, 110 insertions, 76 deletions

diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp
index f27373b57c..868ff88d08 100644
--- a/src/rpc/blockchain.cpp
+++ b/src/rpc/blockchain.cpp
@@ -2407,7 +2407,7 @@ static const CRPCCommand commands[] =
     { "hidden",             "dumptxoutset",           &dumptxoutset,           {"path"} },
 };
 // clang-format on
-
-    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+    for (const auto& c : commands) {
+        t.appendCommand(c.name, &c);
+    }
 }
diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp
index 2f849d6f4b..d8d7cf47b0 100644
--- a/src/rpc/mining.cpp
+++ b/src/rpc/mining.cpp
@@ -1200,7 +1200,7 @@ static const CRPCCommand commands[] =
     { "hidden",             "estimaterawfee",         &estimaterawfee,         {"conf_target", "threshold"} },
 };
 // clang-format on
-
-    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+    for (const auto& c : commands) {
+        t.appendCommand(c.name, &c);
+    }
 }
diff --git a/src/rpc/misc.cpp b/src/rpc/misc.cpp
index 53d38f4e11..ff31bee1e3 100644
--- a/src/rpc/misc.cpp
+++ b/src/rpc/misc.cpp
@@ -27,9 +27,9 @@
 
 #include <univalue.h>
 
-static UniValue validateaddress(const JSONRPCRequest& request)
+static RPCHelpMan validateaddress()
 {
-            RPCHelpMan{"validateaddress",
+    return RPCHelpMan{"validateaddress",
                 "\nReturn information about the given bitcoin address.\n",
                 {
                     {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to validate"},
@@ -50,8 +50,8 @@ static UniValue validateaddress(const JSONRPCRequest& request)
                     HelpExampleCli("validateaddress", "\"" + EXAMPLE_ADDRESS[0] + "\"") +
                     HelpExampleRpc("validateaddress", "\"" + EXAMPLE_ADDRESS[0] + "\"")
                 },
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     CTxDestination dest = DecodeDestination(request.params[0].get_str());
     bool isValid = IsValidDestination(dest);
 
@@ -69,11 +69,13 @@ static UniValue validateaddress(const JSONRPCRequest& request)
         ret.pushKVs(detail);
     }
     return ret;
+},
+    };
 }
 
-static UniValue createmultisig(const JSONRPCRequest& request)
+static RPCHelpMan createmultisig()
 {
-            RPCHelpMan{"createmultisig",
+    return RPCHelpMan{"createmultisig",
                 "\nCreates a multi-signature address with n signature of m keys required.\n"
                 "It returns a json object with the address and redeemScript.\n",
                 {
@@ -98,8 +100,8 @@ static UniValue createmultisig(const JSONRPCRequest& request)
             "\nAs a JSON-RPC call\n"
             + HelpExampleRpc("createmultisig", "2, \"[\\\"03789ed0bb717d88f7d321a368d905e7430207ebbd82bd342cf11ae157a7ace5fd\\\",\\\"03dbc6764b8884a92e871274b87583e6d5c2a58819473e17e107ef3f6aa5a61626\\\"]\"")
                 },
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     int required = request.params[0].get_int();
 
     // Get the public keys
@@ -135,11 +137,13 @@ static UniValue createmultisig(const JSONRPCRequest& request)
     result.pushKV("descriptor", descriptor->ToString());
 
     return result;
+},
+    };
 }
 
-UniValue getdescriptorinfo(const JSONRPCRequest& request)
+static RPCHelpMan getdescriptorinfo()
 {
-            RPCHelpMan{"getdescriptorinfo",
+    return RPCHelpMan{"getdescriptorinfo",
             {"\nAnalyses a descriptor.\n"},
             {
                 {"descriptor", RPCArg::Type::STR, RPCArg::Optional::NO, "The descriptor."},
@@ -157,8 +161,9 @@ UniValue getdescriptorinfo(const JSONRPCRequest& request)
             RPCExamples{
                 "Analyse a descriptor\n" +
                 HelpExampleCli("getdescriptorinfo", "\"wpkh([d34db33f/84h/0h/0h]0279be667ef9dcbbac55a06295Ce870b07029Bfcdb2dce28d959f2815b16f81798)\"")
-            }}.Check(request);
-
+            },
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     RPCTypeCheck(request.params, {UniValue::VSTR});
 
     FlatSigningProvider provider;
@@ -175,11 +180,13 @@ UniValue getdescriptorinfo(const JSONRPCRequest& request)
     result.pushKV("issolvable", desc->IsSolvable());
     result.pushKV("hasprivatekeys", provider.keys.size() > 0);
     return result;
+},
+    };
 }
 
-UniValue deriveaddresses(const JSONRPCRequest& request)
+static RPCHelpMan deriveaddresses()
 {
-            RPCHelpMan{"deriveaddresses",
+    return RPCHelpMan{"deriveaddresses",
             {"\nDerives one or more addresses corresponding to an output descriptor.\n"
             "Examples of output descriptors are:\n"
             "    pkh(<pubkey>)                        P2PKH outputs for the given pubkey\n"
@@ -202,8 +209,9 @@ UniValue deriveaddresses(const JSONRPCRequest& request)
             RPCExamples{
                 "First three native segwit receive addresses\n" +
                 HelpExampleCli("deriveaddresses", "\"wpkh([d34db33f/84h/0h/0h]xpub6DJ2dNUysrn5Vt36jH2KLBT2i1auw1tTSSomg8PhqNiUtx8QX2SvC9nrHu81fT41fvDUnhMjEzQgXnQjKEu3oaqMSzhSrHMxyyoEAmUHQbY/0/*)#cjjspncu\" \"[0,2]\"")
-            }}.Check(request);
-
+            },
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     RPCTypeCheck(request.params, {UniValue::VSTR, UniValueType()}); // Range argument is checked later
     const std::string desc_str = request.params[0].get_str();
 
@@ -254,11 +262,13 @@ UniValue deriveaddresses(const JSONRPCRequest& request)
     }
 
     return addresses;
+},
+    };
 }
 
-static UniValue verifymessage(const JSONRPCRequest& request)
+static RPCHelpMan verifymessage()
 {
-            RPCHelpMan{"verifymessage",
+    return RPCHelpMan{"verifymessage",
                 "\nVerify a signed message\n",
                 {
                     {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address to use for the signature."},
@@ -278,8 +288,8 @@ static UniValue verifymessage(const JSONRPCRequest& request)
             "\nAs a JSON-RPC call\n"
             + HelpExampleRpc("verifymessage", "\"1D1ZrZNe3JUo7ZycKEYQQiQAWd9y54F4XX\", \"signature\", \"my message\"")
                 },
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     LOCK(cs_main);
 
     std::string strAddress  = request.params[0].get_str();
@@ -301,11 +311,13 @@ static UniValue verifymessage(const JSONRPCRequest& request)
     }
 
     return false;
+},
+    };
 }
 
-static UniValue signmessagewithprivkey(const JSONRPCRequest& request)
+static RPCHelpMan signmessagewithprivkey()
 {
-            RPCHelpMan{"signmessagewithprivkey",
+    return RPCHelpMan{"signmessagewithprivkey",
                 "\nSign a message with the private key of an address\n",
                 {
                     {"privkey", RPCArg::Type::STR, RPCArg::Optional::NO, "The private key to sign the message with."},
@@ -322,8 +334,8 @@ static UniValue signmessagewithprivkey(const JSONRPCRequest& request)
             "\nAs a JSON-RPC call\n"
             + HelpExampleRpc("signmessagewithprivkey", "\"privkey\", \"my message\"")
                 },
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     std::string strPrivkey = request.params[0].get_str();
     std::string strMessage = request.params[1].get_str();
 
@@ -339,11 +351,13 @@ static UniValue signmessagewithprivkey(const JSONRPCRequest& request)
     }
 
     return signature;
+},
+    };
 }
 
-static UniValue setmocktime(const JSONRPCRequest& request)
+static RPCHelpMan setmocktime()
 {
-            RPCHelpMan{"setmocktime",
+    return RPCHelpMan{"setmocktime",
                 "\nSet the local time to given timestamp (-regtest only)\n",
                 {
                     {"timestamp", RPCArg::Type::NUM, RPCArg::Optional::NO, UNIX_EPOCH_TIME + "\n"
@@ -351,8 +365,8 @@ static UniValue setmocktime(const JSONRPCRequest& request)
                 },
                 RPCResult{RPCResult::Type::NONE, "", ""},
                 RPCExamples{""},
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     if (!Params().IsMockableChain()) {
         throw std::runtime_error("setmocktime is for regression testing (-regtest mode) only");
     }
@@ -374,19 +388,21 @@ static UniValue setmocktime(const JSONRPCRequest& request)
     }
 
     return NullUniValue;
+},
+    };
 }
 
-static UniValue mockscheduler(const JSONRPCRequest& request)
+static RPCHelpMan mockscheduler()
 {
-    RPCHelpMan{"mockscheduler",
+    return RPCHelpMan{"mockscheduler",
         "\nBump the scheduler into the future (-regtest only)\n",
         {
             {"delta_time", RPCArg::Type::NUM, RPCArg::Optional::NO, "Number of seconds to forward the scheduler into the future." },
         },
         RPCResult{RPCResult::Type::NONE, "", ""},
         RPCExamples{""},
-    }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     if (!Params().IsMockableChain()) {
         throw std::runtime_error("mockscheduler is for regression testing (-regtest mode) only");
     }
@@ -405,6 +421,8 @@ static UniValue mockscheduler(const JSONRPCRequest& request)
     node.scheduler->MockForward(std::chrono::seconds(delta_seconds));
 
     return NullUniValue;
+},
+    };
 }
 
 static UniValue RPCLockedMemoryInfo()
@@ -439,12 +457,12 @@ static std::string RPCMallocInfo()
 }
 #endif
 
-static UniValue getmemoryinfo(const JSONRPCRequest& request)
+static RPCHelpMan getmemoryinfo()
 {
     /* Please, avoid using the word "pool" here in the RPC interface or help,
      * as users will undoubtedly confuse it with the other "memory pool"
      */
-            RPCHelpMan{"getmemoryinfo",
+    return RPCHelpMan{"getmemoryinfo",
                 "Returns an object containing information about memory usage.\n",
                 {
                     {"mode", RPCArg::Type::STR, /* default */ "\"stats\"", "determines what kind of information is returned.\n"
@@ -474,8 +492,8 @@ static UniValue getmemoryinfo(const JSONRPCRequest& request)
                     HelpExampleCli("getmemoryinfo", "")
             + HelpExampleRpc("getmemoryinfo", "")
                 },
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     std::string mode = request.params[0].isNull() ? "stats" : request.params[0].get_str();
     if (mode == "stats") {
         UniValue obj(UniValue::VOBJ);
@@ -490,6 +508,8 @@ static UniValue getmemoryinfo(const JSONRPCRequest& request)
     } else {
         throw JSONRPCError(RPC_INVALID_PARAMETER, "unknown mode " + mode);
     }
+},
+    };
 }
 
 static void EnableOrDisableLogCategories(UniValue cats, bool enable) {
@@ -510,9 +530,9 @@ static void EnableOrDisableLogCategories(UniValue cats, bool enable) {
     }
 }
 
-UniValue logging(const JSONRPCRequest& request)
+static RPCHelpMan logging()
 {
-            RPCHelpMan{"logging",
+    return RPCHelpMan{"logging",
             "Gets and sets the logging configuration.\n"
             "When called without an argument, returns the list of categories with status that are currently being debug logged or not.\n"
             "When called with arguments, adds or removes categories from debug logging and return the lists above.\n"
@@ -543,8 +563,8 @@ UniValue logging(const JSONRPCRequest& request)
                     HelpExampleCli("logging", "\"[\\\"all\\\"]\" \"[\\\"http\\\"]\"")
             + HelpExampleRpc("logging", "[\"all\"], [\"libevent\"]")
                 },
-            }.Check(request);
-
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
     uint32_t original_log_categories = LogInstance().GetCategoryMask();
     if (request.params[0].isArray()) {
         EnableOrDisableLogCategories(request.params[0], true);
@@ -575,28 +595,47 @@ UniValue logging(const JSONRPCRequest& request)
     }
 
     return result;
+},
+    };
 }
 
-static UniValue echo(const JSONRPCRequest& request)
+static RPCHelpMan echo(const std::string& name)
 {
-    if (request.fHelp)
-        throw std::runtime_error(
-            RPCHelpMan{"echo|echojson ...",
+    return RPCHelpMan{name,
                 "\nSimply echo back the input arguments. This command is for testing.\n"
-                "\nIt will return an internal bug report when exactly 100 arguments are passed.\n"
+                "\nIt will return an internal bug report when arg9='trigger_internal_bug' is passed.\n"
                 "\nThe difference between echo and echojson is that echojson has argument conversion enabled in the client-side table in "
                 "bitcoin-cli and the GUI. There is no server-side difference.",
-                {},
+                {
+                    {"arg0", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg1", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg2", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg3", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg4", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg5", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg6", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg7", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg8", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                    {"arg9", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, ""},
+                },
                 RPCResult{RPCResult::Type::NONE, "", "Returns whatever was passed in"},
                 RPCExamples{""},
-            }.ToString()
-        );
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
+{
+    if (request.fHelp) throw std::runtime_error(self.ToString());
 
-    CHECK_NONFATAL(request.params.size() != 100);
+    if (request.params[9].isStr()) {
+        CHECK_NONFATAL(request.params[9].get_str() != "trigger_internal_bug");
+    }
 
     return request.params;
+},
+    };
 }
 
+static RPCHelpMan echo() { return echo("echo"); }
+static RPCHelpMan echojson() { return echo("echojson"); }
+
 void RegisterMiscRPCCommands(CRPCTable &t)
 {
 // clang-format off
@@ -616,10 +655,10 @@ static const CRPCCommand commands[] =
     { "hidden",             "setmocktime",            &setmocktime,            {"timestamp"}},
     { "hidden",             "mockscheduler",          &mockscheduler,          {"delta_time"}},
     { "hidden",             "echo",                   &echo,                   {"arg0","arg1","arg2","arg3","arg4","arg5","arg6","arg7","arg8","arg9"}},
-    { "hidden",             "echojson",               &echo,                   {"arg0","arg1","arg2","arg3","arg4","arg5","arg6","arg7","arg8","arg9"}},
+    { "hidden",             "echojson",               &echojson,               {"arg0","arg1","arg2","arg3","arg4","arg5","arg6","arg7","arg8","arg9"}},
 };
 // clang-format on
-
-    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+    for (const auto& c : commands) {
+        t.appendCommand(c.name, &c);
+    }
 }
diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp
index 77d24a6e79..9bd7c15992 100644
--- a/src/rpc/net.cpp
+++ b/src/rpc/net.cpp
@@ -843,7 +843,7 @@ static const CRPCCommand commands[] =
     { "hidden",             "addpeeraddress",         &addpeeraddress,         {"address", "port"} },
 };
 // clang-format on
-
-    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+    for (const auto& c : commands) {
+        t.appendCommand(c.name, &c);
+    }
 }
diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 67ded27769..abc8168c55 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -1821,7 +1821,7 @@ static const CRPCCommand commands[] =
     { "blockchain",         "verifytxoutproof",             &verifytxoutproof,          {"proof"} },
 };
 // clang-format on
-
-    for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
-        t.appendCommand(commands[vcidx].name, &commands[vcidx]);
+    for (const auto& c : commands) {
+        t.appendCommand(c.name, &c);
+    }
 }
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index e5f6b1b9f1..9c8e7fe04a 100644
--- a/src/rpc/server.cpp
+++ b/src/rpc/server.cpp
@@ -256,13 +256,8 @@ static const CRPCCommand vRPCCommands[] =
 
 CRPCTable::CRPCTable()
 {
-    unsigned int vcidx;
-    for (vcidx = 0; vcidx < (sizeof(vRPCCommands) / sizeof(vRPCCommands[0])); vcidx++)
-    {
-        const CRPCCommand *pcmd;
-
-        pcmd = &vRPCCommands[vcidx];
-        mapCommands[pcmd->name].push_back(pcmd);
+    for (const auto& c : vRPCCommands) {
+        appendCommand(c.name, &c);
     }
 }
 
diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp
index 073a7688a9..40dfdb587e 100644
--- a/src/rpc/util.cpp
+++ b/src/rpc/util.cpp
@@ -504,7 +504,7 @@ std::string RPCHelpMan::ToString() const
     ret += m_name;
     bool was_optional{false};
     for (const auto& arg : m_args) {
-        if (arg.m_hidden) continue;
+        if (arg.m_hidden) break; // Any arg that follows is also hidden
         const bool optional = arg.IsOptional();
         ret += " ";
         if (optional) {
@@ -526,7 +526,7 @@ std::string RPCHelpMan::ToString() const
     Sections sections;
     for (size_t i{0}; i < m_args.size(); ++i) {
         const auto& arg = m_args.at(i);
-        if (arg.m_hidden) continue;
+        if (arg.m_hidden) break; // Any arg that follows is also hidden
 
         if (i == 0) ret += "\nArguments:\n";
 
diff --git a/test/functional/rpc_misc.py b/test/functional/rpc_misc.py
index c8517d719e..cc5a264adb 100755
--- a/test/functional/rpc_misc.py
+++ b/test/functional/rpc_misc.py
@@ -27,8 +27,8 @@ class RpcMiscTest(BitcoinTestFramework):
         self.log.info("test CHECK_NONFATAL")
         assert_raises_rpc_error(
             -1,
-            "Internal bug detected: 'request.params.size() != 100'",
-            lambda: node.echo(*[0] * 100),
+            'Internal bug detected: \'request.params[9].get_str() != "trigger_internal_bug"\'',
+            lambda: node.echo(arg9='trigger_internal_bug'),
         )
 
         self.log.info("test getmemoryinfo")