Merge pull request #5057 from str4d/sync-backports

Sync backports

configure.ac

@@ -308,6 +308,7 @@ if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wformat],[CXXFLAGS="$CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wvla],[CXXFLAGS="$CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wformat-security],[CXXFLAGS="$CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[CXXFLAGS="$CXXFLAGS -Wthread-safety-analysis"],,[[$CXXFLAG_WERROR]])
 
   ## Some compilers (gcc) ignore unknown -Wno-* options, but warn about all
   ## unknown options if any other warning is produced. Test the -Wfoo case, and
@@ -610,6 +611,7 @@ if test x$use_hardening != xno; then
   AX_CHECK_COMPILE_FLAG([-Wformat-security],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wformat-security"],[AC_MSG_ERROR(Cannot enable -Wformat-security)],[-Wformat])
   AX_CHECK_COMPILE_FLAG([-Wstack-protector],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wstack-protector"],[AC_MSG_ERROR(Cannot enable -Wstack-protector)])
   AX_CHECK_COMPILE_FLAG([-fstack-protector-all],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -fstack-protector-all"],[AC_MSG_ERROR(Cannot enable -fstack-protector-all)])
+  AX_CHECK_COMPILE_FLAG([-Wthread-safety-analysis],[HARDENED_CXXFLAGS="$HARDENED_CXXFLAGS -Wthread-safety-analysis"],[AC_MSG_ERROR(Cannot enable -Wthread-safety-analysis)])
 
   AX_CHECK_PREPROC_FLAG([-D_FORTIFY_SOURCE=2],[
     AX_CHECK_PREPROC_FLAG([-U_FORTIFY_SOURCE],[
@@ -700,6 +702,29 @@ AC_LINK_IFELSE([AC_LANG_SOURCE([
   ]
 )
 
+TEMP_LDFLAGS="$LDFLAGS"
+LDFLAGS="$TEMP_LDFLAGS $PTHREAD_CFLAGS"
+AC_MSG_CHECKING([for thread_local support])
+AC_LINK_IFELSE([AC_LANG_SOURCE([
+  #include <thread>
+  static thread_local int foo = 0;
+  static void run_thread() { foo++;}
+  int main(){
+  for(int i = 0; i < 10; i++) { std::thread(run_thread).detach();}
+  return foo;
+  }
+  ])],
+  [
+    AC_DEFINE(HAVE_THREAD_LOCAL,1,[Define if thread_local is supported.])
+    AC_MSG_RESULT(yes)
+  ],
+  [
+    AC_MSG_RESULT(no)
+  ]
+)
+LDFLAGS="$TEMP_LDFLAGS"
+
+# Check for reduced exports
 if test x$use_reduce_exports = xyes; then
   AX_CHECK_COMPILE_FLAG([-fvisibility=hidden],[RE_CXXFLAGS="-fvisibility=hidden"],
   [AC_MSG_ERROR([Cannot set default symbol visibility. Use --disable-reduce-exports.])])

src/Makefile.test.include

@@ -107,6 +107,7 @@ BITCOIN_TESTS =\
   test/sigopcount_tests.cpp \
   test/skiplist_tests.cpp \
   test/streams_tests.cpp \
+  test/sync_tests.cpp \
   test/test_random.h \
   test/test_util.cpp \
   test/test_util.h \

src/bitcoin-tx.cpp

@@ -334,22 +334,6 @@ static bool findSighashFlags(int& flags, const std::string& flagStr)
     return false;
 }
 
-uint256 ParseHashUO(std::map<std::string,UniValue>& o, std::string strKey)
-{
-    if (!o.count(strKey))
-        return uint256();
-    return ParseHashUV(o[strKey], strKey);
-}
-
-std::vector<unsigned char> ParseHexUO(std::map<std::string,UniValue>& o, std::string strKey)
-{
-    if (!o.count(strKey)) {
-        std::vector<unsigned char> emptyVec;
-        return emptyVec;
-    }
-    return ParseHexUV(o[strKey], strKey);
-}
-
 static CAmount AmountFromValue(const UniValue& value)
 {
     if (!value.isNum() && !value.isStr())

src/dbwrapper.h

@@ -121,10 +121,6 @@ public:
         return true;
     }
 
-    unsigned int GetKeySize() {
-        return piter->key().size();
-    }
-
     template<typename V> bool GetValue(V& value) {
         leveldb::Slice slValue = piter->value();
         try {

src/httpserver.cpp

@@ -69,7 +69,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;
@@ -88,7 +88,7 @@ public:
     /** Enqueue a work item */
     bool Enqueue(WorkItem* item)
     {
-        std::unique_lock<std::mutex> lock(cs);
+        LOCK(cs);
         if (queue.size() >= maxDepth) {
             return false;
         }
@@ -102,7 +102,7 @@ public:
         while (true) {
             std::unique_ptr<WorkItem> i;
             {
-                std::unique_lock<std::mutex> lock(cs);
+                WAIT_LOCK(cs, lock);
                 while (running && queue.empty())
                     cond.wait(lock);
                 if (!running)
@@ -116,17 +116,10 @@ public:
     /** Interrupt and exit loops */
     void Interrupt()
     {
-        std::unique_lock<std::mutex> lock(cs);
+        LOCK(cs);
         running = false;
         cond.notify_all();
     }
-
-    /** Return current depth of queue */
-    size_t Depth()
-    {
-        std::unique_lock<std::mutex> lock(cs);
-        return queue.size();
-    }
 };
 
 struct HTTPPathHandler

src/init.cpp

@@ -555,6 +555,21 @@ static void TxExpiryNotifyCallback(const uint256& txid)
     boost::thread t(runCommand, strCmd); // thread runs free
 }
 
+static bool fHaveGenesis = false;
+static Mutex g_genesis_wait_mutex;
+static std::condition_variable g_genesis_wait_cv;
+
+static void BlockNotifyGenesisWait(bool, const CBlockIndex *pBlockIndex)
+{
+    if (pBlockIndex != NULL) {
+        {
+            LOCK(g_genesis_wait_mutex);
+            fHaveGenesis = true;
+        }
+        g_genesis_wait_cv.notify_all();
+    }
+}
+
 struct CImportingNow
 {
     CImportingNow() {
@@ -1711,7 +1726,7 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
                     break;
                 }
 
-                if (!fReindex) {
+                if (!fReindex && chainActive.Tip() != NULL) {
                     uiInterface.InitMessage(_("Rewinding blocks if needed..."));
                     if (!RewindBlockIndex(chainparams, clearWitnessCaches)) {
                         strLoadError = _("Unable to rewind the database to a pre-upgrade state. You will need to redownload the blockchain");
@@ -1868,6 +1883,17 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
 
     // ********************************************************* Step 10: import blocks
 
+    if (!CheckDiskSpace())
+        return false;
+
+    // Either install a handler to notify us when genesis activates, or set fHaveGenesis directly.
+    // No locking, as this happens before any background thread is started.
+    if (chainActive.Tip() == NULL) {
+        uiInterface.NotifyBlockTip.connect(BlockNotifyGenesisWait);
+    } else {
+        fHaveGenesis = true;
+    }
+
     if (mapArgs.count("-blocknotify"))
         uiInterface.NotifyBlockTip.connect(BlockNotifyCallback);
 
@@ -1880,28 +1906,29 @@ bool AppInit2(boost::thread_group& threadGroup, CScheduler& scheduler)
         for (const std::string& strFile : mapMultiArgs["-loadblock"])
             vImportFiles.push_back(strFile);
     }
+
     threadGroup.create_thread(boost::bind(&ThreadImport, vImportFiles, chainparams));
 
     // Wait for genesis block to be processed
-    bool fHaveGenesis = false;
+    {
-    while (!fHaveGenesis && !fRequestShutdown) {
+        WAIT_LOCK(g_genesis_wait_mutex, lock);
-        {
+        // We previously could hang here if StartShutdown() is called prior to
-            LOCK(cs_main);
+        // ThreadImport getting started, so instead we just wait on a timer to
-            fHaveGenesis = (chainActive.Tip() != NULL);
+        // check ShutdownRequested() regularly.
-        }
+        while (!fHaveGenesis && !ShutdownRequested()) {
-
+            g_genesis_wait_cv.wait_for(lock, std::chrono::milliseconds(500));
-        if (!fHaveGenesis) {
-            MilliSleep(10);
         }
+        uiInterface.NotifyBlockTip.disconnect(BlockNotifyGenesisWait);
     }
     if (!fHaveGenesis) {
         return false;
     }
 
-    // ********************************************************* Step 11: start node
+    if (ShutdownRequested()) {
-
-    if (!CheckDiskSpace())
         return false;
+    }
+
+    // ********************************************************* Step 11: start node
 
     if (!strErrors.str().empty())
         return InitError(strErrors.str());

src/main.cpp

@@ -60,14 +60,24 @@ using namespace std;
  * Global state
  */
 
-CCriticalSection cs_main;
+/**
+ * Mutex to guard access to validation specific variables, such as reading
+ * or changing the chainstate.
+ *
+ * This may also need to be locked when updating the transaction pool, e.g. on
+ * AcceptToMemoryPool. See CTxMemPool::cs comment for details.
+ *
+ * The transaction pool has a separate lock to allow reading from it and the
+ * chainstate at the same time.
+ */
+RecursiveMutex cs_main;
 
 BlockMap mapBlockIndex;
 CChain chainActive;
 CBlockIndex *pindexBestHeader = NULL;
 static std::atomic<int64_t> nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block
-CWaitableCriticalSection g_best_block_mutex;
+Mutex g_best_block_mutex;
-CConditionVariable g_best_block_cv;
+std::condition_variable g_best_block_cv;
 uint256 g_best_block;
 int g_best_block_height;
 int nScriptCheckThreads = 0;
@@ -3863,7 +3873,7 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
     RenderPoolMetrics("transparent", transparentPool);
 
     {
-        boost::unique_lock<boost::mutex> lock(g_best_block_mutex);
+        WAIT_LOCK(g_best_block_mutex, lock);
         g_best_block = pindexNew->GetBlockHash();
         g_best_block_height = pindexNew->nHeight;
         g_best_block_cv.notify_all();
@@ -5818,8 +5828,11 @@ bool InitBlockIndex(const CChainParams& chainparams)
             CBlockIndex *pindex = AddToBlockIndex(block, chainparams.GetConsensus());
             if (!ReceivedBlockTransactions(block, state, chainparams, pindex, blockPos))
                 return error("LoadBlockIndex(): genesis block not accepted");
-            if (!ActivateBestChain(state, chainparams, &block))
+            // Before the genesis block, there was an empty tree. We set its root here so
-                return error("LoadBlockIndex(): genesis block cannot be activated");
+            // that the block import thread doesn't race other methods that need to query
+            // the Sprout tree (namely CWallet::ScanForWalletTransactions).
+            SproutMerkleTree tree;
+            pindex->hashSproutAnchor = tree.root();
             // Force a chainstate write so that when we VerifyDB in a moment, it doesn't check stale data
             return FlushStateToDisk(chainparams, state, FLUSH_STATE_ALWAYS);
         } catch (const std::runtime_error& e) {

src/main.h

@@ -169,8 +169,8 @@ extern const std::string strMessageMagic;
 //! (and cannot) hold cs_main. So the g_best_block_height and g_best_block variables
 //! (protected by g_best_block_mutex) provide the needed height and block
 //! hash respectively to getblocktemplate without it requiring cs_main.
-extern CWaitableCriticalSection g_best_block_mutex;
+extern Mutex g_best_block_mutex;
-extern CConditionVariable g_best_block_cv;
+extern std::condition_variable g_best_block_cv;
 extern int g_best_block_height;
 extern uint256 g_best_block;
 

src/net.cpp

@@ -1122,24 +1122,21 @@ void ThreadSocketHandler()
             for (CNode* pnode : vNodesDisconnectedCopy)
             {
                 // wait until threads are done using it
-                if (pnode->GetRefCount() <= 0)
+                if (pnode->GetRefCount() <= 0) {
-                {
                     bool fDelete = false;
                     {
-                        TRY_LOCK(pnode->cs_vSend, lockSend);
+                        TRY_LOCK(pnode->cs_inventory, lockInv);
-                        if (lockSend)
+                        if (lockInv) {
-                        {
                             TRY_LOCK(pnode->cs_vRecvMsg, lockRecv);
-                            if (lockRecv)
+                            if (lockRecv) {
-                            {
+                                TRY_LOCK(pnode->cs_vSend, lockSend);
-                                TRY_LOCK(pnode->cs_inventory, lockInv);
+                                if (lockSend) {
-                                if (lockInv)
                                     fDelete = true;
+                                }
                             }
                         }
                     }
-                    if (fDelete)
+                    if (fDelete) {
-                    {
                         vNodesDisconnected.remove(pnode);
                         delete pnode;
                     }
@@ -1201,8 +1198,8 @@ void ThreadSocketHandler()
 
                 bool select_send;
                 {
-                    TRY_LOCK(pnode->cs_vSend, lockSend);
+                    LOCK(pnode->cs_vSend);
-                    select_send = lockSend && !pnode->vSendMsg.empty();
+                    select_send = !pnode->vSendMsg.empty();
                 }
 
                 bool select_recv;
@@ -1343,9 +1340,8 @@ void ThreadSocketHandler()
             //
             if (sendSet)
             {
-                TRY_LOCK(pnode->cs_vSend, lockSend);
+                LOCK(pnode->cs_vSend);
-                if (lockSend)
+                SocketSendData(pnode);
-                    SocketSendData(pnode);
             }
 
             //
@@ -1722,9 +1718,8 @@ void ThreadMessageHandler()
 
             // Send messages
             {
-                TRY_LOCK(pnode->cs_vSend, lockSend);
+                LOCK(pnode->cs_sendProcessing);
-                if (lockSend)
+                g_signals.SendMessages(chainparams.GetConsensus(), pnode);
-                    g_signals.SendMessages(chainparams.GetConsensus(), pnode);
             }
             boost::this_thread::interruption_point();
         }

src/net.h

@@ -266,6 +266,8 @@ public:
     CCriticalSection cs_hSocket;
     CCriticalSection cs_vRecv;
 
+    CCriticalSection cs_sendProcessing;
+
     std::deque<CInv> vRecvGetData;
     std::deque<CNetMessage> vRecvMsg;
     CCriticalSection cs_vRecvMsg;

src/random.cpp

@@ -10,13 +10,13 @@
 #ifdef WIN32
 #include "compat.h" // for Windows API
 #endif
-#include "serialize.h"        // for begin_ptr(vec)
+#include "logging.h"  // for LogPrint()
-#include "util.h"             // for LogPrint()
+#include "utiltime.h" // for GetTime()
-#include "utilstrencodings.h" // for GetTime()
 
 #include <limits>
 
 #ifndef WIN32
+#include <fcntl.h>
 #include <sys/time.h>
 #endif
 

src/rpc/mining.cpp

@@ -601,7 +601,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
     {
         // Wait to respond until either the best block changes, OR some time passes and there are more transactions
         uint256 hashWatchedChain;
-        boost::system_time checktxtime;
+        std::chrono::steady_clock::time_point checktxtime;
         unsigned int nTransactionsUpdatedLastLP;
 
         if (lpval.isStr())
@@ -623,9 +623,9 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
         // Don't call chainActive->Tip() without holding cs_main
         LEAVE_CRITICAL_SECTION(cs_main);
         {
-            checktxtime = boost::get_system_time() + boost::posix_time::seconds(10);
+            checktxtime = std::chrono::steady_clock::now() + std::chrono::seconds(10);
 
-            boost::unique_lock<boost::mutex> lock(g_best_block_mutex);
+            WAIT_LOCK(g_best_block_mutex, lock);
             while (g_best_block == hashWatchedChain && IsRPCRunning())
             {
                 // Before waiting, generate the coinbase for the block following the next
@@ -640,7 +640,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
                         Params(), CAmount{0}, minerAddress, cached_next_cb_height);
                     next_cb_mtx = cached_next_cb_mtx;
                 }
-                bool timedout = !g_best_block_cv.timed_wait(lock, checktxtime);
+                bool timedout = g_best_block_cv.wait_until(lock, checktxtime) == std::cv_status::timeout;
 
                 // Optimization: even if timed out, a new block may have arrived
                 // while waiting for cs_main; if so, don't discard next_cb_mtx.
@@ -652,7 +652,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
                     next_cb_mtx = nullopt;
                     break;
                 }
-                checktxtime += boost::posix_time::seconds(10);
+                checktxtime += std::chrono::seconds(10);
             }
             if (g_best_block_height != nHeight + 1) {
                 // Unexpected height (reorg or >1 blocks arrived while waiting) invalidates coinbase tx.

src/sync.cpp

@@ -5,14 +5,19 @@
 
 #include "sync.h"
 
-#include "util.h"
+#include "logging.h"
 #include "utilstrencodings.h"
 
 #include <stdio.h>
 
-#include <boost/thread.hpp>
+#include <map>
+#include <memory>
+#include <set>
 
 #ifdef DEBUG_LOCKCONTENTION
+#if !defined(HAVE_THREAD_LOCAL)
+static_assert(false, "thread_local is not supported");
+#endif
 void PrintLockContention(const char* pszName, const char* pszFile, int nLine)
 {
     LogPrintf("LOCKCONTENTION: %s\n", pszName);
@@ -24,8 +29,8 @@ void PrintLockContention(const char* pszName, const char* pszFile, int nLine)
 //
 // Early deadlock detection.
 // Problem being solved:
-//    Thread 1 locks  A, then B, then C
+//    Thread 1 locks A, then B, then C
-//    Thread 2 locks  D, then C, then A
+//    Thread 2 locks D, then C, then A
 //     --> may result in deadlock between the two threads, depending on when they run.
 // Solution implemented here:
 // Keep track of pairs of locks: (A before B), (A before C), etc.
@@ -46,10 +51,8 @@ struct CLockLocation {
         return mutexName + "  " + sourceFile + ":" + itostr(sourceLine) + (fTry ? " (TRY)" : "");
     }
 
-    std::string MutexName() const { return mutexName; }
-
-    bool fTry;
 private:
+    bool fTry;
     std::string mutexName;
     std::string sourceFile;
     int sourceLine;
@@ -70,101 +73,76 @@ struct LockData {
 
     LockOrders lockorders;
     InvLockOrders invlockorders;
-    boost::mutex dd_mutex;
+    std::mutex dd_mutex;
 };
 LockData& GetLockData() {
     static LockData lockdata;
     return lockdata;
 }
 
-boost::thread_specific_ptr<LockStack> lockstack;
+static thread_local LockStack g_lockstack;
 
 static void potential_deadlock_detected(const std::pair<void*, void*>& mismatch, const LockStack& s1, const LockStack& s2)
 {
-    // We attempt to not assert on probably-not deadlocks by assuming that
-    // a try lock will immediately have otherwise bailed if it had
-    // failed to get the lock
-    // We do this by, for the locks which triggered the potential deadlock,
-    // in either lockorder, checking that the second of the two which is locked
-    // is only a TRY_LOCK, ignoring locks if they are reentrant.
-    bool firstLocked = false;
-    bool secondLocked = false;
-    bool onlyMaybeDeadlock = false;
-
     LogPrintf("POTENTIAL DEADLOCK DETECTED\n");
     LogPrintf("Previous lock order was:\n");
     for (const std::pair<void*, CLockLocation> & i : s2) {
         if (i.first == mismatch.first) {
-            LogPrintf(" (1)");
+            LogPrintf(" (1)"); /* Continued */
-            if (!firstLocked && secondLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            firstLocked = true;
         }
         if (i.first == mismatch.second) {
-            LogPrintf(" (2)");
+            LogPrintf(" (2)"); /* Continued */
-            if (!secondLocked && firstLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            secondLocked = true;
         }
         LogPrintf(" %s\n", i.second.ToString());
     }
-    firstLocked = false;
-    secondLocked = false;
     LogPrintf("Current lock order is:\n");
     for (const std::pair<void*, CLockLocation> & i : s1) {
         if (i.first == mismatch.first) {
-            LogPrintf(" (1)");
+            LogPrintf(" (1)"); /* Continued */
-            if (!firstLocked && secondLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            firstLocked = true;
         }
         if (i.first == mismatch.second) {
-            LogPrintf(" (2)");
+            LogPrintf(" (2)"); /* Continued */
-            if (!secondLocked && firstLocked && i.second.fTry)
-                onlyMaybeDeadlock = true;
-            secondLocked = true;
         }
         LogPrintf(" %s\n", i.second.ToString());
     }
-    assert(onlyMaybeDeadlock);
+    if (g_debug_lockorder_abort) {
+        fprintf(stderr, "Assertion failed: detected inconsistent lock order at %s:%i, details in debug log.\n", __FILE__, __LINE__);
+        abort();
+    }
+    throw std::logic_error("potential deadlock detected");
 }
 
-static void push_lock(void* c, const CLockLocation& locklocation, bool fTry)
+static void push_lock(void* c, const CLockLocation& locklocation)
 {
-    if (lockstack.get() == NULL)
-        lockstack.reset(new LockStack);
-
     LockData& lockdata = GetLockData();
-    boost::unique_lock<boost::mutex> lock(lockdata.dd_mutex);
+    std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
 
-    (*lockstack).push_back(std::make_pair(c, locklocation));
+    g_lockstack.push_back(std::make_pair(c, locklocation));
 
-    if (!fTry) {
+    for (const std::pair<void*, CLockLocation>& i : g_lockstack) {
-        for (const std::pair<void*, CLockLocation> & i : (*lockstack)) {
+        if (i.first == c)
-            if (i.first == c)
+            break;
-                break;
 
-            std::pair<void*, void*> p1 = std::make_pair(i.first, c);
+        std::pair<void*, void*> p1 = std::make_pair(i.first, c);
-            if (lockdata.lockorders.count(p1))
+        if (lockdata.lockorders.count(p1))
-                continue;
+            continue;
-            lockdata.lockorders[p1] = (*lockstack);
+        lockdata.lockorders[p1] = g_lockstack;
 
-            std::pair<void*, void*> p2 = std::make_pair(c, i.first);
+        std::pair<void*, void*> p2 = std::make_pair(c, i.first);
-            lockdata.invlockorders.insert(p2);
+        lockdata.invlockorders.insert(p2);
-            if (lockdata.lockorders.count(p2))
+        if (lockdata.lockorders.count(p2))
-                potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]);
+            potential_deadlock_detected(p1, lockdata.lockorders[p2], lockdata.lockorders[p1]);
-        }
     }
 }
 
 static void pop_lock()
 {
-    (*lockstack).pop_back();
+    g_lockstack.pop_back();
 }
 
 void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry)
 {
-    push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry), fTry);
+    push_lock(cs, CLockLocation(pszName, pszFile, nLine, fTry));
 }
 
 void LeaveCritical()
@@ -175,14 +153,14 @@ void LeaveCritical()
 std::string LocksHeld()
 {
     std::string result;
-    for (const std::pair<void*, CLockLocation> & i : *lockstack)
+    for (const std::pair<void*, CLockLocation>& i : g_lockstack)
         result += i.second.ToString() + std::string("\n");
     return result;
 }
 
 void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
 {
-    for (const std::pair<void*, CLockLocation> & i : *lockstack)
+    for (const std::pair<void*, CLockLocation>& i : g_lockstack)
         if (i.first == cs)
             return;
     fprintf(stderr, "Assertion failed: lock %s not held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str());
@@ -191,7 +169,7 @@ void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine,
 
 void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs)
 {
-    for (const std::pair<void*, CLockLocation>& i : *lockstack) {
+    for (const std::pair<void*, CLockLocation>& i : g_lockstack) {
         if (i.first == cs) {
             fprintf(stderr, "Assertion failed: lock %s held in %s:%i; locks held:\n%s", pszName, pszFile, nLine, LocksHeld().c_str());
             abort();
@@ -206,7 +184,7 @@ void DeleteLock(void* cs)
         // We're already shutting down.
         return;
     }
-    boost::unique_lock<boost::mutex> lock(lockdata.dd_mutex);
+    std::lock_guard<std::mutex> lock(lockdata.dd_mutex);
     std::pair<void*, void*> item = std::make_pair(cs, (void*)0);
     LockOrders::iterator it = lockdata.lockorders.lower_bound(item);
     while (it != lockdata.lockorders.end() && it->first.first == cs) {
@@ -222,4 +200,6 @@ void DeleteLock(void* cs)
     }
 }
 
+bool g_debug_lockorder_abort = true;
+
 #endif /* DEBUG_LOCKORDER */

src/sync.h

@@ -9,10 +9,9 @@
 
 #include "threadsafety.h"
 
-#include <boost/thread/condition_variable.hpp>
+#include <condition_variable>
-#include <boost/thread/locks.hpp>
+#include <thread>
-#include <boost/thread/mutex.hpp>
+#include <mutex>
-#include <boost/thread/recursive_mutex.hpp>
 
 
 ////////////////////////////////////////////////
@@ -22,18 +21,18 @@
 ////////////////////////////////////////////////
 
 /*
-CCriticalSection mutex;
+RecursiveMutex mutex;
-    boost::recursive_mutex mutex;
+    std::recursive_mutex mutex;
 
 LOCK(mutex);
-    boost::unique_lock<boost::recursive_mutex> criticalblock(mutex);
+    std::unique_lock<std::recursive_mutex> criticalblock(mutex);
 
 LOCK2(mutex1, mutex2);
-    boost::unique_lock<boost::recursive_mutex> criticalblock1(mutex1);
+    std::unique_lock<std::recursive_mutex> criticalblock1(mutex1);
-    boost::unique_lock<boost::recursive_mutex> criticalblock2(mutex2);
+    std::unique_lock<std::recursive_mutex> criticalblock2(mutex2);
 
 TRY_LOCK(mutex, name);
-    boost::unique_lock<boost::recursive_mutex> name(mutex, boost::try_to_lock_t);
+    std::unique_lock<std::recursive_mutex> name(mutex, std::try_to_lock_t);
 
 ENTER_CRITICAL_SECTION(mutex); // no RAII
     mutex.lock();
@@ -48,14 +47,42 @@ LEAVE_CRITICAL_SECTION(mutex); // no RAII
 //                           //
 ///////////////////////////////
 
+#ifdef DEBUG_LOCKORDER
+void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
+void LeaveCritical();
+std::string LocksHeld();
+void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs);
+void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
+void DeleteLock(void* cs);
+
 /**
- * Template mixin that adds -Wthread-safety locking
+ * Call abort() if a potential lock order deadlock bug is detected, instead of
- * annotations to a subset of the mutex API.
+ * just logging information and throwing a logic_error. Defaults to true, and
+ * set to false in DEBUG_LOCKORDER unit tests.
+ */
+extern bool g_debug_lockorder_abort;
+#else
+void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
+void static inline LeaveCritical() {}
+void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) ASSERT_EXCLUSIVE_LOCK(cs) {}
+void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
+void static inline DeleteLock(void* cs) {}
+#endif
+#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
+#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
+
+/**
+ * Template mixin that adds -Wthread-safety locking annotations and lock order
+ * checking to a subset of the mutex API.
  */
 template <typename PARENT>
 class LOCKABLE AnnotatedMixin : public PARENT
 {
 public:
+    ~AnnotatedMixin() {
+        DeleteLock((void*)this);
+    }
+
     void lock() EXCLUSIVE_LOCK_FUNCTION()
     {
         PARENT::lock();
@@ -70,63 +97,37 @@ public:
     {
         return PARENT::try_lock();
     }
-};
 
-#ifdef DEBUG_LOCKORDER
+    using UniqueLock = std::unique_lock<PARENT>;
-void EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false);
+};
-void LeaveCritical();
-std::string LocksHeld();
-void AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
-void AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs);
-void DeleteLock(void* cs);
-#else
-void static inline EnterCritical(const char* pszName, const char* pszFile, int nLine, void* cs, bool fTry = false) {}
-void static inline LeaveCritical() {}
-void static inline AssertLockHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
-void static inline AssertLockNotHeldInternal(const char* pszName, const char* pszFile, int nLine, void* cs) {}
-void static inline DeleteLock(void* cs) {}
-#endif
-#define AssertLockHeld(cs) AssertLockHeldInternal(#cs, __FILE__, __LINE__, &cs)
-#define AssertLockNotHeld(cs) AssertLockNotHeldInternal(#cs, __FILE__, __LINE__, &cs)
 
 /**
- * Wrapped boost mutex: supports recursive locking, but no waiting
+ * Wrapped mutex: supports recursive locking, but no waiting
  * TODO: We should move away from using the recursive lock by default.
  */
-class CCriticalSection : public AnnotatedMixin<boost::recursive_mutex>
+using RecursiveMutex = AnnotatedMixin<std::recursive_mutex>;
-{
+typedef AnnotatedMixin<std::recursive_mutex> CCriticalSection;
-public:
-    ~CCriticalSection() {
-        DeleteLock((void*)this);
-    }
-};
 
-typedef CCriticalSection CDynamicCriticalSection;
+/** Wrapped mutex: supports waiting but not recursive locking */
-/** Wrapped boost mutex: supports waiting but not recursive locking */
+typedef AnnotatedMixin<std::mutex> Mutex;
-typedef AnnotatedMixin<boost::mutex> CWaitableCriticalSection;
-
-/** Just a typedef for boost::condition_variable, can be wrapped later if desired */
-typedef boost::condition_variable CConditionVariable;
 
 #ifdef DEBUG_LOCKCONTENTION
 void PrintLockContention(const char* pszName, const char* pszFile, int nLine);
 #endif
 
-/** Wrapper around boost::unique_lock<Mutex> */
+/** Wrapper around std::unique_lock style lock for Mutex. */
-template <typename Mutex>
+template <typename Mutex, typename Base = typename Mutex::UniqueLock>
-class SCOPED_LOCKABLE CMutexLock
+class SCOPED_LOCKABLE UniqueLock : public Base
 {
 private:
-    boost::unique_lock<Mutex> lock;
-
     void Enter(const char* pszName, const char* pszFile, int nLine)
     {
-        EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()));
+        EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()));
 #ifdef DEBUG_LOCKCONTENTION
-        if (!lock.try_lock()) {
+        if (!Base::try_lock()) {
             PrintLockContention(pszName, pszFile, nLine);
 #endif
-            lock.lock();
+            Base::lock();
 #ifdef DEBUG_LOCKCONTENTION
         }
 #endif
@@ -134,15 +135,15 @@ private:
 
     bool TryEnter(const char* pszName, const char* pszFile, int nLine)
     {
-        EnterCritical(pszName, pszFile, nLine, (void*)(lock.mutex()), true);
+        EnterCritical(pszName, pszFile, nLine, (void*)(Base::mutex()), true);
-        lock.try_lock();
+        Base::try_lock();
-        if (!lock.owns_lock())
+        if (!Base::owns_lock())
             LeaveCritical();
-        return lock.owns_lock();
+        return Base::owns_lock();
     }
 
 public:
-    CMutexLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : lock(mutexIn, boost::defer_lock)
+    UniqueLock(Mutex& mutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(mutexIn) : Base(mutexIn, std::defer_lock)
     {
         if (fTry)
             TryEnter(pszName, pszFile, nLine);
@@ -150,37 +151,41 @@ public:
             Enter(pszName, pszFile, nLine);
     }
 
-    CMutexLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)
+    UniqueLock(Mutex* pmutexIn, const char* pszName, const char* pszFile, int nLine, bool fTry = false) EXCLUSIVE_LOCK_FUNCTION(pmutexIn)
     {
         if (!pmutexIn) return;
 
-        lock = boost::unique_lock<Mutex>(*pmutexIn, boost::defer_lock);
+        *static_cast<Base*>(this) = Base(*pmutexIn, std::defer_lock);
         if (fTry)
             TryEnter(pszName, pszFile, nLine);
         else
             Enter(pszName, pszFile, nLine);
     }
 
-    ~CMutexLock() UNLOCK_FUNCTION()
+    ~UniqueLock() UNLOCK_FUNCTION()
     {
-        if (lock.owns_lock())
+        if (Base::owns_lock())
             LeaveCritical();
     }
 
     operator bool()
     {
-        return lock.owns_lock();
+        return Base::owns_lock();
     }
 };
 
-typedef CMutexLock<CCriticalSection> CCriticalBlock;
+template<typename MutexArg>
+using DebugLock = UniqueLock<typename std::remove_reference<typename std::remove_pointer<MutexArg>::type>::type>;
 
 #define PASTE(x, y) x ## y
 #define PASTE2(x, y) PASTE(x, y)
 
-#define LOCK(cs) CCriticalBlock PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
+#define LOCK(cs) DebugLock<decltype(cs)> PASTE2(criticalblock, __COUNTER__)(cs, #cs, __FILE__, __LINE__)
-#define LOCK2(cs1, cs2) CCriticalBlock criticalblock1(cs1, #cs1, __FILE__, __LINE__), criticalblock2(cs2, #cs2, __FILE__, __LINE__)
+#define LOCK2(cs1, cs2)                                               \
-#define TRY_LOCK(cs, name) CCriticalBlock name(cs, #cs, __FILE__, __LINE__, true)
+    DebugLock<decltype(cs1)> criticalblock1(cs1, #cs1, __FILE__, __LINE__); \
+    DebugLock<decltype(cs2)> criticalblock2(cs2, #cs2, __FILE__, __LINE__);
+#define TRY_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__, true)
+#define WAIT_LOCK(cs, name) DebugLock<decltype(cs)> name(cs, #cs, __FILE__, __LINE__)
 
 #define ENTER_CRITICAL_SECTION(cs)                            \
     {                                                         \
@@ -197,8 +202,8 @@ typedef CMutexLock<CCriticalSection> CCriticalBlock;
 class CSemaphore
 {
 private:
-    boost::condition_variable condition;
+    std::condition_variable condition;
-    boost::mutex mutex;
+    std::mutex mutex;
     int value;
 
 public:
@@ -206,16 +211,14 @@ public:
 
     void wait()
     {
-        boost::unique_lock<boost::mutex> lock(mutex);
+        std::unique_lock<std::mutex> lock(mutex);
-        while (value < 1) {
+        condition.wait(lock, [&]() { return value >= 1; });
-            condition.wait(lock);
-        }
         value--;
     }
 
     bool try_wait()
     {
-        boost::unique_lock<boost::mutex> lock(mutex);
+        std::lock_guard<std::mutex> lock(mutex);
         if (value < 1)
             return false;
         value--;
@@ -225,7 +228,7 @@ public:
     void post()
     {
         {
-            boost::unique_lock<boost::mutex> lock(mutex);
+            std::lock_guard<std::mutex> lock(mutex);
             value++;
         }
         condition.notify_one();

src/test/key_tests.cpp

@@ -37,35 +37,6 @@ static const std::string addr2C = "t1VJL2dPUyXK7avDRGqhqQA5bw2eEMdhyg6";
 static const std::string strAddressBad = "t1aMkLwU1LcMZYN7TgXUJAwzA1r44dbLkSp";
 
 
-#ifdef KEY_TESTS_DUMPINFO
-void dumpKeyInfo(uint256 privkey)
-{
-    CKey key;
-    key.resize(32);
-    memcpy(&secret[0], &privkey, 32);
-    vector<unsigned char> sec;
-    sec.resize(32);
-    memcpy(&sec[0], &secret[0], 32);
-    printf("  * secret (hex): %s\n", HexStr(sec).c_str());
-
-    KeyIO keyIO(Params());
-    for (int nCompressed=0; nCompressed<2; nCompressed++)
-    {
-        bool fCompressed = nCompressed == 1;
-        printf("  * %s:\n", fCompressed ? "compressed" : "uncompressed");
-        CBitcoinSecret bsecret;
-        bsecret.SetSecret(secret, fCompressed);
-        printf("    * secret (base58): %s\n", bsecret.ToString().c_str());
-        CKey key;
-        key.SetSecret(secret, fCompressed);
-        vector<unsigned char> vchPubKey = key.GetPubKey();
-        printf("    * pubkey (hex): %s\n", HexStr(vchPubKey).c_str());
-        printf("    * address (base58): %s\n", keyIO.EncodeDestination(vchPubKey).c_str());
-    }
-}
-#endif
-
-
 BOOST_FIXTURE_TEST_SUITE(key_tests, BasicTestingSetup)
 
 BOOST_AUTO_TEST_CASE(key_test1)

src/test/script_tests.cpp

@@ -305,11 +305,6 @@ public:
     {
         return comment;
     }
-
-    const CScript& GetScriptPubKey()
-    {
-        return creditTx.vout[0].scriptPubKey;
-    }
 };
 }
 

src/test/sync_tests.cpp

@@ -0,0 +1,52 @@
+// Copyright (c) 2012-2017 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 <sync.h>
+#include <test/test_bitcoin.h>
+
+#include <boost/test/unit_test.hpp>
+
+namespace {
+template <typename MutexType>
+void TestPotentialDeadLockDetected(MutexType& mutex1, MutexType& mutex2)
+{
+    {
+        LOCK2(mutex1, mutex2);
+    }
+    bool error_thrown = false;
+    try {
+        LOCK2(mutex2, mutex1);
+    } catch (const std::logic_error& e) {
+        BOOST_CHECK_EQUAL(e.what(), "potential deadlock detected");
+        error_thrown = true;
+    }
+    #ifdef DEBUG_LOCKORDER
+    BOOST_CHECK(error_thrown);
+    #else
+    BOOST_CHECK(!error_thrown);
+    #endif
+}
+} // namespace
+
+BOOST_FIXTURE_TEST_SUITE(sync_tests, BasicTestingSetup)
+
+BOOST_AUTO_TEST_CASE(potential_deadlock_detected)
+{
+    #ifdef DEBUG_LOCKORDER
+    bool prev = g_debug_lockorder_abort;
+    g_debug_lockorder_abort = false;
+    #endif
+
+    CCriticalSection rmutex1, rmutex2;
+    TestPotentialDeadLockDetected(rmutex1, rmutex2);
+
+    Mutex mutex1, mutex2;
+    TestPotentialDeadLockDetected(mutex1, mutex2);
+
+    #ifdef DEBUG_LOCKORDER
+    g_debug_lockorder_abort = prev;
+    #endif
+}
+
+BOOST_AUTO_TEST_SUITE_END()

src/test/test_bitcoin.cpp

@@ -90,6 +90,11 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
         pcoinsdbview = new CCoinsViewDB(1 << 23, true);
         pcoinsTip = new CCoinsViewCache(pcoinsdbview);
         InitBlockIndex(chainparams);
+        {
+            CValidationState state;
+            bool ok = ActivateBestChain(state, chainparams);
+            BOOST_CHECK(ok);
+        }
         nScriptCheckThreads = 3;
         for (int i=0; i < nScriptCheckThreads-1; i++)
             threadGroup.create_thread(&ThreadScriptCheck);

src/threadsafety.h

@@ -32,6 +32,7 @@
 #define EXCLUSIVE_LOCKS_REQUIRED(...) __attribute__((exclusive_locks_required(__VA_ARGS__)))
 #define SHARED_LOCKS_REQUIRED(...) __attribute__((shared_locks_required(__VA_ARGS__)))
 #define NO_THREAD_SAFETY_ANALYSIS __attribute__((no_thread_safety_analysis))
+#define ASSERT_EXCLUSIVE_LOCK(...) __attribute((assert_exclusive_lock(__VA_ARGS__)))
 #else
 #define LOCKABLE
 #define SCOPED_LOCKABLE
@@ -51,6 +52,7 @@
 #define EXCLUSIVE_LOCKS_REQUIRED(...)
 #define SHARED_LOCKS_REQUIRED(...)
 #define NO_THREAD_SAFETY_ANALYSIS
+#define ASSERT_EXCLUSIVE_LOCK(...)
 #endif // __GNUC__
 
 #endif // BITCOIN_THREADSAFETY_H

src/txmempool.h

@@ -235,7 +235,43 @@ public:
         >
     > indexed_transaction_set;
 
-    mutable CCriticalSection cs;
+    /**
+     * This mutex needs to be locked when accessing `mapTx` or other members
+     * that are guarded by it.
+     *
+     * @par Consistency guarantees
+     *
+     * By design, it is guaranteed that:
+     *
+     * 1. Locking both `cs_main` and `mempool.cs` will give a view of mempool
+     *    that is consistent with current chain tip (`chainActive` and
+     *    `pcoinsTip`) and is fully populated. Fully populated means that if the
+     *    current active chain is missing transactions that were present in a
+     *    previously active chain, all the missing transactions will have been
+     *    re-added to the mempool and should be present if they meet size and
+     *    consistency constraints.
+     *
+     * 2. Locking `mempool.cs` without `cs_main` will give a view of a mempool
+     *    consistent with some chain that was active since `cs_main` was last
+     *    locked, and that is fully populated as described above. It is ok for
+     *    code that only needs to query or remove transactions from the mempool
+     *    to lock just `mempool.cs` without `cs_main`.
+     *
+     * To provide these guarantees, it is necessary to lock both `cs_main` and
+     * `mempool.cs` whenever adding transactions to the mempool and whenever
+     * changing the chain tip. It's necessary to keep both mutexes locked until
+     * the mempool is consistent with the new chain tip and fully populated.
+     *
+     * @par Consistency bug
+     *
+     * The second guarantee above is not currently enforced, but
+     * https://github.com/bitcoin/bitcoin/pull/14193 will fix it. No known code
+     * in bitcoin currently depends on second guarantee, but it is important to
+     * fix for third party code that needs be able to frequently poll the
+     * mempool without locking `cs_main` and without encountering missing
+     * transactions during reorgs.
+     */
+    mutable RecursiveMutex cs;
     indexed_transaction_set mapTx;
     typedef indexed_transaction_set::nth_index<0>::type::iterator txiter;
 

src/util.h

@@ -28,6 +28,7 @@
 #include <string>
 #include <vector>
 
+#include <boost/thread/condition_variable.hpp> // for boost::thread_interrupted
 #include <boost/thread/exceptions.hpp>
 
 extern std::map<std::string, std::string> mapArgs;

src/wallet/test/wallet_tests.cpp

@@ -91,7 +91,7 @@ BOOST_AUTO_TEST_CASE(coin_selection_tests)
         // we can't make 3 cents of mature coins
         BOOST_CHECK(!CWallet::SelectCoinsMinConf( 3 * CENT, 1, 6, vCoins, setCoinsRet, nValueRet));
 
-        // we can make 3 cents of new  coins
+        // we can make 3 cents of new coins
         BOOST_CHECK( CWallet::SelectCoinsMinConf( 3 * CENT, 1, 1, vCoins, setCoinsRet, nValueRet));
         BOOST_CHECK_EQUAL(nValueRet, 3 * CENT);
 

src/wallet/wallet.cpp

@@ -5581,7 +5581,7 @@ bool CWallet::CommitTransaction(CWalletTx& wtxNew, std::optional<std::reference_
 {
     {
         LOCK2(cs_main, cs_wallet);
-        LogPrintf("CommitTransaction:\n%s", wtxNew.ToString());
+        LogPrintf("CommitTransaction:\n%s", wtxNew.ToString()); /* Continued */
         {
             // This is only to keep the database open to defeat the auto-flush for the
             // duration of this scope.  This is the only place where this optimization

test/lint/lint-includes.sh

@@ -91,7 +91,6 @@ EXPECTED_BOOST_INCLUDES=(
     boost/thread/locks.hpp
     boost/thread/mutex.hpp
     boost/thread/once.hpp
-    boost/thread/recursive_mutex.hpp
     boost/thread/synchronized_value.hpp
     boost/thread/thread.hpp
     boost/thread/tss.hpp