Skip to content

Commit 1e57d14

Browse files
author
MarcoFalke
committed
Merge bitcoin#21035: Remove pointer cast in CRPCTable::dumpArgMap
9048c58 Remove pointer cast in CRPCTable::dumpArgMap (Russell Yanofsky) 14f3d9b refactor: Add RPC server ExecuteCommands function (Russell Yanofsky) 6158a6d refactor: Replace JSONRPCRequest fHelp field with mode field (Russell Yanofsky) Pull request description: This change is needed to fix the `rpc_help.py` test failing in bitcoin#10102: https://cirrus-ci.com/task/5469433013469184?command=ci#L2275 The [`CRPCTable::dumpArgMap`](https://github.com/bitcoin/bitcoin/blob/16b784d953365bb2d7ae65acd2b20a79ef8ba7b6/src/rpc/server.cpp#L492) method currently works by casting RPC `unique_id` integer field to a function pointer, and then calling it. The `unique_id` field wasn't supposed to be used this way (it's meant to be used to detect RPC aliases) and as a result, this code segfaults in the `rpc_help.py` test in multiprocess PR bitcoin#10102 because wallet RPC functions aren't directly accessible from the node process. Fix this by adding a new `GET_ARGS` RPC request mode to retrieve argument information similar to the way the `GET_HELP` mode retrieves help information. --- This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). ACKs for top commit: MarcoFalke: re-ACK 9048c58 👑 Tree-SHA512: cd1a01c1daa5bde2c2455b63548371ee4cf39688313969ad2016d9a0fd4344102e3fd43034058f253364518e9632d57cf21abffad0d6a2c0c94b7a6921cbe615
2 parents 6bc51af + 9048c58 commit 1e57d14

File tree

7 files changed

+49
-34
lines changed

7 files changed

+49
-34
lines changed

‎src/rpc/request.h‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,19 +34,19 @@ class JSONRPCRequest
3434
UniValue id;
3535
std::string strMethod;
3636
UniValue params;
37-
bool fHelp;
37+
enum Mode { EXECUTE, GET_HELP, GET_ARGS } mode = EXECUTE;
3838
std::string URI;
3939
std::string authUser;
4040
std::string peerAddr;
4141
const util::Ref& context;
4242

43-
explicit JSONRPCRequest(const util::Ref& context) : id(NullUniValue), params(NullUniValue), fHelp(false), context(context) {}
43+
explicit JSONRPCRequest(const util::Ref& context) : id(NullUniValue), params(NullUniValue), context(context) {}
4444

4545
//! Initializes request information from another request object and the
4646
//! given context. The implementation should be updated if any members are
4747
//! added or removed above.
4848
JSONRPCRequest(const JSONRPCRequest& other, const util::Ref& context)
49-
: id(other.id), strMethod(other.strMethod), params(other.params), fHelp(other.fHelp), URI(other.URI),
49+
: id(other.id), strMethod(other.strMethod), params(other.params), mode(other.mode), URI(other.URI),
5050
authUser(other.authUser), peerAddr(other.peerAddr), context(context)
5151
{
5252
}

‎src/rpc/server.cpp‎

Lines changed: 23 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
8888
sort(vCommands.begin(), vCommands.end());
8989

9090
JSONRPCRequest jreq(helpreq);
91-
jreq.fHelp = true;
91+
jreq.mode = JSONRPCRequest::GET_HELP;
9292
jreq.params = UniValue();
9393

9494
for (const std::pair<std::string, const CRPCCommand*>& command : vCommands)
@@ -149,7 +149,7 @@ static RPCHelpMan help()
149149
}
150150
if (strCommand == "dump_all_command_conversions") {
151151
// Used for testing only, undocumented
152-
return tableRPC.dumpArgMap();
152+
return tableRPC.dumpArgMap(jsonRequest);
153153
}
154154

155155
return tableRPC.help(strCommand, jsonRequest);
@@ -437,6 +437,16 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
437437
return out;
438438
}
439439

