Merge #10973: Refactor: separate wallet from node

d358466de Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b2389 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9f8 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e628 Remove use CValidationInterface in wallet code (Russell Yanofsky)

Pull request description:

  This PR is the last in a chain of PRs (#14437, #14711, and #15288) that make the wallet code access node state through an abstract [`Chain`](https://github.com/ryanofsky/bitcoin/blob/pr/wipc-sep/src/interfaces/chain.h) class in [`src/interfaces/`](https://github.com/ryanofsky/bitcoin/tree/pr/wipc-sep/src/interfaces) instead of using global variables like `cs_main`, `chainActive`, and `g_connman`. After this PR, wallet code no longer accesses global variables declared outside the wallet directory, and no longer calls functions accessing those globals (as verified by the `hide-globals` script in #10244).

  This PR and the previous PRs have been refactoring changes that do not affect behavior. Previous PRs have consisted of lots of mechanical changes like:

  ```diff
  -    wtx.nTimeReceived = GetAdjustedTime();
  +    wtx.nTimeReceived = m_chain->getAdjustedTime();
  ```

  This PR is smaller, but less mechanical. It replaces last few bits of wallet code that access node state directly (through `CValidationInterface`, `CRPCTable`, and `CCoinsViewMemPool` interfaces) with code that uses the `Chain` interface.

  These changes allow followup PR #10102 (multiprocess gui & wallet PR) to work without any significant updates to wallet code. Additionally they:

  * Provide a single place to describe the interface between wallet and node code.
  * Can make better wallet testing possible, because the `Chain` object consists of virtual methods that can be overloaded for mocking. (This could be used to test edge cases in the rescan code, for example).

Tree-SHA512: e6291d8a3c50bdff18a9c8ad11e729beb30b5b7040d7aaf31ba678800b4a97b2dd2be76340b1e5c01fe2827d67d37ed1bb4c8380cf8ed653aadfea003e9b22e7

3 files changed, 82 insertions, 54 deletions

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
index 1d58725a58..0cd1494512 100644
--- a/src/rpc/rawtransaction.cpp
+++ b/src/rpc/rawtransaction.cpp
@@ -11,6 +11,7 @@
 #include <core_io.h>
 #include <index/txindex.h>
 #include <init.h>
+#include <interfaces/chain.h>
 #include <key_io.h>
 #include <keystore.h>
 #include <merkleblock.h>
@@ -791,23 +792,20 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
     return EncodeHexTx(CTransaction(mergedTx));
 }
 
+// TODO(https://github.com/bitcoin/bitcoin/pull/10973#discussion_r267084237):
+// This function is called from both wallet and node rpcs
+// (signrawtransactionwithwallet and signrawtransactionwithkey). It should be
+// moved to a util file so wallet code doesn't need to link against node code.
+// Also the dependency on interfaces::Chain should be removed, so
+// signrawtransactionwithkey doesn't need access to a Chain instance.
 UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, const UniValue& prevTxsUnival, CBasicKeyStore *keystore, bool is_temp_keystore, const UniValue& hashType)
 {
     // Fetch previous transactions (inputs):
-    CCoinsView viewDummy;
-    CCoinsViewCache view(&viewDummy);
-    {
-        LOCK2(cs_main, mempool.cs);
-        CCoinsViewCache &viewChain = *pcoinsTip;
-        CCoinsViewMemPool viewMempool(&viewChain, mempool);
-        view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view
-
-        for (const CTxIn& txin : mtx.vin) {
-            view.AccessCoin(txin.prevout); // Load entries from viewChain into view; can fail.
-        }
-
-        view.SetBackend(viewDummy); // switch back to avoid locking mempool for too long
+    std::map<COutPoint, Coin> coins;
+    for (const CTxIn& txin : mtx.vin) {
+        coins[txin.prevout]; // Create empty map entry keyed by prevout.
     }
+    chain.findCoins(coins);
 
     // Add previous txouts given in the RPC call:
     if (!prevTxsUnival.isNull()) {
@@ -839,10 +837,10 @@ UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, con
             CScript scriptPubKey(pkData.begin(), pkData.end());
 
             {
-                const Coin& coin = view.AccessCoin(out);
-                if (!coin.IsSpent() && coin.out.scriptPubKey != scriptPubKey) {
+                auto coin = coins.find(out);
+                if (coin != coins.end() && !coin->second.IsSpent() && coin->second.out.scriptPubKey != scriptPubKey) {
                     std::string err("Previous output scriptPubKey mismatch:\n");
-                    err = err + ScriptToAsmStr(coin.out.scriptPubKey) + "\nvs:\n"+
+                    err = err + ScriptToAsmStr(coin->second.out.scriptPubKey) + "\nvs:\n"+
                         ScriptToAsmStr(scriptPubKey);
                     throw JSONRPCError(RPC_DESERIALIZATION_ERROR, err);
                 }
@@ -853,7 +851,7 @@ UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, con
                     newcoin.out.nValue = AmountFromValue(find_value(prevOut, "amount"));
                 }
                 newcoin.nHeight = 1;
-                view.AddCoin(out, std::move(newcoin), true);
+                coins[out] = std::move(newcoin);
             }
 
             // if redeemScript and private keys were given, add redeemScript to the keystore so it can be signed
@@ -897,15 +895,15 @@ UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, con
     // Sign what we can:
     for (unsigned int i = 0; i < mtx.vin.size(); i++) {
         CTxIn& txin = mtx.vin[i];
-        const Coin& coin = view.AccessCoin(txin.prevout);
-        if (coin.IsSpent()) {
+        auto coin = coins.find(txin.prevout);
+        if (coin == coins.end() || coin->second.IsSpent()) {
             TxInErrorToJSON(txin, vErrors, "Input not found or already spent");
             continue;
         }
-        const CScript& prevPubKey = coin.out.scriptPubKey;
-        const CAmount& amount = coin.out.nValue;
+        const CScript& prevPubKey = coin->second.out.scriptPubKey;
+        const CAmount& amount = coin->second.out.nValue;
 
-        SignatureData sigdata = DataFromTransaction(mtx, i, coin.out);
+        SignatureData sigdata = DataFromTransaction(mtx, i, coin->second.out);
         // Only sign SIGHASH_SINGLE if there's a corresponding output:
         if (!fHashSingle || (i < mtx.vout.size())) {
             ProduceSignature(*keystore, MutableTransactionSignatureCreator(&mtx, i, amount, nHashType), prevPubKey, sigdata);
@@ -915,7 +913,7 @@ UniValue SignTransaction(interfaces::Chain& chain, CMutableTransaction& mtx, con
 
         // amount must be specified for valid segwit signature
         if (amount == MAX_MONEY && !txin.scriptWitness.IsNull()) {
-            throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing amount for %s", coin.out.ToString()));
+            throw JSONRPCError(RPC_TYPE_ERROR, strprintf("Missing amount for %s", coin->second.out.ToString()));
         }
 
         ScriptError serror = SCRIPT_ERR_OK;
diff --git a/src/rpc/server.cpp b/src/rpc/server.cpp
index cd90573da0..e803fabcc6 100644
--- a/src/rpc/server.cpp
+++ b/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)
diff --git a/src/rpc/server.h b/src/rpc/server.h
index 2d62a76f3c..e2a85887ba 100644
--- a/src/rpc/server.h
+++ b/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 @@ public:
 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,9 +189,7 @@ public:
      *
      * 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
@@ -179,6 +197,7 @@ public:
      * 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);