Skip to content

Commit

Permalink
Remove use of CRPCTable::appendCommand in wallet code
Browse files Browse the repository at this point in the history
This commit does not change behavior.
  • Loading branch information
ryanofsky committed Mar 5, 2019
1 parent 91868e6 commit 4e4d9e9
Show file tree
Hide file tree
Showing 10 changed files with 128 additions and 40 deletions.
47 changes: 46 additions & 1 deletion src/interfaces/chain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,15 @@
#include <primitives/block.h>
#include <primitives/transaction.h>
#include <protocol.h>
#include <rpc/protocol.h>
#include <rpc/server.h>
#include <sync.h>
#include <threadsafety.h>
#include <timedata.h>
#include <txmempool.h>
#include <ui_interface.h>
#include <uint256.h>
#include <univalue.h>
#include <util/system.h>
#include <validation.h>
#include <validationinterface.h>
Expand Down Expand Up @@ -212,6 +215,45 @@ class NotificationsHandlerImpl : public Handler, CValidationInterface
Chain::Notifications* m_notifications;
};

class RpcHandlerImpl : public Handler
{
public:
RpcHandlerImpl(const CRPCCommand& command) : m_command(command), m_wrapped_command(&command)
{
m_command.actor = [this](const JSONRPCRequest& request, UniValue& result, bool last_handler) {
if (!m_wrapped_command) return false;
try {
return m_wrapped_command->actor(request, result, last_handler);
} catch (const UniValue& e) {
// If this is not the last handler and a wallet not found
// exception was thrown, return false so the next handler can
// try to handle the request. Otherwise, reraise the exception.
if (!last_handler) {
const UniValue& code = e["code"];
if (code.isNum() && code.get_int() == RPC_WALLET_NOT_FOUND) {
return false;
}
}
throw;
}
};
::tableRPC.appendCommand(m_command.name, &m_command);
}

void disconnect() override final
{
if (m_wrapped_command) {
m_wrapped_command = nullptr;
::tableRPC.removeCommand(m_command.name, &m_command);
}
}

~RpcHandlerImpl() override { disconnect(); }

CRPCCommand m_command;
const CRPCCommand* m_wrapped_command;
};

class ChainImpl : public Chain
{
public:
Expand Down Expand Up @@ -310,8 +352,11 @@ class ChainImpl : public Chain
return MakeUnique<NotificationsHandlerImpl>(*this, notifications);
}
void waitForNotifications() override { SyncWithValidationInterfaceQueue(); }
std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) override
{
return MakeUnique<RpcHandlerImpl>(command);
}
};

} // namespace

std::unique_ptr<Chain> MakeChain() { return MakeUnique<ChainImpl>(); }
Expand Down
5 changes: 5 additions & 0 deletions src/interfaces/chain.h
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

class CBlock;
class CFeeRate;
class CRPCCommand;
class CScheduler;
class CValidationState;
class uint256;
Expand Down Expand Up @@ -243,6 +244,10 @@ class Chain

//! Wait for pending notifications to be handled.
virtual void waitForNotifications() = 0;

//! Register handler for RPC. Command is not copied, so reference
//! needs to remain valid until Handler is disconnected.
virtual std::unique_ptr<Handler> handleRpc(const CRPCCommand& command) = 0;
};

//! Interface to let node manage chain clients (wallets, or maybe tools for
Expand Down
3 changes: 2 additions & 1 deletion src/interfaces/wallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ class WalletClientImpl : public ChainClient
: m_chain(chain), m_wallet_filenames(std::move(wallet_filenames))
{
}
void registerRpcs() override { return RegisterWalletRPCCommands(::tableRPC); }
void registerRpcs() override { return RegisterWalletRPCCommands(m_chain, m_rpc_handlers); }
bool verify() override { return VerifyWallets(m_chain, m_wallet_filenames); }
bool load() override { return LoadWallets(m_chain, m_wallet_filenames); }
void start(CScheduler& scheduler) override { return StartWallets(scheduler); }
Expand All @@ -524,6 +524,7 @@ class WalletClientImpl : public ChainClient

