Optimizations & simplifications following #25717

sipa commented at 2:59 pm on August 31, 2022: member

This contains a list of most-unrelated simplifications and optimizations to the code merged in #25717:

Make ProcessNextHeaders replace the headers argument: this allows reusing the same vector storage for input and output headers to HeadersSync::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.
Make ProcessNextHeaders support empty headers message: remove the special case in ProcessHeadersMessage dealing with empty headers messages, and instead let HeadersSync deal with it correctly.
Simplify TryLowWorkHeadersSync invocation: make use of the fact that now TryLowWorkHeadersSync is (like IsContinuationOfLowWorkHeadersSync) an operation that partially processes headers, it can be invoked in a similar way, bailing out when there is nothing left to do.
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 (it inherits from it): There is no need for a function to convert a CBlock to a CBlockHeader, as it’s a child class of it.

These are not reactions to review feedback, and isn’t intended for 24.0.

fanquake added the label P2P on Aug 31, 2022

sipa force-pushed on Aug 31, 2022

fanquake added this to the milestone 25.0 on Aug 31, 2022

naumenkogs commented at 7:12 am on September 1, 2022: member

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.

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.

4e7ac7b94d

Make ProcessNextHeaders support empty headers message

Remove the special case in ProcessHeadersMessage dealing with empty headers
messages, and instead let the HeadersSync class deal with it correctly.

ab52fb4e95

Simplify TryLowWorkHeadersSync invocation

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.

7f1cf440ca

Avoid an IsAncestorOfBestHeaderOrTip call

Just don't call this function when it won't have any effect.

1fcce40157

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.

565a78c560

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.

746a829489

sipa force-pushed on Sep 1, 2022

sipa commented at 12:30 pm on September 1, 2022: member

@naumenkogs Fair point, done.

in src/headerssync.cpp:142 in 746a829489

141-    if (headers.size() == 0) return true;
142-
143     Assume(m_download_state == State::PRESYNC);
144     if (m_download_state != State::PRESYNC) return false;
145 
146+    if (headers.size() == 0) return true;

aureleoules commented at 12:45 pm on September 5, 2022:

nit

0    if (headers.empty()) return true;

in src/validation.cpp:3444 in 746a829489

3446     arith_uint256 total_work{0};
3447-    for (const CBlockHeader& header : headers) {
3448-        CBlockIndex dummy(header);
3449-        total_work += GetBlockProof(dummy);
3450-    }
3451+    for (const CBlockHeader& header : headers) total_work += GetBlockProof(header);

aureleoules commented at 1:13 pm on September 5, 2022:

(nit) i believe in most cases std::accumulate is faster than a for loop

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index d029b883c..648139b1f 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -3440,9 +3440,8 @@ bool HasValidProofOfWork(Span<const CBlockHeader> headers, const Consensus::Para
 5 
 6 arith_uint256 CalculateHeadersWork(Span<const CBlockHeader> headers)
 7 {
 8-    arith_uint256 total_work{0};
 9-    for (const CBlockHeader& header : headers) total_work += GetBlockProof(header);
10-    return total_work;
11+    return std::accumulate(headers.begin(), headers.end(), arith_uint256{0},
12+            [](arith_uint256 total_work, const CBlockHeader& header) { return total_work + GetBlockProof(header);});
13 }

sipa commented at 4:15 pm on February 17, 2023:

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.

aureleoules commented at 1:20 pm on September 5, 2022: member

Some additional improvements, feel free to ignore.

This param seem to be unused

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index d029b883c..d099f9d90 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -3708,7 +3708,7 @@ bool ChainstateManager::ProcessNewBlockHeaders(const std::vector<CBlockHeader>&
 5     return true;
 6 }
 7 
 8-void ChainstateManager::ReportHeadersPresync(const arith_uint256& work, int64_t height, int64_t timestamp)
 9+void ChainstateManager::ReportHeadersPresync(int64_t height, int64_t timestamp)
10 {
11     AssertLockNotHeld(cs_main);
12     const auto& chainstate = ActiveChainstate();
13diff --git a/src/validation.h b/src/validation.h
14index af7e65a54..ac5a3e5c4 100644
15--- a/src/validation.h
16+++ b/src/validation.h
17@@ -1051,7 +1051,7 @@ public:
18      *  headers are not yet fed to validation during that time, but validation is (for now)
19      *  responsible for logging and signalling through NotifyHeaderTip, so it needs this
20      *  information. */
21-    void ReportHeadersPresync(const arith_uint256& work, int64_t height, int64_t timestamp);
22+    void ReportHeadersPresync(int64_t height, int64_t timestamp);
23 
24     ~ChainstateManager();
25 };
26diff --git a/src/net_processing.cpp b/src/net_processing.cpp
27index b74675916..1a1c75406 100644
28--- a/src/net_processing.cpp
29+++ b/src/net_processing.cpp
30@@ -4380,7 +4380,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
31                 if (it != m_headers_presync_stats.end()) stats = it->second;
32             }
33             if (stats.second) {
34-                m_chainman.ReportHeadersPresync(stats.first, stats.second->first, stats.second->second);
35+                m_chainman.ReportHeadersPresync(stats.second->first, stats.second->second);
36             }
37         }

