From 7e603674eb31fd3ad230b81155503e2532acd1e7 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 10 Mar 2023 14:18:31 +0100 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#23813: Add test and docs for getblockfrompeer with pruning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fe329dc936d1e02da406345e4223e11d1fa6fb38 test: Add test for getblockfrompeer on pruned nodes (Fabian Jahr) cd761e6b2cfade038b8018886c334bf25a0bcbcf rpc: Add note on guarantees to getblockfrompeer (Fabian Jahr) Pull request description: These are additions to `getblockfrompeer` that I already [suggested on the original PR](https://github.com/bitcoin/bitcoin/pull/20295#pullrequestreview-817157738). The two commits do the following: 1. Add a test for `getblockfrompeer` usage on pruned nodes. This is important because many use-cases for `getblockfrompeer` are in a context of a pruned node. 2. Add some information on how long the users of pruned nodes can expect the block to be available after they have used the RPC. I think the behavior is not very intuitive for users and I would not be surprised if users expect the block to be available indefinitely. ACKs for top commit: Sjors: re-utACK fe329dc936d1e02da406345e4223e11d1fa6fb38 MarcoFalke: review ACK fe329dc936d1e02da406345e4223e11d1fa6fb38 🍉 stratospher: ACK fe329dc. brunoerg: re-ACK fe329dc936d1e02da406345e4223e11d1fa6fb38 Tree-SHA512: a686bd8955d9c3baf365db384e497d6ee1aa9ce2fdb0733fe6150f7e3d94bae19d55bc1b347f1c9f619e749e18b41a52b9f8c0aa2042dd311a968a4b5d251fac --- src/rpc/blockchain.cpp | 3 +- test/functional/rpc_getblockfrompeer.py | 49 ++++++++++++++++++++++--- 2 files changed, 46 insertions(+), 6 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index eca003933174..a0fa0a311895 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -487,7 +487,8 @@ static RPCHelpMan getblockfrompeer() "We must have the header for this block, e.g. using submitheader.\n" "Subsequent calls for the same block and a new peer will cause the response from the previous peer to be ignored.\n" "Peers generally ignore requests for a stale block that they never fully verified, or one that is more than a month old.\n" - "When a peer does not respond with a block, we will disconnect.\n\n" + "When a peer does not respond with a block, we will disconnect.\n" + "Note: The block could be re-pruned as soon as it is received.\n\n" "Returns an empty JSON object if the request was successfully scheduled.", { {"blockhash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The block hash to try to fetch"}, diff --git a/test/functional/rpc_getblockfrompeer.py b/test/functional/rpc_getblockfrompeer.py index 3bed07485f31..d2cab1afec6c 100755 --- a/test/functional/rpc_getblockfrompeer.py +++ b/test/functional/rpc_getblockfrompeer.py @@ -23,14 +23,19 @@ class GetBlockFromPeerTest(BitcoinTestFramework): def set_test_params(self): - self.num_nodes = 2 + self.num_nodes = 3 + self.extra_args = [ + [], + [], + ["-fastprune", "-prune=1"] + ] def setup_network(self): self.setup_nodes() - def check_for_block(self, hash): + def check_for_block(self, node, hash): try: - self.nodes[0].getblock(hash) + self.nodes[node].getblock(hash) return True except JSONRPCException: return False @@ -47,7 +52,7 @@ def run_test(self): self.log.info("Connect nodes to sync headers") self.connect_nodes(0, 1) - self.sync_blocks() + self.sync_blocks(self.nodes[0:2]) self.log.info("Node 0 should only have the header for node 1's block 3") x = next(filter(lambda x: x['hash'] == short_tip, self.nodes[0].getchaintips())) @@ -73,7 +78,7 @@ def run_test(self): self.log.info("Successful fetch") result = self.nodes[0].getblockfrompeer(short_tip, peer_0_peer_1_id) - self.wait_until(lambda: self.check_for_block(short_tip), timeout=1) + self.wait_until(lambda: self.check_for_block(node=0, hash=short_tip), timeout=1) assert_equal(result, {}) self.log.info("Don't fetch blocks we already have") @@ -104,6 +109,40 @@ def run_test(self): assert_raises_rpc_error(-1, error_msg, self.nodes[1].getblockfrompeer, blockhash, node1_interface_id) self.stop_node(1, expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE) + self.log.info("Connect pruned node") + # We need to generate more blocks to be able to prune + self.connect_nodes(0, 2) + pruned_node = self.nodes[2] + self.generate(self.nodes[0], 400, sync_fun=self.no_op) + self.sync_blocks([self.nodes[0], pruned_node]) + pruneheight = pruned_node.pruneblockchain(300) + assert_equal(pruneheight, 248) + # Ensure the block is actually pruned + pruned_block = self.nodes[0].getblockhash(2) + assert_raises_rpc_error(-1, "Block not available (pruned data)", pruned_node.getblock, pruned_block) + + self.log.info("Fetch pruned block") + peers = pruned_node.getpeerinfo() + assert_equal(len(peers), 1) + pruned_node_peer_0_id = peers[0]["id"] + result = pruned_node.getblockfrompeer(pruned_block, pruned_node_peer_0_id) + self.wait_until(lambda: self.check_for_block(node=2, hash=pruned_block), timeout=1) + assert_equal(result, {}) + + self.log.info("Fetched block persists after next pruning event") + self.generate(self.nodes[0], 250, sync_fun=self.no_op) + self.sync_blocks([self.nodes[0], pruned_node]) + pruneheight += 251 + assert_equal(pruned_node.pruneblockchain(700), pruneheight) + assert_equal(pruned_node.getblock(pruned_block)["hash"], "36c56c5b5ebbaf90d76b0d1a074dcb32d42abab75b7ec6fa0ffd9b4fbce8f0f7") + + self.log.info("Fetched block can be pruned again when prune height exceeds the height of the tip at the time when the block was fetched") + self.generate(self.nodes[0], 250, sync_fun=self.no_op) + self.sync_blocks([self.nodes[0], pruned_node]) + pruneheight += 250 + assert_equal(pruned_node.pruneblockchain(1000), pruneheight) + assert_raises_rpc_error(-1, "Block not available (pruned data)", pruned_node.getblock, pruned_block) + if __name__ == '__main__': GetBlockFromPeerTest().main() From ce4f2a305423de3cbfb9e188ad47851219e1dbe1 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 22 Mar 2023 17:45:13 +0000 Subject: [PATCH 2/4] Merge bitcoin/bitcoin#27297: test: Remove unused Check* default constructors fae349076db03ddfbf23c5d828368d538b5d52d5 test: Remove unused Check* default constructors (MarcoFalke) Pull request description: They are no longer needed after the removal of `swap`, see https://github.com/bitcoin/bitcoin/pull/26749#discussion_r1144532693 Also, flatten a redundant `if` check. ACKs for top commit: hebasto: ACK fae349076db03ddfbf23c5d828368d538b5d52d5 Tree-SHA512: c0bc0c16b5df0f16fc25e18d2414a2a3c4769da1aa30d53f8d267bc2e97dd79a0296db94c1e49cd1ca89bd42275d8c462f7bf47f03f105dfe867ebea6563454b --- src/test/checkqueue_tests.cpp | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index a338511c9fad..3fecdf8b85d4 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -58,7 +58,6 @@ struct FakeCheckCheckCompletion { struct FailingCheck { bool fails; FailingCheck(bool _fails) : fails(_fails){}; - FailingCheck() : fails(true){}; bool operator()() const { return !fails; @@ -74,7 +73,6 @@ struct UniqueCheck { static std::unordered_multiset results GUARDED_BY(m); size_t check_id; UniqueCheck(size_t check_id_in) : check_id(check_id_in){}; - UniqueCheck() : check_id(0){}; bool operator()() { LOCK(m); @@ -95,7 +93,6 @@ struct MemoryCheck { { return true; } - MemoryCheck() = default; MemoryCheck(const MemoryCheck& x) { // We have to do this to make sure that destructor calls are paired @@ -182,9 +179,7 @@ static void Correct_Queue_range(std::vector range) control.Add(vChecks); } BOOST_REQUIRE(control.Wait()); - if (FakeCheckCheckCompletion::n_calls != i) { - BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); - } + BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); } small_queue->StopWorkerThreads(); } From 04718ab46fc09bdfb4b951f8286bec82f3b001b7 Mon Sep 17 00:00:00 2001 From: fanquake Date: Fri, 7 Jul 2023 10:57:22 +0100 Subject: [PATCH 3/4] Merge bitcoin/bitcoin#28015: fuzz: Generate rpc fuzz targets individually fa1e27fe8ec42764d0250c82a83d774c15798c4a fuzz: Generate rpc fuzz targets individually (MarcoFalke) Pull request description: The `rpc` fuzz target was added more than two years ago in e45863166f5e44cc2c380f4667812fcd3cddc73b. However, the bug https://github.com/bitcoin/bitcoin/issues/27913 was only found recently. Thus, it is pretty clear that fuzz engines can't deal with a search space that is too broad and can be extended in too many directions. Fix that by limiting the search space to each RPC method name and then iterate over all names, instead of letting the fuzz engine do the iteration. With this, the bug can be found in seconds, as opposed to years of CPU time (or never). ACKs for top commit: brunoerg: ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a dergoegge: ACK fa1e27fe8ec42764d0250c82a83d774c15798c4a Tree-SHA512: 45ccba842367650d010320603153276b1b303deda9ba8c6bb31a4d2473b00aa5bca866db95f541485d65efd8276e2575026968c037872ef344fa33cf45bcdcd7 --- test/fuzz/test_runner.py | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/test/fuzz/test_runner.py b/test/fuzz/test_runner.py index f5dd73565078..5eb4dadd5760 100755 --- a/test/fuzz/test_runner.py +++ b/test/fuzz/test_runner.py @@ -203,22 +203,40 @@ def generate_corpus(*, fuzz_pool, src_dir, fuzz_bin, corpus_dir, targets): {corpus_dir}. """ logging.info("Generating corpus to {}".format(corpus_dir)) + rpc_target = "rpc" + has_rpc = rpc_target in targets + if has_rpc: + targets.remove(rpc_target) + targets = [(t, {}) for t in targets] + if has_rpc: + lines = subprocess.run( + ["git", "grep", "--function-context", "RPC_COMMANDS_SAFE_FOR_FUZZING{", os.path.join(src_dir, "src", "test", "fuzz", "rpc.cpp")], + check=True, + stdout=subprocess.PIPE, + text=True, + ).stdout.splitlines() + lines = [l.split("\"", 1)[1].split("\"")[0] for l in lines if l.startswith("src/test/fuzz/rpc.cpp- \"")] + targets += [(rpc_target, {"LIMIT_TO_RPC_COMMAND": r}) for r in lines] - def job(command, t): - logging.debug("Running '{}'\n".format(" ".join(command))) + def job(command, t, t_env): + logging.debug(f"Running '{command}'") logging.debug("Command '{}' output:\n'{}'\n".format( - ' '.join(command), + command, subprocess.run( command, - env=get_fuzz_env(target=t, source_dir=src_dir), + env={ + **t_env, + **get_fuzz_env(target=t, source_dir=src_dir), + }, check=True, stderr=subprocess.PIPE, text=True, - ).stderr)) + ).stderr, + )) futures = [] - for target in targets: - target_corpus_dir = os.path.join(corpus_dir, target) + for target, t_env in targets: + target_corpus_dir = corpus_dir / target os.makedirs(target_corpus_dir, exist_ok=True) use_value_profile = int(random.random() < .3) command = [ @@ -229,7 +247,7 @@ def job(command, t): f"-use_value_profile={use_value_profile}", target_corpus_dir, ] - futures.append(fuzz_pool.submit(job, command, target)) + futures.append(fuzz_pool.submit(job, command, target, t_env)) for future in as_completed(futures): future.result() From cc691ed8affb3de70e13bfa2a39c962e8878d5f8 Mon Sep 17 00:00:00 2001 From: fanquake Date: Wed, 22 Mar 2023 10:57:45 +0000 Subject: [PATCH 4/4] Merge bitcoin/bitcoin#26749: refactor: Use move semantics instead of custom swap functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 95ad70ab652ddde7de65f633c36c1378b26a313a test: Default initialize `should_freeze` to `true` (Hennadii Stepanov) cea50521fe810111a8a3c84ad14f944eafb5b658 refactor: Drop no longer used `swap` member functions (Hennadii Stepanov) a87fb6bee5a7fb0879b3adea9a29997f1331acb0 clang-tidy: Fix modernize-use-default-member-init in `CScriptCheck` (Hennadii Stepanov) b4bed5c1f98c0eed18f52fdcea11a420c10ed98d refactor: Drop no longer used `CScriptCheck()` default constructor (Hennadii Stepanov) d8427cc28e3a9ac3319fb452b16661957c812b8f refactor: Use move semantics in `CCheckQueue::Loop` (Hennadii Stepanov) 9a0b5241396efe3b3ceb3931717c30bb94f99bfb clang-tidy, test: Fix bugprone-use-after-move in `Correct_Queue_range()` (Hennadii Stepanov) 04831fee6dca3eb86cd1d6b9ef879b296263fe35 refactor: Make move semantics explicit for callers (Hennadii Stepanov) 6c2d5972f3544c4f3e987828a99e88f27b62cf87 refactor: Use move semantics in `CCheckQueue::Add` (Hennadii Stepanov) 06820032142a75cc3c5b832045058bc6f6f74786 test, refactor: Avoid `CScriptCheck::swap` in `transaction_tests` (Hennadii Stepanov) 15209d97c6aad7d5c199fe007ad39b91c8ee6562 consensus, refactor: Avoid `CScriptCheck::swap` in `CheckInputScripts` (Hennadii Stepanov) Pull request description: This PR makes code more succinct and readable by using move semantics. ACKs for top commit: martinus: re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a achow101: ACK 95ad70ab652ddde7de65f633c36c1378b26a313a TheCharlatan: re-ACK https://github.com/bitcoin/bitcoin/commit/95ad70ab652ddde7de65f633c36c1378b26a313a MarcoFalke: re-ACK 95ad70ab652ddde7de65f633c36c1378b26a313a 🚥 Tree-SHA512: adda760891b12d252dc9b823fe7c41eed660364b6fb1a69f17607d7a31eb0bbb82a80d154a7acfaa241b5de37d42a293c2b6e059f26a8e92d88d3a87c99768fb --- src/bench/checkqueue.cpp | 7 +---- src/checkqueue.h | 25 +++++++---------- src/test/checkqueue_tests.cpp | 51 ++++++++++++++--------------------- src/test/fuzz/checkqueue.cpp | 12 +++------ src/validation.cpp | 5 ++-- src/validation.h | 21 +++++---------- 6 files changed, 43 insertions(+), 78 deletions(-) diff --git a/src/bench/checkqueue.cpp b/src/bench/checkqueue.cpp index 8591bb958b0b..a0043e443f8c 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}; @@ -61,7 +56,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 14cef8841888..5ab91175028d 100644 --- a/src/checkqueue.h +++ b/src/checkqueue.h @@ -10,6 +10,7 @@ #include #include +#include #include template @@ -110,13 +111,9 @@ class CCheckQueue // * 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; } @@ -163,7 +160,7 @@ class CCheckQueue } //! 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; @@ -171,10 +168,7 @@ class CCheckQueue { 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(); } @@ -236,10 +230,11 @@ class CCheckQueueControl 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 3fecdf8b85d4..7e19625e9442 100644 --- a/src/test/checkqueue_tests.cpp +++ b/src/test/checkqueue_tests.cpp @@ -42,7 +42,6 @@ struct FakeCheck { { return true; } - void swap(FakeCheck& x) noexcept {}; }; struct FakeCheckCheckCompletion { @@ -52,7 +51,6 @@ struct FakeCheckCheckCompletion { n_calls.fetch_add(1, std::memory_order_relaxed); return true; } - void swap(FakeCheckCheckCompletion& x) noexcept {}; }; struct FailingCheck { @@ -62,10 +60,6 @@ struct FailingCheck { { return !fails; } - void swap(FailingCheck& x) noexcept - { - std::swap(fails, x.fails); - }; }; struct UniqueCheck { @@ -79,10 +73,6 @@ struct UniqueCheck { results.insert(check_id); return true; } - void swap(UniqueCheck& x) noexcept - { - std::swap(x.check_id, check_id); - }; }; @@ -109,19 +99,13 @@ struct MemoryCheck { { fake_allocated_memory.fetch_sub(b, std::memory_order_relaxed); }; - void swap(MemoryCheck& x) noexcept - { - std::swap(b, x.b); - }; }; 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; @@ -136,10 +120,17 @@ struct FrozenCleanupCheck { cv.wait(l, []{ return nFrozen.load(std::memory_order_relaxed) == 0;}); } } - void swap(FrozenCleanupCheck& x) noexcept + FrozenCleanupCheck(FrozenCleanupCheck&& other) noexcept { - std::swap(should_freeze, x.should_freeze); - }; + 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; + } }; // Static Allocations @@ -169,14 +160,16 @@ 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(vChecks); + control.Add(std::move(vChecks)); } BOOST_REQUIRE(control.Wait()); BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); @@ -236,7 +229,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) { @@ -261,7 +254,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); @@ -287,7 +280,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)); } } { @@ -325,7 +318,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); @@ -343,11 +336,7 @@ 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(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 7d107995aa41..4ad72ebe6b1e 100644 --- a/src/test/fuzz/checkqueue.cpp +++ b/src/test/fuzz/checkqueue.cpp @@ -13,9 +13,7 @@ namespace { struct DumbCheck { - const bool result = false; - - DumbCheck() = default; + bool result = false; explicit DumbCheck(const bool _result) : result(_result) { @@ -25,10 +23,6 @@ struct DumbCheck { { return result; } - - void swap(DumbCheck& x) noexcept - { - } }; } // namespace @@ -48,7 +42,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 +50,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/validation.cpp b/src/validation.cpp index 06d45ca61bcb..7e0e707c1210 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1907,8 +1907,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // Verify signature CScriptCheck check(coin.out, tx, i, flags, cacheSigStore, &txdata); if (pvChecks) { - pvChecks->push_back(CScriptCheck()); - check.swap(pvChecks->back()); + pvChecks->emplace_back(std::move(check)); } else if (!check()) { const bool hasNonMandatoryFlags = (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) != 0; @@ -2422,7 +2421,7 @@ bool CChainState::ConnectBlock(const CBlock& block, BlockValidationState& state, tx.GetHash().ToString(), state.ToString()); return false; } - control.Add(vChecks); + control.Add(std::move(vChecks)); } CTxUndo undoDummy; diff --git a/src/validation.h b/src/validation.h index 5d9d09061d6c..dce7d8b36204 100644 --- a/src/validation.h +++ b/src/validation.h @@ -336,26 +336,19 @@ class CScriptCheck unsigned int nIn; unsigned int nFlags; bool cacheStore; - ScriptError error; + ScriptError error{SCRIPT_ERR_UNKNOWN_ERROR}; 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) { } + m_tx_out(outIn), ptxTo(&txToIn), nIn(nInIn), nFlags(nFlagsIn), cacheStore(cacheIn), txdata(txdataIn) { } - bool operator()(); + CScriptCheck(const CScriptCheck&) = delete; + CScriptCheck& operator=(const CScriptCheck&) = delete; + CScriptCheck(CScriptCheck&&) = default; + CScriptCheck& operator=(CScriptCheck&&) = default; - 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); - } + bool operator()(); ScriptError GetScriptError() const { return error; } };