Merge bitcoin/bitcoin#26188: test: silence TSAN false positive in coinstatsindex_initial_sync

861cb3fadce88cfaee27088185a48f03fb9dafe7 test: move SyncWithValidationInterfaceQueue() before Stop() in txindex_tests (Vasil Dimov)
6526dc3b78d9ca2b5c67564b04dcacbc75b857e1 test: silence TSAN false positive in coinstatsindex_initial_sync (Vasil Dimov)

Pull request description:

  Silence false positives from TSAN about unsynchronized calls to `BaseIndex::~BaseIndex()` and `BaseIndex::SetBestBlockIndex()`. They are synchronized, but beyond the comprehension of TSAN - by `SyncWithValidationInterfaceQueue()`, called from `BaseIndex::BlockUntilSyncedToCurrentChain()`.

  Fixes https://github.com/bitcoin/bitcoin/issues/25365

ACKs for top commit:
  MarcoFalke:
    review ACK 861cb3fadce88cfaee27088185a48f03fb9dafe7
  ryanofsky:
    Code review ACK 861cb3fadce88cfaee27088185a48f03fb9dafe7. Just comment change since last review.

Tree-SHA512: 8c30fdf2fd11d54e9adfa68a67185ab820bd7bd9f7f3ad6456e7e6d219fa9cf6d34b41e98e723eae86cb0c1baef7f3fc57b1b011a13dc3fe3d78334b9b5596de

2 files changed, 16 insertions, 5 deletions

diff --git a/src/test/coinstatsindex_tests.cpp b/src/test/coinstatsindex_tests.cpp
index 2a6a777cfe..8a2b0792fd 100644
--- a/src/test/coinstatsindex_tests.cpp
+++ b/src/test/coinstatsindex_tests.cpp
@@ -76,10 +76,16 @@ BOOST_FIXTURE_TEST_CASE(coinstatsindex_initial_sync, TestChain100Setup)
 
     BOOST_CHECK(block_index != new_block_index);
 
+    // It is not safe to stop and destroy the index until it finishes handling
+    // the last BlockConnected notification. The BlockUntilSyncedToCurrentChain()
+    // call above is sufficient to ensure this, but the
+    // SyncWithValidationInterfaceQueue() call below is also needed to ensure
+    // TSAN always sees the test thread waiting for the notification thread, and
+    // avoid potential false positive reports.
+    SyncWithValidationInterfaceQueue();
+
     // Shutdown sequence (c.f. Shutdown() in init.cpp)
     coin_stats_index.Stop();
-
-    // Rest of shutdown sequence and destructors happen in ~TestingSetup()
 }
 
 // Test shutdown between BlockConnected and ChainStateFlushed notifications,
diff --git a/src/test/txindex_tests.cpp b/src/test/txindex_tests.cpp
index 62c7ddb673..643d9221fe 100644
--- a/src/test/txindex_tests.cpp
+++ b/src/test/txindex_tests.cpp
@@ -69,11 +69,16 @@ BOOST_FIXTURE_TEST_CASE(txindex_initial_sync, TestChain100Setup)
         }
     }
 
+    // It is not safe to stop and destroy the index until it finishes handling
+    // the last BlockConnected notification. The BlockUntilSyncedToCurrentChain()
+    // call above is sufficient to ensure this, but the
+    // SyncWithValidationInterfaceQueue() call below is also needed to ensure
+    // TSAN always sees the test thread waiting for the notification thread, and
+    // avoid potential false positive reports.
+    SyncWithValidationInterfaceQueue();
+
     // shutdown sequence (c.f. Shutdown() in init.cpp)
     txindex.Stop();
-
-    // Let scheduler events finish running to avoid accessing any memory related to txindex after it is destructed
-    SyncWithValidationInterfaceQueue();
 }
 
 BOOST_AUTO_TEST_SUITE_END()