Use initializer list instead of constructor body + const

 0diff --git a/src/headerssync.h b/src/headerssync.h
 1index 566e95ae2..d59df8e0c 100644
 2--- a/src/headerssync.h
 3+++ b/src/headerssync.h
 4@@ -31,16 +31,14 @@ struct CompressedHeader {
 5         hashMerkleRoot.SetNull();
 6     }
 7 
 8-    CompressedHeader(const CBlockHeader& header)
 9-    {
10-        nVersion = header.nVersion;
11-        hashMerkleRoot = header.hashMerkleRoot;
12-        nTime = header.nTime;
13-        nBits = header.nBits;
14-        nNonce = header.nNonce;
15-    }
16-
17-    CBlockHeader GetFullHeader(const uint256& hash_prev_block) {
18+    CompressedHeader(const CBlockHeader& header) :
19+        nVersion(header.nVersion),
20+        hashMerkleRoot(header.hashMerkleRoot),
21+        nTime(header.nTime),
22+        nBits(header.nBits),
23+        nNonce(header.nNonce) {}
24+
25+    CBlockHeader GetFullHeader(const uint256& hash_prev_block) const {
26         CBlockHeader ret;
27         ret.nVersion = nVersion;
28         ret.hashPrevBlock = hash_prev_block;

in src/net_processing.cpp:2728 in ab52fb4e95 outdated

2725@@ -2726,20 +2726,6 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
2726 {
2727     size_t nCount = headers.size();
2728

ariard commented at 7:00 pm on September 5, 2022:

Can be moved just before usage L2774 to save a size() invocation.

in src/net_processing.cpp:2738 in ab52fb4e95 outdated

2733-        // (perhaps it reorged to our chain). Clear download state for this peer.
2734-        LOCK(peer.m_headers_sync_mutex);
2735-        if (peer.m_headers_sync) {
2736-            peer.m_headers_sync.reset(nullptr);
2737-            LOCK(m_headers_presync_mutex);
2738-            m_headers_presync_stats.erase(pfrom.GetId());

ariard commented at 7:01 pm on September 5, 2022:

Note to reviewers the state is finalized L133 in ProcessNextHeaders().

in src/net_processing.cpp:2812 in 7f1cf440ca outdated

2813-        // should be no headers to process any further.
2814-        Assume(headers.empty());
2815-        return;
2816+    if (!already_validated_work) {
2817+        already_validated_work = TryLowWorkHeadersSync(peer, pfrom, chain_start_header, headers);
2818+        have_headers_sync = already_validated_work;

ariard commented at 7:10 pm on September 5, 2022:

Why not assigning directly the result of TryLowWOrkHeadersSync() to have_headers_sync here? For some code clarity?

in src/net_processing.cpp:2514 in 746a829489

2513@@ -2514,12 +2514,6 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro
2514             }

maflcko commented at 12:04 pm on September 6, 2022:

unrelated 25717 cleanup, in this file: No need to pass members into member functions:

 0diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 1index 74700580ad..8e80bd7aef 100644
 2--- a/src/net_processing.cpp
 3+++ b/src/net_processing.cpp
 4@@ -604,7 +604,7 @@ private:
 5         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, !m_headers_presync_mutex);
 6     /** Various helpers for headers processing, invoked by ProcessHeadersMessage() */
 7     /** Return true if headers are continuous and have valid proof-of-work (DoS points assigned on failure) */
 8-    bool CheckHeadersPoW(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams, Peer& peer);
 9+    bool CheckHeadersPoW(const std::vector<CBlockHeader>& headers, Peer& peer);
10     /** Calculate an anti-DoS work threshold for headers chains */
11     arith_uint256 GetAntiDoSWorkThreshold();
12     /** Deal with state tracking and headers sync for peers that send the
13@@ -2360,10 +2360,10 @@ void PeerManagerImpl::SendBlockTransactions(CNode& pfrom, Peer& peer, const CBlo
14     m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::BLOCKTXN, resp));
15 }
16 
17-bool PeerManagerImpl::CheckHeadersPoW(const std::vector<CBlockHeader>& headers, const Consensus::Params& consensusParams, Peer& peer)
18+bool PeerManagerImpl::CheckHeadersPoW(const std::vector<CBlockHeader>& headers, Peer& peer)
19 {
20     // Do these headers have proof-of-work matching what's claimed?
21-    if (!HasValidProofOfWork(headers, consensusParams)) {
22+    if (!HasValidProofOfWork(headers, m_chainparams.GetConsensus())) {
23         Misbehaving(peer, 100, "header with invalid proof of work");
24         return false;
25     }
26@@ -2750,7 +2750,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
27     // We'll rely on headers having valid proof-of-work further down, as an
28     // anti-DoS criteria (note: this check is required before passing any
29     // headers into HeadersSyncState).
30-    if (!CheckHeadersPoW(headers, m_chainparams.GetConsensus(), peer)) {
31+    if (!CheckHeadersPoW(headers, peer)) {
32         // Misbehaving() calls are handled within CheckHeadersPoW(), so we can
33         // just return. (Note that even if a header is announced via compact
34         // block, the header itself should be valid, so this type of error can

in src/net_processing.cpp:2811 in 746a829489

2812-        // If we successfully started a low-work headers sync, then there
2813-        // should be no headers to process any further.
2814-        Assume(headers.empty());
2815-        return;
2816+    if (!already_validated_work) {
2817+        already_validated_work = TryLowWorkHeadersSync(peer, pfrom, chain_start_header, headers);

maflcko commented at 12:20 pm on September 6, 2022:

unrelated, but in this line: A raw pointer that is never null is better passed as a reference.

  0diff --git a/src/headerssync.cpp b/src/headerssync.cpp
  1index 757b942cd9..18ffe65161 100644
  2--- a/src/headerssync.cpp
  3+++ b/src/headerssync.cpp
  4@@ -23,14 +23,14 @@ constexpr size_t REDOWNLOAD_BUFFER_SIZE{13959}; // 13959/584 = ~23.9 commitments
  5 static_assert(sizeof(CompressedHeader) == 48);
  6 
  7 HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
  8-        const CBlockIndex* chain_start, const arith_uint256& minimum_required_work) :
  9+        const CBlockIndex& chain_start, const arith_uint256& minimum_required_work) :
 10     m_id(id), m_consensus_params(consensus_params),
 11     m_chain_start(chain_start),
 12     m_minimum_required_work(minimum_required_work),
 13-    m_current_chain_work(chain_start->nChainWork),
 14+    m_current_chain_work{chain_start.nChainWork},
 15     m_commit_offset(GetRand<unsigned>(HEADER_COMMITMENT_PERIOD)),
 16-    m_last_header_received(m_chain_start->GetBlockHeader()),
 17-    m_current_height(chain_start->nHeight)
 18+    m_last_header_received{chain_start.GetBlockHeader()},
 19+    m_current_height{chain_start.nHeight}
 20 {
 21     // Estimate the number of blocks that could possibly exist on the peer's
 22     // chain *right now* using 6 blocks/second (fastest blockrate given the MTP
 23@@ -40,7 +40,7 @@ HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus
 24     // exceeds this bound, because it's not possible for a consensus-valid
 25     // chain to be longer than this (at the current time -- in the future we
 26     // could try again, if necessary, to sync a longer chain).
 27-    m_max_commitments = 6*(Ticks<std::chrono::seconds>(GetAdjustedTime() - NodeSeconds{std::chrono::seconds{chain_start->GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
 28+    m_max_commitments = 6*(Ticks<std::chrono::seconds>(GetAdjustedTime() - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}}) + MAX_FUTURE_BLOCK_TIME) / HEADER_COMMITMENT_PERIOD;
 29 
 30     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());
 31 }
 32@@ -164,10 +164,10 @@ bool HeadersSyncState::ValidateAndStoreHeadersCommitments(const std::vector<CBlo
 33 
 34     if (m_current_chain_work >= m_minimum_required_work) {
 35         m_redownloaded_headers.clear();
 36-        m_redownload_buffer_last_height = m_chain_start->nHeight;
 37-        m_redownload_buffer_first_prev_hash = m_chain_start->GetBlockHash();
 38-        m_redownload_buffer_last_hash = m_chain_start->GetBlockHash();
 39-        m_redownload_chain_work = m_chain_start->nChainWork;
 40+        m_redownload_buffer_last_height = m_chain_start.nHeight;
 41+        m_redownload_buffer_first_prev_hash = m_chain_start.GetBlockHash();
 42+        m_redownload_buffer_last_hash = m_chain_start.GetBlockHash();
 43+        m_redownload_chain_work = m_chain_start.nChainWork;
 44         m_download_state = State::REDOWNLOAD;
 45         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);
 46     }
 47@@ -231,7 +231,7 @@ bool HeadersSyncState::ValidateAndStoreRedownloadedHeader(const CBlockHeader& he
 48     if (!m_redownloaded_headers.empty()) {
 49         previous_nBits = m_redownloaded_headers.back().nBits;
 50     } else {
 51-        previous_nBits = m_chain_start->nBits;
 52+        previous_nBits = m_chain_start.nBits;
 53     }
 54 
 55     if (!PermittedDifficultyTransition(m_consensus_params, next_height,
 56@@ -298,7 +298,7 @@ CBlockLocator HeadersSyncState::NextHeadersRequestLocator() const
 57     Assume(m_download_state != State::FINAL);
 58     if (m_download_state == State::FINAL) return {};
 59 
 60-    auto chain_start_locator = LocatorEntries(m_chain_start);
 61+    auto chain_start_locator = LocatorEntries(&m_chain_start);
 62     std::vector<uint256> locator;
 63 
 64     if (m_download_state == State::PRESYNC) {
 65diff --git a/src/headerssync.h b/src/headerssync.h
 66index 16da964246..6455b7dae6 100644
 67--- a/src/headerssync.h
 68+++ b/src/headerssync.h
 69@@ -136,7 +136,7 @@ public:
 70      * minimum_required_work: amount of chain work required to accept the chain
 71      */
 72     HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
 73-            const CBlockIndex* chain_start, const arith_uint256& minimum_required_work);
 74+            const CBlockIndex& chain_start, const arith_uint256& minimum_required_work);
 75 
 76     /** Result data structure for ProcessNextHeaders. */
 77     struct ProcessingResult {
 78@@ -208,7 +208,7 @@ private:
 79     const Consensus::Params& m_consensus_params;
 80 
 81     /** Store the last block in our block index that the peer's chain builds from */
 82-    const CBlockIndex* m_chain_start{nullptr};
 83+    const CBlockIndex& m_chain_start;
 84 
 85     /** Minimum work that we're looking for on this chain. */
 86     const arith_uint256 m_minimum_required_work;
 87diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 88index 8e80bd7aef..2dbac5f6cb 100644
 89--- a/src/net_processing.cpp
 90+++ b/src/net_processing.cpp
 91@@ -647,7 +647,7 @@ private:
 92      *              otherwise.
 93      */
 94     bool TryLowWorkHeadersSync(Peer& peer, CNode& pfrom,
 95-                                  const CBlockIndex* chain_start_header,
 96+                                  const CBlockIndex& chain_start_header,
 97                                   std::vector<CBlockHeader>& headers)
 98         EXCLUSIVE_LOCKS_REQUIRED(!peer.m_headers_sync_mutex, !m_peer_mutex, !m_headers_presync_mutex);
 99 
100@@ -2528,10 +2528,10 @@ bool PeerManagerImpl::IsContinuationOfLowWorkHeadersSync(Peer& peer, CNode& pfro
101     return false;
102 }
103 
104-bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlockIndex* chain_start_header, std::vector<CBlockHeader>& headers)
105+bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlockIndex& chain_start_header, std::vector<CBlockHeader>& headers)
106 {
107     // Calculate the total work on this chain.
108-    arith_uint256 total_work = chain_start_header->nChainWork + CalculateHeadersWork(headers);
109+    arith_uint256 total_work = chain_start_header.nChainWork + CalculateHeadersWork(headers);
110 
111     // Our dynamic anti-DoS threshold (minimum work required on a headers chain
112     // before we'll store it)
113@@ -2561,7 +2561,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
114             // process the headers using it as normal.
115             return IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers);
116         } else {
117-            LogPrint(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header->nHeight + headers.size(), pfrom.GetId());
118+            LogPrint(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header.nHeight + headers.size(), pfrom.GetId());
119             // Since this is a low-work headers chain, no further processing is required.
120             headers = {};
121             return true;
122@@ -2831,7 +2831,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer,
123     // Do anti-DoS checks to determine if we should process or store for later
124     // processing.
125     if (!already_validated_work && TryLowWorkHeadersSync(peer, pfrom,
126-                chain_start_header, headers)) {
127+                *chain_start_header, headers)) {
128         // If we successfully started a low-work headers sync, then there
129         // should be no headers to process any further.
130         Assume(headers.empty());
131diff --git a/src/test/headers_sync_chainwork_tests.cpp b/src/test/headers_sync_chainwork_tests.cpp
132index 41241ebee2..f7be84a356 100644
133--- a/src/test/headers_sync_chainwork_tests.cpp
134+++ b/src/test/headers_sync_chainwork_tests.cpp
135@@ -85,7 +85,7 @@ BOOST_AUTO_TEST_CASE(headers_sync_state)
136             Params().GenesisBlock().nVersion, Params().GenesisBlock().nTime,
137             ArithToUint256(1), Params().GenesisBlock().nBits);
138 
139-    const CBlockIndex* chain_start = WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash()));
140+    const CBlockIndex& chain_start { *Assert(WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(Params().GenesisBlock().GetHash())))};
141     std::vector<CBlockHeader> headers_batch;
142 
143     // Feed the first chain to HeadersSyncState, by delivering 1 header

maflcko commented at 9:58 am on September 9, 2022: member

left some more cleanup ideas

DrahtBot commented at 1:50 pm on September 13, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	naumenkogs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

fanquake commented at 3:00 pm on February 17, 2023: member

@sipa are you inteterested in addressing the review feedback here?

achow101 removed this from the milestone 25.0 on Apr 13, 2023

sipa added the label Up for grabs on Apr 25, 2023

sipa commented at 3:04 pm on April 25, 2023: member

Closing as up for grabs.

sipa closed this on Apr 25, 2023

bitcoin locked this on Apr 24, 2024

Optimizations & simplifications following #25717 #25968

Reviews

Conflicts