From 5ad82a09b409d416236092062a4201e238dfd68b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 30 Sep 2022 08:44:04 -0400 Subject: index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability Since commit f08c9fb0c6a799e3cb75ca5f763a746471625beb from PR https://github.com/bitcoin/bitcoin/pull/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 f08c9fb0c6a799e3cb75ca5f763a746471625beb, 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 https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133 This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors https://github.com/bitcoin/bitcoin/issues/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 Co-authored-by: MacroFake Github-Pull: #26215 Rebased-From: 8891949bdcb25093d3a6703ae8228c3c3687d3a4 --- src/index/base.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/index/base.cpp b/src/index/base.cpp index 88c2ce98fa..3eea09b17d 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -298,6 +298,10 @@ void BaseIndex::BlockConnected(const std::shared_ptr& 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; } -- cgit v1.2.3