Merge bitcoin/bitcoin#29867: index: race fix, lock cs_main while 'm_synced' is subject to change

65951e0418 index: race fix, lock cs_main while 'm_synced' is subject to change (Ryan Ofsky)

Pull request description:

  Fixes #29831 and #29863. Thanks to Marko for the detailed description of the issue.

  The race occurs because a block could be connected and its event signaled in-between reading the 'next block' and setting `m_synced` during the index initial synchronization. This is because `cs_main` is not locked through the process of determining the final index sync state.
  To address the issue, the `m_synced` flag set has been moved under `cs_main` guard.

ACKs for top commit:
  fjahr:
    Code review ACK 65951e0418
  achow101:
    ACK 65951e0418
  ryanofsky:
    Code review ACK 65951e0418

Tree-SHA512: 77286e22de164a27939d2681b7baa6552eb75e99c541d3b9631f4340d7dd01742667c86899b6987fd2d97799d959e0a913a7749b2b69d9e50505128cd3ae0e69
This commit is contained in:
Ava Chow
2024-04-25 14:05:37 -04:00

View File

@@ -160,12 +160,24 @@ void BaseIndex::Sync()
}
const CBlockIndex* pindex_next = WITH_LOCK(cs_main, return NextSyncBlock(pindex, m_chainstate->m_chain));
// If pindex_next is null, it means pindex is the chain tip, so
// commit data indexed so far.
if (!pindex_next) {
SetBestBlockIndex(pindex);
// No need to handle errors in Commit. See rationale above.
Commit();
m_synced = true;
break;
// If pindex is still the chain tip after committing, exit the
// sync loop. It is important for cs_main to be locked while
// setting m_synced = true, otherwise a new block could be
// attached while m_synced is still false, and it would not be
// indexed.
LOCK(::cs_main);
pindex_next = NextSyncBlock(pindex, m_chainstate->m_chain);
if (!pindex_next) {
m_synced = true;
break;
}
}
if (pindex_next->pprev != pindex && !Rewind(pindex, pindex_next->pprev)) {
FatalErrorf("%s: Failed to rewind index %s to a previous chain tip", __func__, GetName());