Skip to content

Commit 4e4d9e9

Browse files
ryanofskyryanofsky
committed
Remove use of CRPCTable::appendCommand in wallet code
This commit does not change behavior.
1 parent 91868e6 commit 4e4d9e9

File tree

10 files changed

+128
-40
lines changed

10 files changed

+128
-40
lines changed

‎src/interfaces/chain.cpp‎

Lines changed: 46 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,15 @@
1515
#include <primitives/block.h>
1616
#include <primitives/transaction.h>
1717
#include <protocol.h>
18+
#include <rpc/protocol.h>
19+
#include <rpc/server.h>
1820
#include <sync.h>
1921
#include <threadsafety.h>
2022
#include <timedata.h>
2123
#include <txmempool.h>
2224
#include <ui_interface.h>
2325
#include <uint256.h>
26+
#include <univalue.h>
2427
#include <util/system.h>
2528
#include <validation.h>
2629
#include <validationinterface.h>
@@ -212,6 +215,45 @@ class NotificationsHandlerImpl : public Handler, CValidationInterface
212215
Chain::Notifications* m_notifications;
213216
};
214217

218+
class RpcHandlerImpl : public Handler
219+
{
220+
public:
221+
RpcHandlerImpl(const CRPCCommand& command) : m_command(command), m_wrapped_command(&command)
222+
{
223+
m_command.actor = [this](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
224+
if (!m_wrapped_command) return false;
225+
try {
226+
return m_wrapped_command->actor(request, result, last_handler);
227+
} catch (const UniValue& e) {
228+
// If this is not the last handler and a wallet not found
229+
// exception was thrown, return false so the next handler can
230+
// try to handle the request. Otherwise, reraise the exception.
231+
if (!last_handler) {
232+
const UniValue& code = e["code"];
233+
if (code.isNum() && code.get_int() == RPC_WALLET_NOT_FOUND) {
234+
return false;
235+
}
236+
}
237+
throw;
238+
}
239+
};
240+
::tableRPC.appendCommand(m_command.name, &m_command);
241+
}
242+
243+
void disconnect() override final
244+
{
245+
if (m_wrapped_command) {
246+
m_wrapped_command = nullptr;
247+
::tableRPC.removeCommand(m_command.name, &m_command);
248+
}
249+
}
250+
251+
~RpcHandlerImpl() override { disconnect(); }
252+
253+
CRPCCommand m_command;
254+
const CRPCCommand* m_wrapped_command;
255+
};
256+
215257
class ChainImpl : public Chain
216258
{
217259
public:
@@ -310,8 +352,11 @@ class ChainImpl : public Chain
310352
return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
311353
}
312354
void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
355+
std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override
356+
{
357+
return MakeUnique<RpcHandlerImpl>(command);
358+
}
313359
};
314-
315360
} // namespace
316361

317362
std::unique_ptr<Chain> MakeChain() { return MakeUnique<ChainImpl>(); }

‎src/interfaces/chain.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
class CBlock;
1818
class CFeeRate;
19+
class CRPCCommand;
1920
class CScheduler;
2021
class CValidationState;
2122
class uint256;
@@ -243,6 +244,10 @@ class Chain
243244

244245
//! Wait for pending notifications to be handled.
245246
virtual void waitForNotifications() = 0;
247+
248+
//! Register handler for RPC. Command is not copied, so reference
249+
//! needs to remain valid until Handler is disconnected.
250+
virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
246251
};
247252

248253
//! Interface to let node manage chain clients (wallets, or maybe tools for

‎src/interfaces/wallet.cpp‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -514,7 +514,7 @@ class WalletClientImpl : public ChainClient
514514
: m_chain(chain), m_wallet_filenames(std::move(wallet_filenames))
515515
{
516516
}
517-
void registerRpcs() override { return RegisterWalletRPCCommands(::tableRPC); }
517+
void registerRpcs() override { return RegisterWalletRPCCommands(m_chain, m_rpc_handlers); }
518518
bool verify() override { return VerifyWallets(m_chain, m_wallet_filenames); }
519519
bool load() override { return LoadWallets(m_chain, m_wallet_filenames); }
520520
void start(CScheduler& scheduler) override { return StartWallets(scheduler); }
@@ -524,6 +524,7 @@ class WalletClientImpl : public ChainClient
524524

525525
Chain& m_chain;
526526
std::vector<std::string> m_wallet_filenames;
527+
std::vector<std::unique_ptr<Handler>> m_rpc_handlers;
527528
};
528529

529530
} // namespace

‎src/rpc/server.cpp‎

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,7 @@ static std::string rpcWarmupStatus GUARDED_BY(cs_rpcWarmup) = "RPC server starte
3030
static RPCTimerInterface* timerInterface = nullptr;
3131
/* Map of name to timer. */
3232
static std::map<std::string, std::unique_ptr<RPCTimerBase> > deadlineTimers;
33+
static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler);
3334

