foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-02 09:46:52 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#26215: index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability

Browse source 
8891949bdc index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky)

Pull request description:

  Since commit f08c9fb0c6 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 f08c9fb0c6, 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: vasild
  Co-authored-by: MarcoFalke

ACKs for top commit:
  mzumsande:
    re-ACK 8891949bdc

Tree-SHA512: 52e29e3772a0c92873c54e5ffb31dd66a909b68a2031b7585713cd1d976811289c98bd9bb41679a8689062f03be4f97bb8368696e789caa4607c2fd8b1fe289b
This commit is contained in:
fanquake 2022-10-10 14:15:18 +08:00
commit 4175c332b9
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
1 changed files with 12 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

13
src/index/base.cpp
View file

@ -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;
}