From 4e7ac7b94d04e056e9994ed1c8273c52b7b23931 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Tue, 23 Aug 2022 17:29:37 -0400 Subject: [PATCH] Make ProcessNextHeaders use the headers argument as in/out This allows reusing the same vector storage for input and output headers to HeadersSync::ProcessNextHeaders. It's also natural in the sense that this argument just represents the headers-to-be-processed, both in the caller and the callee, and both before and after the calls. --- src/headerssync.cpp | 29 ++++++++++++----------- src/headerssync.h | 18 ++++++-------- src/net_processing.cpp | 6 ----- src/test/headers_sync_chainwork_tests.cpp | 25 ++++++++++++------- 4 files changed, 38 insertions(+), 40 deletions(-) diff --git a/src/headerssync.cpp b/src/headerssync.cpp index 757b942cd96..cdd50ca0f80 100644 --- a/src/headerssync.cpp +++ b/src/headerssync.cpp @@ -65,13 +65,13 @@ void HeadersSyncState::Finalize() /** Process the next batch of headers received from our peer. * Validate and store commitments, and compare total chainwork to our target to * see if we can switch to REDOWNLOAD mode. */ -HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(const - std::vector& received_headers, const bool full_headers_message) +HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders( + std::vector& headers, const bool full_headers_message) { ProcessingResult ret; - Assume(!received_headers.empty()); - if (received_headers.empty()) return ret; + Assume(!headers.empty()); + if (headers.empty()) return ret; Assume(m_download_state != State::FINAL); if (m_download_state == State::FINAL) return ret; @@ -80,8 +80,9 @@ HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(const // During PRESYNC, we minimally validate block headers and // occasionally add commitments to them, until we reach our work // threshold (at which point m_download_state is updated to REDOWNLOAD). - ret.success = ValidateAndStoreHeadersCommitments(received_headers); + ret.success = ValidateAndStoreHeadersCommitments(headers); if (ret.success) { + headers = {}; if (full_headers_message || m_download_state == State::REDOWNLOAD) { // A full headers message means the peer may have more to give us; // also if we just switched to REDOWNLOAD then we need to re-request @@ -101,7 +102,7 @@ HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(const // gets big enough (meaning that we've checked enough commitments), // we'll return a batch of headers to the caller for processing. ret.success = true; - for (const auto& hdr : received_headers) { + for (const auto& hdr : headers) { if (!ValidateAndStoreRedownloadedHeader(hdr)) { // Something went wrong -- the peer gave us an unexpected chain. // We could consider looking at the reason for failure and @@ -112,8 +113,8 @@ HeadersSyncState::ProcessingResult HeadersSyncState::ProcessNextHeaders(const } if (ret.success) { - // Return any headers that are ready for acceptance. - ret.pow_validated_headers = PopHeadersReadyForAcceptance(); + // Return any headers that are ready for acceptance, reusing the headers vector. + PopHeadersReadyForAcceptance(headers); // If we hit our target blockhash, then all remaining headers will be // returned and we can clear any leftover internal state. @@ -277,20 +278,20 @@ bool HeadersSyncState::ValidateAndStoreRedownloadedHeader(const CBlockHeader& he return true; } -std::vector HeadersSyncState::PopHeadersReadyForAcceptance() +void HeadersSyncState::PopHeadersReadyForAcceptance(std::vector& headers) { - std::vector ret; + headers.clear(); Assume(m_download_state == State::REDOWNLOAD); - if (m_download_state != State::REDOWNLOAD) return ret; + if (m_download_state != State::REDOWNLOAD) return; while (m_redownloaded_headers.size() > REDOWNLOAD_BUFFER_SIZE || (m_redownloaded_headers.size() > 0 && m_process_all_remaining_headers)) { - ret.emplace_back(m_redownloaded_headers.front().GetFullHeader(m_redownload_buffer_first_prev_hash)); + headers.emplace_back(m_redownloaded_headers.front().GetFullHeader(m_redownload_buffer_first_prev_hash)); m_redownloaded_headers.pop_front(); - m_redownload_buffer_first_prev_hash = ret.back().GetHash(); + m_redownload_buffer_first_prev_hash = headers.back().GetHash(); } - return ret; + return; } CBlockLocator HeadersSyncState::NextHeadersRequestLocator() const diff --git a/src/headerssync.h b/src/headerssync.h index 16da9642462..566e95ae2ad 100644 --- a/src/headerssync.h +++ b/src/headerssync.h @@ -140,20 +140,17 @@ public: /** Result data structure for ProcessNextHeaders. */ struct ProcessingResult { - std::vector pow_validated_headers; bool success{false}; bool request_more{false}; }; /** Process a batch of headers, once a sync via this mechanism has started * - * received_headers: headers that were received over the network for processing. - * Assumes the caller has already verified the headers - * are continuous, and has checked that each header - * satisfies the proof-of-work target included in the - * header (but not necessarily verified that the - * proof-of-work target is correct and passes consensus - * rules). + * headers: headers that were received over the network for processing. Assumes the caller has + * already verified the headers are continuous, and has checked that each header + * satisfies the proof-of-work target included in the header (but not necessarily + * verified that the proof-of-work target is correct and passes consensus rules). + * On output, this will be replaced with headers the caller has to process. * full_headers_message: true if the message was at max capacity, * indicating more headers may be available * ProcessingResult.pow_validated_headers: will be filled in with any @@ -165,8 +162,7 @@ public: * ProcessingResult.request_more: if true, the caller is suggested to call * NextHeadersRequestLocator and send a getheaders message using it. */ - ProcessingResult ProcessNextHeaders(const std::vector& - received_headers, bool full_headers_message); + ProcessingResult ProcessNextHeaders(std::vector& headers, bool full_headers_message); /** Issue the next GETHEADERS message to our peer. * @@ -198,7 +194,7 @@ private: bool ValidateAndStoreRedownloadedHeader(const CBlockHeader& header); /** Return a set of headers that satisfy our proof-of-work threshold */ - std::vector PopHeadersReadyForAcceptance(); + void PopHeadersReadyForAcceptance(std::vector& headers); private: /** NodeId of the peer (used for log messages) **/ diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 74700580ad7..d2336c05d02 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2514,12 +2514,6 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro } } - if (result.success) { - // We only overwrite the headers passed in if processing was - // successful. - headers.swap(result.pow_validated_headers); - } - return result.success; } // Either we didn't have a sync in progress, or something went wrong diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp index 41241ebee28..fb27e35d5d8 100644 --- a/src/test/headers_sync_chainwork_tests.cpp +++ b/src/test/headers_sync_chainwork_tests.cpp @@ -93,9 +93,11 @@ BOOST_AUTO_TEST_CASE(headers_sync_state) headers_batch.insert(headers_batch.end(), std::next(first_chain.begin()), first_chain.end()); hss.reset(new HeadersSyncState(0, Params().GetConsensus(), chain_start, chain_work)); - (void)hss->ProcessNextHeaders({first_chain.front()}, true); + std::vector to_proc{first_chain.front()}; + (void)hss->ProcessNextHeaders(to_proc, true); // Pretend the first header is still "full", so we don't abort. - auto result = hss->ProcessNextHeaders(headers_batch, true); + to_proc = headers_batch; + auto result = hss->ProcessNextHeaders(to_proc, true); // This chain should look valid, and we should have met the proof-of-work // requirement. @@ -104,20 +106,23 @@ BOOST_AUTO_TEST_CASE(headers_sync_state) BOOST_CHECK(hss->GetState() == HeadersSyncState::State::REDOWNLOAD); // Try to sneakily feed back the second chain. - result = hss->ProcessNextHeaders(second_chain, true); + to_proc = second_chain; + result = hss->ProcessNextHeaders(to_proc, true); BOOST_CHECK(!result.success); // foiled! BOOST_CHECK(hss->GetState() == HeadersSyncState::State::FINAL); // Now try again, this time feeding the first chain twice. hss.reset(new HeadersSyncState(0, Params().GetConsensus(), chain_start, chain_work)); - (void)hss->ProcessNextHeaders(first_chain, true); + to_proc = first_chain; + (void)hss->ProcessNextHeaders(to_proc, true); BOOST_CHECK(hss->GetState() == HeadersSyncState::State::REDOWNLOAD); - result = hss->ProcessNextHeaders(first_chain, true); + to_proc = first_chain; + result = hss->ProcessNextHeaders(to_proc, true); BOOST_CHECK(result.success); BOOST_CHECK(!result.request_more); // All headers should be ready for acceptance: - BOOST_CHECK(result.pow_validated_headers.size() == first_chain.size()); + BOOST_CHECK(to_proc.size() == first_chain.size()); // Nothing left for the sync logic to do: BOOST_CHECK(hss->GetState() == HeadersSyncState::State::FINAL); @@ -126,7 +131,8 @@ BOOST_AUTO_TEST_CASE(headers_sync_state) hss.reset(new HeadersSyncState(0, Params().GetConsensus(), chain_start, chain_work)); BOOST_CHECK(hss->GetState() == HeadersSyncState::State::PRESYNC); // Pretend just the first message is "full", so we don't abort. - (void)hss->ProcessNextHeaders({second_chain.front()}, true); + to_proc = {second_chain.front()}; + (void)hss->ProcessNextHeaders(to_proc, true); BOOST_CHECK(hss->GetState() == HeadersSyncState::State::PRESYNC); headers_batch.clear(); @@ -134,9 +140,10 @@ BOOST_AUTO_TEST_CASE(headers_sync_state) // Tell the sync logic that the headers message was not full, implying no // more headers can be requested. For a low-work-chain, this should causes // the sync to end with no headers for acceptance. - result = hss->ProcessNextHeaders(headers_batch, false); + to_proc = headers_batch; + result = hss->ProcessNextHeaders(to_proc, false); BOOST_CHECK(hss->GetState() == HeadersSyncState::State::FINAL); - BOOST_CHECK(result.pow_validated_headers.empty()); + BOOST_CHECK(to_proc.empty()); BOOST_CHECK(!result.request_more); // Nevertheless, no validation errors should have been detected with the // chain: