[validation] RewindBlockIndex no longer needed

Instead of rewinding blocks, we request that the user deletes blocks dir and chain state dir to IBD

Author: dhruv

src/init.cpp

@@ -1686,26 +1686,24 @@ bool AppInitMain(const util::Ref& context, NodeContext& node, interfaces::BlockA
                 break;
             }
 
-            bool failed_rewind{false};
-            // Can't hold cs_main while calling RewindBlockIndex, so retrieve the relevant
-            // chainstates beforehand.
-            for (CChainState* chainstate : WITH_LOCK(::cs_main, return chainman.GetAll())) {
-                if (!fReset) {
-                    // Note that RewindBlockIndex MUST run even if we're about to -reindex-chainstate.
-                    // It both disconnects blocks based on the chainstate, and drops block data in
-                    // BlockIndex() based on lack of available witness data.
-                    uiInterface.InitMessage(_("Rewinding blocks...").translated);
-                    if (!chainstate->RewindBlockIndex(chainparams)) {
-                        strLoadError = _(
-                            "Unable to rewind the database to a pre-fork state. "
-                            "You will need to redownload the blockchain");
-                        failed_rewind = true;
-                        break; // out of the per-chainstate loop
+            bool needs_ibd{false};
+            {
+                LOCK(cs_main);
+                for (CChainState* chainstate : chainman.GetAll()) {
+                    if (!fReset) {
+                        uiInterface.InitMessage(_("Checking if IBD is needed...").translated);
+                        if (chainstate->NeedsIBD(chainparams)) {
+                            strLoadError = _(
+                                "Segwit blocks are insufficiently validated. "
+                                "Please delete blocks dir and chain state dir and restart.");
+                            needs_ibd = true;
+                            break; // out of the per-chainstate loop
+                        }
                     }
                 }
             }
 
-            if (failed_rewind) {
+            if (needs_ibd) {
                 break; // out of the chainstate activation do-while
             }
 

src/validation.cpp

@@ -4390,143 +4390,22 @@ bool CChainState::ReplayBlocks(const CChainParams& params)
     return true;
 }
 
-//! Helper for CChainState::RewindBlockIndex
-void CChainState::EraseBlockData(CBlockIndex* index)
+bool CChainState::NeedsIBD(const CChainParams& params)
 {
     AssertLockHeld(cs_main);
-    assert(!m_chain.Contains(index)); // Make sure this block isn't active
-
-    // Reduce validity
-    index->nStatus = std::min<unsigned int>(index->nStatus & BLOCK_VALID_MASK, BLOCK_VALID_TREE) | (index->nStatus & ~BLOCK_VALID_MASK);
-    // Remove have-data flags.
-    index->nStatus &= ~(BLOCK_HAVE_DATA | BLOCK_HAVE_UNDO);
-    // Remove storage location.
-    index->nFile = 0;
-    index->nDataPos = 0;
-    index->nUndoPos = 0;
-    // Remove various other things
-    index->nTx = 0;
-    index->nChainTx = 0;
-    index->nSequenceId = 0;
-    // Make sure it gets written.
-    setDirtyBlockIndex.insert(index);
-    // Update indexes
-    setBlockIndexCandidates.erase(index);
-    auto ret = m_blockman.m_blocks_unlinked.equal_range(index->pprev);
-    while (ret.first != ret.second) {
-        if (ret.first->second == index) {
-            m_blockman.m_blocks_unlinked.erase(ret.first++);
-        } else {
-            ++ret.first;
-        }
-    }
-    // Mark parent as eligible for main chain again
-    if (index->pprev && index->pprev->IsValid(BLOCK_VALID_TRANSACTIONS) && index->pprev->HaveTxsDownloaded()) {
-        setBlockIndexCandidates.insert(index->pprev);
-    }
-}
-
-bool CChainState::RewindBlockIndex(const CChainParams& params)
-{
-    // Note that during -reindex-chainstate we are called with an empty m_chain!
 
-    // First erase all post-segwit blocks without witness not in the main chain,
-    // as this can we done without costly DisconnectTip calls. Active
-    // blocks will be dealt with below (releasing cs_main in between).
-    {
-        LOCK(cs_main);
-        for (const auto& entry : m_blockman.m_block_index) {
-            if (IsWitnessEnabled(entry.second->pprev, params.GetConsensus()) && !(entry.second->nStatus & BLOCK_OPT_WITNESS) && !m_chain.Contains(entry.second)) {
-                EraseBlockData(entry.second);
-            }
-        }
-    }
+    // At and above params.SegwitHeight, segwit consensus rules must be validated
+    CBlockIndex* block = m_chain[params.GetConsensus().SegwitHeight];
 
-    // Find what height we need to reorganize to.
-    CBlockIndex *tip;
-    int nHeight = 1;
-    {
-        LOCK(cs_main);
-        while (nHeight <= m_chain.Height()) {
-            // Although SCRIPT_VERIFY_WITNESS is now generally enforced on all
-            // blocks in ConnectBlock, we don't need to go back and
-            // re-download/re-verify blocks from before segwit actually activated.
-            if (IsWitnessEnabled(m_chain[nHeight - 1], params.GetConsensus()) && !(m_chain[nHeight]->nStatus & BLOCK_OPT_WITNESS)) {
-                break;
-            }
-            nHeight++;
-        }
-
-        tip = m_chain.Tip();
-    }
-    // nHeight is now the height of the first insufficiently-validated block, or tipheight + 1
-
-    BlockValidationState state;
-    // Loop until the tip is below nHeight, or we reach a pruned block.
-    while (!ShutdownRequested()) {
-        {
-            LOCK(cs_main);
-            LOCK(m_mempool.cs);
-            // Make sure nothing changed from under us (this won't happen because RewindBlockIndex runs before importing/network are active)
-            assert(tip == m_chain.Tip());
-            if (tip == nullptr || tip->nHeight < nHeight) break;
-            if (fPruneMode && !(tip->nStatus & BLOCK_HAVE_DATA)) {
-                // If pruning, don't try rewinding past the HAVE_DATA point;
-                // since older blocks can't be served anyway, there's
-                // no need to walk further, and trying to DisconnectTip()
-                // will fail (and require a needless reindex/redownload
-                // of the blockchain).
-                break;
-            }
-
-            // Disconnect block
-            if (!DisconnectTip(state, params, nullptr)) {
-                return error("RewindBlockIndex: unable to disconnect block at height %i (%s)", tip->nHeight, state.ToString());
-            }
-
-            // Reduce validity flag and have-data flags.
-            // We do this after actual disconnecting, otherwise we'll end up writing the lack of data
-            // to disk before writing the chainstate, resulting in a failure to continue if interrupted.
-            // Note: If we encounter an insufficiently validated block that
-            // is on m_chain, it must be because we are a pruning node, and
-            // this block or some successor doesn't HAVE_DATA, so we were unable to
-            // rewind all the way.  Blocks remaining on m_chain at this point
-            // must not have their validity reduced.
-            EraseBlockData(tip);
-
-            tip = tip->pprev;
-        }
-        // Make sure the queue of validation callbacks doesn't grow unboundedly.
-        LimitValidationInterfaceQueue();
-
-        // Occasionally flush state to disk.
-        if (!FlushStateToDisk(params, state, FlushStateMode::PERIODIC)) {
-            LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
-            return false;
-        }
-    }
-
-    {
-        LOCK(cs_main);
-        if (m_chain.Tip() != nullptr) {
-            // We can't prune block index candidates based on our tip if we have
-            // no tip due to m_chain being empty!
-            PruneBlockIndexCandidates();
-
-            CheckBlockIndex(params.GetConsensus());
-
-            // FlushStateToDisk can possibly read ::ChainActive(). Be conservative
-            // and skip it here, we're about to -reindex-chainstate anyway, so
-            // it'll get called a bunch real soon.
-            BlockValidationState state;
-            if (!FlushStateToDisk(params, state, FlushStateMode::ALWAYS)) {
-                LogPrintf("RewindBlockIndex: unable to flush state to disk (%s)\n", state.ToString());
-                return false;
-            }
+    while (block != nullptr) {
+        if (!(block->nStatus & BLOCK_OPT_WITNESS)) {
+            // block is insufficiently validated for a segwit client
+            return true;
         }
+        block = m_chain.Next(block);
     }
 
-    return true;
+    return false;
 }
 
 void CChainState::UnloadBlockIndex() {

src/validation.h

@@ -684,7 +684,7 @@ class CChainState
 
     /** Replay blocks that aren't fully applied to the database. */
     bool ReplayBlocks(const CChainParams& params);
-    bool RewindBlockIndex(const CChainParams& params) LOCKS_EXCLUDED(cs_main);
+    [[nodiscard]] bool NeedsIBD(const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
     bool LoadGenesisBlock(const CChainParams& chainparams);
 
     void PruneBlockIndexCandidates();
@@ -730,9 +730,6 @@ class CChainState
 
     bool RollforwardBlock(const CBlockIndex* pindex, CCoinsViewCache& inputs, const CChainParams& params) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 
-    //! Mark a block as not having block data
-    void EraseBlockData(CBlockIndex* index) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
-
     friend ChainstateManager;
 };
 

test/functional/p2p_segwit.py

@@ -5,7 +5,9 @@
 """Test segwit transactions and blocks on P2P network."""
 from decimal import Decimal
 import math
+import os
 import random
+import shutil
 import struct
 import time
 
@@ -83,6 +85,7 @@
     softfork_active,
     hex_str_to_bytes,
     assert_raises_rpc_error,
+    get_datadir_path,
 )
 
 # The versionbit bit used to signal activation of SegWit
@@ -1956,21 +1959,49 @@ def test_non_standard_witness(self):
     def test_upgrade_after_activation(self):
         """Test the behavior of starting up a segwit-aware node after the softfork has activated."""
 
-        self.restart_node(2, extra_args=["-segwitheight={}".format(SEGWIT_HEIGHT)])
+        # All nodes are caught up and node 2 is a pre-segwit node that will soon upgrade.
+        assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
+        assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
+        assert SEGWIT_HEIGHT < self.nodes[2].getblockcount()
+        assert softfork_active(self.nodes[0], 'segwit')
+        assert softfork_active(self.nodes[1], 'segwit')
+        assert 'segwit' not in self.nodes[2].getblockchaininfo()['softforks']
+
+        # Restarting node 2 should result in a shutdown because the blockchain consists of
+        # insufficiently validated blocks per segwit consensus rules.
+        self.stop_node(2)
+        with self.nodes[2].assert_debug_log(expected_msgs=["Segwit blocks are insufficiently validated. Please delete blocks dir and chain state dir and restart."], timeout=10):
+            self.nodes[2].start(["-segwitheight={}".format(SEGWIT_HEIGHT)])
+
+        # As directed, the user deletes the blocks and chainstate directories and restarts the node
+        datadir = get_datadir_path(self.options.tmpdir, 2)
+        blocks_dir = os.path.join(datadir, "regtest", "blocks")
+        chainstate_dir = os.path.join(datadir, "regtest", "chainstate")
+        shutil.rmtree(blocks_dir)
+        shutil.rmtree(chainstate_dir)
+
+        self.start_node(2, extra_args=["-segwitheight={}".format(SEGWIT_HEIGHT)])
+        assert_equal(self.nodes[2].getblockcount(), 0)
         self.connect_nodes(0, 2)
 
         # We reconnect more than 100 blocks, give it plenty of time
         self.sync_blocks(timeout=240)
 
-        # Make sure that this peer thinks segwit has activated.
+        # The upgraded node syncs headers and performs IBD
+        assert_equal(self.nodes[0].getblockcount(), self.nodes[2].getblockcount())
+        assert_equal(self.nodes[1].getblockcount(), self.nodes[2].getblockcount())
+        assert softfork_active(self.nodes[0], 'segwit')
+        assert softfork_active(self.nodes[1], 'segwit')
         assert softfork_active(self.nodes[2], 'segwit')
 
-        # Make sure this peer's blocks match those of node0.
+        # Make sure all peers have the same blocks.
         height = self.nodes[2].getblockcount()
         while height >= 0:
             block_hash = self.nodes[2].getblockhash(height)
             assert_equal(block_hash, self.nodes[0].getblockhash(height))
+            assert_equal(block_hash, self.nodes[1].getblockhash(height))
             assert_equal(self.nodes[0].getblock(block_hash), self.nodes[2].getblock(block_hash))
+            assert_equal(self.nodes[1].getblock(block_hash), self.nodes[2].getblock(block_hash))
             height -= 1
 
     @subtest  # type: ignore