3435
struct RPCCommandExecutionInfo
3536
{
@@ -173,11 +174,11 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
173174
{
174175
std::string strRet;
175176
std::string category;
176-
std::set<rpcfn_type> setDone;
177+
std::set<intptr_t> setDone;
177178
std::vector<std::pair<std::string, const CRPCCommand*> > vCommands;
178179

179180
for (const auto& entry : mapCommands)
180-
vCommands.push_back(make_pair(entry.second->category + entry.first, entry.second));
181+
vCommands.push_back(make_pair(entry.second.front()->category + entry.first, entry.second.front()));
181182
sort(vCommands.begin(), vCommands.end());
182183

183184
JSONRPCRequest jreq(helpreq);
@@ -193,9 +194,9 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
193194
jreq.strMethod = strMethod;
194195
try
195196
{
196-
rpcfn_type pfn = pcmd->actor;
197-
if (setDone.insert(pfn).second)
198-
(*pfn)(jreq);
197+
UniValue unused_result;
198+
if (setDone.insert(pcmd->unique_id).second)
199+
pcmd->actor(jreq, unused_result, true /* last_handler */);
199200
}
200201
catch (const std::exception& e)
201202
{
@@ -337,32 +338,32 @@ CRPCTable::CRPCTable()
337338
const CRPCCommand *pcmd;
338339

339340
pcmd = &vRPCCommands[vcidx];
340-
mapCommands[pcmd->name] = pcmd;
341+
mapCommands[pcmd->name].push_back(pcmd);
341342
}
342343
}
343344

344-
const CRPCCommand *CRPCTable::operator[](const std::string &name) const
345-
{
346-
std::map<std::string, const CRPCCommand*>::const_iterator it = mapCommands.find(name);
347-
if (it == mapCommands.end())
348-
return nullptr;
349-
return (*it).second;
350-
}
351-
352345
bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
353346
{
354347
if (IsRPCRunning())
355348
return false;
356349

357-
// don't allow overwriting for now
358-
std::map<std::string, const CRPCCommand*>::const_iterator it = mapCommands.find(name);
359-
if (it != mapCommands.end())
360-
return false;
361-
362-
mapCommands[name] = pcmd;
350+
mapCommands[name].push_back(pcmd);
363351
return true;
364352
}
365353

354+
bool CRPCTable::removeCommand(const std::string& name, const CRPCCommand* pcmd)
355+
{
356+
auto it = mapCommands.find(name);
357+
if (it != mapCommands.end()) {
358+
auto new_end = std::remove(it->second.begin(), it->second.end(), pcmd);
359+
if (it->second.end() != new_end) {
360+
it->second.erase(new_end, it->second.end());
361+
return true;
362+
}
363+
}
364+
return false;
365+
}
366+
366367
void StartRPC()
367368
{
368369
LogPrint(BCLog::RPC, "Starting RPC\n");
@@ -543,18 +544,28 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
543544
}
544545

545546
// Find method
546-
const CRPCCommand *pcmd = tableRPC[request.strMethod];
547-
if (!pcmd)
548-
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
547+
auto it = mapCommands.find(request.strMethod);
548+
if (it != mapCommands.end()) {
549+
UniValue result;
550+
for (const auto& command : it->second) {
551+
if (ExecuteCommand(*command, request, result, &command == &it->second.back())) {
552+
return result;
553+
}
554+
}
555+
}
556+
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
557+
}
549558

