Merge #10973: Refactor: separate wallet from node

d358466 Remove remaining wallet accesses to node globals (Russell Yanofsky)
b1b2b23 Remove use of CCoinsViewMemPool::GetCoin in wallet code (Russell Yanofsky)
4e4d9e9 Remove use of CRPCTable::appendCommand in wallet code (Russell Yanofsky)
91868e6 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

Author: meshcollider

src/Makefile.am

@@ -154,6 +154,7 @@ BITCOIN_CORE_H = \
   netaddress.h \
   netbase.h \
   netmessagemaker.h \
+  node/coin.h \
   node/transaction.h \
   noui.h \
   optional.h \
@@ -262,6 +263,7 @@ libbitcoin_server_a_SOURCES = \
   miner.cpp \
   net.cpp \
   net_processing.cpp \
+  node/coin.cpp \
   node/transaction.cpp \
   noui.cpp \
   outputtype.cpp \

src/interfaces/chain.cpp

@@ -6,22 +6,29 @@
 
 #include <chain.h>
 #include <chainparams.h>
+#include <interfaces/handler.h>
 #include <interfaces/wallet.h>
 #include <net.h>
+#include <node/coin.h>
 #include <policy/fees.h>
 #include <policy/policy.h>
 #include <policy/rbf.h>
 #include <primitives/block.h>
 #include <primitives/transaction.h>
 #include <protocol.h>
+#include <rpc/protocol.h>
+#include <rpc/server.h>
+#include <shutdown.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>
 
 #include <memory>
 #include <utility>
@@ -161,6 +168,94 @@ class LockingStateImpl : public LockImpl, public UniqueLock<CCriticalSection>
     using UniqueLock::UniqueLock;
 };
 
+class NotificationsHandlerImpl : public Handler, CValidationInterface
+{
+public:
+    explicit NotificationsHandlerImpl(Chain& chain, Chain::Notifications& notifications)
+        : m_chain(chain), m_notifications(&notifications)
+    {
+        RegisterValidationInterface(this);
+    }
+    ~NotificationsHandlerImpl() override { disconnect(); }
+    void disconnect() override
+    {
+        if (m_notifications) {
+            m_notifications = nullptr;
+            UnregisterValidationInterface(this);
+        }
+    }
+    void TransactionAddedToMempool(const CTransactionRef& tx) override
+    {
+        m_notifications->TransactionAddedToMempool(tx);
+    }
+    void TransactionRemovedFromMempool(const CTransactionRef& tx) override
+    {
+        m_notifications->TransactionRemovedFromMempool(tx);
+    }
+    void BlockConnected(const std::shared_ptr<const CBlock>& block,
+        const CBlockIndex* index,
+        const std::vector<CTransactionRef>& tx_conflicted) override
+    {
+        m_notifications->BlockConnected(*block, tx_conflicted);
+    }
+    void BlockDisconnected(const std::shared_ptr<const CBlock>& block) override
+    {
+        m_notifications->BlockDisconnected(*block);
+    }
+    void ChainStateFlushed(const CBlockLocator& locator) override { m_notifications->ChainStateFlushed(locator); }
+    void ResendWalletTransactions(int64_t best_block_time, CConnman*) override
+    {
+        // `cs_main` is always held when this method is called, so it is safe to
+        // call `assumeLocked`. This is awkward, and the `assumeLocked` method
+        // should be able to be removed entirely if `ResendWalletTransactions`
+        // is replaced by a wallet timer as suggested in
+        // https://github.com/bitcoin/bitcoin/issues/15619
+        auto locked_chain = m_chain.assumeLocked();
+        m_notifications->ResendWalletTransactions(*locked_chain, best_block_time);
+    }
+    Chain& m_chain;
+    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:
@@ -194,6 +289,7 @@ class ChainImpl : public Chain
         }
         return true;
     }
+    void findCoins(std::map<COutPoint, Coin>& coins) override { return FindCoins(coins); }
     double guessVerificationProgress(const uint256& block_hash) override
     {
         LOCK(cs_main);
@@ -245,17 +341,33 @@ class ChainImpl : public Chain
     {
         return ::mempool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000);
     }
+    CFeeRate relayMinFee() override { return ::minRelayTxFee; }
+    CFeeRate relayIncrementalFee() override { return ::incrementalRelayFee; }
+    CFeeRate relayDustFee() override { return ::dustRelayFee; }
     CAmount maxTxFee() override { return ::maxTxFee; }
     bool getPruneMode() override { return ::fPruneMode; }
     bool p2pEnabled() override { return g_connman != nullptr; }
     bool isInitialBlockDownload() override { return IsInitialBlockDownload(); }
