Remove uses of cs_main in wallet code

This commit does not change behavior.

It is easiest to review this change with:

    git log -p -n1 -U0

src/interfaces/chain.cpp

@@ -4,13 +4,37 @@
 
 #include <interfaces/chain.h>
 
+#include <sync.h>
 #include <util/system.h>
+#include <validation.h>
+
+#include <memory>
+#include <utility>
 
 namespace interfaces {
 namespace {
 
+class LockImpl : public Chain::Lock
+{
+};
+
+class LockingStateImpl : public LockImpl, public UniqueLock<CCriticalSection>
+{
+    using UniqueLock::UniqueLock;
+};
+
 class ChainImpl : public Chain
 {
+public:
+    std::unique_ptr<Chain::Lock> lock(bool try_lock) override
+    {
+        auto result = MakeUnique<LockingStateImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
+        if (try_lock && result && !*result) return {};
+        // std::move necessary on some compilers due to conversion from
+        // LockingStateImpl to Lock pointer
+        return std::move(result);
+    }
+    std::unique_ptr<Chain::Lock> assumeLocked() override { return MakeUnique<LockImpl>(); }
 };
 
 } // namespace

src/interfaces/chain.h

@@ -18,6 +18,26 @@ class Chain
 {
 public:
     virtual ~Chain() {}
+
+    //! Interface for querying locked chain state, used by legacy code that
+    //! assumes state won't change between calls. New code should avoid using
+    //! the Lock interface and instead call higher-level Chain methods
+    //! that return more information so the chain doesn't need to stay locked
+    //! between calls.
+    class Lock
+    {
+    public:
+        virtual ~Lock() {}
+    };
+
+    //! Return Lock interface. Chain is locked when this is called, and
+    //! unlocked when the returned interface is freed.
+    virtual std::unique_ptr<Lock> lock(bool try_lock = false) = 0;
+
+    //! Return Lock interface assuming chain is already locked. This
+    //! method is temporary and is only used in a few places to avoid changing
+    //! behavior while code is transitioned to use the Chain::Lock interface.
+    virtual std::unique_ptr<Lock> assumeLocked() = 0;
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for

src/interfaces/wallet.cpp

@@ -53,7 +53,8 @@ class PendingWalletTxImpl : public PendingWalletTx
         WalletOrderForm order_form,
         std::string& reject_reason) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         CValidationState state;
         if (!m_wallet.CommitTransaction(m_tx, std::move(value_map), std::move(order_form), m_key, g_connman.get(), state)) {
             reject_reason = state.GetRejectReason();
@@ -209,22 +210,26 @@ class WalletImpl : public Wallet
     }
     void lockCoin(const COutPoint& output) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.LockCoin(output);
     }
     void unlockCoin(const COutPoint& output) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.UnlockCoin(output);
     }
     bool isLockedCoin(const COutPoint& output) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.IsLockedCoin(output.hash, output.n);
     }
     void listLockedCoins(std::vector<COutPoint>& outputs) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.ListLockedCoins(outputs);
     }
     std::unique_ptr<PendingWalletTx> createTransaction(const std::vector<CRecipient>& recipients,
@@ -234,7 +239,8 @@ class WalletImpl : public Wallet
         CAmount& fee,
         std::string& fail_reason) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         auto pending = MakeUnique<PendingWalletTxImpl>(m_wallet);
         if (!m_wallet.CreateTransaction(recipients, pending->m_tx, pending->m_key, fee, change_pos,
                 fail_reason, coin_control, sign)) {
@@ -245,7 +251,8 @@ class WalletImpl : public Wallet
     bool transactionCanBeAbandoned(const uint256& txid) override { return m_wallet.TransactionCanBeAbandoned(txid); }
     bool abandonTransaction(const uint256& txid) override
     {
-        LOCK2(cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.AbandonTransaction(txid);
     }
     bool transactionCanBeBumped(const uint256& txid) override
@@ -274,7 +281,8 @@ class WalletImpl : public Wallet
     }
     CTransactionRef getTx(const uint256& txid) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         auto mi = m_wallet.mapWallet.find(txid);
         if (mi != m_wallet.mapWallet.end()) {
             return mi->second.tx;
@@ -283,7 +291,8 @@ class WalletImpl : public Wallet
     }
     WalletTx getWalletTx(const uint256& txid) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         auto mi = m_wallet.mapWallet.find(txid);
         if (mi != m_wallet.mapWallet.end()) {
             return MakeWalletTx(m_wallet, mi->second);
@@ -292,7 +301,8 @@ class WalletImpl : public Wallet
     }
     std::vector<WalletTx> getWalletTxs() override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         std::vector<WalletTx> result;
         result.reserve(m_wallet.mapWallet.size());
         for (const auto& entry : m_wallet.mapWallet) {
@@ -304,7 +314,7 @@ class WalletImpl : public Wallet
         interfaces::WalletTxStatus& tx_status,
         int& num_blocks) override
     {
-        TRY_LOCK(::cs_main, locked_chain);
+        auto locked_chain = m_wallet.chain().lock(true /* try_lock */);
         if (!locked_chain) {
             return false;
         }
@@ -326,7 +336,8 @@ class WalletImpl : public Wallet
         bool& in_mempool,
         int& num_blocks) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         auto mi = m_wallet.mapWallet.find(txid);
         if (mi != m_wallet.mapWallet.end()) {
             num_blocks = ::chainActive.Height();
@@ -353,7 +364,7 @@ class WalletImpl : public Wallet
     }
     bool tryGetBalances(WalletBalances& balances, int& num_blocks) override
     {
-        TRY_LOCK(cs_main, locked_chain);
+        auto locked_chain = m_wallet.chain().lock(true /* try_lock */);
         if (!locked_chain) return false;
         TRY_LOCK(m_wallet.cs_wallet, locked_wallet);
         if (!locked_wallet) {
@@ -370,27 +381,32 @@ class WalletImpl : public Wallet
     }
     isminetype txinIsMine(const CTxIn& txin) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.IsMine(txin);
     }
     isminetype txoutIsMine(const CTxOut& txout) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.IsMine(txout);
     }
     CAmount getDebit(const CTxIn& txin, isminefilter filter) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.GetDebit(txin, filter);
     }
     CAmount getCredit(const CTxOut& txout, isminefilter filter) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         return m_wallet.GetCredit(txout, filter);
     }
     CoinsList listCoins() override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         CoinsList result;
         for (const auto& entry : m_wallet.ListCoins()) {
             auto& group = result[entry.first];
@@ -403,7 +419,8 @@ class WalletImpl : public Wallet
     }
     std::vector<WalletTxOut> getCoins(const std::vector<COutPoint>& outputs) override
     {
-        LOCK2(::cs_main, m_wallet.cs_wallet);
+        auto locked_chain = m_wallet.chain().lock();
+        LOCK(m_wallet.cs_wallet);
         std::vector<WalletTxOut> result;
         result.reserve(outputs.size());
         for (const auto& output : outputs) {

src/qt/test/wallettests.cpp

@@ -143,7 +143,7 @@ void TestGUI()
         wallet->AddKeyPubKey(test.coinbaseKey, test.coinbaseKey.GetPubKey());
     }
     {
-        LOCK(cs_main);
+        auto locked_chain = wallet->chain().lock();
         WalletRescanReserver reserver(wallet.get());
         reserver.reserve();
         wallet->ScanForWalletTransactions(chainActive.Genesis(), nullptr, reserver, true);

src/wallet/feebumper.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <consensus/validation.h>
+#include <interfaces/chain.h>
 #include <wallet/coincontrol.h>
 #include <wallet/feebumper.h>
 #include <wallet/fees.h>
@@ -64,7 +65,8 @@ namespace feebumper {
 
 bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
 {
-    LOCK2(cs_main, wallet->cs_wallet);
+    auto locked_chain = wallet->chain().lock();
+    LOCK(wallet->cs_wallet);
     const CWalletTx* wtx = wallet->GetWalletTx(txid);
     if (wtx == nullptr) return false;
 
@@ -76,7 +78,8 @@ bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
 Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
                          CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
 {
-    LOCK2(cs_main, wallet->cs_wallet);
+    auto locked_chain = wallet->chain().lock();
+    LOCK(wallet->cs_wallet);
     errors.clear();
     auto it = wallet->mapWallet.find(txid);
     if (it == wallet->mapWallet.end()) {
@@ -212,13 +215,15 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
 }
 
 bool SignTransaction(CWallet* wallet, CMutableTransaction& mtx) {
-    LOCK2(cs_main, wallet->cs_wallet);
+    auto locked_chain = wallet->chain().lock();
+    LOCK(wallet->cs_wallet);
     return wallet->SignTransaction(mtx);
 }
 
 Result CommitTransaction(CWallet* wallet, const uint256& txid, CMutableTransaction&& mtx, std::vector<std::string>& errors, uint256& bumped_txid)
 {
-    LOCK2(cs_main, wallet->cs_wallet);
+    auto locked_chain = wallet->chain().lock();
+    LOCK(wallet->cs_wallet);
     if (!errors.empty()) {
         return Result::MISC_ERROR;
     }

src/wallet/rpcdump.cpp

@@ -3,6 +3,7 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
 #include <chain.h>
+#include <interfaces/chain.h>
 #include <key_io.h>
 #include <rpc/server.h>
 #include <validation.h>
@@ -133,7 +134,8 @@ UniValue importprivkey(const JSONRPCRequest& request)
     WalletRescanReserver reserver(pwallet);
     bool fRescan = true;
     {
-        LOCK2(cs_main, pwallet->cs_wallet);
+        auto locked_chain = pwallet->chain().lock();
+        LOCK(pwallet->cs_wallet);
 
         EnsureWalletIsUnlocked(pwallet);
 
@@ -305,7 +307,8 @@ UniValue importaddress(const JSONRPCRequest& request)
         fP2SH = request.params[3].get_bool();
 
     {
-        LOCK2(cs_main, pwallet->cs_wallet);
+        auto locked_chain = pwallet->chain().lock();
+        LOCK(pwallet->cs_wallet);
 
         CTxDestination dest = DecodeDestination(request.params[0].get_str());
         if (IsValidDestination(dest)) {
@@ -362,7 +365,7 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
     unsigned int txnIndex = 0;
     if (merkleBlock.txn.ExtractMatches(vMatch, vIndex) == merkleBlock.header.hashMerkleRoot) {
 
-        LOCK(cs_main);
+        auto locked_chain = pwallet->chain().lock();
         const CBlockIndex* pindex = LookupBlockIndex(merkleBlock.header.GetHash());
         if (!pindex || !chainActive.Contains(pindex)) {
             throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Block not found in chain");
@@ -382,7 +385,8 @@ UniValue importprunedfunds(const JSONRPCRequest& request)
     wtx.nIndex = txnIndex;
     wtx.hashBlock = merkleBlock.header.GetHash();
 
-    LOCK2(cs_main, pwallet->cs_wallet);
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(pwallet->cs_wallet);
 
     if (pwallet->IsMine(*wtx.tx)) {
         pwallet->AddToWallet(wtx, false);
@@ -412,7 +416,8 @@ UniValue removeprunedfunds(const JSONRPCRequest& request)
             + HelpExampleRpc("removeprunedfunds", "\"a8d0c0184dde994a09ec054286f1ce581bebf46446a512166eae7628734ea0a5\"")
         );
 
-    LOCK2(cs_main, pwallet->cs_wallet);
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(pwallet->cs_wallet);
 
     uint256 hash(ParseHashV(request.params[0], "txid"));
     std::vector<uint256> vHash;
@@ -483,7 +488,8 @@ UniValue importpubkey(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Pubkey is not a valid public key");
 
     {
-        LOCK2(cs_main, pwallet->cs_wallet);
+        auto locked_chain = pwallet->chain().lock();
+        LOCK(pwallet->cs_wallet);
 
         for (const auto& dest : GetAllDestinationsForKey(pubKey)) {
             ImportAddress(pwallet, dest, strLabel);
@@ -535,7 +541,8 @@ UniValue importwallet(const JSONRPCRequest& request)
     int64_t nTimeBegin = 0;
     bool fGood = true;
     {
-        LOCK2(cs_main, pwallet->cs_wallet);
+        auto locked_chain = pwallet->chain().lock();
+        LOCK(pwallet->cs_wallet);
 
         EnsureWalletIsUnlocked(pwallet);
 
@@ -653,7 +660,8 @@ UniValue dumpprivkey(const JSONRPCRequest& request)
             + HelpExampleRpc("dumpprivkey", "\"myaddress\"")
         );
 
-    LOCK2(cs_main, pwallet->cs_wallet);
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(pwallet->cs_wallet);
 
     EnsureWalletIsUnlocked(pwallet);
 
@@ -700,7 +708,8 @@ UniValue dumpwallet(const JSONRPCRequest& request)
             + HelpExampleRpc("dumpwallet", "\"test\"")
         );
 
-    LOCK2(cs_main, pwallet->cs_wallet);
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(pwallet->cs_wallet);
 
     EnsureWalletIsUnlocked(pwallet);
 
@@ -1134,7 +1143,8 @@ UniValue importmulti(const JSONRPCRequest& mainRequest)
     int64_t nLowestTimestamp = 0;
     UniValue response(UniValue::VARR);
     {
-        LOCK2(cs_main, pwallet->cs_wallet);
+        auto locked_chain = pwallet->chain().lock();
+        LOCK(pwallet->cs_wallet);
         EnsureWalletIsUnlocked(pwallet);
 
         // Verify all timestamps are present before importing any keys.