Merge bitcoin/bitcoin#25193: indexes: Read the locator's top block during init, allow interaction with reindex-chainstate

src/index/base.cpp

@@ -18,6 +18,8 @@ using node::PruneLockInfo;
 using node::ReadBlockFromDisk;
 using node::fPruneMode;
 
+using node::g_indexes_ready_to_sync;
+
 constexpr uint8_t DB_BEST_BLOCK{'B'};
 
 constexpr auto SYNC_LOG_INTERVAL{30s};
@@ -69,14 +71,22 @@ bool BaseIndex::Init()
     if (locator.IsNull()) {
         SetBestBlockIndex(nullptr);
     } else {
-        SetBestBlockIndex(m_chainstate->FindForkInGlobalIndex(locator));
+        // Setting the best block to the locator's top block. If it is not part of the
+        // best chain, we will rewind to the fork point during index sync
+        const CBlockIndex* locator_index{m_chainstate->m_blockman.LookupBlockIndex(locator.vHave.at(0))};
+        if (!locator_index) {
+            return InitError(strprintf(Untranslated("%s: best block of the index not found. Please rebuild the index."), GetName()));
+        }
+        SetBestBlockIndex(locator_index);
     }
 
     // Note: this will latch to true immediately if the user starts up with an empty
     // datadir and an index enabled. If this is the case, indexation will happen solely
     // via `BlockConnected` signals until, possibly, the next restart.
     m_synced = m_best_block_index.load() == active_chain.Tip();
