Updates in synchronization mutex for Shared data (#276)

* Change CCriticalSection to Mutex
Add guarded by and lock required annotations

* changed Mutex to RecursiveMutex wherever necessary

* remove lock not held assertions for recursive mutex csmain

* remove guarded by from leveldb

* remove new mutex causing deadlock in network connection

* remove lock   causing build failure

* remove guarded by and new locking from argsmanager as it caused functional test failures

* remove locking changed causing deadlock

* review feedback - change recursive mutexes to mutex where possible
cs_sendProcessing needs to be recursive. otherwise it causes deadlocks

src/addrman.h

@@ -189,10 +189,11 @@ class CAddrInfo : public CAddress
  */
 class CAddrMan
 {
-private:
+protected:
     //! critical section to protect the inner data structures
-    mutable CCriticalSection cs;
+    mutable RecursiveMutex cs;
 
+private:
     //! last used nId
     int nIdCount;
 
@@ -231,58 +232,58 @@ class CAddrMan
     FastRandomContext insecure_rand;
 
     //! Find an entry.
-    CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr);
+    CAddrInfo* Find(const CNetAddr& addr, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! find an entry, creating it if necessary.
     //! nTime and nServices of the found node are updated, if necessary.
-    CAddrInfo* Create(const CAddress &addr, const CNetAddr &addrSource, int *pnId = nullptr);
+    CAddrInfo* Create(const CAddress &addr, const CNetAddr &addrSource, int *pnId = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Swap two elements in vRandom.
-    void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2);
+    void SwapRandom(unsigned int nRandomPos1, unsigned int nRandomPos2) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Move an entry from the "new" table(s) to the "tried" table
-    void MakeTried(CAddrInfo& info, int nId);
+    void MakeTried(CAddrInfo& info, int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Delete an entry. It must not be in tried, and have refcount 0.
-    void Delete(int nId);
+    void Delete(int nId) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Clear a position in a "new" table. This is the only place where entries are actually deleted.
-    void ClearNew(int nUBucket, int nUBucketPos);
+    void ClearNew(int nUBucket, int nUBucketPos) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Mark an entry "good", possibly moving it from "new" to "tried".
-    void Good_(const CService &addr, bool test_before_evict, int64_t time);
+    void Good_(const CService &addr, bool test_before_evict, int64_t time) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Add an entry to the "new" table.
-    bool Add_(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty);
+    bool Add_(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Mark an entry as attempted to connect.
-    void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime);
+    void Attempt_(const CService &addr, bool fCountFailure, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Select an address to connect to, if newOnly is set to true, only the new table is selected from.
-    CAddrInfo Select_(bool newOnly);
+    CAddrInfo Select_(bool newOnly) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
-    void ResolveCollisions_();
+    void ResolveCollisions_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Return a random to-be-evicted tried table address.
-    CAddrInfo SelectTriedCollision_();
+    CAddrInfo SelectTriedCollision_() EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Wraps GetRandInt to allow tests to override RandomInt and make it determinismistic.
-    virtual int RandomInt(int nMax);
+    virtual int RandomInt(int nMax) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
 #ifdef DEBUG_ADDRMAN
     //! Perform consistency check. Returns an error code or zero.
     int Check_();
 #endif
 
     //! Select several addresses at once.
-    void GetAddr_(std::vector<CAddress> &vAddr);
+    void GetAddr_(std::vector<CAddress> &vAddr) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Mark an entry as currently-connected-to.
-    void Connected_(const CService &addr, int64_t nTime);
+    void Connected_(const CService &addr, int64_t nTime) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
     //! Update an entry's service bits.
-    void SetServices_(const CService &addr, ServiceFlags nServices);
+    void SetServices_(const CService &addr, ServiceFlags nServices) EXCLUSIVE_LOCKS_REQUIRED(cs);
 
 public:
     /**
@@ -315,7 +316,7 @@ class CAddrMan
      * very little in common.
      */
     template<typename Stream>
-    void Serialize(Stream &s) const
+    void Serialize(Stream &s) const EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
 
@@ -365,11 +366,10 @@ class CAddrMan
     }
 
     template<typename Stream>
-    void Unserialize(Stream& s)
+    void Unserialize(Stream& s) EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
-        LOCK(cs);
-
         Clear();
+        LOCK(cs);
 
         unsigned char nVersion;
         s >> nVersion;
@@ -470,7 +470,7 @@ class CAddrMan
         Check();
     }
 
-    void Clear()
+    void Clear() EXCLUSIVE_LOCKS_REQUIRED(!cs)
     {
         LOCK(cs);
         std::vector<int>().swap(vRandom);
@@ -525,7 +525,7 @@ class CAddrMan
     }
 
     //! Add a single address.
-    bool Add(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty = 0)
+    bool Add(const CAddress &addr, const CNetAddr& source, int64_t nTimePenalty = 0) LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         bool fRet = false;
@@ -539,7 +539,7 @@ class CAddrMan
     }
 
     //! Add multiple addresses.
-    bool Add(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty = 0)
+    bool Add(const std::vector<CAddress> &vAddr, const CNetAddr& source, int64_t nTimePenalty = 0) LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         int nAdd = 0;
@@ -554,7 +554,7 @@ class CAddrMan
     }
 
     //! Mark an entry as accessible.
