From 5c5f88b80ad783f66cd0fe314bf657e128ddfa80 Mon Sep 17 00:00:00 2001
From: Martin Zumsande <mzumsande@gmail.com>
Date: Wed, 22 Jan 2025 14:35:52 -0500
Subject: [PATCH] validation: Do less work in NeedsRedownload

Iterating over hundred thousands of block indexes
with each startup doesn't seem necessary more than 7 years after segwit
activation. Only check the first segwit block instead.
This is less thorough, but the most typical case of upgrading a non-segwit client
that has synced beyond the segwit height will still lead to an error.
---
 src/chain.h        |  4 +---
 src/validation.cpp | 20 ++++++++------------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/src/chain.h b/src/chain.h
index 13f7582385d..aa26490d941 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -177,9 +177,7 @@ public:
 
     //! Verification status of this block. See enum BlockStatus
     //!
-    //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
-    //! load to avoid a spurious startup failure requiring -reindex.
-    //! @sa NeedsRedownload
+    //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot load.
     //! @sa ActivateSnapshot
     uint32_t nStatus GUARDED_BY(::cs_main){0};
 
diff --git a/src/validation.cpp b/src/validation.cpp
index 64588e802d7..b343d6c5edf 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2457,7 +2457,7 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
     // may have let in a block that violates the rule prior to updating the
     // software, and we would NOT be enforcing the rule here. Fully solving
     // upgrade from one software version to the next after a consensus rule
-    // change is potentially tricky and issue-specific (see NeedsRedownload()
+    // change is potentially tricky and issue-specific (see historical versions of NeedsRedownload()
     // for one approach that was used for BIP 141 deployment).
     // Also, currently the rule against blocks more than 2 hours in the future
     // is enforced in ContextualCheckBlockHeader(); we wouldn't want to
@@ -4989,16 +4989,12 @@ bool Chainstate::NeedsRedownload() const
     AssertLockHeld(cs_main);
 
     // At and above m_params.SegwitHeight, segwit consensus rules must be validated
-    CBlockIndex* block{m_chain.Tip()};
-
-    while (block != nullptr && DeploymentActiveAt(*block, m_chainman, Consensus::DEPLOYMENT_SEGWIT)) {
-        if (!(block->nStatus & BLOCK_OPT_WITNESS)) {
-            // block is insufficiently validated for a segwit client
-            return true;
-        }
-        block = block->pprev;
+    int segwit_height{m_chainman.GetConsensus().DeploymentHeight(Consensus::DEPLOYMENT_SEGWIT)};
+    CBlockIndex* block{m_chain[segwit_height]};
+    if (block && !(block->nStatus & BLOCK_OPT_WITNESS)) {
+        // block is insufficiently validated for a segwit client
+        return true;
     }
-
     return false;
 }
 
@@ -6070,8 +6066,8 @@ util::Result<void> ChainstateManager::PopulateAndValidateSnapshot(
     for (int i = AFTER_GENESIS_START; i <= snapshot_chainstate.m_chain.Height(); ++i) {
         index = snapshot_chainstate.m_chain[i];
 
-        // Fake BLOCK_OPT_WITNESS so that Chainstate::NeedsRedownload()
-        // won't ask for -reindex on startup.
+        // Set BLOCK_OPT_WITNESS, which would normally be done in ReceivedBlockTransactions
+        // if we had downloaded the block.
         if (DeploymentActiveAt(*index, *this, Consensus::DEPLOYMENT_SEGWIT)) {
             index->nStatus |= BLOCK_OPT_WITNESS;
         }