-    if (!m_synced) {
+
+    // Skip pruning check if indexes are not ready to sync (because reindex-chainstate has wiped the chain).
+    if (!m_synced && g_indexes_ready_to_sync) {
         bool prune_violation = false;
         if (!m_best_block_index) {
             // index is not built yet
@@ -131,6 +141,12 @@ static const CBlockIndex* NextSyncBlock(const CBlockIndex* pindex_prev, CChain&
 
 void BaseIndex::ThreadSync()
 {
+    // Wait for a possible reindex-chainstate to finish until continuing
+    // with the index sync
+    while (!g_indexes_ready_to_sync) {
+        if (!m_interrupt.sleep_for(std::chrono::milliseconds(500))) return;
+    }
+
     const CBlockIndex* pindex = m_best_block_index.load();
     if (!m_synced) {
         auto& consensus_params = Params().GetConsensus();

src/init.cpp

@@ -149,6 +149,7 @@ using node::DEFAULT_PRINTPRIORITY;
 using node::DEFAULT_SPENTINDEX;
 using node::DEFAULT_STOPAFTERBLOCKIMPORT;
 using node::DEFAULT_TIMESTAMPINDEX;
+using node::g_indexes_ready_to_sync;
 using node::LoadChainstate;
 using node::NodeContext;
 using node::ThreadImport;
@@ -572,7 +573,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-pid=<file>", strprintf("Specify pid file. Relative paths will be prefixed by a net-specific datadir location. (default: %s)", BITCOIN_PID_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks, and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex, -rescan and -disablegovernance=false. "
             "Warning: Reverting this setting requires re-downloading the entire blockchain. "
-            "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+            "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-settings=<file>", strprintf("Specify path to dynamic settings data file. Can be disabled with -nosettings. File is written at runtime and not meant to be edited by users (use %s instead for custom settings). Relative paths will be prefixed by datadir location. (default: %s)", BITCOIN_CONF_FILENAME, BITCOIN_SETTINGS_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-syncmempool", strprintf("Sync mempool from other nodes on start (default: %u)", DEFAULT_SYNC_MEMPOOL), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 #if HAVE_SYSTEM
@@ -588,7 +589,7 @@ void SetupServerArgs(ArgsManager& argsman)
 
     argsman.AddArg("-addressindex", strprintf("Maintain a full address index, used to query for the balance, txids and unspent outputs for addresses (default: %u)", DEFAULT_ADDRESSINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
-    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead. Deactivate all optional indexes before running this.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
+    argsman.AddArg("-reindex-chainstate", "Rebuild chain state from the currently indexed blocks. When in pruning mode or if blocks on disk might be corrupted, use full -reindex instead.", ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-spentindex", strprintf("Maintain a full spent index, used to query the spending txid and input index for an outpoint (default: %u)", DEFAULT_SPENTINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-timestampindex", strprintf("Maintain a timestamp index for block hashes, used to query blocks hashes by a range of timestamps (default: %u)", DEFAULT_TIMESTAMPINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
     argsman.AddArg("-txindex", strprintf("Maintain a full transaction index, used by the getrawtransaction rpc call (default: %u)", DEFAULT_TXINDEX), ArgsManager::ALLOW_ANY, OptionsCategory::INDEXING);
@@ -1386,19 +1387,6 @@ bool AppInitParameterInteraction(const ArgsManager& args)
 
     nMaxTipAge = args.GetIntArg("-maxtipage", DEFAULT_MAX_TIP_AGE);
 
-    if (args.GetBoolArg("-reindex-chainstate", false)) {
-        // indexes that must be deactivated to prevent index corruption, see #24630
-        if (args.GetBoolArg("-coinstatsindex", DEFAULT_COINSTATSINDEX)) {
-            return InitError(_("-reindex-chainstate option is not compatible with -coinstatsindex. Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
-        }
-        if (g_enabled_filter_types.count(BlockFilterType::BASIC_FILTER)) {
-            return InitError(_("-reindex-chainstate option is not compatible with -blockfilterindex. Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
-        }
-        if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
-            return InitError(_("-reindex-chainstate option is not compatible with -txindex. Please temporarily disable txindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes."));
-        }
-    }
-
     try {
         const bool fRecoveryEnabled{llmq::QuorumDataRecoveryEnabled()};
         const bool fQuorumVvecRequestsEnabled{llmq::GetEnabledQuorumVvecSyncEntries().size() > 0};
@@ -2235,6 +2223,9 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
     }
 
     // ********************************************************* Step 8: start indexers
+
+    // If reindex-chainstate was specified, delay syncing indexes until ThreadImport has reindexed the chain
+    if (!fReindexChainState) g_indexes_ready_to_sync = true;
     if (args.GetBoolArg("-txindex", DEFAULT_TXINDEX)) {
         g_txindex = std::make_unique<TxIndex>(cache_sizes.tx_index, false, fReindex);
         if (!g_txindex->Start(chainman.ActiveChainstate())) {

src/node/blockstorage.cpp

@@ -26,6 +26,7 @@
 namespace node {
 std::atomic_bool fImporting(false);
 std::atomic_bool fReindex(false);
+std::atomic_bool g_indexes_ready_to_sync{false};
 bool fPruneMode = false;
 uint64_t nPruneTarget = 0;
 
@@ -898,5 +899,6 @@ void ThreadImport(ChainstateManager& chainman, std::vector<fs::path> vImportFile
     } // End scope of CImportingNow
 
     chainman.ActiveChainstate().LoadMempool(args);
+    g_indexes_ready_to_sync = true;
 }
 } // namespace node

src/node/blockstorage.h

@@ -50,6 +50,7 @@ static constexpr size_t BLOCK_SERIALIZATION_HEADER_SIZE = CMessageHeader::MESSAG
 
 extern std::atomic_bool fImporting;
 extern std::atomic_bool fReindex;
+extern std::atomic_bool g_indexes_ready_to_sync;
 /** Pruning-related variables and constants */
 /** True if we're running in -prune mode. */
 extern bool fPruneMode;

src/test/util/setup_common.cpp

@@ -229,6 +229,7 @@ BasicTestingSetup::BasicTestingSetup(const std::string& chainName, const std::ve
         noui_connect();
         noui_connected = true;
     }
+    node::g_indexes_ready_to_sync = true;
 
     bls::bls_legacy_scheme.store(true);
 }

test/functional/feature_coinstatsindex.py

@@ -54,6 +54,7 @@ def run_test(self):
         self._test_use_index_option()
         self._test_reorg_index()
         self._test_index_rejects_hash_serialized()
+        self._test_init_index_after_reorg()
 
     def block_subsidy(self, prev_bits):
         # Subsidy calculations valid till block 4500
@@ -75,6 +76,9 @@ def block_sanity_check(self, block_info, prev_height):
             block_info['new_outputs_ex_coinbase'] + block_info['coinbase'] + block_info['unspendable']
         )
 
+    def sync_index_node(self):
+        self.wait_until(lambda: self.nodes[1].getindexinfo()['coinstatsindex']['synced'] is True)
+
     def _test_coin_stats_index(self):
         node = self.nodes[0]
         index_node = self.nodes[1]
@@ -241,18 +245,16 @@ def _test_coin_stats_index(self):
         self.log.info("Test that the index works with -reindex")
 
         self.restart_node(1, extra_args=["-coinstatsindex", "-reindex"])
+        self.sync_index_node()
         res11 = index_node.gettxoutsetinfo('muhash')
         assert_equal(res11, res10)
 
-        self.log.info("Test that -reindex-chainstate is disallowed with coinstatsindex")
+        self.log.info("Test that the index works with -reindex-chainstate")
 
-        self.stop_node(1)
-        self.nodes[1].assert_start_raises_init_error(
-            expected_msg='Error: -reindex-chainstate option is not compatible with -coinstatsindex. '
-            'Please temporarily disable coinstatsindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.',
-            extra_args=['-coinstatsindex', '-reindex-chainstate'],
-        )
-        self.restart_node(1, extra_args=["-coinstatsindex"])
+        self.restart_node(1, extra_args=["-coinstatsindex", "-reindex-chainstate"])
+        self.sync_index_node()
+        res12 = index_node.gettxoutsetinfo('muhash')
+        assert_equal(res12, res10)
 
         self.log.info("Test obtaining info for a non-existent block hash")
         assert_raises_rpc_error(-5, "Block not found", index_node.gettxoutsetinfo, hash_type="none", hash_or_height="ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff", use_index=True)
@@ -274,6 +276,7 @@ def _test_reorg_index(self):
         index_node = self.nodes[1]
         reorg_blocks = self.generatetoaddress(index_node, 2, getnewdestination()[2])
         reorg_block = reorg_blocks[1]
+        self.sync_index_node()
         res_invalid = index_node.gettxoutsetinfo('muhash')
         index_node.invalidateblock(reorg_blocks[0])
         assert_equal(index_node.gettxoutsetinfo('muhash')['height'], 110)
@@ -313,6 +316,26 @@ def _test_index_rejects_hash_serialized(self):
         for use_index in {True, False, None}:
             assert_raises_rpc_error(-8, msg, self.nodes[1].gettxoutsetinfo, hash_type='hash_serialized_2', hash_or_height=111, use_index=use_index)
 
+    def _test_init_index_after_reorg(self):
+        self.log.info("Test a reorg while the index is deactivated")
+        index_node = self.nodes[1]
+        block = self.nodes[0].getbestblockhash()
+        self.generate(index_node, 2, sync_fun=self.no_op)
+        self.sync_index_node()
+
+        # Restart without index
+        self.restart_node(1, extra_args=[])
+        self.connect_nodes(0, 1)
+        index_node.invalidateblock(block)
+        self.generatetoaddress(index_node, 5, getnewdestination()[2])
+        res = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=False)
+
+        # Restart with index that still has its best block on the old chain
+        self.restart_node(1, extra_args=self.extra_args[1])
+        self.sync_index_node()
+        res1 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=True)
+        assert_equal(res["muhash"], res1["muhash"])
+
 
 if __name__ == '__main__':
     CoinStatsIndexTest().main()

test/functional/p2p_blockfilters.py

@@ -293,13 +293,6 @@ def run_test(self):
         msg = "Error: Unknown -blockfilterindex value abc."
         self.nodes[0].assert_start_raises_init_error(expected_msg=msg)
 
-        self.log.info("Test -blockfilterindex with -reindex-chainstate raises an error")
-        self.nodes[0].assert_start_raises_init_error(
-            expected_msg='Error: -reindex-chainstate option is not compatible with -blockfilterindex. '
-            'Please temporarily disable blockfilterindex while using -reindex-chainstate, or replace -reindex-chainstate with -reindex to fully rebuild all indexes.',
-            extra_args=['-blockfilterindex', '-reindex-chainstate'],
-        )
-
         self.test_special_transactions_in_filters()
 
     def create_simple_assetlock(self, base_tx, base_tx_value, amount_locked_1, amount_locked_2=0):