Merge bitcoin/bitcoin#33212: index: Don't commit state in BaseIndex::Rewind

a602f6fb7b test: index with an unclean restart after a reorg (Martin Zumsande)
01b95ac6f4 index: don't commit state in BaseIndex::Rewind (Martin Zumsande)

Pull request description:

  The committed state of an index should never be ahead of the flushed chainstate.
  Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
  from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
  Instead, the index state will be committed with the next ChainStateFlushed notification.

  Fixes #33208

ACKs for top commit:
  achow101:
    ACK a602f6fb7b
  stickies-v:
    re-ACK a602f6fb7b

Tree-SHA512: 2559ea3fe066caf746a54ad7daac5031332f3976848e937c3dc8b35fa2ce925674115d8742458bf3703b3916f04f851c26523b6b94aeb1da651ba5a1b167a419

src/index/base.cpp

@@ -299,18 +299,13 @@ bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_ti
         }
     }
 
-    // In the case of a reorg, ensure persisted block locator is not stale.
+    // Don't commit here - the committed index state must never be ahead of the
+    // flushed chainstate, otherwise unclean restarts would lead to index corruption.
     // Pruning has a minimum of 288 blocks-to-keep and getting the index
     // out of sync may be possible but a users fault.
     // In case we reorg beyond the pruned depth, ReadBlock would
     // throw and lead to a graceful shutdown
     SetBestBlockIndex(new_tip);
-    if (!Commit()) {
-        // If commit fails, revert the best block index to avoid corruption.
-        SetBestBlockIndex(current_tip);
-        return false;
-    }
-
     return true;
 }
 

test/functional/feature_coinstatsindex.py

@@ -321,6 +321,21 @@ class CoinStatsIndexTest(BitcoinTestFramework):
         res1 = index_node.gettxoutsetinfo(hash_type='muhash', hash_or_height=None, use_index=True)
         assert_equal(res["muhash"], res1["muhash"])
 
+        self.log.info("Test index with an unclean restart after a reorg")
+        self.restart_node(1, extra_args=self.extra_args[1])
+        committed_height = index_node.getblockcount()
+        self.generate(index_node, 2, sync_fun=self.no_op)
+        self.sync_index_node()
+        block2 = index_node.getbestblockhash()
+        index_node.invalidateblock(block2)
+        self.generatetoaddress(index_node, 1, getnewdestination()[2], sync_fun=self.no_op)
+        self.sync_index_node()
+        index_node.kill_process()
+        self.start_node(1, extra_args=self.extra_args[1])
+        self.sync_index_node()
+        # Because of the unclean shutdown above, indexes reset to the point we last committed them to disk.
+        assert_equal(index_node.getindexinfo()['coinstatsindex']['best_block_height'], committed_height)
+
 
 if __name__ == '__main__':
     CoinStatsIndexTest(__file__).main()