From 11fed833b3ed6d5c96957de5addc4f903b2cee6c Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 19 May 2025 21:51:30 +0000 Subject: [PATCH 1/5] threading: add LOCK_ARGS macro This is useful for initializing locks in a constructor. --- src/sync.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/sync.h b/src/sync.h index b22956ef1ab..b71d5ef97a4 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) \ { \ From 4c8c90b5567a3f31444bf0b151c3109e85ac2329 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 6 May 2025 14:43:38 +0000 Subject: [PATCH 2/5] validation: only create a CCheckQueueControl if it's actually going to be used This will allow CCheckQueueControl to require a CCheckQueue. --- src/validation.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 5ad2ebdcd7e..c69a9959121 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2611,7 +2611,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 (fScriptChecks && parallel_script_checks) control.emplace(&m_chainman.GetCheckQueue()); + std::vector txsdata(block.vtx.size()); std::vector prevheights; @@ -2680,7 +2682,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, tx_state.GetRejectReason(), tx_state.GetDebugMessage()); break; } - control.Add(std::move(vChecks)); + if (control) control->Add(std::move(vChecks)); } CTxUndo undoDummy; @@ -2702,10 +2704,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()); From c3b0e6c7f4828291cd136717fddf1df878f3ca20 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Tue, 6 May 2025 15:14:50 +0000 Subject: [PATCH 3/5] validation: make CCheckQueueControl's CCheckQueue non-optional This simplifies the construction logic and will allow the constructor and destructor to lock and unlock uncondiationally. --- src/bench/checkqueue.cpp | 2 +- src/checkqueue.h | 20 ++++++-------------- src/test/checkqueue_tests.cpp | 16 ++++++++-------- src/test/fuzz/checkqueue.cpp | 2 +- src/test/transaction_tests.cpp | 2 +- src/validation.cpp | 2 +- 6 files changed, 18 insertions(+), 26 deletions(-) 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..e9eb96034cb 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -208,43 +208,35 @@ template ( class CCheckQueueControl { private: - CCheckQueue * const pqueue; + CCheckQueue& m_queue; bool fDone; public: CCheckQueueControl() = delete; CCheckQueueControl(const CCheckQueueControl&) = delete; CCheckQueueControl& operator=(const CCheckQueueControl&) = delete; - explicit CCheckQueueControl(CCheckQueue * const pqueueIn) : pqueue(pqueueIn), fDone(false) + explicit CCheckQueueControl(CCheckQueue& queueIn) : m_queue(queueIn), fDone(false) { - // passed queue is supposed to be unused, or nullptr - if (pqueue != nullptr) { - ENTER_CRITICAL_SECTION(pqueue->m_control_mutex); - } + ENTER_CRITICAL_SECTION(m_queue.m_control_mutex); } 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() { if (!fDone) Complete(); - if (pqueue != nullptr) { - LEAVE_CRITICAL_SECTION(pqueue->m_control_mutex); - } + LEAVE_CRITICAL_SECTION(m_queue.m_control_mutex); } }; 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 c69a9959121..d8e4bdfb7ef 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2612,7 +2612,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // doesn't invalidate pointers into the vector, and keep txsdata in scope // for as long as `control`. std::optional> control; - if (fScriptChecks && parallel_script_checks) control.emplace(&m_chainman.GetCheckQueue()); + if (fScriptChecks && parallel_script_checks) control.emplace(m_chainman.GetCheckQueue()); std::vector txsdata(block.vtx.size()); From 1a37507895402ee08b1f248262701d4f848647e1 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Fri, 9 May 2025 17:31:37 +0000 Subject: [PATCH 4/5] validation: use a lock for CCheckQueueControl Uses an RAII lock for the exact same behavior as the old critical sections. Co-authored-by: Ryan Ofsky --- src/checkqueue.h | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/checkqueue.h b/src/checkqueue.h index e9eb96034cb..5920d2935dc 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -205,20 +205,18 @@ public: * queue is finished before continuing. */ template ()().value())>> -class CCheckQueueControl +class SCOPED_LOCKABLE CCheckQueueControl { private: CCheckQueue& m_queue; + UniqueLock m_lock; bool fDone; public: CCheckQueueControl() = delete; CCheckQueueControl(const CCheckQueueControl&) = delete; CCheckQueueControl& operator=(const CCheckQueueControl&) = delete; - explicit CCheckQueueControl(CCheckQueue& queueIn) : m_queue(queueIn), fDone(false) - { - ENTER_CRITICAL_SECTION(m_queue.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() { @@ -232,11 +230,10 @@ public: m_queue.Add(std::move(vChecks)); } - ~CCheckQueueControl() + ~CCheckQueueControl() UNLOCK_FUNCTION() { if (!fDone) Complete(); - LEAVE_CRITICAL_SECTION(m_queue.m_control_mutex); } }; From fd290730f530a8b76a9607392f49830697cdd7c5 Mon Sep 17 00:00:00 2001 From: Cory Fields Date: Mon, 19 May 2025 21:53:39 +0000 Subject: [PATCH 5/5] validation: clean up and clarify CheckInputScripts logic CheckInputScripts behaves differently depending on whether or not it was called with a vector for checks. Make this difference clear by calling it differently depending on whether or not control exists. Though more verbose, it should be more straightforward to understand what's happening this way. Also remove parallel_script_checks, as `if(control)` is a better test. Co-authored-by: Ryan Ofsky --- src/validation.cpp | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d8e4bdfb7ef..8eafba5e218 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()}; @@ -2612,7 +2611,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, // doesn't invalidate pointers into the vector, and keep txsdata in scope // for as long as `control`. std::optional> control; - if (fScriptChecks && parallel_script_checks) control.emplace(m_chainman.GetCheckQueue()); + if (auto& queue = m_chainman.GetCheckQueue(); queue.HasThreads() && fScriptChecks) control.emplace(queue); std::vector txsdata(block.vtx.size()); @@ -2671,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; } - if (control) control->Add(std::move(vChecks)); } CTxUndo undoDummy;