index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability

Since commit f08c9fb from PR
bitcoin/bitcoin#21726, index
`BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has
also been a race condition in the `coinstatsindex_initial_sync` unit test.

It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the
last connected block to be fully processed, than to be able to return before
prune locks are set, so this switches the order of `m_best_block_index =
block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more
reliable.

Also since commit f08c9fb, there has been a
race condition in the `coinstatsindex_initial_sync` test. Before that commit,
the atomic index best block pointer `m_best_block_index` was updated as the
last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain`
could safely be used in tests to wait for the last `BlockConnected`
notification to be finished before stopping and destroying the index. But
after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer
sufficient, and there is a race between the test shutdown code which destroys
the index object and the new code introduced in that commit calling
`AllowPrune()` and `GetName()` on the index object. Reproducibility
instructions for this are in
bitcoin/bitcoin#25365 (comment)

This commit fixes the `coinstatsindex_initial_sync` race condition, even though
it will require an additional change to silence TSAN false positives,
bitcoin/bitcoin#26188, after it is fixed. So this
partially addresses but does not resolve the bug reporting TSAN errors
bitcoin/bitcoin#25365.

There is no known race condition outside of test code currently, because the
bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not
`BlockUntilSyncedToCurrentChain` to safely shut down.

Co-authored-by: Vasil Dimov <[email protected]>
Co-authored-by: MacroFake <[email protected]>

Author: 3 people

src/index/base.cpp

@@ -298,6 +298,10 @@ void BaseIndex::BlockConnected(const std::shared_ptr<const CBlock>& block, const
     }
     interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex, block.get());
     if (CustomAppend(block_info)) {
+        // Setting the best block index is intentionally the last step of this
+        // function, so BlockUntilSyncedToCurrentChain callers waiting for the
+        // best block index to be updated can rely on the block being fully
+        // processed, and the index object being safe to delete.
         SetBestBlockIndex(pindex);
     } else {
         FatalError("%s: Failed to write block %s to index",
@@ -414,10 +418,17 @@ IndexSummary BaseIndex::GetSummary() const
 void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) {
     assert(!node::fPruneMode || AllowPrune());
 
-    m_best_block_index = block;
     if (AllowPrune() && block) {
         node::PruneLockInfo prune_lock;
         prune_lock.height_first = block->nHeight;
         WITH_LOCK(::cs_main, m_chainstate->m_blockman.UpdatePruneLock(GetName(), prune_lock));
     }
+
+    // Intentionally set m_best_block_index as the last step in this function,
+    // after updating prune locks above, and after making any other references
+    // to *this, so the BlockUntilSyncedToCurrentChain function (which checks
+    // m_best_block_index as an optimization) can be used to wait for the last
+    // BlockConnected notification and safely assume that prune locks are
+    // updated and that the index object is safe to delete.
+    m_best_block_index = block;
 }