refactor: 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

Also slightly reformat HeaderSyncState constructor to make clang-format
happy

Lastly, remove `const` from `chain_start` declaration in
headers_sync_chainwork_tests, to work aroud a false-positive
dangling-reference warning in gcc 13.0

Co-Authored-By: maflcko <[email protected]>

Author: danielabrozzoni

src/headerssync.cpp

@@ -14,18 +14,21 @@
 // CompressedHeader (we should re-calculate parameters if we compress further).
 static_assert(sizeof(CompressedHeader) == 48);
 
-HeadersSyncState::HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
-        const HeadersSyncParams& params, const CBlockIndex* chain_start,
-        const arith_uint256& minimum_required_work) :
-    m_commit_offset((assert(params.commitment_period > 0), // HeadersSyncParams field must be initialized to non-zero.
-                     FastRandomContext().randrange(params.commitment_period))),
-    m_id(id), m_consensus_params(consensus_params),
-    m_params(params),
-    m_chain_start(chain_start),
-    m_minimum_required_work(minimum_required_work),
-    m_current_chain_work(chain_start->nChainWork),
-    m_last_header_received(m_chain_start->GetBlockHeader()),
-    m_current_height(chain_start->nHeight)
+HeadersSyncState::HeadersSyncState(NodeId id,
+                                   const Consensus::Params& consensus_params,
+                                   const HeadersSyncParams& params,
+                                   const CBlockIndex& chain_start,
+                                   const arith_uint256& minimum_required_work)
+    : m_commit_offset((assert(params.commitment_period > 0), // HeadersSyncParams field must be initialized to non-zero.
+                       FastRandomContext().randrange(params.commitment_period))),
+      m_id(id),
+      m_consensus_params(consensus_params),
+      m_params(params),
+      m_chain_start(chain_start),
+      m_minimum_required_work(minimum_required_work),
+      m_current_chain_work(chain_start.nChainWork),
+      m_last_header_received(m_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
@@ -35,7 +38,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).
-    const auto max_seconds_since_start{(Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start->GetMedianTimePast()}}))
+    const auto max_seconds_since_start{(Ticks<std::chrono::seconds>(NodeClock::now() - NodeSeconds{std::chrono::seconds{chain_start.GetMedianTimePast()}}))
                                        + MAX_FUTURE_BLOCK_TIME};
     m_max_commitments = 6 * max_seconds_since_start / m_params.commitment_period;
 
@@ -161,10 +164,10 @@ bool HeadersSyncState::ValidateAndStoreHeadersCommitments(std::span<const CBlock
 
     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;
         LogDebug(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);
     }
@@ -228,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,
@@ -295,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) {

src/headerssync.h

@@ -137,8 +137,8 @@ class HeadersSyncState {
      * minimum_required_work: amount of chain work required to accept the chain
      */
     HeadersSyncState(NodeId id, const Consensus::Params& consensus_params,
-            const HeadersSyncParams& params, const CBlockIndex* chain_start,
-            const arith_uint256& minimum_required_work);
+                     const HeadersSyncParams& params, const CBlockIndex& chain_start,
+                     const arith_uint256& minimum_required_work);
 
     /** Result data structure for ProcessNextHeaders. */
     struct ProcessingResult {
@@ -220,7 +220,7 @@ class HeadersSyncState {
     const HeadersSyncParams m_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;

src/net_processing.cpp

@@ -686,8 +686,8 @@ class PeerManagerImpl final : public PeerManager
      *              calling); false otherwise.
      */
     bool TryLowWorkHeadersSync(Peer& peer, CNode& pfrom,
-                                  const CBlockIndex* chain_start_header,
-                                  std::vector<CBlockHeader>& headers)
+                               const CBlockIndex& chain_start_header,
+                               std::vector<CBlockHeader>& headers)
         EXCLUSIVE_LOCKS_REQUIRED(!peer.m_headers_sync_mutex, !m_peer_mutex, !m_headers_presync_mutex, g_msgproc_mutex);
 
     /** Return true if the given header is an ancestor of
@@ -2633,10 +2633,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 claimed total work on this chain.
-    arith_uint256 total_work = chain_start_header->nChainWork + CalculateClaimedHeadersWork(headers);
+    arith_uint256 total_work = chain_start_header.nChainWork + CalculateClaimedHeadersWork(headers);
 
     // Our dynamic anti-DoS threshold (minimum work required on a headers chain
     // before we'll store it)
@@ -2667,7 +2667,7 @@ bool PeerManagerImpl::TryLowWorkHeadersSync(Peer& peer, CNode& pfrom, const CBlo
             // handled inside of IsContinuationOfLowWorkHeadersSync.
             (void)IsContinuationOfLowWorkHeadersSync(peer, pfrom, headers);
         } else {
-            LogDebug(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header->nHeight + headers.size(), pfrom.GetId());
+            LogDebug(BCLog::NET, "Ignoring low-work chain (height=%u) from peer=%d\n", chain_start_header.nHeight + headers.size(), pfrom.GetId());
         }
 
         // The peer has not yet given us a chain that meets our work threshold,
@@ -2933,7 +2933,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());

src/test/fuzz/headerssync.cpp

@@ -44,7 +44,7 @@ class FuzzedHeadersSyncState : public HeadersSyncState
 {
 public:
     FuzzedHeadersSyncState(const HeadersSyncParams& sync_params, const size_t commit_offset,
-                           const CBlockIndex* chain_start, const arith_uint256& minimum_required_work)
+                           const CBlockIndex& chain_start, const arith_uint256& minimum_required_work)
         : HeadersSyncState(/*id=*/0, Params().GetConsensus(), sync_params, chain_start, minimum_required_work)
     {
         const_cast<size_t&>(m_commit_offset) = commit_offset;
@@ -74,7 +74,7 @@ FUZZ_TARGET(headers_sync_state, .init = initialize_headers_sync_state_fuzz)
     FuzzedHeadersSyncState headers_sync(
         params,
         /*commit_offset=*/fuzzed_data_provider.ConsumeIntegralInRange<size_t>(0, params.commitment_period - 1),
-        /*chain_start=*/&start_index,
+        /*chain_start=*/start_index,
         /*minimum_required_work=*/min_work);
 
     // Store headers for potential redownload phase.

src/test/headers_sync_chainwork_tests.cpp

@@ -52,7 +52,7 @@ constexpr size_t COMMITMENT_PERIOD{600}; // Somewhat close to mainnet.
 
 struct HeadersGeneratorSetup : public RegTestingSetup {
     const CBlock& genesis{Params().GenesisBlock()};
-    const CBlockIndex* chain_start{WITH_LOCK(::cs_main, return m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash()))};
+    CBlockIndex& chain_start{WITH_LOCK(::cs_main, return *Assert(m_node.chainman->m_blockman.LookupBlockIndex(genesis.GetHash())))};
 
     // Generate headers for two different chains (using differing merkle roots
     // to ensure the headers are different).