Chain& m_chain;
std::vector<std::string> m_wallet_filenames;
std::vector<std::unique_ptr<Handler>> m_rpc_handlers;
};

} // namespace
Expand Down
61 changes: 36 additions & 25 deletions src/rpc/server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ static std::string rpcWarmupStatus GUARDED_BY(cs_rpcWarmup) = "RPC server starte
static RPCTimerInterface* timerInterface = nullptr;
/* Map of name to timer. */
static std::map<std::string, std::unique_ptr<RPCTimerBase> > deadlineTimers;
static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler);

struct RPCCommandExecutionInfo
{
Expand Down Expand Up @@ -173,11 +174,11 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
{
std::string strRet;
std::string category;
std::set<rpcfn_type> setDone;
std::set<intptr_t> setDone;
std::vector<std::pair<std::string, const CRPCCommand*> > vCommands;

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

JSONRPCRequest jreq(helpreq);
Expand All @@ -193,9 +194,9 @@ std::string CRPCTable::help(const std::string& strCommand, const JSONRPCRequest&
jreq.strMethod = strMethod;
try
{
rpcfn_type pfn = pcmd->actor;
if (setDone.insert(pfn).second)
(*pfn)(jreq);
UniValue unused_result;
if (setDone.insert(pcmd->unique_id).second)
pcmd->actor(jreq, unused_result, true /* last_handler */);
}
catch (const std::exception& e)
{
Expand Down Expand Up @@ -337,32 +338,32 @@ CRPCTable::CRPCTable()
const CRPCCommand *pcmd;

pcmd = &vRPCCommands[vcidx];
mapCommands[pcmd->name] = pcmd;
mapCommands[pcmd->name].push_back(pcmd);
}
}

const CRPCCommand *CRPCTable::operator[](const std::string &name) const
{
std::map<std::string, const CRPCCommand*>::const_iterator it = mapCommands.find(name);
if (it == mapCommands.end())
return nullptr;
return (*it).second;
}

bool CRPCTable::appendCommand(const std::string& name, const CRPCCommand* pcmd)
{
if (IsRPCRunning())
return false;

// don't allow overwriting for now
std::map<std::string, const CRPCCommand*>::const_iterator it = mapCommands.find(name);
if (it != mapCommands.end())
return false;

mapCommands[name] = pcmd;
mapCommands[name].push_back(pcmd);
return true;
}

bool CRPCTable::removeCommand(const std::string& name, const CRPCCommand* pcmd)
{
auto it = mapCommands.find(name);
if (it != mapCommands.end()) {
auto new_end = std::remove(it->second.begin(), it->second.end(), pcmd);
if (it->second.end() != new_end) {
it->second.erase(new_end, it->second.end());
return true;
}
}
return false;
}

void StartRPC()
{
LogPrint(BCLog::RPC, "Starting RPC\n");
Expand Down Expand Up @@ -543,18 +544,28 @@ UniValue CRPCTable::execute(const JSONRPCRequest &request) const
}

// Find method
const CRPCCommand *pcmd = tableRPC[request.strMethod];
if (!pcmd)
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
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;
}
}
}
throw JSONRPCError(RPC_METHOD_NOT_FOUND, "Method not found");
}

static bool ExecuteCommand(const CRPCCommand& command, const JSONRPCRequest& request, UniValue& result, bool last_handler)
{
try
{
RPCCommandExecution execution(request.strMethod);
// Execute, convert arguments to array if necessary
if (request.params.isObject()) {
return pcmd->actor(transformNamedArguments(request, pcmd->argNames));
return command.actor(transformNamedArguments(request, command.argNames), result, last_handler);
} else {
return pcmd->actor(request);
return command.actor(request, result, last_handler);
}
}
catch (const std::exception& e)
Expand Down
31 changes: 25 additions & 6 deletions src/rpc/server.h
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,31 @@ typedef UniValue(*rpcfn_type)(const JSONRPCRequest& jsonRequest);
class CRPCCommand
{
public:
//! RPC method handler reading request and assigning result. Should return
//! true if request is fully handled, false if it should be passed on to
//! subsequent handlers.
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)),
unique_id(unique_id)
{
}

