Merge bitcoin/bitcoin#32878: index: fix wrong assert of current_tip == m_best_block_index

3aef38f44b test: exercise index reorg assertion failure (furszy)
acf50233cd index: fix wrong assert of current_tip == m_best_block_index (Hao Xu)

Pull request description:

  In BaseIndex::Sync(), pindex in `Rewind(pindex, pindex_next->pprev)` isn't always equal to m_best_block_index since m_best_block_index is updated every SYNC_LOCATOR_WRITE_INTERVAL seconds, during which multiple pindex update could happen. Thus the assert here is wrong.

ACKs for top commit:
  achow101:
    ACK 3aef38f44b
  furszy:
    ACK 3aef38f
  mzumsande:
    Code Review ACK 3aef38f44b

Tree-SHA512: 3ef9cc6dfdec10a9f95d7414c6a11aa216e4cf5974440d80ab19fc919abd2a3bd4c875718c9dc94523c33826f8582ec5a016374deb8fb2d35cd2fb7799b5c82e

src/index/base.cpp

@@ -186,8 +186,8 @@ void BaseIndex::Sync()
 {
     const CBlockIndex* pindex = m_best_block_index.load();
     if (!m_synced) {
-        std::chrono::steady_clock::time_point last_log_time{0s};
-        std::chrono::steady_clock::time_point last_locator_write_time{0s};
+        auto last_log_time{NodeClock::now()};
+        auto last_locator_write_time{last_log_time};
         while (true) {
             if (m_interrupt) {
                 LogInfo("%s: m_interrupt set; exiting ThreadSync", GetName());
@@ -229,14 +229,13 @@ void BaseIndex::Sync()
 
             if (!ProcessBlock(pindex)) return; // error logged internally
 
-            auto current_time{std::chrono::steady_clock::now()};
-            if (last_log_time + SYNC_LOG_INTERVAL < current_time) {
-                LogInfo("Syncing %s with block chain from height %d",
-                          GetName(), pindex->nHeight);
+            auto current_time{NodeClock::now()};
+            if (current_time - last_log_time >= SYNC_LOG_INTERVAL) {
+                LogInfo("Syncing %s with block chain from height %d", GetName(), pindex->nHeight);
                 last_log_time = current_time;
             }
 
-            if (last_locator_write_time + SYNC_LOCATOR_WRITE_INTERVAL < current_time) {
+            if (current_time - last_locator_write_time >= SYNC_LOCATOR_WRITE_INTERVAL) {
                 SetBestBlockIndex(pindex);
                 last_locator_write_time = current_time;
                 // No need to handle errors in Commit. See rationale above.
@@ -274,7 +273,6 @@ bool BaseIndex::Commit()
 
 bool BaseIndex::Rewind(const CBlockIndex* current_tip, const CBlockIndex* new_tip)
 {
-    assert(current_tip == m_best_block_index);
     assert(current_tip->GetAncestor(new_tip->nHeight) == new_tip);
 
     CBlock block;

src/test/blockfilter_index_tests.cpp

@@ -16,6 +16,7 @@
 #include <validation.h>
 
 #include <boost/test/unit_test.hpp>
+#include <future>
 
 using node::BlockAssembler;
 using node::BlockManager;
@@ -305,4 +306,81 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_init_destroy, BasicTestingSetup)
     BOOST_CHECK(filter_index == nullptr);
 }
 
+class IndexReorgCrash : public BaseIndex
+{
+private:
+    std::unique_ptr<BaseIndex::DB> m_db;
+    std::shared_future<void> m_blocker;
+    int m_blocking_height;
+
+public:
+    explicit IndexReorgCrash(std::unique_ptr<interfaces::Chain> chain, std::shared_future<void> blocker,
+                             int blocking_height) : BaseIndex(std::move(chain), "test index"), m_blocker(blocker),
+                                                    m_blocking_height(blocking_height)
+    {
+        const fs::path path = gArgs.GetDataDirNet() / "index";
+        fs::create_directories(path);
+        m_db = std::make_unique<BaseIndex::DB>(path / "db", /*n_cache_size=*/0, /*f_memory=*/true, /*f_wipe=*/false);
+    }
+
+    bool AllowPrune() const override { return false; }
+    BaseIndex::DB& GetDB() const override { return *m_db; }
+
+    bool CustomAppend(const interfaces::BlockInfo& block) override
+    {
+        // Simulate a delay so new blocks can get connected during the initial sync
+        if (block.height == m_blocking_height) m_blocker.wait();
+
+        // Move mock time forward so the best index gets updated only when we are not at the blocking height
+        if (block.height == m_blocking_height - 1 || block.height > m_blocking_height) {
+            SetMockTime(GetTime<std::chrono::seconds>() + 31s);
+        }
+
+        return true;
+    }
+};
+
+BOOST_FIXTURE_TEST_CASE(index_reorg_crash, BuildChainTestingSetup)
+{
+    // Enable mock time
+    SetMockTime(GetTime<std::chrono::minutes>());
+
+    std::promise<void> promise;
+    std::shared_future<void> blocker(promise.get_future());
+    int blocking_height = WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()->nHeight);
+
+    IndexReorgCrash index(interfaces::MakeChain(m_node), blocker, blocking_height);
+    BOOST_REQUIRE(index.Init());
+    BOOST_REQUIRE(index.StartBackgroundSync());
+
+    auto func_wait_until = [&](int height, std::chrono::milliseconds timeout) {
+        auto deadline = std::chrono::steady_clock::now() + timeout;
+        while (index.GetSummary().best_block_height < height) {
+            if (std::chrono::steady_clock::now() > deadline) {
+                BOOST_FAIL(strprintf("Timeout waiting for index height %d (current: %d)", height, index.GetSummary().best_block_height));
+                return;
+            }
+            std::this_thread::sleep_for(100ms);
+        }
+    };
+
+    // Wait until the index is one block before the fork point
+    func_wait_until(blocking_height - 1, /*timeout=*/5s);
+
+    // Create a fork to trigger the reorg
+    std::vector<std::shared_ptr<CBlock>> fork;
+    const CBlockIndex* prev_tip = WITH_LOCK(cs_main, return m_node.chainman->ActiveChain().Tip()->pprev);
+    BOOST_REQUIRE(BuildChain(prev_tip, GetScriptForDestination(PKHash(GenerateRandomKey().GetPubKey())), 3, fork));
+
+    for (const auto& block : fork) {
+        BOOST_REQUIRE(m_node.chainman->ProcessNewBlock(block, /*force_processing=*/true, /*min_pow_checked=*/true, nullptr));
+    }
+
+    // Unblock the index thread so it can process the reorg
+    promise.set_value();
+    // Wait for the index to reach the new tip
+    func_wait_until(blocking_height + 2, 5s);
+    index.Stop();
+}
+
 BOOST_AUTO_TEST_SUITE_END()