From 15209d97c6aad7d5c199fe007ad39b91c8ee6562 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:03:16 +0000 Subject: [PATCH 01/10] consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts` --- src/validation.cpp | 3 +-- src/validation.h | 5 +++++ 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index d1b941f081..dce066412c 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1815,8 +1815,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // Verify signature CScriptCheck check(txdata.m_spent_outputs[i], tx, i, flags, cacheSigStore, &txdata); if (pvChecks) { - pvChecks->push_back(CScriptCheck()); - check.swap(pvChecks->back()); + pvChecks->emplace_back(std::move(check)); } else if (!check()) { if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { // Check whether the failure was caused by a diff --git a/src/validation.h b/src/validation.h index 53e9a7ee50..7bcb35d9a4 100644 --- a/src/validation.h +++ b/src/validation.h @@ -322,6 +322,11 @@ public: CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) : m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR), txdata(txdataIn) { } + CScriptCheck(const CScriptCheck&) = delete; + CScriptCheck& operator=(const CScriptCheck&) = delete; + CScriptCheck(CScriptCheck&&) = default; + CScriptCheck& operator=(CScriptCheck&&) = default; + bool operator()(); void swap(CScriptCheck& check) noexcept From 06820032142a75cc3c5b832045058bc6f6f74786 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:03:30 +0000 Subject: [PATCH 02/10] test, refactor: Avoid `CScriptCheck::swap` in `transaction_tests` --- src/test/transaction_tests.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 11efb6a5c3..f7ea99f6a6 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -547,9 +547,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) for(uint32_t i = 0; i < mtx.vin.size(); i++) { std::vector vChecks; - CScriptCheck check(coins[tx.vin[i].prevout.n].out, tx, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata); - vChecks.push_back(CScriptCheck()); - check.swap(vChecks.back()); + vChecks.emplace_back(coins[tx.vin[i].prevout.n].out, tx, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata); control.Add(vChecks); } From 6c2d5972f3544c4f3e987828a99e88f27b62cf87 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:03:41 +0000 Subject: [PATCH 03/10] refactor: Use move semantics in `CCheckQueue::Add` Co-authored-by: Martin Leitner-Ankerl --- src/checkqueue.h | 6 ++---- src/test/checkqueue_tests.cpp | 11 +++++++++++ src/test/fuzz/checkqueue.cpp | 2 +- 3 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/checkqueue.h b/src/checkqueue.h index d83f717fb7..63628860c5 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -11,6 +11,7 @@ #include #include +#include #include template @@ -173,10 +174,7 @@ public: { LOCK(m_mutex); - for (T& check : vChecks) { - queue.emplace_back(); - check.swap(queue.back()); - } + queue.insert(queue.end(), std::make_move_iterator(vChecks.begin()), std::make_move_iterator(vChecks.end())); nTodo += vChecks.size(); } diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 135f107159..90154c8757 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -140,6 +140,17 @@ struct FrozenCleanupCheck { cv.wait(l, []{ return nFrozen.load(std::memory_order_relaxed) == 0;}); } } + FrozenCleanupCheck(FrozenCleanupCheck&& other) noexcept + { + should_freeze = other.should_freeze; + other.should_freeze = false; + } + FrozenCleanupCheck& operator=(FrozenCleanupCheck&& other) noexcept + { + should_freeze = other.should_freeze; + other.should_freeze = false; + return *this; + } void swap(FrozenCleanupCheck& x) noexcept { std::swap(should_freeze, x.should_freeze); diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index 6256aefaa3..0c776e1b2f 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -13,7 +13,7 @@ namespace { struct DumbCheck { - const bool result = false; + bool result = false; DumbCheck() = default; From 04831fee6dca3eb86cd1d6b9ef879b296263fe35 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:04:01 +0000 Subject: [PATCH 04/10] refactor: Make move semantics explicit for callers --- src/bench/checkqueue.cpp | 2 +- src/checkqueue.h | 9 +++++---- src/test/checkqueue_tests.cpp | 12 ++++++------ src/test/fuzz/checkqueue.cpp | 4 ++-- src/test/transaction_tests.cpp | 2 +- src/validation.cpp | 2 +- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index dfd7275f46..41a4a216bf 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -60,7 +60,7 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) // Make insecure_rand here so that each iteration is identical. CCheckQueueControl control(&queue); for (auto vChecks : vBatches) { - control.Add(vChecks); + control.Add(std::move(vChecks)); } // control waits for completion by RAII, but // it is done explicitly here for clarity diff --git a/src/checkqueue.h b/src/checkqueue.h index 63628860c5..a4818ad44f 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -166,7 +166,7 @@ public: } //! Add a batch of checks to the queue - void Add(std::vector& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) + void Add(std::vector&& vChecks) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) { if (vChecks.empty()) { return; @@ -237,10 +237,11 @@ public: return fRet; } - void Add(std::vector& vChecks) + void Add(std::vector&& vChecks) { - if (pqueue != nullptr) - pqueue->Add(vChecks); + if (pqueue != nullptr) { + pqueue->Add(std::move(vChecks)); + } } ~CCheckQueueControl() diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 90154c8757..8d55ce75f7 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -191,7 +191,7 @@ static void Correct_Queue_range(std::vector range) while (total) { vChecks.resize(std::min(total, (size_t) InsecureRandRange(10))); total -= vChecks.size(); - control.Add(vChecks); + control.Add(std::move(vChecks)); } BOOST_REQUIRE(control.Wait()); if (FakeCheckCheckCompletion::n_calls != i) { @@ -253,7 +253,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Catches_Failure) vChecks.reserve(r); for (size_t k = 0; k < r && remaining; k++, remaining--) vChecks.emplace_back(remaining == 1); - control.Add(vChecks); + control.Add(std::move(vChecks)); } bool success = control.Wait(); if (i > 0) { @@ -278,7 +278,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Recovers_From_Failure) std::vector vChecks; vChecks.resize(100, false); vChecks[99] = end_fails; - control.Add(vChecks); + control.Add(std::move(vChecks)); } bool r =control.Wait(); BOOST_REQUIRE(r != end_fails); @@ -304,7 +304,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_UniqueCheck) std::vector vChecks; for (size_t k = 0; k < r && total; k++) vChecks.emplace_back(--total); - control.Add(vChecks); + control.Add(std::move(vChecks)); } } { @@ -342,7 +342,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_Memory) // to catch any sort of deallocation failure vChecks.emplace_back(total == 0 || total == i || total == i/2); } - control.Add(vChecks); + control.Add(std::move(vChecks)); } } BOOST_REQUIRE_EQUAL(MemoryCheck::fake_allocated_memory, 0U); @@ -364,7 +364,7 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) // swaps in default initialized Checks (otherwise freezing destructor // would get called twice). vChecks[0].should_freeze = true; - control.Add(vChecks); + control.Add(std::move(vChecks)); bool waitResult = control.Wait(); // Hangs here assert(waitResult); }); diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index 0c776e1b2f..cd2089de59 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -48,7 +48,7 @@ FUZZ_TARGET(checkqueue) checks_2.emplace_back(result); } if (fuzzed_data_provider.ConsumeBool()) { - check_queue_1.Add(checks_1); + check_queue_1.Add(std::move(checks_1)); } if (fuzzed_data_provider.ConsumeBool()) { (void)check_queue_1.Wait(); @@ -56,7 +56,7 @@ FUZZ_TARGET(checkqueue) CCheckQueueControl check_queue_control{&check_queue_2}; if (fuzzed_data_provider.ConsumeBool()) { - check_queue_control.Add(checks_2); + check_queue_control.Add(std::move(checks_2)); } if (fuzzed_data_provider.ConsumeBool()) { (void)check_queue_control.Wait(); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index f7ea99f6a6..7511dff9e8 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -548,7 +548,7 @@ BOOST_AUTO_TEST_CASE(test_big_witness_transaction) for(uint32_t i = 0; i < mtx.vin.size(); i++) { std::vector vChecks; vChecks.emplace_back(coins[tx.vin[i].prevout.n].out, tx, i, SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, false, &txdata); - control.Add(vChecks); + control.Add(std::move(vChecks)); } bool controlCheck = control.Wait(); diff --git a/src/validation.cpp b/src/validation.cpp index dce066412c..9d069a8357 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2324,7 +2324,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, return error("ConnectBlock(): CheckInputScripts on %s failed with %s", tx.GetHash().ToString(), state.ToString()); } - control.Add(vChecks); + control.Add(std::move(vChecks)); } CTxUndo undoDummy; From 9a0b5241396efe3b3ceb3931717c30bb94f99bfb Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:04:12 +0000 Subject: [PATCH 05/10] clang-tidy, test: Fix bugprone-use-after-move in `Correct_Queue_range()` --- src/test/checkqueue_tests.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 8d55ce75f7..2a8f4f0444 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -184,12 +184,14 @@ static void Correct_Queue_range(std::vector range) small_queue->StartWorkerThreads(SCRIPT_CHECK_THREADS); // Make vChecks here to save on malloc (this test can be slow...) std::vector vChecks; + vChecks.reserve(9); for (const size_t i : range) { size_t total = i; FakeCheckCheckCompletion::n_calls = 0; CCheckQueueControl control(small_queue.get()); while (total) { - vChecks.resize(std::min(total, (size_t) InsecureRandRange(10))); + vChecks.clear(); + vChecks.resize(std::min(total, InsecureRandRange(10))); total -= vChecks.size(); control.Add(std::move(vChecks)); } From d8427cc28e3a9ac3319fb452b16661957c812b8f Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:04:21 +0000 Subject: [PATCH 06/10] refactor: Use move semantics in `CCheckQueue::Loop` Co-authored-by: Martin Leitner-Ankerl --- src/bench/checkqueue.cpp | 5 ----- src/checkqueue.h | 10 +++------- src/test/fuzz/checkqueue.cpp | 6 ------ 3 files changed, 3 insertions(+), 18 deletions(-) diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index 41a4a216bf..8ad6fde6bf 100644 --- a/src/bench/checkqueue.cpp +++ b/src/bench/checkqueue.cpp @@ -29,7 +29,6 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) struct PrevectorJob { prevector p; - PrevectorJob() = default; explicit PrevectorJob(FastRandomContext& insecure_rand){ p.resize(insecure_rand.randrange(PREVECTOR_SIZE*2)); } @@ -37,10 +36,6 @@ static void CCheckQueueSpeedPrevectorJob(benchmark::Bench& bench) { return true; } - void swap(PrevectorJob& x) noexcept - { - p.swap(x.p); - }; }; CCheckQueue queue {QUEUE_BATCH_SIZE}; // The main thread should be counted to prevent thread oversubscription, and diff --git a/src/checkqueue.h b/src/checkqueue.h index a4818ad44f..2c21e5f7d0 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -112,13 +112,9 @@ private: // * Try to account for idle jobs which will instantly start helping. // * Don't do batches smaller than 1 (duh), or larger than nBatchSize. nNow = std::max(1U, std::min(nBatchSize, (unsigned int)queue.size() / (nTotal + nIdle + 1))); - vChecks.resize(nNow); - for (unsigned int i = 0; i < nNow; i++) { - // We want the lock on the m_mutex to be as short as possible, so swap jobs from the global - // queue to the local batch vector instead of copying. - vChecks[i].swap(queue.back()); - queue.pop_back(); - } + auto start_it = queue.end() - nNow; + vChecks.assign(std::make_move_iterator(start_it), std::make_move_iterator(queue.end())); + queue.erase(start_it, queue.end()); // Check whether we need to do work at all fOk = fAllOk; } diff --git a/src/test/fuzz/checkqueue.cpp b/src/test/fuzz/checkqueue.cpp index cd2089de59..429570526f 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -15,8 +15,6 @@ namespace { struct DumbCheck { bool result = false; - DumbCheck() = default; - explicit DumbCheck(const bool _result) : result(_result) { } @@ -25,10 +23,6 @@ struct DumbCheck { { return result; } - - void swap(DumbCheck& x) noexcept - { - } }; } // namespace From b4bed5c1f98c0eed18f52fdcea11a420c10ed98d Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:04:35 +0000 Subject: [PATCH 07/10] refactor: Drop no longer used `CScriptCheck()` default constructor --- src/validation.h | 1 - 1 file changed, 1 deletion(-) diff --git a/src/validation.h b/src/validation.h index 7bcb35d9a4..9135a1d05a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -318,7 +318,6 @@ private: PrecomputedTransactionData *txdata; public: - CScriptCheck(): ptxTo(nullptr), nIn(0), nFlags(0), cacheStore(false), error(SCRIPT_ERR_UNKNOWN_ERROR) {} CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) : m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR), txdata(txdataIn) { } From a87fb6bee5a7fb0879b3adea9a29997f1331acb0 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:04:44 +0000 Subject: [PATCH 08/10] clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck` --- src/validation.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/validation.h b/src/validation.h index 9135a1d05a..a12d33b9be 100644 --- a/src/validation.h +++ b/src/validation.h @@ -314,12 +314,12 @@ private: unsigned int nIn; unsigned int nFlags; bool cacheStore; - ScriptError error; + ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR}; PrecomputedTransactionData *txdata; public: CScriptCheck(const CTxOut& outIn, const CTransaction& txToIn, unsigned int nInIn, unsigned int nFlagsIn, bool cacheIn, PrecomputedTransactionData* txdataIn) : - m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), error(SCRIPT_ERR_UNKNOWN_ERROR), txdata(txdataIn) { } + m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn) { } CScriptCheck(const CScriptCheck&) = delete; CScriptCheck& operator=(const CScriptCheck&) = delete; From cea50521fe810111a8a3c84ad14f944eafb5b658 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:04:53 +0000 Subject: [PATCH 09/10] refactor: Drop no longer used `swap` member functions --- src/test/checkqueue_tests.cpp | 18 ------------------ src/validation.h | 11 ----------- 2 files changed, 29 deletions(-) diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index 2a8f4f0444..e99a880280 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -43,7 +43,6 @@ struct FakeCheck { { return true; } - void swap(FakeCheck& x) noexcept {}; }; struct FakeCheckCheckCompletion { @@ -53,7 +52,6 @@ struct FakeCheckCheckCompletion { n_calls.fetch_add(1, std::memory_order_relaxed); return true; } - void swap(FakeCheckCheckCompletion& x) noexcept {}; }; struct FailingCheck { @@ -64,10 +62,6 @@ struct FailingCheck { { return !fails; } - void swap(FailingCheck& x) noexcept - { - std::swap(fails, x.fails); - }; }; struct UniqueCheck { @@ -82,10 +76,6 @@ struct UniqueCheck { results.insert(check_id); return true; } - void swap(UniqueCheck& x) noexcept - { - std::swap(x.check_id, check_id); - }; }; @@ -113,10 +103,6 @@ struct MemoryCheck { { fake_allocated_memory.fetch_sub(b, std::memory_order_relaxed); }; - void swap(MemoryCheck& x) noexcept - { - std::swap(b, x.b); - }; }; struct FrozenCleanupCheck { @@ -151,10 +137,6 @@ struct FrozenCleanupCheck { other.should_freeze = false; return *this; } - void swap(FrozenCleanupCheck& x) noexcept - { - std::swap(should_freeze, x.should_freeze); - }; }; // Static Allocations diff --git a/src/validation.h b/src/validation.h index a12d33b9be..aba863db09 100644 --- a/src/validation.h +++ b/src/validation.h @@ -328,17 +328,6 @@ public: bool operator()(); - void swap(CScriptCheck& check) noexcept - { - std::swap(ptxTo, check.ptxTo); - std::swap(m_tx_out, check.m_tx_out); - std::swap(nIn, check.nIn); - std::swap(nFlags, check.nFlags); - std::swap(cacheStore, check.cacheStore); - std::swap(error, check.error); - std::swap(txdata, check.txdata); - } - ScriptError GetScriptError() const { return error; } }; From 95ad70ab652ddde7de65f633c36c1378b26a313a Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Tue, 21 Mar 2023 13:05:00 +0000 Subject: [PATCH 10/10] test: Default initialize `should_freeze` to `true` It is safe now, when move semantics is used instead of a custom swap function. --- src/test/checkqueue_tests.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index e99a880280..012b0eb321 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -109,9 +109,7 @@ struct FrozenCleanupCheck { static std::atomic nFrozen; static std::condition_variable cv; static std::mutex m; - // Freezing can't be the default initialized behavior given how the queue - // swaps in default initialized Checks. - bool should_freeze {false}; + bool should_freeze{true}; bool operator()() const { return true; @@ -344,10 +342,6 @@ BOOST_AUTO_TEST_CASE(test_CheckQueue_FrozenCleanup) std::thread t0([&]() { CCheckQueueControl control(queue.get()); std::vector vChecks(1); - // Freezing can't be the default initialized behavior given how the queue - // swaps in default initialized Checks (otherwise freezing destructor - // would get called twice). - vChecks[0].should_freeze = true; control.Add(std::move(vChecks)); bool waitResult = control.Wait(); // Hangs here assert(waitResult);