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/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/src/test/checkqueue_tests.cpp b/src/test/checkqueue_tests.cpp index a338511c9fad..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,21 +51,15 @@ struct FakeCheckCheckCompletion { n_calls.fetch_add(1, std::memory_order_relaxed); return true; } - void swap(FakeCheckCheckCompletion& x) noexcept {}; }; struct FailingCheck { bool fails; FailingCheck(bool _fails) : fails(_fails){}; - FailingCheck() : fails(true){}; bool operator()() const { return !fails; } - void swap(FailingCheck& x) noexcept - { - std::swap(fails, x.fails); - }; }; struct UniqueCheck { @@ -74,17 +67,12 @@ 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); results.insert(check_id); return true; } - void swap(UniqueCheck& x) noexcept - { - std::swap(x.check_id, check_id); - }; }; @@ -95,7 +83,6 @@ struct MemoryCheck { { return true; } - MemoryCheck() = default; MemoryCheck(const MemoryCheck& x) { // We have to do this to make sure that destructor calls are paired @@ -112,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; @@ -139,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 @@ -172,19 +160,19 @@ 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()); - if (FakeCheckCheckCompletion::n_calls != i) { - BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); - } + BOOST_REQUIRE_EQUAL(FakeCheckCheckCompletion::n_calls, i); } small_queue->StopWorkerThreads(); } @@ -241,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) { @@ -266,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); @@ -292,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)); } } { @@ -330,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); @@ -348,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; } }; 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() 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()