merge bitcoin#17342: Clean up nScriptCheckThreads

17342

Author: kwvg

src/init.cpp

@@ -1326,15 +1326,6 @@ bool AppInitParameterInteraction()
         incrementalRelayFee = CFeeRate(n);
     }
 
-    // -par=0 means autodetect, but nScriptCheckThreads==0 means no concurrency
-    nScriptCheckThreads = gArgs.GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
-    if (nScriptCheckThreads <= 0)
-        nScriptCheckThreads += GetNumCores();
-    if (nScriptCheckThreads <= 1)
-        nScriptCheckThreads = 0;
-    else if (nScriptCheckThreads > MAX_SCRIPTCHECK_THREADS)
-        nScriptCheckThreads = MAX_SCRIPTCHECK_THREADS;
-
     // block pruning; get the amount of disk space (in MiB) to allot for block & undo files
     int64_t nPruneArg = gArgs.GetArg("-prune", 0);
     if (nPruneArg < 0) {
@@ -1730,10 +1721,25 @@ bool AppInitMain()
     InitSignatureCache();
     InitScriptExecutionCache();
 
-    LogPrintf("Using %u threads for script verification\n", nScriptCheckThreads);
-    if (nScriptCheckThreads) {
-        for (int i=0; i<nScriptCheckThreads-1; i++)
+    int script_threads = gArgs.GetArg("-par", DEFAULT_SCRIPTCHECK_THREADS);
+    if (script_threads <= 0) {
+        // -par=0 means autodetect (number of cores - 1 script threads)
+        // -par=-n means "leave n cores free" (number of cores - n - 1 script threads)
+        script_threads += GetNumCores();
+    }
+
+    // Subtract 1 because the main thread counts towards the par threads
+    script_threads = std::max(script_threads - 1, 0);
+
+    // Number of script-checking threads <= MAX_SCRIPTCHECK_THREADS
+    script_threads = std::min(script_threads, MAX_SCRIPTCHECK_THREADS);
+
+    LogPrintf("Script verification uses %d additional threads\n", script_threads);
+    if (script_threads >= 1) {
+        g_parallel_script_checks = true;
+        for (int i = 0; i < script_threads; ++i) {
             threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
+        }
     }
 
     std::vector<std::string> vSporkAddresses;

src/test/checkqueue_tests.cpp

@@ -5,7 +5,6 @@
 #include <sync.h>
 #include <util.h>
 #include <utiltime.h>
-#include <validation.h>
 
 #include <test/test_dash.h>
 #include <checkqueue.h>
@@ -21,11 +20,10 @@
 #include <memory>
 #include <random.h>
 
-// BasicTestingSetup not sufficient because nScriptCheckThreads is not set
-// otherwise.
 BOOST_FIXTURE_TEST_SUITE(checkqueue_tests, TestingSetup)
 
 static const unsigned int QUEUE_BATCH_SIZE = 128;
+static const int SCRIPT_CHECK_THREADS = 3;
 
 struct FakeCheck {
     bool operator()()
@@ -151,7 +149,7 @@ static void Correct_Queue_range(std::vector<size_t> range)
 {
     auto small_queue = std::unique_ptr<Correct_Queue>(new Correct_Queue {QUEUE_BATCH_SIZE});
     boost::thread_group tg;
-    for (auto x = 0; x < nScriptCheckThreads; ++x) {
+    for (auto x = 0; x < SCRIPT_CHECK_THREADS; ++x) {
        tg.create_thread([&]{small_queue->Thread();});
     }
     // Make vChecks here to save on malloc (this test can be slow...)
@@ -217,7 +215,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure)
     auto fail_queue = std::unique_ptr<Failing_Queue>(new Failing_Queue {QUEUE_BATCH_SIZE});
 
     boost::thread_group tg;
-    for (auto x = 0; x < nScriptCheckThreads; ++x) {
+    for (auto x = 0; x < SCRIPT_CHECK_THREADS; ++x) {
        tg.create_thread([&]{fail_queue->Thread();});
     }
 
@@ -249,7 +247,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure)
 {
     auto fail_queue = std::unique_ptr<Failing_Queue>(new Failing_Queue {QUEUE_BATCH_SIZE});
     boost::thread_group tg;
-    for (auto x = 0; x < nScriptCheckThreads; ++x) {
+    for (auto x = 0; x < SCRIPT_CHECK_THREADS; ++x) {
        tg.create_thread([&]{fail_queue->Thread();});
     }
 
@@ -277,7 +275,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck)
 {
     auto queue = std::unique_ptr<Unique_Queue>(new Unique_Queue {QUEUE_BATCH_SIZE});
     boost::thread_group tg;
-    for (auto x = 0; x < nScriptCheckThreads; ++x) {
+    for (auto x = 0; x < SCRIPT_CHECK_THREADS; ++x) {
        tg.create_thread([&]{queue->Thread();});
 
     }
@@ -317,7 +315,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory)
 {
     auto queue = std::unique_ptr<Memory_Queue>(new Memory_Queue {QUEUE_BATCH_SIZE});
     boost::thread_group tg;
-    for (auto x = 0; x < nScriptCheckThreads; ++x) {
+    for (auto x = 0; x < SCRIPT_CHECK_THREADS; ++x) {
        tg.create_thread([&]{queue->Thread();});
     }
     for (size_t i = 0; i < 1000; ++i) {
@@ -349,7 +347,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup)
     auto queue = std::unique_ptr<FrozenCleanup_Queue>(new FrozenCleanup_Queue {QUEUE_BATCH_SIZE});
     boost::thread_group tg;
     bool fails = false;
-    for (auto x = 0; x < nScriptCheckThreads; ++x) {
+    for (auto x = 0; x < SCRIPT_CHECK_THREADS; ++x) {
         tg.create_thread([&]{queue->Thread();});
     }
     std::thread t0([&]() {

src/test/test_dash.cpp

@@ -120,9 +120,12 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
                 throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", FormatStateMessage(state)));
             }
         }
-        nScriptCheckThreads = 3;
-        for (int i=0; i < nScriptCheckThreads-1; i++)
+        // Start script-checking threads. Set g_parallel_script_checks to true so they are used.
+        constexpr int script_check_threads = 2;
+        for (int i = 0; i < script_check_threads; ++i) {
             threadGroup.create_thread([i]() { return ThreadScriptCheck(i); });
+        }
+        g_parallel_script_checks = true;
         peerLogic.reset(new PeerLogicValidation(connman, scheduler, /*enable_bip61=*/true));
 }
 

src/validation.cpp

@@ -227,7 +227,7 @@ CBlockIndex *pindexBestHeader = nullptr;
 Mutex g_best_block_mutex;
 std::condition_variable g_best_block_cv;
 uint256 g_best_block;
-int nScriptCheckThreads = 0;
+bool g_parallel_script_checks{false};
 std::atomic_bool fImporting(false);
 std::atomic_bool fReindex(false);
 bool fAddressIndex = false;
@@ -2151,7 +2151,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
 
     CBlockUndo blockundo;
 
-    CCheckQueueControl<CScriptCheck> control(fScriptChecks && nScriptCheckThreads ? &scriptcheckqueue : nullptr);
+    CCheckQueueControl<CScriptCheck> control(fScriptChecks && g_parallel_script_checks ? &scriptcheckqueue : nullptr);
 
     std::vector<int> prevheights;
     CAmount nFees = 0;
@@ -2264,9 +2264,10 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl
 
             std::vector<CScriptCheck> vChecks;
             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
-            if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr))
+            if (fScriptChecks && !CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], g_parallel_script_checks ? &vChecks : nullptr)) {
                 return error("ConnectBlock(): CheckInputs on %s failed with %s",
                     tx.GetHash().ToString(), FormatStateMessage(state));
+            }
             control.Add(vChecks);
         }
 

src/validation.h

@@ -79,8 +79,8 @@ static const unsigned int BLOCKFILE_CHUNK_SIZE = 0x1000000; // 16 MiB
 /** The pre-allocation chunk size for rev?????.dat files (since 0.8) */
 static const unsigned int UNDOFILE_CHUNK_SIZE = 0x100000; // 1 MiB
 
-/** Maximum number of script-checking threads allowed */
-static const int MAX_SCRIPTCHECK_THREADS = 16;
+/** Maximum number of dedicated script-checking threads allowed */
+static const int MAX_SCRIPTCHECK_THREADS = 15;
 /** -par default (number of script-checking threads, 0 = auto) */
 static const int DEFAULT_SCRIPTCHECK_THREADS = 0;
 /** Number of blocks that can be requested at any given time from a single peer. */
@@ -162,11 +162,14 @@ extern std::condition_variable g_best_block_cv;
 extern uint256 g_best_block;
 extern std::atomic_bool fImporting;
 extern std::atomic_bool fReindex;
-extern int nScriptCheckThreads;
 extern bool fAddressIndex;
 extern bool fTimestampIndex;
 extern bool fSpentIndex;
 extern bool fIsBareMultisigStd;
+/** Whether there are dedicated script-checking threads running.
+ * False indicates all script checking is done on the main threadMessageHandler thread.
+ */
+extern bool g_parallel_script_checks;
 extern bool fRequireStandard;
 extern unsigned int nBytesPerSigOp;
 extern bool fCheckBlockIndex;