From 4693f8165ff01a7354f78b219794616b351f582a Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 20 Mar 2018 21:04:27 -0700 Subject: [PATCH 1/3] Fix csBestBlock/cvBlockChange waiting in rpc/mining (cherry picked from commit bitcoin/bitcoin@45dd13503918e75a45ce33eb5c934b998790fdc8) --- src/main.cpp | 9 ++++++++- src/main.h | 5 +++++ src/rpc/mining.cpp | 14 +++++++------- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index a3f1d506e..0a7c44417 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -68,6 +68,8 @@ CBlockIndex *pindexBestHeader = NULL; static std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block CWaitableCriticalSection csBestBlock; CConditionVariable cvBlockChange; +uint256 hashBestBlock; +int heightBestBlock; 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 lock(csBestBlock); + hashBestBlock = pindexNew->GetBlockHash(); + heightBestBlock = pindexNew->nHeight; + cvBlockChange.notify_all(); + } } /** diff --git a/src/main.h b/src/main.h index a76fe63e6..f9df0527a 100644 --- a/src/main.h +++ b/src/main.h @@ -160,8 +160,13 @@ extern BlockMap mapBlockIndex; extern std::optional last_block_num_txs; extern std::optional last_block_size; extern const std::string strMessageMagic; + +// These prevent lock-ordering problems in getblocktemplate() RPC extern CWaitableCriticalSection csBestBlock; extern CConditionVariable cvBlockChange; +extern uint256 hashBestBlock; +extern int heightBestBlock; + extern std::atomic_bool fImporting; extern std::atomic_bool fReindex; extern int nScriptCheckThreads; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index c9f6fc73d..03d7fa291 100644 --- a/src/rpc/mining.cpp +++ b/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 lock(csBestBlock); - while (chainActive.Tip()->GetBlockHash() == hashWatchedChain && IsRPCRunning()) + while (hashBestBlock == 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. @@ -629,11 +629,10 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) next_cb_mtx = cached_next_cb_mtx; } bool timedout = !cvBlockChange.timed_wait(lock, checktxtime); - ENTER_CRITICAL_SECTION(cs_main); // 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 (hashBestBlock != 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 (heightBestBlock != 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"); From c079a518c066a0c8e24d844892f8a10c7638b9d1 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 3 Apr 2018 21:53:07 -0700 Subject: [PATCH 2/3] Modernize best block mutex/cv/hash variable naming (cherry picked from commit bitcoin/bitcoin@4a6c0e3dcfdca98270cb96b73db4c3d4446dba50) --- src/init.cpp | 2 +- src/main.cpp | 16 ++++++++-------- src/main.h | 8 ++++---- src/rpc/mining.cpp | 10 +++++----- 4 files changed, 18 insertions(+), 18 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index f815416e5..e070bf944 100644 --- a/src/init.cpp +++ b/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"); } diff --git a/src/main.cpp b/src/main.cpp index 0a7c44417..244624bb5 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -66,10 +66,10 @@ BlockMap mapBlockIndex; CChain chainActive; CBlockIndex *pindexBestHeader = NULL; static std::atomic nTimeBestReceived(0); // Used only to inform the wallet of when we last received a block -CWaitableCriticalSection csBestBlock; -CConditionVariable cvBlockChange; -uint256 hashBestBlock; -int heightBestBlock; +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); @@ -3760,10 +3760,10 @@ void static UpdateTip(CBlockIndex *pindexNew, const CChainParams& chainParams) { RenderPoolMetrics("transparent", transparentPool); { - boost::unique_lock lock(csBestBlock); - hashBestBlock = pindexNew->GetBlockHash(); - heightBestBlock = pindexNew->nHeight; - cvBlockChange.notify_all(); + boost::unique_lock lock(g_best_block_mutex); + g_best_block = pindexNew->GetBlockHash(); + g_best_block_height = pindexNew->nHeight; + g_best_block_cv.notify_all(); } } diff --git a/src/main.h b/src/main.h index f9df0527a..b5c8ba90c 100644 --- a/src/main.h +++ b/src/main.h @@ -162,10 +162,10 @@ extern std::optional last_block_size; extern const std::string strMessageMagic; // These prevent lock-ordering problems in getblocktemplate() RPC -extern CWaitableCriticalSection csBestBlock; -extern CConditionVariable cvBlockChange; -extern uint256 hashBestBlock; -extern int heightBestBlock; +extern CWaitableCriticalSection g_best_block_mutex; +extern CConditionVariable g_best_block_cv; +extern uint256 g_best_block; +extern int g_best_block_height; extern std::atomic_bool fImporting; extern std::atomic_bool fReindex; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 03d7fa291..ef3621d7b 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -613,8 +613,8 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) { checktxtime = boost::get_system_time() + boost::posix_time::seconds(10); - boost::unique_lock lock(csBestBlock); - while (hashBestBlock == hashWatchedChain && IsRPCRunning()) + boost::unique_lock lock(g_best_block_mutex); + while (g_best_block == hashWatchedChain && IsRPCRunning()) { // Before waiting, generate the coinbase for the block following the next // block (since this is cpu-intensive), so that when next block arrives, @@ -628,11 +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); + 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 (hashBestBlock != hashWatchedChain) break; + if (g_best_block != hashWatchedChain) break; // Timeout: Check transactions for update if (timedout && mempool.GetTransactionsUpdated() != nTransactionsUpdatedLastLP) { @@ -642,7 +642,7 @@ UniValue getblocktemplate(const UniValue& params, bool fHelp) } checktxtime += boost::posix_time::seconds(10); } - if (heightBestBlock != 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; } From e170c3abd6e4ea448d37b08819ed94b4ed822eba Mon Sep 17 00:00:00 2001 From: Larry Ruane Date: Tue, 1 Mar 2022 17:43:43 -0700 Subject: [PATCH 3/3] document global variables --- src/main.h | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main.h b/src/main.h index b5c8ba90c..da065f63a 100644 --- a/src/main.h +++ b/src/main.h @@ -161,11 +161,17 @@ extern std::optional last_block_num_txs; extern std::optional last_block_size; extern const std::string strMessageMagic; -// These prevent lock-ordering problems in getblocktemplate() RPC +//! 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 uint256 g_best_block; extern int g_best_block_height; +extern uint256 g_best_block; extern std::atomic_bool fImporting; extern std::atomic_bool fReindex;