//! 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,
[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;
rpcfn_type actor;
Actor actor;
std::vector<std::string> argNames;
intptr_t unique_id;
};

/**
Expand All @@ -143,10 +164,9 @@ class CRPCCommand
class CRPCTable
{
private:
std::map<std::string, const CRPCCommand*> mapCommands;
std::map<std::string, std::vector<const CRPCCommand*>> mapCommands;
public:
CRPCTable();
const CRPCCommand* operator[](const std::string& name) const;
std::string help(const std::string& name, const JSONRPCRequest& helpreq) const;

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

bool IsDeprecatedRPCEnabled(const std::string& method);
Expand Down
5 changes: 2 additions & 3 deletions src/test/rpc_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,9 @@ UniValue CallRPC(std::string args)
request.strMethod = strMethod;
request.params = RPCConvertValues(strMethod, vArgs);
request.fHelp = false;
BOOST_CHECK(tableRPC[strMethod]);
rpcfn_type method = tableRPC[strMethod]->actor;
if (RPCIsInWarmup(nullptr)) SetRPCWarmupFinished();
try {
UniValue result = (*method)(request);
UniValue result = tableRPC.execute(request);
return result;
}
catch (const UniValue& objError) {
Expand Down
4 changes: 2 additions & 2 deletions src/wallet/rpcwallet.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4156,8 +4156,8 @@ static const CRPCCommand commands[] =
};
// clang-format on

void RegisterWalletRPCCommands(CRPCTable &t)
void RegisterWalletRPCCommands(interfaces::Chain& chain, std::vector<std::unique_ptr<interfaces::Handler>>& handlers)
{
for (unsigned int vcidx = 0; vcidx < ARRAYLEN(commands); vcidx++)
t.appendCommand(commands[vcidx].name, &commands[vcidx]);
handlers.emplace_back(chain.handleRpc(commands[vcidx]));
}
9 changes: 8 additions & 1 deletion src/wallet/rpcwallet.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,9 @@
#ifndef BITCOIN_WALLET_RPCWALLET_H
#define BITCOIN_WALLET_RPCWALLET_H

#include <memory>
#include <string>
#include <vector>

class CRPCTable;
class CWallet;
Expand All @@ -14,7 +16,12 @@ class UniValue;
struct PartiallySignedTransaction;
class CTransaction;

void RegisterWalletRPCCommands(CRPCTable &t);
namespace interfaces {
class Chain;
class Handler;
}

void RegisterWalletRPCCommands(interfaces::Chain& chain, std::vector<std::unique_ptr<interfaces::Handler>>& handlers);

/**
* Figures out what wallet, if any, to use for a JSONRPCRequest.
Expand Down
2 changes: 1 addition & 1 deletion src/wallet/test/wallet_test_fixture.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,5 @@ WalletTestingSetup::WalletTestingSetup(const std::string& chainName):
m_wallet.LoadWallet(fFirstRun);
m_wallet.m_chain_notifications_handler = m_chain->handleNotifications(m_wallet);

RegisterWalletRPCCommands(tableRPC);
m_chain_client->registerRpcs();
}
1 change: 1 addition & 0 deletions src/wallet/test/wallet_test_fixture.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ struct WalletTestingSetup: public TestingSetup {
explicit WalletTestingSetup(const std::string& chainName = CBaseChainParams::MAIN);

std::unique_ptr<interfaces::Chain> m_chain = interfaces::MakeChain();
std::unique_ptr<interfaces::ChainClient> m_chain_client = interfaces::MakeWalletClient(*m_chain, {});
CWallet m_wallet;
};

Expand Down

0 comments on commit 4e4d9e9

Please sign in to comment.