From 6158a6d3978a18d5ac4166ca2f59056d8ef71c3d Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 29 Jan 2021 18:09:46 -0500 Subject: [PATCH 1/3] refactor: Replace JSONRPCRequest fHelp field with mode field No change in behavior --- src/rpc/request.h | 6 +++--- src/rpc/server.cpp | 2 +- src/rpc/util.cpp | 12 ++++++++++++ src/rpc/util.h | 16 +--------------- src/test/rpc_tests.cpp | 1 - src/wallet/rpcwallet.cpp | 2 +- 6 files changed, 18 insertions(+), 21 deletions(-) diff --git a/src/rpc/request.h b/src/rpc/request.h index de3a4ae8403..1751c24cd3f 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -34,19 +34,19 @@ public: UniValue id; std::string strMethod; UniValue params; - bool fHelp; + enum Mode { EXECUTE, GET_HELP } mode = EXECUTE; std::string URI; std::string authUser; std::string peerAddr; const util::Ref& context; - explicit JSONRPCRequest(const util::Ref& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {} + explicit JSONRPCRequest(const util::Ref& context) : id(NullUniValue), params(NullUniValue), context(context) {} //! Initializes request information from another request object and the //! given context. The implementation should be updated if any members are //! added or removed above. JSONRPCRequest(const JSONRPCRequest& other, const util::Ref& context) - : id(other.id), strMethod(other.strMethod), params(other.params), fHelp(other.fHelp), URI(other.URI), + : id(other.id), strMethod(other.strMethod), params(other.params), mode(other.mode), URI(other.URI), authUser(other.authUser), peerAddr(other.peerAddr), context(context) { } diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 9a9b3713f32..1b8bbafcef0 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -88,7 +88,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest& sort(vCommands.begin(), vCommands.end()); JSONRPCRequest jreq(helpreq); - jreq.fHelp = true; + jreq.mode = JSONRPCRequest::GET_HELP; jreq.params = UniValue(); for (const std::pair& command : vCommands) diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index bfdba5253c0..dc57d2be072 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -476,6 +476,18 @@ std::string RPCExamples::ToDescriptionString() const return m_examples.empty() ? m_examples : "\nExamples:\n" + m_examples; } +UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) +{ + /* + * Check if the given request is valid according to this command or if + * the user is asking for help information, and throw help when appropriate. + */ + if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) { + throw std::runtime_error(ToString()); + } + return m_fun(*this, request); +} + bool RPCHelpMan::IsValidNumArgs(size_t num_args) const { size_t num_required_args = 0; diff --git a/src/rpc/util.h b/src/rpc/util.h index 444a013ca17..aaca8879009 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -335,26 +335,12 @@ public: using RPCMethodImpl = std::function; RPCHelpMan(std::string name, std::string description, std::vector args, RPCResults results, RPCExamples examples, RPCMethodImpl fun); + UniValue HandleRequest(const JSONRPCRequest& request); std::string ToString() const; /** Append the named args that need to be converted from string to another JSON type */ void AppendArgMap(UniValue& arr) const; - UniValue HandleRequest(const JSONRPCRequest& request) - { - Check(request); - return m_fun(*this, request); - } /** If the supplied number of args is neither too small nor too high */ bool IsValidNumArgs(size_t num_args) const; - /** - * Check if the given request is valid according to this command or if - * the user is asking for help information, and throw help when appropriate. - */ - inline void Check(const JSONRPCRequest& request) const { - if (request.fHelp || !IsValidNumArgs(request.params.size())) { - throw std::runtime_error(ToString()); - } - } - std::vector GetArgNames() const; const std::string m_name; diff --git a/src/test/rpc_tests.cpp b/src/test/rpc_tests.cpp index b54cbb3f002..e88d60033f2 100644 --- a/src/test/rpc_tests.cpp +++ b/src/test/rpc_tests.cpp @@ -36,7 +36,6 @@ UniValue RPCTestingSetup::CallRPC(std::string args) JSONRPCRequest request(context); request.strMethod = strMethod; request.params = RPCConvertValues(strMethod, vArgs); - request.fHelp = false; if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished(); try { UniValue result = tableRPC.execute(request); diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index 46de273d635..2b613a0d32a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -96,7 +96,7 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& std::shared_ptr GetWalletForJSONRPCRequest(const JSONRPCRequest& request) { - CHECK_NONFATAL(!request.fHelp); + CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE); std::string wallet_name; if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) { std::shared_ptr pwallet = GetWallet(wallet_name); From 14f3d9b908ed9e78997bfaad3d8a06357a89d46e Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 29 Jan 2021 18:12:19 -0500 Subject: [PATCH 2/3] refactor: Add RPC server ExecuteCommands function No change in behavior. New function is split from CRPCTable::execute and used in the next commit. --- src/rpc/server.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 1b8bbafcef0..66134a77b2f 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -437,6 +437,16 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c return out; } +static bool ExecuteCommands(const std::vector& commands, const JSONRPCRequest& request, UniValue& result) +{ + for (const auto& command : commands) { + if (ExecuteCommand(*command, request, result, &command == &commands.back())) { + return true; + } + } + return false; +} + UniValue CRPCTable::execute(const JSONRPCRequest &request) const { // Return immediately if in warmup @@ -450,10 +460,8 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const auto it = mapCommands.find(request.strMethod); if (it != mapCommands.end()) { UniValue result; - for (const auto& command : it->second) { - if (ExecuteCommand(*command, request, result, &command == &it->second.back())) { - return result; - } + if (ExecuteCommands(it->second, request, result)) { + return result; } } throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found"); From 9048c58e10841d9e1d709c0a325dd14684cec325 Mon Sep 17 00:00:00 2001 From: Russell Yanofsky Date: Fri, 29 Jan 2021 18:15:48 -0500 Subject: [PATCH 3/3] Remove pointer cast in CRPCTable::dumpArgMap CRPCTable::dumpArgMap currently works by casting RPC command unique_id integer field to a function pointer, and then calling the function. The unique_id field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases), and this code segfaults in the rpc_help.py test in multiprocess PR https://github.com/bitcoin/bitcoin/pull/10102 because wallet RPC functions aren't directly accessible from the node process. Fix this by adding a new GET_ARGS request mode to retrieve argument information similar to the way the GET_HELP mode retrieves help information. --- src/rpc/request.h | 2 +- src/rpc/server.cpp | 15 ++++++++++----- src/rpc/server.h | 2 +- src/rpc/util.cpp | 7 ++++++- src/rpc/util.h | 4 ++-- 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/src/rpc/request.h b/src/rpc/request.h index 1751c24cd3f..27d06f3c928 100644 --- a/src/rpc/request.h +++ b/src/rpc/request.h @@ -34,7 +34,7 @@ public: UniValue id; std::string strMethod; UniValue params; - enum Mode { EXECUTE, GET_HELP } mode = EXECUTE; + enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE; std::string URI; std::string authUser; std::string peerAddr; diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp index 66134a77b2f..39938f4eb98 100644 --- a/src/rpc/server.cpp +++ b/src/rpc/server.cpp @@ -149,7 +149,7 @@ static RPCHelpMan help() } if (strCommand == "dump_all_command_conversions") { // Used for testing only, undocumented - return tableRPC.dumpArgMap(); + return tableRPC.dumpArgMap(jsonRequest); } return tableRPC.help(strCommand, jsonRequest); @@ -492,13 +492,18 @@ std::vector CRPCTable::listCommands() const return commandList; } -UniValue CRPCTable::dumpArgMap() const +UniValue CRPCTable::dumpArgMap(const JSONRPCRequest& args_request) const { + JSONRPCRequest request(args_request); + request.mode = JSONRPCRequest::GET_ARGS; + UniValue ret{UniValue::VARR}; for (const auto& cmd : mapCommands) { - for (const auto& c : cmd.second) { - const auto help = RpcMethodFnType(c->unique_id)(); - help.AppendArgMap(ret); + UniValue result; + if (ExecuteCommands(cmd.second, request, result)) { + for (const auto& values : result.getValues()) { + ret.push_back(values); + } } } return ret; diff --git a/src/rpc/server.h b/src/rpc/server.h index fe5a791e1e3..03967020c2e 100644 --- a/src/rpc/server.h +++ b/src/rpc/server.h @@ -148,7 +148,7 @@ public: /** * Return all named arguments that need to be converted by the client from string to another JSON type */ - UniValue dumpArgMap() const; + UniValue dumpArgMap(const JSONRPCRequest& request) const; /** * Appends a CRPCCommand to the dispatch table. diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index dc57d2be072..c7472fc5c10 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -478,6 +478,9 @@ std::string RPCExamples::ToDescriptionString() const UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request) { + if (request.mode == JSONRPCRequest::GET_ARGS) { + return GetArgMap(); + } /* * Check if the given request is valid according to this command or if * the user is asking for help information, and throw help when appropriate. @@ -561,8 +564,9 @@ std::string RPCHelpMan::ToString() const return ret; } -void RPCHelpMan::AppendArgMap(UniValue& arr) const +UniValue RPCHelpMan::GetArgMap() const { + UniValue arr{UniValue::VARR}; for (int i{0}; i < int(m_args.size()); ++i) { const auto& arg = m_args.at(i); std::vector arg_names; @@ -577,6 +581,7 @@ void RPCHelpMan::AppendArgMap(UniValue& arr) const arr.push_back(map); } } + return arr; } std::string RPCArg::GetFirstName() const diff --git a/src/rpc/util.h b/src/rpc/util.h index aaca8879009..90521949a67 100644 --- a/src/rpc/util.h +++ b/src/rpc/util.h @@ -337,8 +337,8 @@ public: UniValue HandleRequest(const JSONRPCRequest& request); std::string ToString() const; - /** Append the named args that need to be converted from string to another JSON type */ - void AppendArgMap(UniValue& arr) const; + /** Return the named args that need to be converted from string to another JSON type */ + UniValue GetArgMap() const; /** If the supplied number of args is neither too small nor too high */ bool IsValidNumArgs(size_t num_args) const; std::vector GetArgNames() const;