Merge bitcoin/bitcoin#27720: index: prevent race by calling 'CustomInit' prior setting 'synced' flag

3126454dcfa1dd29bb66500d5f2b5261684d6c58 index: prevent race by calling 'CustomInit' prior setting 'synced' flag (furszy)

Pull request description:

  Decoupled from #27607.

  Fixed a potential race condition in master (not possible so far) that could become an actual issue soon.
  Where the index's  `CustomAppend` method could be called (from `BlockConnected`) before its
  `CustomInit` method, causing the index to try to update itself before it is initialized.

  This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
  before calling to the child class init function (`CustomInit`). So, for example, the block filter index could
  process a block before initialize the next filter position field and end up overwriting the first stored filter.

  This race was introduced in https://github.com/bitcoin/bitcoin/commit/bef4e405f3de2718dfee279a9abff4daf016da26 from https://github.com/bitcoin/bitcoin/pull/25494.

ACKs for top commit:
  achow101:
    ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58
  mzumsande:
    Code review ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58
  TheCharlatan:
    Nice, ACK 3126454dcfa1dd29bb66500d5f2b5261684d6c58

Tree-SHA512: 7a53fed1d2035cb4c1f331d6ee9f92d499b6cbb618ea534c6440f5a45ff9b3ac4dcff5fb4b88937f45a0be249e3a9c6dc6c3ac77180f12ae25fc56856ba39735

1 files changed, 15 insertions, 13 deletions

diff --git a/src/index/base.cpp b/src/index/base.cpp
index 7444579395..3f91910db2 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -103,15 +103,12 @@ bool BaseIndex::Init()
         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();
-
     // 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) {
+    const CBlockIndex* start_block = m_best_block_index.load();
+    bool synced = start_block == active_chain.Tip();
+    if (!synced && g_indexes_ready_to_sync) {
         bool prune_violation = false;
-        if (!m_best_block_index) {
+        if (!start_block) {
             // index is not built yet
             // make sure we have all block data back to the genesis
             prune_violation = m_chainstate->m_blockman.GetFirstStoredBlock(*active_chain.Tip()) != active_chain.Genesis();
@@ -119,7 +116,7 @@ bool BaseIndex::Init()
         // in case the index has a best block set and is not fully synced
         // check if we have the required blocks to continue building the index
         else {
-            const CBlockIndex* block_to_test = m_best_block_index.load();
+            const CBlockIndex* block_to_test = start_block;
             if (!active_chain.Contains(block_to_test)) {
                 // if the bestblock is not part of the mainchain, find the fork
                 // and make sure we have all data down to the fork
@@ -143,6 +140,16 @@ bool BaseIndex::Init()
             return InitError(strprintf(Untranslated("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)"), GetName()));
         }
     }
+
+    // Child init
+    if (!CustomInit(start_block ? std::make_optional(interfaces::BlockKey{start_block->GetBlockHash(), start_block->nHeight}) : std::nullopt)) {
+        return false;
+    }
+
+    // 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 = synced;
     return true;
 }
 
@@ -408,11 +415,6 @@ bool BaseIndex::Start()
     RegisterValidationInterface(this);
     if (!Init()) return false;
 
-    const CBlockIndex* index = m_best_block_index.load();
-    if (!CustomInit(index ? std::make_optional(interfaces::BlockKey{index->GetBlockHash(), index->nHeight}) : std::nullopt)) {
-        return false;
-    }
-
     m_thread_sync = std::thread(&util::TraceThread, GetName(), [this] { ThreadSync(); });
     return true;
 }