Merge dashpay#6051: refactor: proper support for composite commands such as 'bls generate'

9413ecd fix: disable linter 'check-rpc-mapping.py' for composite commands (Konstantin Akimov)
f4bc19f refactor: proper support for composite commands such as 'bls generate' (Konstantin Akimov)

Pull request description:

  ## Issue being fixed or feature implemented
  We have composite commands such as 'bls generate' that do not exist in bitcoin's implementation of rpc.
  It doesn't let to backport yet bitcoin#18531 which enforced extra checks for arguments name (name of rpc and list arguments in rpc help and actual implementation must match).

  ## What was done?
  This PR improves support of composite commands in Dash Core. New style of composite commands are applied for `bls` composite commands: `bls generate` and `bls fromsecret` as proof of concept.
  Once this PR is merged, I will provide similar fixes for other "compose" rpc commands: `protx`, `masternode` (and everything else if any).

  Beside better validation of arguments and command names, it improves suggest menu in Qt app (see a screenshot) for composite commands.

  ![image](https://github.com/dashpay/dash/assets/545784/08dcc0b4-df92-4090-b163-af498bf200ef)

  ## How Has This Been Tested?
  Run unit and functional tests. Also extra tests to conduct in qt app:
   - check suggest for 'bls ....' and 'help bls ...'
   - check output of next commands:
  ```
  help bls
  help bls generate
  help bls from secret
  bls
  bls generate
  bls generate 1
  ```
   - also let's see that old-fashion composite commands are not broken:
  ```
  help protx
  help protx diff
  protx diff 00000021c7604b9992254f9f1ed91de5d65eaade33c773abea63a7b0e93293ee 000000e44f9894838ebf768b464177cfce8859dcf92b0509f5c2fba774315996
  ```

  ## Breaking Changes
  N/A

  ## Checklist:
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas
  - [x] I have added or updated relevant unit/integration/functional/e2e tests
  - [x] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone

ACKs for top commit:
  PastaPastaPasta:
    utACK 9413ecd

Tree-SHA512: c8accd2f1ea1d2168d7a2bf25311c92b7839782fc300c587c7537880df5d3fbbf7e79cedb2c6fa88ab067b696994beaf7d92185ae3e64fe1a5f70ad9e8620bec

Author: PastaPastaPasta

src/interfaces/node.h

@@ -253,7 +253,7 @@ class Node
     virtual UniValue executeRpc(const std::string& command, const UniValue& params, const std::string& uri) = 0;
 
     //! List rpc commands.
-    virtual std::vector<std::string> listRpcCommands() = 0;
+    virtual std::vector<std::pair<std::string, std::string>> listRpcCommands() = 0;
 
     //! Set RPC timer interface if unset.
     virtual void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) = 0;

src/node/interfaces.cpp

@@ -506,7 +506,7 @@ class NodeImpl : public Node
         req.URI = uri;
         return ::tableRPC.execute(req);
     }
-    std::vector<std::string> listRpcCommands() override { return ::tableRPC.listCommands(); }
+    std::vector<std::pair<std::string, std::string>> listRpcCommands() override { return ::tableRPC.listCommands(); }
     void rpcSetTimerInterfaceIfUnset(RPCTimerInterface* iface) override { RPCSetTimerInterfaceIfUnset(iface); }
     void rpcUnsetTimerInterface(RPCTimerInterface* iface) override { RPCUnsetTimerInterface(iface); }
     bool getUnspentOutput(const COutPoint& output, Coin& coin) override
@@ -710,14 +710,14 @@ class RpcHandlerImpl : public Handler
                 throw;
             }
         };
-        ::tableRPC.appendCommand(m_command.name, &m_command);
+        ::tableRPC.appendCommand(m_command.name, m_command.subname, &m_command);
     }
 
     void disconnect() override final
     {
         if (m_wrapped_command) {
             m_wrapped_command = nullptr;
-            ::tableRPC.removeCommand(m_command.name, &m_command);
+            ::tableRPC.removeCommand(m_command.name, m_command.subname, &m_command);
         }
     }
 