440+
static bool ExecuteCommands(const std::vector<const CRPCCommand*>& commands, const JSONRPCRequest& request, UniValue& result)
441+
{
442+
for (const auto& command : commands) {
443+
if (ExecuteCommand(*command, request, result, &command == &commands.back())) {
444+
return true;
445+
}
446+
}
447+
return false;
448+
}
449+
440450
UniValue CRPCTable::execute(const JSONRPCRequest &request) const
441451
{
442452
// Return immediately if in warmup
@@ -450,10 +460,8 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
450460
auto it = mapCommands.find(request.strMethod);
451461
if (it != mapCommands.end()) {
452462
UniValue result;
453-
for (const auto& command : it->second) {
454-
if (ExecuteCommand(*command, request, result, &command == &it->second.back())) {
455-
return result;
456-
}
463+
if (ExecuteCommands(it->second, request, result)) {
464+
return result;
457465
}
458466
}
459467
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
@@ -484,13 +492,18 @@ std::vector<std::string> CRPCTable::listCommands() const
484492
return commandList;
485493
}
486494

487-
UniValue CRPCTable::dumpArgMap() const
495+
UniValue CRPCTable::dumpArgMap(const JSONRPCRequest& args_request) const
488496
{
497+
JSONRPCRequest request(args_request);
498+
request.mode = JSONRPCRequest::GET_ARGS;
499+
489500
UniValue ret{UniValue::VARR};
490501
for (const auto& cmd : mapCommands) {
491-
for (const auto& c : cmd.second) {
492-
const auto help = RpcMethodFnType(c->unique_id)();
493-
help.AppendArgMap(ret);
502+
UniValue result;
503+
if (ExecuteCommands(cmd.second, request, result)) {
504+
for (const auto& values : result.getValues()) {
505+
ret.push_back(values);
506+
}
494507
}
495508
}
496509
return ret;

‎src/rpc/server.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -148,7 +148,7 @@ class CRPCTable
148148
/**
149149
* Return all named arguments that need to be converted by the client from string to another JSON type
150150
*/
151-
UniValue dumpArgMap() const;
151+
UniValue dumpArgMap(const JSONRPCRequest& request) const;
152152

153153
/**
154154
* Appends a CRPCCommand to the dispatch table.

‎src/rpc/util.cpp‎

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -459,6 +459,21 @@ std::string RPCExamples::ToDescriptionString() const
459459
return m_examples.empty() ? m_examples : "\nExamples:\n" + m_examples;
460460
}
461461

462+
UniValue RPCHelpMan::HandleRequest(const JSONRPCRequest& request)
463+
{
464+
if (request.mode == JSONRPCRequest::GET_ARGS) {
465+
return GetArgMap();
466+
}
467+
/*
468+
* Check if the given request is valid according to this command or if
469+
* the user is asking for help information, and throw help when appropriate.
470+
*/
471+
if (request.mode == JSONRPCRequest::GET_HELP || !IsValidNumArgs(request.params.size())) {
472+
throw std::runtime_error(ToString());
473+
}
474+
return m_fun(*this, request);
475+
}
476+
462477
bool RPCHelpMan::IsValidNumArgs(size_t num_args) const
463478
{
464479
size_t num_required_args = 0;
@@ -532,8 +547,9 @@ std::string RPCHelpMan::ToString() const
532547
return ret;
533548
}
534549

535-
void RPCHelpMan::AppendArgMap(UniValue& arr) const
550+
UniValue RPCHelpMan::GetArgMap() const
536551
{
552+
UniValue arr{UniValue::VARR};
537553
for (int i{0}; i < int(m_args.size()); ++i) {
538554
const auto& arg = m_args.at(i);
539555
std::vector<std::string> arg_names;
@@ -548,6 +564,7 @@ void RPCHelpMan::AppendArgMap(UniValue& arr) const
548564
arr.push_back(map);
549565
}
550566
}
567+
return arr;
551568
}
552569

553570
std::string RPCArg::GetFirstName() const

‎src/rpc/util.h‎

Lines changed: 3 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -333,26 +333,12 @@ class RPCHelpMan
333333
using RPCMethodImpl = std::function<UniValue(const RPCHelpMan&, const JSONRPCRequest&)>;
334334
RPCHelpMan(std::string name, std::string description, std::vector<RPCArg> args, RPCResults results, RPCExamples examples, RPCMethodImpl fun);
335335

336+
UniValue HandleRequest(const JSONRPCRequest& request);
336337
std::string ToString() const;
337-
/** Append the named args that need to be converted from string to another JSON type */
338-
void AppendArgMap(UniValue& arr) const;
339-
UniValue HandleRequest(const JSONRPCRequest& request)
340-
{
341-
Check(request);
342-
return m_fun(*this, request);
343-
}
338+
/** Return the named args that need to be converted from string to another JSON type */
339+
UniValue GetArgMap() const;
344340
/** If the supplied number of args is neither too small nor too high */
345341
bool IsValidNumArgs(size_t num_args) const;
346-
/**
347-
* Check if the given request is valid according to this command or if
348-
* the user is asking for help information, and throw help when appropriate.
349-
*/
350-
inline void Check(const JSONRPCRequest& request) const {
351-
if (request.fHelp || !IsValidNumArgs(request.params.size())) {
352-
throw std::runtime_error(ToString());
353-
}
354-
}
355-
356342
std::vector<std::string> GetArgNames() const;
357343

358344
const std::string m_name;

‎src/test/rpc_tests.cpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,6 @@ UniValue RPCTestingSetup::CallRPC(std::string args)
3636
JSONRPCRequest request(context);
3737
request.strMethod = strMethod;
3838
request.params = RPCConvertValues(strMethod, vArgs);
39-
request.fHelp = false;
4039
if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished();
4140
try {
4241
UniValue result = tableRPC.execute(request);

‎src/wallet/rpcwallet.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,7 @@ bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string&
9696

9797
std::shared_ptr<CWallet> GetWalletForJSONRPCRequest(const JSONRPCRequest& request)
9898
{
99-
CHECK_NONFATAL(!request.fHelp);
99+
CHECK_NONFATAL(request.mode == JSONRPCRequest::EXECUTE);
100100
std::string wallet_name;
101101
if (GetWalletNameFromJSONRPCRequest(request, wallet_name)) {
102102
std::shared_ptr<CWallet> pwallet = GetWallet(wallet_name);

0 commit comments

Comments
 (0)