diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index 8134154eb11..7255b4f1376 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -56,7 +56,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) bench.minEpochIterations(10).batch(BATCH_SIZE * BATCHES).unit("job").run([&] { // Make insecure_rand here so that each iteration is identical. - CCheckQueueControl control(&queue); + CCheckQueueControl control(queue); for (auto vChecks : vBatches) { control.Add(std::move(vChecks)); } diff --git a/src/checkqueue.h b/src/checkqueue.h index 934f672ae39..5920d2935dc 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -205,46 +205,35 @@ public: * queue is finished before continuing. */ template ()().value())>> -class CCheckQueueControl +class SCOPED_LOCKABLE CCheckQueueControl { private: - CCheckQueue * const pqueue; + CCheckQueue& m_queue; + UniqueLock m_lock; bool fDone; public: CCheckQueueControl() = delete; CCheckQueueControl(const CCheckQueueControl&) = delete; CCheckQueueControl& operator=(const CCheckQueueControl&) = delete; - explicit CCheckQueueControl(CCheckQueue * const pqueueIn) : pqueue(pqueueIn), fDone(false) - { - // passed queue is supposed to be unused, or nullptr - if (pqueue != nullptr) { - ENTER_CRITICAL_SECTION(pqueue->m_control_mutex); - } - } + explicit CCheckQueueControl(CCheckQueue& queueIn) EXCLUSIVE_LOCK_FUNCTION(queueIn.m_control_mutex) : m_queue(queueIn), m_lock(LOCK_ARGS(queueIn.m_control_mutex)), fDone(false) {} std::optional Complete() { - if (pqueue == nullptr) return std::nullopt; - auto ret = pqueue->Complete(); + auto ret = m_queue.Complete(); fDone = true; return ret; } void Add(std::vector&& vChecks) { - if (pqueue != nullptr) { - pqueue->Add(std::move(vChecks)); - } + m_queue.Add(std::move(vChecks)); } - ~CCheckQueueControl() + ~CCheckQueueControl() UNLOCK_FUNCTION() { if (!fDone) Complete(); - if (pqueue != nullptr) { - LEAVE_CRITICAL_SECTION(pqueue->m_control_mutex); - } } }; diff --git a/src/sync.h b/src/sync.h index 6eb45f29884..64694237f0e 100644 --- a/src/sync.h +++ b/src/sync.h @@ -258,8 +258,9 @@ inline MutexType* MaybeCheckNotHeld(MutexType* m) LOCKS_EXCLUDED(m) LOCK_RETURNE #define LOCK2(cs1, cs2) \ UniqueLock criticalblock1(MaybeCheckNotHeld(cs1), #cs1, __FILE__, __LINE__); \ UniqueLock criticalblock2(MaybeCheckNotHeld(cs2), #cs2, __FILE__, __LINE__) -#define TRY_LOCK(cs, name) UniqueLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__, true) -#define WAIT_LOCK(cs, name) UniqueLock name(MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__) +#define LOCK_ARGS(cs) MaybeCheckNotHeld(cs), #cs, __FILE__, __LINE__ +#define TRY_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs), true) +#define WAIT_LOCK(cs, name) UniqueLock name(LOCK_ARGS(cs)) #define ENTER_CRITICAL_SECTION(cs) \ { \ diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 2463ce6da56..da9c3aa85d8 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -165,7 +165,7 @@ void CheckQueueTest::Correct_Queue_range(std::vector range) for (const size_t i : range) { size_t total = i; FakeCheckCheckCompletion::n_calls = 0; - CCheckQueueControl control(small_queue.get()); + CCheckQueueControl control(*small_queue); while (total) { vChecks.clear(); vChecks.resize(std::min(total, m_rng.randrange(10))); @@ -220,7 +220,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) { auto fixed_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (size_t i = 0; i < 1001; ++i) { - CCheckQueueControl control(fixed_queue.get()); + CCheckQueueControl control(*fixed_queue); size_t remaining = i; while (remaining) { size_t r = m_rng.randrange(10); @@ -246,7 +246,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) auto fail_queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); for (auto times = 0; times < 10; ++times) { for (const bool end_fails : {true, false}) { - CCheckQueueControl control(fail_queue.get()); + CCheckQueueControl control(*fail_queue); { std::vector vChecks; vChecks.resize(100, FixedCheck(std::nullopt)); @@ -268,7 +268,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) size_t COUNT = 100000; size_t total = COUNT; { - CCheckQueueControl control(queue.get()); + CCheckQueueControl control(*queue); while (total) { size_t r = m_rng.randrange(10); std::vector vChecks; @@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory) for (size_t i = 0; i < 1000; ++i) { size_t total = i; { - CCheckQueueControl control(queue.get()); + CCheckQueueControl control(*queue); while (total) { size_t r = m_rng.randrange(10); std::vector vChecks; @@ -324,7 +324,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) auto queue = std::make_unique(QUEUE_BATCH_SIZE, SCRIPT_CHECK_THREADS); bool fails = false; std::thread t0([&]() { - CCheckQueueControl control(queue.get()); + CCheckQueueControl control(*queue); std::vector vChecks(1); control.Add(std::move(vChecks)); auto result = control.Complete(); // Hangs here @@ -364,7 +364,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks) for (size_t i = 0; i < 3; ++i) { tg.emplace_back( [&]{ - CCheckQueueControl control(queue.get()); + CCheckQueueControl control(*queue); // While sleeping, no other thread should execute to this point auto observed = ++nThreads; UninterruptibleSleep(std::chrono::milliseconds{10}); @@ -387,7 +387,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueueControl_Locks) { std::unique_lock l(m); tg.emplace_back([&]{ - CCheckQueueControl control(queue.get()); + CCheckQueueControl control(*queue); std::unique_lock ll(m); has_lock = true; cv.notify_one(); diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index 6b93886c711..e053fa618d4 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -49,7 +49,7 @@ FUZZ_TARGET(checkqueue) (void)check_queue_1.Complete(); } - CCheckQueueControl check_queue_control{&check_queue_2}; + CCheckQueueControl check_queue_control{check_queue_2}; if (fuzzed_data_provider.ConsumeBool()) { check_queue_control.Add(std::move(checks_2)); } diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 1375672a418..7a60ea25d3f 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -568,7 +568,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) // check all inputs concurrently, with the cache PrecomputedTransactionData txdata(tx); CCheckQueue scriptcheckqueue(/*batch_size=*/128, /*worker_threads_num=*/20); - CCheckQueueControl control(&scriptcheckqueue); + CCheckQueueControl control(scriptcheckqueue); std::vector coins; for(uint32_t i = 0; i < mtx.vin.size(); i++) { diff --git a/src/validation.cpp b/src/validation.cpp index 13dbe83f806..b11142f431e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2421,7 +2421,6 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, uint256 block_hash{block.GetHash()}; assert(*pindex->phashBlock == block_hash); - const bool parallel_script_checks{m_chainman.GetCheckQueue().HasThreads()}; const auto time_start{SteadyClock::now()}; const CChainParams& params{m_chainman.GetParams()}; @@ -2611,7 +2610,9 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // in multiple threads). Preallocate the vector size so a new allocation // doesn't invalidate pointers into the vector, and keep txsdata in scope // for as long as `control`. - CCheckQueueControl control(fScriptChecks && parallel_script_checks ? &m_chainman.GetCheckQueue() : nullptr); + std::optional> control; + if (auto& queue = m_chainman.GetCheckQueue(); queue.HasThreads() && fScriptChecks) control.emplace(queue); + std::vector txsdata(block.vtx.size()); std::vector prevheights; @@ -2669,18 +2670,26 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, break; } - if (!tx.IsCoinBase()) + if (!tx.IsCoinBase() && fScriptChecks) { - std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ + bool tx_ok; TxValidationState tx_state; - if (fScriptChecks && !CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, parallel_script_checks ? &vChecks : nullptr)) { + // If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case + // they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning. + if (control) { + std::vector vChecks; + tx_ok = CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks); + if (tx_ok) control->Add(std::move(vChecks)); + } else { + tx_ok = CheckInputScripts(tx, tx_state, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache); + } + if (!tx_ok) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); break; } - control.Add(std::move(vChecks)); } CTxUndo undoDummy; @@ -2702,10 +2711,11 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, "bad-cb-amount", strprintf("coinbase pays too much (actual=%d vs limit=%d)", block.vtx[0]->GetValueOut(), blockReward)); } - - auto parallel_result = control.Complete(); - if (parallel_result.has_value() && state.IsValid()) { - state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(parallel_result->first)), parallel_result->second); + if (control) { + auto parallel_result = control->Complete(); + if (parallel_result.has_value() && state.IsValid()) { + state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(parallel_result->first)), parallel_result->second); + } } if (!state.IsValid()) { LogInfo("Block validation error: %s", state.ToString());