-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Optimizations & simplifications following #25717 #25968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
4d58277 to
e0025e6
Compare
|
Concept ACK, light code review ACK e0025e6c21d0429e66d33b0520b71b5180ffc0cc I think it would save us some time in the future if you duplicate the descriptions from the PR post in commit messages. |
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.
Remove the special case in ProcessHeadersMessage dealing with empty headers messages, and instead let the HeadersSync class deal with it correctly.
Make use of the fact that now TryLowWorkHeadersSync is (like IsContinuationOfLowWorkHeadersSync) an operation that partially processes headers, so it can be invoked in a similar way, only bailing out when there is nothing left to do.
Just don't call this function when it won't have any effect.
Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible.
There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it.
e0025e6 to
746a829
Compare
|
@naumenkogs Fair point, done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some additional improvements, feel free to ignore.
This param seem to be unused
Git diff
diff --git a/src/validation.cpp b/src/validation.cpp
index d029b883c..d099f9d90 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3708,7 +3708,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>&
return true;
}
-void ChainstateManager::ReportHeadersPresync(const arith_uint256& work, int64_t height, int64_t timestamp)
+void ChainstateManager::ReportHeadersPresync(int64_t height, int64_t timestamp)
{
AssertLockNotHeld(cs_main);
const auto& chainstate = ActiveChainstate();
diff --git a/src/validation.h b/src/validation.h
index af7e65a54..ac5a3e5c4 100644
--- a/src/validation.h
+++ b/src/validation.h
@@ -1051,7 +1051,7 @@ public:
* headers are not yet fed to validation during that time, but validation is (for now)
* responsible for logging and signalling through NotifyHeaderTip, so it needs this
* information. */
- void ReportHeadersPresync(const arith_uint256& work, int64_t height, int64_t timestamp);
+ void ReportHeadersPresync(int64_t height, int64_t timestamp);
~ChainstateManager();
};
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index b74675916..1a1c75406 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -4380,7 +4380,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
if (it != m_headers_presync_stats.end()) stats = it->second;
}
if (stats.second) {
- m_chainman.ReportHeadersPresync(stats.first, stats.second->first, stats.second->second);
+ m_chainman.ReportHeadersPresync(stats.second->first, stats.second->second);
}
}Use initializer list instead of constructor body + const
Git diff
diff --git a/src/headerssync.h b/src/headerssync.h
index 566e95ae2..d59df8e0c 100644
--- a/src/headerssync.h
+++ b/src/headerssync.h
@@ -31,16 +31,14 @@ struct CompressedHeader {
hashMerkleRoot.SetNull();
}
- CompressedHeader(const CBlockHeader& header)
- {
- nVersion = header.nVersion;
- hashMerkleRoot = header.hashMerkleRoot;
- nTime = header.nTime;
- nBits = header.nBits;
- nNonce = header.nNonce;
- }
-
- CBlockHeader GetFullHeader(const uint256& hash_prev_block) {
+ CompressedHeader(const CBlockHeader& header) :
+ nVersion(header.nVersion),
+ hashMerkleRoot(header.hashMerkleRoot),
+ nTime(header.nTime),
+ nBits(header.nBits),
+ nNonce(header.nNonce) {}
+
+ CBlockHeader GetFullHeader(const uint256& hash_prev_block) const {
CBlockHeader ret;
ret.nVersion = nVersion;
ret.hashPrevBlock = hash_prev_block;| Assume(m_download_state == State::PRESYNC); | ||
| if (m_download_state != State::PRESYNC) return false; | ||
|
|
||
| if (headers.size() == 0) return true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit
| if (headers.size() == 0) return true; | |
| if (headers.empty()) return true; |
| CBlockIndex dummy(header); | ||
| total_work += GetBlockProof(dummy); | ||
| } | ||
| for (const CBlockHeader& header : headers) total_work += GetBlockProof(header); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nit) i believe in most cases std::accumulate is faster than a for loop
diff --git a/src/validation.cpp b/src/validation.cpp
index d029b883c..648139b1f 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -3440,9 +3440,8 @@ bool HasValidProofOfWork(Span<const CBlockHeader> headers, const Consensus::Para
arith_uint256 CalculateHeadersWork(Span<const CBlockHeader> headers)
{
- arith_uint256 total_work{0};
- for (const CBlockHeader& header : headers) total_work += GetBlockProof(header);
- return total_work;
+ return std::accumulate(headers.begin(), headers.end(), arith_uint256{0},
+ [](arith_uint256 total_work, const CBlockHeader& header) { return total_work + GetBlockProof(header);});
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How could it be faster? std::accumulate is just a function that runs the provided lambda in a loop.
The reason you'd want to use std::accumulate is because it's more readable. I'm not sure that's the case here.
| bool via_compact_block) | ||
| { | ||
| size_t nCount = headers.size(); | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can be moved just before usage L2774 to save a size() invocation.
| if (peer.m_headers_sync) { | ||
| peer.m_headers_sync.reset(nullptr); | ||
| LOCK(m_headers_presync_mutex); | ||
| m_headers_presync_stats.erase(pfrom.GetId()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewers the state is finalized L133 in ProcessNextHeaders().
| return; | ||
| if (!already_validated_work) { | ||
| already_validated_work = TryLowWorkHeadersSync(peer, pfrom, chain_start_header, headers); | ||
| have_headers_sync = already_validated_work; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not assigning directly the result of TryLowWOrkHeadersSync() to have_headers_sync here? For some code clarity?
maflcko
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some more cleanup ideas
| if (best_updated && stats.second.has_value()) { | ||
| // If the best peer updated, and it is in its first phase, signal. | ||
| m_headers_presync_should_signal = true; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated 25717 cleanup, in this file: No need to pass members into member functions:
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 74700580ad..8e80bd7aef 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -604,7 +604,7 @@ private:
EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex);
/** Various helpers for headers processing, invoked by ProcessHeadersMessage() */
/** Return true if headers are continuous and have valid proof-of-work (DoS points assigned on failure) */
- bool CheckHeadersPoW(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams, Peer& peer);
+ bool CheckHeadersPoW(const std::vector<CBlockHeader>& headers, Peer& peer);
/** Calculate an anti-DoS work threshold for headers chains */
arith_uint256 GetAntiDoSWorkThreshold();
/** Deal with state tracking and headers sync for peers that send the
@@ -2360,10 +2360,10 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp));
}
-bool PeerManagerImpl::CheckHeadersPoW(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams, Peer& peer)
+bool PeerManagerImpl::CheckHeadersPoW(const std::vector<CBlockHeader>& headers, Peer& peer)
{
// Do these headers have proof-of-work matching what's claimed?
- if (!HasValidProofOfWork(headers, consensusParams)) {
+ if (!HasValidProofOfWork(headers, m_chainparams.GetConsensus())) {
Misbehaving(peer, 100, "header with invalid proof of work");
return false;
}
@@ -2750,7 +2750,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
// We'll rely on headers having valid proof-of-work further down, as an
// anti-DoS criteria (note: this check is required before passing any
// headers into HeadersSyncState).
- if (!CheckHeadersPoW(headers, m_chainparams.GetConsensus(), peer)) {
+ if (!CheckHeadersPoW(headers, peer)) {
// Misbehaving() calls are handled within CheckHeadersPoW(), so we can
// just return. (Note that even if a header is announced via compact
// block, the header itself should be valid, so this type of error can| Assume(headers.empty()); | ||
| return; | ||
| if (!already_validated_work) { | ||
| already_validated_work = TryLowWorkHeadersSync(peer, pfrom, chain_start_header, headers); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unrelated, but in this line: A raw pointer that is never null is better passed as a reference.
diff --git a/src/headerssync.cpp b/src/headerssync.cpp
index 757b942cd9..18ffe65161 100644
--- a/src/headerssync.cpp
+++ b/src/headerssync.cpp
@@ -23,14 +23,14 @@ constexpr size_t REDOWNLOAD_BUFFER_SIZE{13959}; // 13959/584 = ~23.9 commitments
static_assert(sizeof(CompressedHeader) == 48);
HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
- const CBlockIndex* chain_start, const arith_uint256& minimum_required_work) :
+ const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) :
m_id(id), m_consensus_params(consensus_params),
m_chain_start(chain_start),
m_minimum_required_work(minimum_required_work),
- m_current_chain_work(chain_start->nChainWork),
+ m_current_chain_work{chain_start.nChainWork},
m_commit_offset(GetRand<unsigned>(HEADER_COMMITMENT_PERIOD)),
- m_last_header_received(m_chain_start->GetBlockHeader()),
- m_current_height(chain_start->nHeight)
+ m_last_header_received{chain_start.GetBlockHeader()},
+ m_current_height{chain_start.nHeight}
{
// Estimate the number of blocks that could possibly exist on the peer's
// chain *right now* using 6 blocks/second (fastest blockrate given the MTP
@@ -40,7 +40,7 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus
// exceeds this bound, because it's not possible for a consensus-valid
// chain to be longer than this (at the current time -- in the future we
// could try again, if necessary, to sync a longer chain).
- m_max_commitments = 6*(Ticks<std::chrono::seconds>(GetAdjustedTime() - NodeSeconds{std::chrono::seconds{chain_start->GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
+ m_max_commitments = 6*(Ticks<std::chrono::seconds>(GetAdjustedTime() - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
LogPrint(BCLog::NET, "Initial headers sync started with peer=%d: height=%i, max_commitments=%i, min_work=%s\n", m_id, m_current_height, m_max_commitments, m_minimum_required_work.ToString());
}
@@ -164,10 +164,10 @@ bool HeadersSyncState::ValidateAndStoreHeadersCommitments(const std::vector<CBlo
if (m_current_chain_work >= m_minimum_required_work) {
m_redownloaded_headers.clear();
- m_redownload_buffer_last_height = m_chain_start->nHeight;
- m_redownload_buffer_first_prev_hash = m_chain_start->GetBlockHash();
- m_redownload_buffer_last_hash = m_chain_start->GetBlockHash();
- m_redownload_chain_work = m_chain_start->nChainWork;
+ m_redownload_buffer_last_height = m_chain_start.nHeight;
+ m_redownload_buffer_first_prev_hash = m_chain_start.GetBlockHash();
+ m_redownload_buffer_last_hash = m_chain_start.GetBlockHash();
+ m_redownload_chain_work = m_chain_start.nChainWork;
m_download_state = State::REDOWNLOAD;
LogPrint(BCLog::NET, "Initial headers sync transition with peer=%d: reached sufficient work at height=%i, redownloading from height=%i\n", m_id, m_current_height, m_redownload_buffer_last_height);
}
@@ -231,7 +231,7 @@ bool HeadersSyncState::ValidateAndStoreRedownloadedHeader(const CBlockHeader& he
if (!m_redownloaded_headers.empty()) {
previous_nBits = m_redownloaded_headers.back().nBits;
} else {
- previous_nBits = m_chain_start->nBits;
+ previous_nBits = m_chain_start.nBits;
}
if (!PermittedDifficultyTransition(m_consensus_params, next_height,
@@ -298,7 +298,7 @@ CBlockLocator HeadersSyncState::NextHeadersRequestLocator() const
Assume(m_download_state != State::FINAL);
if (m_download_state == State::FINAL) return {};
- auto chain_start_locator = LocatorEntries(m_chain_start);
+ auto chain_start_locator = LocatorEntries(&m_chain_start);
std::vector<uint256> locator;
if (m_download_state == State::PRESYNC) {
diff --git a/src/headerssync.h b/src/headerssync.h
index 16da964246..6455b7dae6 100644
--- a/src/headerssync.h
+++ b/src/headerssync.h
@@ -136,7 +136,7 @@ public:
* minimum_required_work: amount of chain work required to accept the chain
*/
HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
- const CBlockIndex* chain_start, const arith_uint256& minimum_required_work);
+ const CBlockIndex& chain_start, const arith_uint256& minimum_required_work);
/** Result data structure for ProcessNextHeaders. */
struct ProcessingResult {
@@ -208,7 +208,7 @@ private:
const Consensus::Params& m_consensus_params;
/** Store the last block in our block index that the peer's chain builds from */
- const CBlockIndex* m_chain_start{nullptr};
+ const CBlockIndex& m_chain_start;
/** Minimum work that we're looking for on this chain. */
const arith_uint256 m_minimum_required_work;
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index 8e80bd7aef..2dbac5f6cb 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -647,7 +647,7 @@ private:
* otherwise.
*/
bool TryLowWorkHeadersSync(Peer& peer, CNode& pfrom,
- const CBlockIndex* chain_start_header,
+ const CBlockIndex& chain_start_header,
std::vector<CBlockHeader>& headers)
EXCLUSIVE_LOCKS_REQUIRED(!peer.m_headers_sync_mutex, !m_peer_mutex, !m_headers_presync_mutex);
@@ -2528,10 +2528,10 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro
return false;
}
-bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlockIndex* chain_start_header, std::vector<CBlockHeader>& headers)
+bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlockIndex& chain_start_header, std::vector<CBlockHeader>& headers)
{
// Calculate the total work on this chain.
- arith_uint256 total_work = chain_start_header->nChainWork + CalculateHeadersWork(headers);
+ arith_uint256 total_work = chain_start_header.nChainWork + CalculateHeadersWork(headers);
// Our dynamic anti-DoS threshold (minimum work required on a headers chain
// before we'll store it)
@@ -2561,7 +2561,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
// process the headers using it as normal.
return IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers);
} else {
- LogPrint(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header->nHeight + headers.size(), pfrom.GetId());
+ LogPrint(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header.nHeight + headers.size(), pfrom.GetId());
// Since this is a low-work headers chain, no further processing is required.
headers = {};
return true;
@@ -2831,7 +2831,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
// Do anti-DoS checks to determine if we should process or store for later
// processing.
if (!already_validated_work && TryLowWorkHeadersSync(peer, pfrom,
- chain_start_header, headers)) {
+ *chain_start_header, headers)) {
// If we successfully started a low-work headers sync, then there
// should be no headers to process any further.
Assume(headers.empty());
diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
index 41241ebee2..f7be84a356 100644
--- a/src/test/headers_sync_chainwork_tests.cpp
+++ b/src/test/headers_sync_chainwork_tests.cpp
@@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(headers_sync_state)
Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
ArithToUint256(1), Params().GenesisBlock().nBits);
- const CBlockIndex* chain_start = WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash()));
+ const CBlockIndex& chain_start { *Assert(WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash())))};
std::vector<CBlockHeader> headers_batch;
// Feed the first chain to HeadersSyncState, by delivering 1 header|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
@sipa are you inteterested in addressing the review feedback here? |
|
Closing as up for grabs. |
de4242f refactor: Use reference for chain_start in HeadersSyncState (Daniela Brozzoni) e37555e refactor: Use initializer list in CompressedHeader (Daniela Brozzoni) 0488bdf refactor: Remove unused parameter in ReportHeadersPresync (Daniela Brozzoni) 256246a refactor: Remove redundant parameter from CheckHeadersPoW (Daniela Brozzoni) ca0243e refactor: Remove useless CBlock::GetBlockHeader (Pieter Wuille) 4568652 refactor: Use std::span in HasValidProofOfWork (Daniela Brozzoni) 4066bfe refactor: Compute work from headers without CBlockIndex (Daniela Brozzoni) 0bf6139 p2p: Avoid an IsAncestorOfBestHeaderOrTip call (Pieter Wuille) Pull request description: This is a partial* revival of #25968 It contains a list of most-unrelated simplifications and optimizations to the code merged in #25717: - Avoid an IsAncestorOfBestHeaderOrTip call: Just don't call this function when it won't have any effect. - Compute work from headers without CBlockIndex: Avoid the need to construct a CBlockIndex object just to compute work for a header, when its nBits value suffices for that. Also use some Spans where possible. - Remove useless CBlock::GetBlockHeader: There is no need for a function to convert a CBlock to a CBlockHeader, as it's a child class of it. It also contains the following code cleanups, which were suggested by reviewers in #25968: - Remove redundant parameter from CheckHeadersPoW: No need to pass consensusParams, as CheckHeadersPow already has access to m_chainparams.GetConsensus() - Remove unused parameter in ReportHeadersPresync - Use initializer list in CompressedHeader, also make GetFullHeader const - Use reference for chain_start in HeadersSyncState: chain_start can never be null, so it's better to pass it as a reference rather than a raw pointer *I decided to leave out three commits that were in #25968 (4e7ac7b, ab52fb4, 7f1cf44), since they're a bit more involved, and I'm a new contributor. If this PR gets merged, I'll comment under #25968 to note that these three commits are still up for grabs :) ACKs for top commit: l0rinc: ACK de4242f polespinasa: re-ACK de4242f sipa: ACK de4242f achow101: ACK de4242f hodlinator: re-ACK de4242f Tree-SHA512: 1de4f3ce0854a196712505f2b52ccb985856f5133769552bf37375225ea8664a3a7a6a9578c4fd461e935cd94a7cbbb08f15751a1da7651f8962c866146d9d4b
This contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:
ProcessNextHeadersreplace the headers argument: this allows reusing the same vector storage for input and output headers toHeadersSync::ProcessNextHeaders. I find it 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.ProcessNextHeaderssupport empty headers message: remove the special case inProcessHeadersMessagedealing with empty headers messages, and instead letHeadersSyncdeal with it correctly.TryLowWorkHeadersSyncinvocation: make use of the fact that nowTryLowWorkHeadersSyncis (likeIsContinuationOfLowWorkHeadersSync) an operation that partially processes headers, it can be invoked in a similar way, bailing out when there is nothing left to do.IsAncestorOfBestHeaderOrTipcall: just don't call this function when it won't have any effectCBlockIndex: avoid the need to construct aCBlockIndexobject just to compute work for a header, when itsnBitsvalue suffices for that. Also use someSpans where possible.CBlock::GetBlockHeader(it inherits from it): There is no need for a function to convert aCBlockto aCBlockHeader, as it's a child class of it.These are not reactions to review feedback, and isn't intended for 24.0.