src/qt/rpcconsole.cpp

@@ -752,11 +752,15 @@ void RPCConsole::setClientModel(ClientModel *model, int bestblock_height, int64_
 
         //Setup autocomplete and attach it
         QStringList wordList;
-        std::vector<std::string> commandList = m_node.listRpcCommands();
+        std::vector<std::pair<std::string, std::string>> commandList = m_node.listRpcCommands();
         for (size_t i = 0; i < commandList.size(); ++i)
         {
-            wordList << commandList[i].c_str();
-            wordList << ("help " + commandList[i]).c_str();
+            std::string command = commandList[i].first;
+            if (!commandList[i].second.empty()) {
+                command = command + " " + commandList[i].second;
+            }
+            wordList << command.c_str();
+            wordList << ("help " + command).c_str();
         }
 
         wordList << "help-console";

src/rpc/evo.cpp

@@ -1706,9 +1706,9 @@ static UniValue protx(const JSONRPCRequest& request)
     }
 }
 
-static void bls_generate_help(const JSONRPCRequest& request)
+static RPCHelpMan bls_generate()
 {
-    RPCHelpMan{"bls generate",
+    return RPCHelpMan{"bls generate",
         "\nReturns a BLS secret/public key pair.\n",
         {
             {"legacy", RPCArg::Type::BOOL, /* default */ "true until the v19 fork is activated, otherwise false", "Use legacy BLS scheme"},
@@ -1723,12 +1723,10 @@ static void bls_generate_help(const JSONRPCRequest& request)
         RPCExamples{
             HelpExampleCli("bls generate", "")
         },
-    }.Check(request);
-}
-
-static UniValue bls_generate(const JSONRPCRequest& request, const ChainstateManager& chainman)
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    bls_generate_help(request);
+    const NodeContext& node = EnsureAnyNodeContext(request.context);
+    const ChainstateManager& chainman = EnsureChainman(node);
 
     CBLSSecretKey sk;
     sk.MakeNewKey();
@@ -1742,11 +1740,13 @@ static UniValue bls_generate(const JSONRPCRequest& request, const ChainstateMana
     std::string bls_scheme_str = bls_legacy_scheme ? "legacy" : "basic";
     ret.pushKV("scheme", bls_scheme_str);
     return ret;
+},
+    };
 }
 