+    bool shutdownRequested() override { return ShutdownRequested(); }
     int64_t getAdjustedTime() override { return GetAdjustedTime(); }
     void initMessage(const std::string& message) override { ::uiInterface.InitMessage(message); }
     void initWarning(const std::string& message) override { InitWarning(message); }
     void initError(const std::string& message) override { InitError(message); }
     void loadWallet(std::unique_ptr<Wallet> wallet) override { ::uiInterface.LoadWallet(wallet); }
+    void showProgress(const std::string& title, int progress, bool resume_possible) override
+    {
+        ::uiInterface.ShowProgress(title, progress, resume_possible);
+    }
+    std::unique_ptr<Handler> handleNotifications(Notifications& notifications) override
+    {
+        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,15 +16,18 @@
 
 class CBlock;
 class CFeeRate;
+class CRPCCommand;
 class CScheduler;
 class CValidationState;
+class Coin;
 class uint256;
 enum class RBFTransactionState;
 struct CBlockLocator;
 struct FeeCalculation;
 
 namespace interfaces {
 
+class Handler;
 class Wallet;
 
 //! Interface giving clients (wallet processes, maybe other analysis tools in
@@ -40,6 +43,12 @@ class Wallet;
 //!   asynchronously
 //!   (https://github.com/bitcoin/bitcoin/pull/10973#issuecomment-380101269).
 //!
+//! * The isPotentialTip() and waitForNotifications() methods are too low-level
+//!   and should be replaced with a higher level
+//!   waitForNotificationsUpTo(block_hash) method that the wallet can call
+//!   instead
+//!   (https://github.com/bitcoin/bitcoin/pull/10973#discussion_r266995234).
+//!
 //! * The relayTransactions() and submitToMemoryPool() methods could be replaced
 //!   with a higher-level broadcastTransaction method
 //!   (https://github.com/bitcoin/bitcoin/pull/14978#issuecomment-459373984).
@@ -160,6 +169,11 @@ class Chain
         int64_t* time = nullptr,
         int64_t* max_time = nullptr) = 0;
 
+    //! Look up unspent output information. Returns coins in the mempool and in
+    //! the current chain UTXO set. Iterates through all the keys in the map and
+    //! populates the values.
+    virtual void findCoins(std::map<COutPoint, Coin>& coins) = 0;
+
     //! Estimate fraction of total transactions verified if blocks up to
     //! the specified block hash are verified.
     virtual double guessVerificationProgress(const uint256& block_hash) = 0;
@@ -188,6 +202,15 @@ class Chain
     //! Mempool minimum fee.
     virtual CFeeRate mempoolMinFee() = 0;
 
+    //! Relay current minimum fee (from -minrelaytxfee and -incrementalrelayfee settings).
+    virtual CFeeRate relayMinFee() = 0;
+
+    //! Relay incremental fee setting (-incrementalrelayfee), reflecting cost of relay.
+    virtual CFeeRate relayIncrementalFee() = 0;
+
+    //! Relay dust fee setting (-dustrelayfee), reflecting lowest rate it's economical to spend.
+    virtual CFeeRate relayDustFee() = 0;
+
     //! Node max tx fee setting (-maxtxfee).
     //! This could be replaced by a per-wallet max fee, as proposed at
     //! https://github.com/bitcoin/bitcoin/issues/15355
@@ -200,9 +223,12 @@ class Chain
     //! Check if p2p enabled.
     virtual bool p2pEnabled() = 0;
 
-    // Check if in IBD.
+    //! Check if in IBD.
     virtual bool isInitialBlockDownload() = 0;
 
+    //! Check if shutdown requested.
+    virtual bool shutdownRequested() = 0;
+
     //! Get adjusted time.
     virtual int64_t getAdjustedTime() = 0;
 
@@ -217,6 +243,32 @@ class Chain
 
     //! Send wallet load notification to the GUI.
     virtual void loadWallet(std::unique_ptr<Wallet> wallet) = 0;
+
+    //! Send progress indicator.
+    virtual void showProgress(const std::string& title, int progress, bool resume_possible) = 0;
+
+    //! Chain notifications.
+    class Notifications
+    {
+    public:
+        virtual ~Notifications() {}
+        virtual void TransactionAddedToMempool(const CTransactionRef& tx) {}
+        virtual void TransactionRemovedFromMempool(const CTransactionRef& ptx) {}
+        virtual void BlockConnected(const CBlock& block, const std::vector<CTransactionRef>& tx_conflicted) {}
+        virtual void BlockDisconnected(const CBlock& block) {}
+        virtual void ChainStateFlushed(const CBlockLocator& locator) {}
+        virtual void ResendWalletTransactions(Lock& locked_chain, int64_t best_block_time) {}
+    };
+
+    //! Register handler for notifications.
+    virtual std::unique_ptr<Handler> handleNotifications(Notifications& notifications) = 0;
+
+    //! 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

@@ -47,8 +47,6 @@ class PendingWalletTxImpl : public PendingWalletTx
 
     const CTransaction& get() override { return *m_tx; }
 
-    int64_t getVirtualSize() override { return GetVirtualTransactionSize(*m_tx); }
-
     bool commit(WalletValueMap value_map,
         WalletOrderForm order_form,
         std::string& reject_reason) override
@@ -99,12 +97,8 @@ WalletTx MakeWalletTx(interfaces::Chain::Lock& locked_chain, CWallet& wallet, co
 //! Construct wallet tx status struct.
 WalletTxStatus MakeWalletTxStatus(interfaces::Chain::Lock& locked_chain, const CWalletTx& wtx)
 {
-    LockAnnotation lock(::cs_main); // Temporary, for mapBlockIndex below. Removed in upcoming commit.
-
     WalletTxStatus result;
-    auto mi = ::mapBlockIndex.find(wtx.hashBlock);
-    CBlockIndex* block = mi != ::mapBlockIndex.end() ? mi->second : nullptr;
-    result.block_height = (block ? block->nHeight : std::numeric_limits<int>::max());
+    result.block_height = locked_chain.getBlockHeight(wtx.hashBlock).get_value_or(std::numeric_limits<int>::max());
     result.blocks_to_maturity = wtx.GetBlocksToMaturity(locked_chain);
     result.depth_in_main_chain = wtx.GetDepthInMainChain(locked_chain);
     result.time_received = wtx.nTimeReceived;
@@ -514,7 +508,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 +518,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/interfaces/wallet.h

@@ -292,9 +292,6 @@ class PendingWalletTx
     //! Get transaction data.
     virtual const CTransaction& get() = 0;
 
-    //! Get virtual transaction size.
-    virtual int64_t getVirtualSize() = 0;
-
     //! Send pending transaction and commit to wallet.
     virtual bool commit(WalletValueMap value_map,
         WalletOrderForm order_form,

src/node/coin.cpp

@@ -0,0 +1,22 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#include <node/coin.h>
+
+#include <txmempool.h>
+#include <validation.h>
+
+void FindCoins(std::map<COutPoint, Coin>& coins)
+{
+    LOCK2(cs_main, ::mempool.cs);
+    assert(pcoinsTip);
+    CCoinsViewCache& chain_view = *::pcoinsTip;
+    CCoinsViewMemPool mempool_view(&chain_view, ::mempool);
+    for (auto& coin : coins) {
+        if (!mempool_view.GetCoin(coin.first, coin.second)) {
+            // Either the coin is not in the CCoinsViewCache or is spent. Clear it.
+            coin.second.Clear();
+        }
+    }
+}

src/node/coin.h

@@ -0,0 +1,22 @@
+// Copyright (c) 2019 The Bitcoin Core developers
+// Distributed under the MIT software license, see the accompanying
+// file COPYING or http://www.opensource.org/licenses/mit-license.php.
+
+#ifndef BITCOIN_NODE_COIN_H
+#define BITCOIN_NODE_COIN_H
+
+#include <map>
+
+class COutPoint;
+class Coin;
+
+/**
+ * Look up unspent output information. Returns coins in the mempool and in the
+ * current chain UTXO set. Iterates through all the keys in the map and
+ * populates the values.
+ *
+ * @param[in,out] coins map to fill
+ */
+void FindCoins(std::map<COutPoint, Coin>& coins);
+
+#endif // BITCOIN_NODE_COIN_H

src/qt/walletmodeltransaction.cpp

@@ -29,7 +29,7 @@ std::unique_ptr<interfaces::PendingWalletTx>& WalletModelTransaction::getWtx()
 
 unsigned int WalletModelTransaction::getTransactionSize()
 {
-    return wtx ? wtx->getVirtualSize() : 0;
+    return wtx ? GetVirtualTransactionSize(wtx->get()) : 0;
 }
 
 CAmount WalletModelTransaction::getTransactionFee() const