-    void Good(const CService &addr, bool test_before_evict = true, int64_t nTime = GetAdjustedTime())
+    void Good(const CService &addr, bool test_before_evict = true, int64_t nTime = GetAdjustedTime()) LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         Check();
@@ -563,7 +563,7 @@ class CAddrMan
     }
 
     //! Mark an entry as connection attempted to.
-    void Attempt(const CService &addr, bool fCountFailure, int64_t nTime = GetAdjustedTime())
+    void Attempt(const CService &addr, bool fCountFailure, int64_t nTime = GetAdjustedTime()) LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         Check();
@@ -572,7 +572,7 @@ class CAddrMan
     }
 
     //! See if any to-be-evicted tried table entries have been tested and if so resolve the collisions.
-    void ResolveCollisions()
+    void ResolveCollisions() LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         Check();
@@ -581,7 +581,7 @@ class CAddrMan
     }
 
     //! Randomly select an address in tried that another address is attempting to evict.
-    CAddrInfo SelectTriedCollision()
+    CAddrInfo SelectTriedCollision() LOCKS_EXCLUDED(cs)
     {
         CAddrInfo ret;
         {
@@ -596,7 +596,7 @@ class CAddrMan
     /**
      * Choose an address to connect to.
      */
-    CAddrInfo Select(bool newOnly = false)
+    CAddrInfo Select(bool newOnly = false) LOCKS_EXCLUDED(cs)
     {
         CAddrInfo addrRet;
         {
@@ -609,7 +609,7 @@ class CAddrMan
     }
 
     //! Return a bunch of addresses, selected at random.
-    std::vector<CAddress> GetAddr()
+    std::vector<CAddress> GetAddr() LOCKS_EXCLUDED(cs)
     {
         Check();
         std::vector<CAddress> vAddr;
@@ -622,15 +622,15 @@ class CAddrMan
     }
 
     //! Mark an entry as currently-connected-to.
-    void Connected(const CService &addr, int64_t nTime = GetAdjustedTime())
+    void Connected(const CService &addr, int64_t nTime = GetAdjustedTime()) LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         Check();
         Connected_(addr, nTime);
         Check();
     }
 
-    void SetServices(const CService &addr, ServiceFlags nServices)
+    void SetServices(const CService &addr, ServiceFlags nServices) LOCKS_EXCLUDED(cs)
     {
         LOCK(cs);
         Check();

src/checkqueue.h

@@ -29,7 +29,7 @@ class CCheckQueue
 {
 private:
     //! Mutex to protect the inner state
-    std::mutex mutex;
+    Mutex mutex;
 
     //! Worker threads block on this when out of work
     std::condition_variable condWorker;

src/httpserver.cpp

@@ -70,7 +70,7 @@ class WorkQueue
 {
 private:
     /** Mutex protects entire object */
-    std::mutex cs;
+    Mutex cs;
     std::condition_variable cond;
     std::deque<std::unique_ptr<WorkItem>> queue;
     bool running;

src/init.cpp

@@ -187,7 +187,7 @@ void Interrupt()
 void Shutdown()
 {
     LogPrintf("%s: In progress...\n", __func__);
-    static CCriticalSection cs_Shutdown;
+    static Mutex cs_Shutdown;
     TRY_LOCK(cs_Shutdown, lockShutdown);
     if (!lockShutdown)
         return;
@@ -562,7 +562,7 @@ static void BlockNotifyCallback(bool initialSync, const CBlockIndex *pBlockIndex
 }
 
 static bool fHaveGenesis = false;
-static CWaitableCriticalSection cs_GenesisWait;
+static Mutex cs_GenesisWait;
 static CConditionVariable condvar_GenesisWait;
 
 static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex)

src/interfaces/node.cpp

@@ -147,7 +147,7 @@ class NodeImpl : public Node
     }
     int64_t getTotalBytesRecv() override { return g_connman ? g_connman->GetTotalBytesRecv() : 0; }
     int64_t getTotalBytesSent() override { return g_connman ? g_connman->GetTotalBytesSent() : 0; }
-    size_t getMempoolSize() override { return ::mempool.size(); }
+    size_t getMempoolSize() override { LOCK(mempool.cs); return ::mempool.size(); }
     size_t getMempoolDynamicUsage() override { return ::mempool.DynamicMemoryUsage(); }
     bool getHeaderTip(int& height, int64_t& block_time) override
     {

src/interfaces/wallet.cpp

@@ -60,6 +60,7 @@ class PendingWalletTxImpl : public PendingWalletTx
 //! Construct wallet tx struct.
 WalletTx MakeWalletTx(CWallet& wallet, const CWalletTx& wtx)
 {
+    LOCK(wallet.cs_wallet);
     WalletTx result;
     result.tx = wtx.tx;
     result.txin_is_mine.reserve(wtx.tx->vin.size());

src/keystore.h

@@ -42,7 +42,7 @@ class CKeyStore : public SigningProvider
 class CBasicKeyStore : public CKeyStore
 {
 protected:
-    mutable CCriticalSection cs_KeyStore;
+    mutable RecursiveMutex cs_KeyStore;
 
     using KeyMap = std::map<CKeyID, CKey>;
     using WatchKeyMap = std::map<CKeyID, CPubKey>;

src/net.cpp

@@ -87,9 +87,9 @@ static const uint64_t RANDOMIZER_ID_LOCALHOSTNONCE = 0xd93e69e2bbfa5735ULL; // S
 bool fDiscover = true;
 bool fListen = true;
 bool fRelayTxes = true;
-CCriticalSection cs_mapLocalHost;
-std::map<CNetAddr, LocalServiceInfo> mapLocalHost;
-static bool vfLimited[NET_MAX] = {};
+Mutex cs_mapLocalHost;
+std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
+static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost)= {};
 std::string strSubVersion;
 
 limitedmap<uint256, int64_t> mapAlreadyAskedFor(MAX_INV_SZ);
@@ -618,7 +618,6 @@ void CConnman::SweepBanned()
     int64_t now = GetTime();
     bool notifyUI = false;
     {
-        LOCK(cs_setBanned);
         banmap_t::iterator it = setBanned.begin();
         while(it != setBanned.end())
         {
@@ -2054,10 +2053,7 @@ void CConnman::ThreadMessageHandler()
             if (flagInterruptMsgProc)
                 return;
             // Send messages
-            {
-                LOCK(pnode->cs_sendProcessing);
-                m_msgproc->SendMessages(pnode);
-            }
+            m_msgproc->SendMessages(pnode);
 
             if (flagInterruptMsgProc)
                 return;