From d9957623b48a7c3eff0ac750d1245fabfb1843a2 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 2 Nov 2019 17:13:06 -0400 Subject: [PATCH 1/2] [tests] Don't use TestingSetup in the checkqueue_tests It's only needed for a hardcoded int, which we can define locally. --- src/test/checkqueue_tests.cpp | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index d796444419f..d1259dc2687 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -5,7 +5,6 @@ #include #include #include -#include #include #include @@ -19,11 +18,10 @@ #include -// 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()() @@ -149,7 +147,7 @@ static void Correct_Queue_range(std::vector range) { auto small_queue = MakeUnique(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...) @@ -214,7 +212,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) auto fail_queue = MakeUnique(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();}); } @@ -246,7 +244,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) { auto fail_queue = MakeUnique(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();}); } @@ -274,7 +272,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) { auto queue = MakeUnique(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();}); } @@ -310,7 +308,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory) { auto queue = MakeUnique(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) { @@ -342,7 +340,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) auto queue = MakeUnique(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([&]() { From 5506ecfe7a65d5705616bc048f2f1735b89993fb Mon Sep 17 00:00:00 2001 From: John Newbery Date: Sat, 2 Nov 2019 17:14:38 -0400 Subject: [PATCH 2/2] [refactor] Replace global int nScriptCheckThreads with bool The global nScriptCheckThreads int is confusing and is only needed for its int-ness in AppInitMain. Move all `-par` parsing logic there and replace the int nScriptCheckThreads with a bool g_parallel_script_checks. Also tidy up logic and improve comments. --- src/init.cpp | 30 ++++++++++++++++++------------ src/test/setup_common.cpp | 7 +++++-- src/validation.cpp | 6 +++--- src/validation.h | 9 ++++++--- 4 files changed, 32 insertions(+), 20 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index 1a99ca9abcd..c8725f25966 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -1061,15 +1061,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) { @@ -1256,10 +1247,25 @@ bool AppInitMain(NodeContext& node) InitSignatureCache(); InitScriptExecutionCache(); - LogPrintf("Script verification uses %d additional threads\n", std::max(nScriptCheckThreads - 1, 0)); - if (nScriptCheckThreads) { - for (int i=0; i= 1) { + g_parallel_script_checks = true; + for (int i = 0; i < script_threads; ++i) { threadGroup.create_thread([i]() { return ThreadScriptCheck(i); }); + } } // Start the lightweight task scheduler thread diff --git a/src/test/setup_common.cpp b/src/test/setup_common.cpp index 3425bd59c1a..5e7da84dfb2 100644 --- a/src/test/setup_common.cpp +++ b/src/test/setup_common.cpp @@ -102,9 +102,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; m_node.banman = MakeUnique(GetDataDir() / "banlist.dat", nullptr, DEFAULT_MISBEHAVING_BANTIME); m_node.connman = MakeUnique(0x1337, 0x1337); // Deterministic randomness for tests. diff --git a/src/validation.cpp b/src/validation.cpp index ca6d2176b3e..df01b3abcfd 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -107,7 +107,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 fHavePruned = false; @@ -2069,7 +2069,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, CBlockUndo blockundo; - CCheckQueueControl control(fScriptChecks && nScriptCheckThreads ? &scriptcheckqueue : nullptr); + CCheckQueueControl control(fScriptChecks && g_parallel_script_checks ? &scriptcheckqueue : nullptr); std::vector prevheights; CAmount nFees = 0; @@ -2130,7 +2130,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ TxValidationState tx_state; - if (fScriptChecks && !CheckInputs(tx, tx_state, view, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) { + if (fScriptChecks && !CheckInputs(tx, tx_state, view, flags, fCacheResults, fCacheResults, txdata[i], g_parallel_script_checks ? &vChecks : nullptr)) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); diff --git a/src/validation.h b/src/validation.h index 7f9582adfd0..fd846e90909 100644 --- a/src/validation.h +++ b/src/validation.h @@ -76,8 +76,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. */ @@ -146,7 +146,10 @@ 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; +/** 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 bool fCheckBlockIndex; extern bool fCheckpointsEnabled;