Defer chain lock to caller to allow atomic lock (#371)

Signed-off-by: Anthony Fieroni <[email protected]>
DeFiCh · May 31, 2021 · 118b8be · 118b8be
1 parent 3d602bc
commit 118b8be
Show file tree

Hide file tree

Showing 16 changed files with 220 additions and 179 deletions.
diff --git a/src/interfaces/chain.cpp b/src/interfaces/chain.cpp
@@ -39,11 +39,17 @@
 namespace interfaces {
 namespace {
 
-class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
+class LockImpl : public Chain::Lock
 {
+    CCriticalSection& m_mutex;
+
+public:
+    LockImpl(CCriticalSection& mutex) : m_mutex(mutex)
+    {
+    }
     Optional<int> getHeight() override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         int height = ::ChainActive().Height();
         if (height >= 0) {
             return height;
@@ -52,7 +58,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     Optional<int> getBlockHeight(const uint256& hash) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         CBlockIndex* block = LookupBlockIndex(hash);
         if (block && ::ChainActive().Contains(block)) {
             return block->nHeight;
@@ -67,34 +73,34 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     uint256 getBlockHash(int height) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetBlockHash();
     }
     int64_t getBlockTime(int height) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetBlockTime();
     }
     int64_t getBlockMedianTimePast(int height) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         CBlockIndex* block = ::ChainActive()[height];
         assert(block != nullptr);
         return block->GetMedianTimePast();
     }
     bool haveBlockOnDisk(int height) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         CBlockIndex* block = ::ChainActive()[height];
         return block && ((block->nStatus & BLOCK_HAVE_DATA) != 0) && block->nTx > 0;
     }
     Optional<int> findFirstBlockWithTimeAndHeight(int64_t time, int height, uint256* hash) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         CBlockIndex* block = ::ChainActive().FindEarliestAtLeast(time, height);
         if (block) {
             if (hash) *hash = block->GetBlockHash();
@@ -104,7 +110,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     Optional<int> findPruned(int start_height, Optional<int> stop_height) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         if (::fPruneMode) {
             CBlockIndex* block = stop_height ? ::ChainActive()[*stop_height] : ::ChainActive().Tip();
             while (block && block->nHeight >= start_height) {
@@ -118,7 +124,7 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     Optional<int> findFork(const uint256& hash, Optional<int>* height) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         const CBlockIndex* block = LookupBlockIndex(hash);
         const CBlockIndex* fork = block ? ::ChainActive().FindFork(block) : nullptr;
         if (height) {
@@ -135,24 +141,26 @@ class LockImpl : public Chain::Lock, public UniqueLock<CCriticalSection>
     }
     CBlockLocator getTipLocator() override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         return ::ChainActive().GetLocator();
     }
     Optional<int> findLocatorFork(const CBlockLocator& locator) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         if (CBlockIndex* fork = FindForkInGlobalIndex(::ChainActive(), locator)) {
             return fork->nHeight;
         }
         return nullopt;
     }
     bool checkFinalTx(const CTransaction& tx) override
     {
-        LockAssertion lock(::cs_main);
+        LockAssertion lock(m_mutex);
         return CheckFinalTx(tx);
     }
-
-    using UniqueLock::UniqueLock;
+    CCriticalSection& mutex() override
+    {
+        return m_mutex;
+    }
 };
 
 class NotificationsHandlerImpl : public Handler, CValidationInterface
@@ -240,13 +248,9 @@ class RpcHandlerImpl : public Handler
 class ChainImpl : public Chain
 {
 public:
-    std::unique_ptr<Chain::Lock> lock(bool try_lock) override
+    std::unique_ptr<Chain::Lock> lock() override
     {
-        auto result = MakeUnique<LockImpl>(::cs_main, "cs_main", __FILE__, __LINE__, try_lock);
-        if (try_lock && result && !*result) return {};
-        // std::move necessary on some compilers due to conversion from
-        // LockImpl to Lock pointer
-        return std::move(result);
+        return MakeUnique<LockImpl>(::cs_main);
     }
     bool findBlock(const uint256& hash, CBlock* block, int64_t* time, int64_t* time_max) override
     {

diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
@@ -7,6 +7,7 @@
 
 #include <optional.h>               // For Optional and nullopt
 #include <primitives/transaction.h> // For CTransactionRef
+#include <sync.h>
 
 #include <memory>
 #include <stddef.h>
@@ -124,11 +125,14 @@ class Chain
 
         //! Check if transaction will be final given chain height current time.
         virtual bool checkFinalTx(const CTransaction& tx) = 0;
+
+        //! Assosiated mutex accessor
+        virtual CCriticalSection& mutex() = 0;
     };
 
-    //! 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. Chain is NOT locked when this is called,
+    //! it's defered to caller
+    virtual std::unique_ptr<Lock> lock() = 0;
 
     //! Return whether node has the block and optionally return block metadata
     //! or contents.

diff --git a/src/masternodes/mn_rpc.cpp b/src/masternodes/mn_rpc.cpp
@@ -208,6 +208,7 @@ std::string ScriptToString(CScript const& script) {
 
 int chainHeight(interfaces::Chain::Lock& locked_chain)
 {
+    LOCK(locked_chain.mutex());
     if (auto height = locked_chain.getHeight())
         return *height;
     return 0;
@@ -252,7 +253,7 @@ static boost::optional<CTxIn> GetAuthInputOnly(CWallet* const pwallet, CTxDestin
     cctl.m_tokenFilter = {DCT_ID{0}};
 
     auto locked_chain = pwallet->chain().lock();
-    LOCK(pwallet->cs_wallet);
+    LOCK2(pwallet->cs_wallet, locked_chain->mutex());
 
     pwallet->AvailableCoins(*locked_chain, vecOutputs, true, &cctl, 1, MAX_MONEY, MAX_MONEY, 1);
 

diff --git a/src/masternodes/rpc_masternodes.cpp b/src/masternodes/rpc_masternodes.cpp
@@ -657,6 +657,7 @@ UniValue listanchors(const JSONRPCRequest& request)
     }.Check(request);
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     auto confirms = pcustomcsview->CAnchorConfirmsView::GetAnchorConfirmData();
 

diff --git a/src/masternodes/rpc_oracles.cpp b/src/masternodes/rpc_oracles.cpp
@@ -717,11 +717,12 @@ UniValue listlatestrawprices(const JSONRPCRequest &request) {
         tokenPair = DecodeTokenCurrencyPair(request.params[0]);
     }
 
-    auto lock = pwallet->chain().lock();
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
-    auto optHeight = lock->getHeight();
+    auto optHeight = locked_chain->getHeight();
     int lastHeight = optHeight ? *optHeight : 0;
-    auto lastBlockTime = lock->getBlockTime(lastHeight);
+    auto lastBlockTime = locked_chain->getBlockTime(lastHeight);
 
     CCustomCSView mnview(*pcustomcsview);
 
@@ -859,11 +860,12 @@ UniValue getprice(const JSONRPCRequest &request) {
 
     auto tokenPair = DecodeTokenCurrencyPair(request.params[0]);
 
-    auto lock = pwallet->chain().lock();
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
-    auto optHeight = lock->getHeight();
+    auto optHeight = locked_chain->getHeight();
     int lastHeight = optHeight ? *optHeight : 0;
-    auto lastBlockTime = lock->getBlockTime(lastHeight);
+    auto lastBlockTime = locked_chain->getBlockTime(lastHeight);
 
     CCustomCSView view(*pcustomcsview);
     auto result = GetAggregatePrice(view, tokenPair.first, tokenPair.second, lastBlockTime);
@@ -897,11 +899,12 @@ UniValue listprices(const JSONRPCRequest& request) {
 
     RPCTypeCheck(request.params, {}, false);
 
-    auto lock = pwallet->chain().lock();
+    auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
-    auto optHeight = lock->getHeight();
+    auto optHeight = locked_chain->getHeight();
     int lastHeight = optHeight ? *optHeight : 0;
-    auto lastBlockTime = lock->getBlockTime(lastHeight);
+    auto lastBlockTime = locked_chain->getBlockTime(lastHeight);
 
     CCustomCSView view(*pcustomcsview);
     return GetAllAggregatePrices(view, lastBlockTime);

diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp
@@ -630,8 +630,7 @@ static UniValue combinerawtransaction(const JSONRPCRequest& request)
     CCoinsView viewDummy;
     CCoinsViewCache view(&viewDummy);
     {
-        LOCK(cs_main);
-        LOCK(mempool.cs);
+        LOCK2(cs_main, mempool.cs);
         CCoinsViewCache &viewChain = ::ChainstateActive().CoinsTip();
         CCoinsViewMemPool viewMempool(&viewChain, mempool);
         view.SetBackend(viewMempool); // temporarily switch cache backend to db+mempool view

diff --git a/src/spv/spv_rpc.cpp b/src/spv/spv_rpc.cpp
@@ -145,6 +145,7 @@ UniValue spv_createanchor(const JSONRPCRequest& request)
     CAnchor anchor;
     {
         auto locked_chain = pwallet->chain().lock();
+        LOCK(locked_chain->mutex());
 
         anchor = panchorauths->CreateBestAnchor(rewardDest);
         prevAnchorHeight = panchors->GetActiveAnchor() ? panchors->GetActiveAnchor()->anchor.height : 0;
@@ -233,6 +234,7 @@ UniValue spv_createanchortemplate(const JSONRPCRequest& request)
     CAnchor anchor;
     {
         auto locked_chain = pwallet->chain().lock();
+        LOCK(locked_chain->mutex());
 
         anchor = panchorauths->CreateBestAnchor(rewardDest);
         prevAnchorHeight = panchors->GetActiveAnchor() ? panchors->GetActiveAnchor()->anchor.height : 0;
@@ -297,6 +299,7 @@ UniValue spv_estimateanchorcost(const JSONRPCRequest& request)
     }
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     // it is unable to create "pure" dummy anchor, cause it needs signing with real key
     CAnchor const anchor = panchorauths->CreateBestAnchor(CTxDestination(PKHash()));
@@ -390,6 +393,7 @@ UniValue spv_gettxconfirmations(const JSONRPCRequest& request)
 //    uint32_t const spvLastHeight = spv::pspv ? spv::pspv->GetLastBlockHeight() : 0;
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 //    panchors->UpdateLastHeight(spvLastHeight);
     return UniValue(panchors->GetAnchorConfirmations(txHash));
 }
@@ -453,6 +457,7 @@ UniValue spv_listanchors(const JSONRPCRequest& request)
     uint32_t const tmp = spv::pspv->GetLastBlockHeight();
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     panchors->UpdateLastHeight(tmp); // may be unnecessary but for sure
     auto const * cur = panchors->GetActiveAnchor();
@@ -503,6 +508,7 @@ UniValue spv_listanchorspending(const JSONRPCRequest& request)
         throw JSONRPCError(RPC_INVALID_REQUEST, "spv module disabled");
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     UniValue result(UniValue::VARR);
     panchors->ForEachPending([&result](uint256 const &, CAnchorIndex::AnchorRec & rec)
@@ -535,6 +541,7 @@ UniValue spv_listanchorauths(const JSONRPCRequest& request)
     }.Check(request);
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     UniValue result(UniValue::VARR);
     CAnchorAuthIndex::Auth const * prev = nullptr;
@@ -639,6 +646,7 @@ UniValue spv_listanchorrewardconfirms(const JSONRPCRequest& request)
     }.Check(request);
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     UniValue result(UniValue::VARR);
 
@@ -705,6 +713,7 @@ UniValue spv_listanchorrewards(const JSONRPCRequest& request)
     }.Check(request);
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     UniValue result(UniValue::VARR);
 
@@ -737,6 +746,7 @@ UniValue spv_listanchorsunrewarded(const JSONRPCRequest& request)
     }.Check(request);
 
     auto locked_chain = pwallet->chain().lock();
+    LOCK(locked_chain->mutex());
 
     UniValue result(UniValue::VARR);
 
@@ -1205,7 +1215,7 @@ static UniValue spv_dumpprivkey(const JSONRPCRequest& request)
     }.Check(request);
 
     auto locked_chain = pwallet->chain().lock();
-    LOCK(pwallet->cs_wallet);
+    LOCK2(pwallet->cs_wallet, locked_chain->mutex());
 
     std::string strAddress = request.params[0].get_str();
 
@@ -1275,7 +1285,7 @@ static UniValue spv_sendtoaddress(const JSONRPCRequest& request)
     }
 
     auto locked_chain = pwallet->chain().lock();
-    LOCK(pwallet->cs_wallet);
+    LOCK2(pwallet->cs_wallet, locked_chain->mutex());
 
     std::string address = request.params[0].get_str();