-static void bls_fromsecret_help(const JSONRPCRequest& request)
+static RPCHelpMan bls_fromsecret()
 {
-    RPCHelpMan{"bls fromsecret",
+    return RPCHelpMan{"bls fromsecret",
         "\nParses a BLS secret key and returns the secret/public key pair.\n",
         {
             {"secret", RPCArg::Type::STR, RPCArg::Optional::NO, "The BLS secret key"},
@@ -1762,12 +1762,10 @@ static void bls_fromsecret_help(const JSONRPCRequest& request)
         RPCExamples{
             HelpExampleCli("bls fromsecret", "000102030405060708090a0b0c0d0e0f101112131415161718191a1b1c1d1e1f")
         },
-    }.Check(request);
-}
-
-static UniValue bls_fromsecret(const JSONRPCRequest& request, const ChainstateManager& chainman)
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    bls_fromsecret_help(request);
+    const NodeContext& node = EnsureAnyNodeContext(request.context);
+    const ChainstateManager& chainman = EnsureChainman(node);
 
     bool bls_legacy_scheme{!DeploymentActiveAfter(WITH_LOCK(cs_main, return chainman.ActiveChain().Tip();), Params().GetConsensus(), Consensus::DEPLOYMENT_V19)};
     if (!request.params[1].isNull()) {
@@ -1780,11 +1778,13 @@ static UniValue bls_fromsecret(const JSONRPCRequest& request, const ChainstateMa
     std::string bls_scheme_str = bls_legacy_scheme ? "legacy" : "basic";
     ret.pushKV("scheme", bls_scheme_str);
     return ret;
+},
+    };
 }
 
-[[ noreturn ]] static void bls_help()
+static RPCHelpMan bls_help()
 {
-    RPCHelpMan{"bls",
+    return RPCHelpMan{"bls",
         "Set of commands to execute BLS related actions.\n"
         "To get help on individual commands, use \"help bls command\".\n"
         "\nAvailable commands:\n"
@@ -1795,35 +1795,26 @@ static UniValue bls_fromsecret(const JSONRPCRequest& request, const ChainstateMa
         },
         RPCResults{},
         RPCExamples{""},
-    }.Throw();
-}
-
-static UniValue _bls(const JSONRPCRequest& request)
+        [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
 {
-    const JSONRPCRequest new_request{request.strMethod == "bls" ? request.squashed() : request};
-    const std::string command{new_request.strMethod};
-
-    const ChainstateManager& chainman = EnsureAnyChainman(request.context);
-
-    if (command == "blsgenerate") {
-        return bls_generate(new_request, chainman);
-    } else if (command == "blsfromsecret") {
-        return bls_fromsecret(new_request, chainman);
-    } else {
-        bls_help();
-    }
+    throw JSONRPCError(RPC_INVALID_PARAMETER, "Must be a valid command");
+},
+    };
 }
+
 void RegisterEvoRPCCommands(CRPCTable &tableRPC)
 {
 // clang-format off
 static const CRPCCommand commands[] =
 { //  category              name                      actor (function)
   //  --------------------- ------------------------  -----------------------
-    { "evo",                "bls",                    &_bls,                   {}  },
+    { "evo",                "bls",                    &bls_help,               {"command"}  },
+    { "evo",                "bls", "generate",        &bls_generate,           {"legacy"}  },
+    { "evo",                "bls", "fromsecret",      &bls_fromsecret,         {"secret", "legacy"}  },
     { "evo",                "protx",                  &protx,                  {}  },
 };
 // clang-format on
     for (const auto& command : commands) {
-        tableRPC.appendCommand(command.name, &command);
+        tableRPC.appendCommand(command.name, command.subname, &command);
     }
 }

src/rpc/server.cpp

@@ -92,7 +92,7 @@ std::string CRPCTable::help(const std::string& strCommand, const std::string& st
     std::vector<std::pair<std::string, const CRPCCommand*> > vCommands;
 
     for (const auto& entry : mapCommands)
-        vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
+        vCommands.push_back(make_pair(entry.second.front()->category + entry.first.first + entry.first.second, entry.second.front()));
     sort(vCommands.begin(), vCommands.end());
 
     JSONRPCRequest jreq = helpreq;
@@ -105,6 +105,9 @@ std::string CRPCTable::help(const std::string& strCommand, const std::string& st
         std::string strMethod = pcmd->name;
         if ((strCommand != "" || pcmd->category == "hidden") && strMethod != strCommand)
             continue;
+
+        if (strSubCommand != pcmd->subname) continue;
+
         jreq.strMethod = strMethod;
         try
         {
@@ -294,15 +297,20 @@ CRPCTable::CRPCTable()
 }
 
 void CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
+{
+    appendCommand(name, "", pcmd);
+}
+
+void CRPCTable::appendCommand(const std::string& name, const std::string& subname, const CRPCCommand* pcmd)
 {
     CHECK_NONFATAL(!IsRPCRunning()); // Only add commands before rpc is running
 
-    mapCommands[name].push_back(pcmd);
+    mapCommands[std::make_pair(name, subname)].push_back(pcmd);
 }
 
-bool CRPCTable::removeCommand(const std::string& name, const CRPCCommand* pcmd)
+bool CRPCTable::removeCommand(const std::string& name, const std::string& subname, const CRPCCommand* pcmd)
 {
-    auto it = mapCommands.find(name);
+    auto it = mapCommands.find(std::make_pair(name, subname));
     if (it != mapCommands.end()) {
         auto new_end = std::remove(it->second.begin(), it->second.end(), pcmd);
         if (it->second.end() != new_end) {
@@ -498,12 +506,22 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
             throw JSONRPCError(RPC_IN_WARMUP, rpcWarmupStatus);
     }
 
+    std::string subcommand;
+    if (request.params.size() > 0 && request.params[0].isStr()) {
+        subcommand = request.params[0].get_str();
+    }
+
     // Find method
-    auto it = mapCommands.find(request.strMethod);
+    auto it = mapCommands.find(std::make_pair(request.strMethod, subcommand));
+    if (it == mapCommands.end() && !subcommand.empty()) {
+        subcommand = "";
+        it = mapCommands.find(std::make_pair(request.strMethod, subcommand));
+    }
     if (it != mapCommands.end()) {
         UniValue result;
         for (const auto& command : it->second) {
-            if (ExecuteCommand(*command, request, result, &command == &it->second.back(), mapPlatformRestrictions)) {
+            const JSONRPCRequest new_request{subcommand.empty() ? request : request.squashed() };
+            if (ExecuteCommand(*command, new_request, result, &command == &it->second.back(), mapPlatformRestrictions)) {
                 return result;
             }
         }
@@ -591,9 +609,9 @@ static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& req
     }
 }
 
-std::vector<std::string> CRPCTable::listCommands() const
+std::vector<std::pair<std::string, std::string>> CRPCTable::listCommands() const
 {
-    std::vector<std::string> commandList;
+    std::vector<std::pair<std::string, std::string>> commandList;
     for (const auto& i : mapCommands) commandList.emplace_back(i.first);
     return commandList;
 }

src/rpc/server.h

@@ -95,8 +95,8 @@ class CRPCCommand
     using Actor = std::function<bool(const JSONRPCRequest& request, UniValue& result, bool last_handler)>;
 
     //! Constructor taking Actor callback supporting multiple handlers.
-    CRPCCommand(std::string category, std::string name, Actor actor, std::vector<std::string> args, intptr_t unique_id)
-        : category(std::move(category)), name(std::move(name)), actor(std::move(actor)), argNames(std::move(args)),
+    CRPCCommand(std::string category, std::string name, std::string subname, Actor actor, std::vector<std::string> args, intptr_t unique_id)
+        : category(std::move(category)), name(std::move(name)), subname(subname), actor(std::move(actor)), argNames(std::move(args)),
           unique_id(unique_id)
     {
     }
@@ -106,6 +106,7 @@ class CRPCCommand
         : CRPCCommand(
               category,
               fn().m_name,
+              "",
               [fn](const JSONRPCRequest& request, UniValue& result, bool) { result = fn().HandleRequest(request); return true; },
               fn().GetArgNames(),
               intptr_t(fn))
@@ -114,16 +115,36 @@ class CRPCCommand
         CHECK_NONFATAL(fn().GetArgNames() == args_in);
     }
 
+    //! Simplified constructor taking plain RpcMethodFnType function pointer with sub-command.
+    CRPCCommand(std::string category, std::string name_in, std::string subname_in, RpcMethodFnType fn, std::vector<std::string> args_in)
+        : CRPCCommand(
+              category,
+              name_in,
+              subname_in,
+              [fn](const JSONRPCRequest& request, UniValue& result, bool) { result = fn().HandleRequest(request); return true; },
+              fn().GetArgNames(),
+              intptr_t(fn))
+    {
+        if (subname_in.empty()) {
+            CHECK_NONFATAL(fn().m_name == name_in);
+        } else {
+            CHECK_NONFATAL(fn().m_name == name_in + " " + subname_in);
+        }
+
+        CHECK_NONFATAL(fn().GetArgNames() == args_in);
+    }
+
     //! Simplified constructor taking plain rpcfn_type function pointer.
     CRPCCommand(const char* category, const char* name, rpcfn_type fn, std::initializer_list<const char*> args)
-        : CRPCCommand(category, name,
+        : CRPCCommand(category, name, "",
                       [fn](const JSONRPCRequest& request, UniValue& result, bool) { result = fn(request); return true; },
                       {args.begin(), args.end()}, intptr_t(fn))
     {
     }
 
     std::string category;
     std::string name;
+    std::string subname;
     Actor actor;
     std::vector<std::string> argNames;
     intptr_t unique_id;
@@ -135,7 +156,7 @@ class CRPCCommand
 class CRPCTable
 {
 private:
-    std::map<std::string, std::vector<const CRPCCommand*>> mapCommands;
+    std::map<std::pair<std::string, std::string>, std::vector<const CRPCCommand*>> mapCommands;
     std::multimap<std::string, std::vector<UniValue>> mapPlatformRestrictions;
 public:
     CRPCTable();
@@ -155,7 +176,7 @@ class CRPCTable
     * Returns a list of registered commands
     * @returns List of registered commands.
     */
-    std::vector<std::string> listCommands() const;
+    std::vector<std::pair<std::string, std::string>> listCommands() const;
 
     /**
      * Appends a CRPCCommand to the dispatch table.
@@ -170,7 +191,8 @@ class CRPCTable
      * register different names, types, and numbers of parameters.
      */
     void appendCommand(const std::string& name, const CRPCCommand* pcmd);
-    bool removeCommand(const std::string& name, const CRPCCommand* pcmd);
+    void appendCommand(const std::string& name, const std::string& subname, const CRPCCommand* pcmd);
+    bool removeCommand(const std::string& name, const std::string& subname, const CRPCCommand* pcmd);
 };
 
 bool IsDeprecatedRPCEnabled(const std::string& method);

src/test/rpc_tests.cpp

@@ -49,7 +49,7 @@ UniValue RPCTestingSetup::TransformParams(const UniValue& params, std::vector<st
 {
     UniValue transformed_params;
     CRPCTable table;
-    CRPCCommand command{"category", "method", [&](const JSONRPCRequest& request, UniValue&, bool) -> bool { transformed_params = request.params; return true; }, arg_names, /*unique_id=*/0};
+    CRPCCommand command{"category", "method", "subcommand", [&](const JSONRPCRequest& request, UniValue&, bool) -> bool { transformed_params = request.params; return true; }, arg_names, /*unique_id=*/0};
     table.appendCommand("method", &command);
     CoreContext context{m_node};
     JSONRPCRequest request(context);

src/wallet/interfaces.cpp

@@ -572,7 +572,7 @@ class WalletLoaderImpl : public WalletLoader
     void registerRpcs() override
     {
         for (const CRPCCommand& command : GetWalletRPCCommands()) {
-            m_rpc_commands.emplace_back(command.category, command.name, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
+            m_rpc_commands.emplace_back(command.category, command.name, command.subname, [this, &command](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
                 return command.actor({request, m_context}, result, last_handler);
             }, command.argNames, command.unique_id);
             m_rpc_handlers.emplace_back(m_context.chain->handleRpc(m_rpc_commands.back()));

test/lint/check-rpc-mappings.py

@@ -60,6 +60,12 @@ def process_commands(fname):
                     in_rpcs = False
                 elif '{' in line and '"' in line:
                     m = re.search(r'{ *("[^"]*"), *("[^"]*"), *&([^,]*), *{([^}]*)} *},', line)
+                    # that's a quick fix for composite command
+                    # no proper implementation is needed so far as this linter would be removed soon with bitcoin#20012
+                    if not m:
+                        m = re.search(r'{ *("[^"]*"), *("[^"]*"), *("[^"]*"), *&([^,]*), *{([^}]*)} *},', line)
+                        if m:
+                            continue
                     assert m, 'No match to table expression: %s' % line
                     name = parse_string(m.group(2))
                     args_str = m.group(4).strip()