Currently, the index_reorg_crash test may rarely crash due to UB in sanitizers like TSan or ASan. This is perfectly fine, because it is just a rare test-only issue.
However, fix it nonetheless by adding a missing drain of the unused in-flight events. Also, add a small check about the synced state while touching this test.
This clarifies the initial index sync thread is blocked.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36148.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Without the drain, a BlockConnected event may execute during shutdown
and lead to memory races.
This is a bit tedious to reproduce, because the failure seems rare. But it happens a bit more often, by adding some sleeps and a stack canary. This diff should fail sanitizers (Asan, Tsan, valgrind) on current master:
diff --git a/src/test/baseindex_tests.cpp b/src/test/baseindex_tests.cpp
index 4e9a03a185..90fa101ca1 100644
--- a/src/test/baseindex_tests.cpp
+++ b/src/test/baseindex_tests.cpp
@@ -164,2 +164,3 @@ private:
std::shared_future<void> m_blocker;
+ std::promise<void> m_callback_entered;
int m_blocking_height;
@@ -178,2 +179,23 @@ public:
+ void WaitForCallbackEntered() { m_callback_entered.get_future().wait(); }
+
+ void BlockConnected(const ChainstateRole& role, const std::shared_ptr<const CBlock>& block, const CBlockIndex* pindex) override
+ {
+ if (pindex->nHeight == m_blocking_height + 2) {
+ m_callback_entered.set_value();
+
+ // Ensure the delegated BaseIndex::BlockConnected() passes its
+ // m_synced guard and reaches the callback body.
+ while (!GetSummary().synced) {
+ std::this_thread::sleep_for(1ms);
+ }
+
+ // Keep this callback active while the test destroys the index.
+ // The trailing BaseIndex::BlockConnected() then accesses the
+ // destroyed object.
+ std::this_thread::sleep_for(500ms);
+ }
+ BaseIndex::BlockConnected(role, block, pindex);
+ }
+
bool CustomAppend(const interfaces::BlockInfo& block) override
@@ -198,2 +220,3 @@ BOOST_FIXTURE_TEST_CASE(index_reorg_crash, TestChain100Setup)
+ {
IndexReorgCrash index{interfaces::MakeChain(m_node), blocker, blocking_height, m_clock};
@@ -225,2 +248,5 @@ BOOST_FIXTURE_TEST_CASE(index_reorg_crash, TestChain100Setup)
+ // The index thread is blocked and not done
+ BOOST_CHECK(!index.GetSummary().synced);
+
// Unblock the index thread so it can process the reorg
@@ -229,3 +255,10 @@ BOOST_FIXTURE_TEST_CASE(index_reorg_crash, TestChain100Setup)
func_wait_until(blocking_height + 2, 5s);
+
+ index.WaitForCallbackEntered();
index.Stop();
+ }
+ volatile std::byte stack_buffer[128]{};
+ for (auto& byte : stack_buffer) {
+ byte = std::byte{0xaa};
+ }
}
ACK fa0f14ef5e76424ed7770936f7d053f27336a601
Without the drain, a BlockConnected event may execute during shutdown
and lead to memory races.
Sorry for the push, but there is another tedious to reproduce race in the other test:
diff --git a/src/index/base.cpp b/src/index/base.cpp
index 5820448..0cbac79 100644
--- a/src/index/base.cpp
+++ b/src/index/base.cpp
@@ -353,2 +353,5 @@ void BaseIndex::BlockConnected(const ChainstateRole& role, const std::shared_ptr
+ // Expand processing time so the test can destroy the index while this event runs.
+ UninterruptibleSleep(55ms);
+
// Ignore BlockConnected signals until we have fully indexed the chain.
diff --git a/src/test/baseindex_tests.cpp b/src/test/baseindex_tests.cpp
index f666f4e..233f85b 100644
--- a/src/test/baseindex_tests.cpp
+++ b/src/test/baseindex_tests.cpp
@@ -35,2 +35,3 @@
+#include <atomic>
#include <chrono>
@@ -63,2 +64,26 @@ BOOST_AUTO_TEST_SUITE(baseindex_tests)
+class BlockConnectedGate final : public CValidationInterface
+{
+public:
+ bool WaitForCallbackAndRelease()
+ {
+ if (m_entered.get_future().wait_for(5s) != std::future_status::ready) return false;
+ m_release.set_value();
+ return true;
+ }
+
+protected:
+ void BlockConnected(const ChainstateRole&, const std::shared_ptr<const CBlock>&, const CBlockIndex*) override
+ {
+ if (!m_armed.exchange(false)) return;
+ m_entered.set_value();
+ m_release.get_future().wait();
+ }
+
+private:
+ std::atomic<bool> m_armed{true};
+ std::promise<void> m_entered;
+ std::promise<void> m_release;
+};
+
// Test that the index does not commit ahead of the chainstate's last
@@ -70,2 +95,7 @@ BOOST_FIXTURE_TEST_CASE(baseindex_no_commit_ahead_of_flush, TestChain100Setup)
Chainstate& chainstate = Assert(m_node.chainman)->ActiveChainstate();
+ // TestChain100Setup queued 100 BlockConnected events. Register this first
+ // so it can hold one of those callbacks after CoinStatsIndex has synced.
+ BlockConnectedGate initial_block_connected_gate;
+ m_node.chain->context()->validation_signals->RegisterValidationInterface(&initial_block_connected_gate);
+ bool release_initial_block_connected_gate{true};
for (const auto& [index_name, make_index] : INDEX_FACTORIES) {
@@ -86,2 +116,8 @@ BOOST_FIXTURE_TEST_CASE(baseindex_no_commit_ahead_of_flush, TestChain100Setup)
BOOST_CHECK_EQUAL(index->GetSummary().best_block_height, expected_commit_height);
+ if (!do_flush && release_initial_block_connected_gate) {
+ release_initial_block_connected_gate = false;
+ BOOST_REQUIRE(initial_block_connected_gate.WaitForCallbackAndRelease());
+ // Let the scheduler enter BlockConnected before destroying index.
+ std::this_thread::sleep_for(1ms);
+ }
index->Stop();
@@ -107,2 +143,4 @@ BOOST_FIXTURE_TEST_CASE(baseindex_no_commit_ahead_of_flush, TestChain100Setup)
}
+ m_node.chain->context()->validation_signals->UnregisterValidationInterface(&initial_block_connected_gate);
+ m_node.chain->context()->validation_signals->SyncWithValidationInterfaceQueue();
}
diff --git a/src/validationinterface.cpp b/src/validationinterface.cpp
index 128f14a..7736f98 100644
--- a/src/validationinterface.cpp
+++ b/src/validationinterface.cpp
@@ -17,2 +17,3 @@
#include <util/task_runner.h>
+#include <util/time.h>
@@ -168,2 +169,3 @@ void ValidationSignals::SyncWithValidationInterfaceQueue()
LOG_EVENT("%s", local_log_msg); \
+ UninterruptibleSleep(std::chrono::milliseconds{55}); \
local_event(); \
This should fail with asan and valgrind.
ACK fab80e82c1087126477e07eda5f6e3a1f25ceb99
FreeBSD CI failure can be ignored :weary:
ACK fab80e82c1
reproduced the scenario master/pr: index_reorg_crash: fails 20/20 without the patch, 0/20 with it. baseindex_no_commit_ahead_of_flush: fails 19/20 without, 0/20 with, index_unclean_shutdown, blockfilter_index_tests, coinstatsindex_tests and txindex_tests: 0/20 (i did not find more test to fix this).
Nit: update PR descripction, it is outdated (now two test are fixed)
Would an Assume() canary be worth it for future tests? ValidationSignalsImpl::ListEntry already counts executions per subscriber, so exposing that count and checking it in ~BaseIndex() is a few lines. It widens the public API , so I am not sure.
Edit: the whole test_bitcoin suite passes with the canary in place and without forcing the race condition by widening the window, so it catches nothing on a normal run. Usless.