Merge pull request #5594 from LarryRuane/2022-02-getblocktemplate-locking

Fix csBestBlock/cvBlockChange waiting in rpc/mining

src/init.cpp

@@ -305,7 +305,7 @@ bool static Bind(const CService &addr, unsigned int flags) {
 
 void OnRPCStopped()
 {
-    cvBlockChange.notify_all();
+    g_best_block_cv.notify_all();
     LogPrint("rpc", "RPC stopped.\n");
 }
 

src/main.cpp

@@ -66,8 +66,10 @@ 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 csBestBlock;
-CConditionVariable cvBlockChange;
+CWaitableCriticalSection g_best_block_mutex;
+CConditionVariable g_best_block_cv;
+uint256 g_best_block;
+int g_best_block_height;
 int nScriptCheckThreads = 0;
 std::atomic_bool fImporting(false);
 std::atomic_bool fReindex(false);
@@ -3757,7 +3759,12 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) {
     RenderPoolMetrics("sapling", saplingPool);
     RenderPoolMetrics("transparent", transparentPool);
 
-    cvBlockChange.notify_all();
+    {
+        boost::unique_lock<boost::mutex> lock(g_best_block_mutex);
+        g_best_block = pindexNew->GetBlockHash();
+        g_best_block_height = pindexNew->nHeight;
+        g_best_block_cv.notify_all();
+    }
 }
 
 /**

src/main.h

@@ -160,8 +160,19 @@ extern BlockMap mapBlockIndex;
 extern std::optional<uint64_t> last_block_num_txs;
 extern std::optional<uint64_t> last_block_size;
 extern const std::string strMessageMagic;
-extern CWaitableCriticalSection csBestBlock;
-extern CConditionVariable cvBlockChange;
+
+//! These four variables are used to notify getblocktemplate RPC of new tips.
+//! When UpdateTip() establishes a new tip (best block), it must awaken a
+//! waiting getblocktemplate RPC (if there is one) immediately. But upon waking
+//! up, getblocktemplate cannot call chainActive->Tip() because it does not
+//! (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 CConditionVariable g_best_block_cv;
+extern int g_best_block_height;
+extern uint256 g_best_block;
+
 extern std::atomic_bool fImporting;
 extern std::atomic_bool fReindex;
 extern int nScriptCheckThreads;

src/rpc/mining.cpp

@@ -607,15 +607,15 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
             nTransactionsUpdatedLastLP = nTransactionsUpdatedLast;
         }
 
+        // Release the main lock while waiting
+        // Don't call chainActive->Tip() without holding cs_main
+        LEAVE_CRITICAL_SECTION(cs_main);
         {
             checktxtime = boost::get_system_time() + boost::posix_time::seconds(10);
 
-            boost::unique_lock<boost::mutex> lock(csBestBlock);
-            while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning())
+            boost::unique_lock<boost::mutex> lock(g_best_block_mutex);
+            while (g_best_block == hashWatchedChain && IsRPCRunning())
             {
-                // Release the main lock while waiting
-                LEAVE_CRITICAL_SECTION(cs_main);
-
                 // Before waiting, generate the coinbase for the block following the next
                 // block (since this is cpu-intensive), so that when next block arrives,
                 // we can quickly respond with a template for following block.
@@ -628,12 +628,11 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
                         Params(), CAmount{0}, minerAddress, cached_next_cb_height);
                     next_cb_mtx = cached_next_cb_mtx;
                 }
-                bool timedout = !cvBlockChange.timed_wait(lock, checktxtime);
-                ENTER_CRITICAL_SECTION(cs_main);
+                bool timedout = !g_best_block_cv.timed_wait(lock, checktxtime);
 
                 // Optimization: even if timed out, a new block may have arrived
                 // while waiting for cs_main; if so, don't discard next_cb_mtx.
-                if (chainActive.Tip()->GetBlockHash() != hashWatchedChain) break;
+                if (g_best_block != hashWatchedChain) break;
 
                 // Timeout: Check transactions for update
                 if (timedout && mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) {
@@ -643,11 +642,12 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp)
                 }
                 checktxtime += boost::posix_time::seconds(10);
             }
-            if (chainActive.Tip()->nHeight != nHeight + 1) {
+            if (g_best_block_height != nHeight + 1) {
                 // Unexpected height (reorg or >1 blocks arrived while waiting) invalidates coinbase tx.
                 next_cb_mtx = nullopt;
             }
         }
+        ENTER_CRITICAL_SECTION(cs_main);
 
         if (!IsRPCRunning())
             throw JSONRPCError(RPC_CLIENT_NOT_CONNECTED, "Shutting down");