559+
static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler)
560+
{
550561
try
551562
{
552563
RPCCommandExecution execution(request.strMethod);
553564
// Execute, convert arguments to array if necessary
554565
if (request.params.isObject()) {
555-
return pcmd->actor(transformNamedArguments(request, pcmd->argNames));
566+
return command.actor(transformNamedArguments(request, command.argNames), result, last_handler);
556567
} else {
557-
return pcmd->actor(request);
568+
return command.actor(request, result, last_handler);
558569
}
559570
}
560571
catch (const std::exception& e)

‎src/rpc/server.h‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -131,10 +131,31 @@ typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
131131
class CRPCCommand
132132
{
133133
public:
134+
//! RPC method handler reading request and assigning result. Should return
135+
//! true if request is fully handled, false if it should be passed on to
136+
//! subsequent handlers.
137+
using Actor = std::function<bool(const JSONRPCRequest& request, UniValue& result, bool last_handler)>;
138+
139+
//! Constructor taking Actor callback supporting multiple handlers.
140+
CRPCCommand(std::string category, std::string name, Actor actor, std::vector<std::string> args, intptr_t unique_id)
141+
: category(std::move(category)), name(std::move(name)), actor(std::move(actor)), argNames(std::move(args)),
142+
unique_id(unique_id)
143+
{
144+
}
145+
146+
//! Simplified constructor taking plain rpcfn_type function pointer.
147+
CRPCCommand(const char* category, const char* name, rpcfn_type fn, std::initializer_list<const char*> args)
148+
: CRPCCommand(category, name,
149+
[fn](const JSONRPCRequest& request, UniValue& result, bool) { result = fn(request); return true; },
150+
{args.begin(), args.end()}, intptr_t(fn))
151+
{
152+
}
153+
134154
std::string category;
135155
std::string name;
136-
rpcfn_type actor;
156+
Actor actor;
137157
std::vector<std::string> argNames;
158+
intptr_t unique_id;
138159
};
139160

140161
/**
@@ -143,10 +164,9 @@ class CRPCCommand
143164
class CRPCTable
144165
{
145166
private:
146-
std::map<std::string, const CRPCCommand*> mapCommands;
167+
std::map<std::string, std::vector<const CRPCCommand*>> mapCommands;
147168
public:
148169
CRPCTable();
149-
const CRPCCommand* operator[](const std::string& name) const;
150170
std::string help(const std::string& name, const JSONRPCRequest& helpreq) const;
151171

152172
/**
@@ -169,16 +189,15 @@ class CRPCTable
169189
*
170190
* Returns false if RPC server is already running (dump concurrency protection).
171191
*
172-
* Commands cannot be overwritten (returns false).
173-
*
174-
* Commands with different method names but the same callback function will
192+
* Commands with different method names but the same unique_id will
175193
* be considered aliases, and only the first registered method name will
176194
* show up in the help text command listing. Aliased commands do not have
177195
* to have the same behavior. Server and client code can distinguish
178196
* between calls based on method name, and aliased commands can also
179197
* register different names, types, and numbers of parameters.
180198
*/
181199
bool appendCommand(const std::string& name, const CRPCCommand* pcmd);
200+
bool removeCommand(const std::string& name, const CRPCCommand* pcmd);
182201
};
183202

184203
bool IsDeprecatedRPCEnabled(const std::string& method);

‎src/test/rpc_tests.cpp‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,10 +31,9 @@ UniValue CallRPC(std::string args)
3131
request.strMethod = strMethod;
3232
request.params = RPCConvertValues(strMethod, vArgs);
3333
request.fHelp = false;
34-
BOOST_CHECK(tableRPC[strMethod]);
35-
rpcfn_type method = tableRPC[strMethod]->actor;
34+
if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished();
3635
try {
37-
UniValue result = (*method)(request);
36+
UniValue result = tableRPC.execute(request);
3837
return result;
3938
}
4039
catch (const UniValue& objError) {

‎src/wallet/rpcwallet.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4156,8 +4156,8 @@ static const CRPCCommand commands[] =
41564156
};
41574157
// clang-format on
41584158

4159-
void RegisterWalletRPCCommands(CRPCTable &t)
4159+
void RegisterWalletRPCCommands(interfaces::Chain& chain, std::vector<std::unique_ptr<interfaces::Handler>>& handlers)
41604160
{
41614161
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
4162-
t.appendCommand(commands[vcidx].name, &commands[vcidx]);
4162+
handlers.emplace_back(chain.handleRpc(commands[vcidx]));
41634163
}

‎src/wallet/rpcwallet.h‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,9 @@
55
#ifndef BITCOIN_WALLET_RPCWALLET_H
66
#define BITCOIN_WALLET_RPCWALLET_H
77

8+
#include <memory>
89
#include <string>
10+
#include <vector>
911

1012
class CRPCTable;
1113
class CWallet;
@@ -14,7 +16,12 @@ class UniValue;
1416
struct PartiallySignedTransaction;
1517
class CTransaction;
1618

17-
void RegisterWalletRPCCommands(CRPCTable &t);
19+
namespace interfaces {
20+
class Chain;
21+
class Handler;
22+
}
23+
24+
void RegisterWalletRPCCommands(interfaces::Chain& chain, std::vector<std::unique_ptr<interfaces::Handler>>& handlers);
1825

1926
/**
2027
* Figures out what wallet, if any, to use for a JSONRPCRequest.

‎src/wallet/test/wallet_test_fixture.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,5 +15,5 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName):
1515
m_wallet.LoadWallet(fFirstRun);
1616
m_wallet.m_chain_notifications_handler = m_chain->handleNotifications(m_wallet);
1717

18-
RegisterWalletRPCCommands(tableRPC);
18+
m_chain_client->registerRpcs();
1919
}

‎src/wallet/test/wallet_test_fixture.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ struct WalletTestingSetup: public TestingSetup {
1919
explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);
2020

2121
std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain();
22+
std::unique_ptr<interfaces::ChainClient> m_chain_client = interfaces::MakeWalletClient(*m_chain, {});
2223
CWallet m_wallet;
2324
};
2425

0 commit comments

Comments
 (0)