Remove use of CRPCTable::appendCommand in wallet code

This commit does not change behavior.

Author: ryanofsky

src/interfaces/chain.cpp

@@ -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>
@@ -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:
@@ -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>(); }

src/interfaces/chain.h

@@ -16,6 +16,7 @@
 
 class CBlock;
 class CFeeRate;
+class CRPCCommand;
 class CScheduler;
 class CValidationState;
 class uint256;
@@ -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

src/interfaces/wallet.cpp

@@ -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); }
@@ -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

src/rpc/server.cpp

@@ -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
 {
@@ -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);
@@ -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)
         {
@@ -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");
@@ -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)

src/rpc/server.h

@@ -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;
 };
 
 /**
@@ -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;
 
     /**
@@ -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);

src/test/rpc_tests.cpp

@@ -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) {

src/wallet/rpcwallet.cpp

@@ -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]));
 }

src/wallet/rpcwallet.h

@@ -5,7 +5,9 @@
 #ifndef BITCOIN_WALLET_RPCWALLET_H
 #define BITCOIN_WALLET_RPCWALLET_H
 
+#include <memory>
 #include <string>
+#include <vector>
 
 class CRPCTable;
 class CWallet;
@@ -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.

src/wallet/test/wallet_test_fixture.cpp

@@ -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();
 }

src/wallet/test/wallet_test_fixture.h

@@ -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;
 };