Merge bitcoin/bitcoin#27720: index: prevent race by calling 'CustomInit' prior setting 'synced' flag

3126454dcf index: prevent race by calling 'CustomInit' prior setting 'synced' flag (furszy)

Pull request description:

  Decoupled from #27607.

  Fixed a potential race condition in master (not possible so far) that could become an actual issue soon.
  Where the index's  `CustomAppend` method could be called (from `BlockConnected`) before its
  `CustomInit` method, causing the index to try to update itself before it is initialized.

  This could happen because we set the index `m_synced` flag (which enables `BlockConnected` events)
  before calling to the child class init function (`CustomInit`). So, for example, the block filter index could
  process a block before initialize the next filter position field and end up overwriting the first stored filter.

  This race was introduced in bef4e405f3 from https://github.com/bitcoin/bitcoin/pull/25494.

ACKs for top commit:
  achow101:
    ACK 3126454dcf
  mzumsande:
    Code review ACK 3126454dcf
  TheCharlatan:
    Nice, ACK 3126454dcf

Tree-SHA512: 7a53fed1d2035cb4c1f331d6ee9f92d499b6cbb618ea534c6440f5a45ff9b3ac4dcff5fb4b88937f45a0be249e3a9c6dc6c3ac77180f12ae25fc56856ba39735
This commit is contained in:
Andrew Chow 2023-05-31 13:45:22 -04:00
commit 3a83d4417b
No known key found for this signature in database
GPG Key ID: 17565732E08E5E41

View File

@ -103,15 +103,12 @@ bool BaseIndex::Init()
SetBestBlockIndex(locator_index);
}
// Note: this will latch to true immediately if the user starts up with an empty
// datadir and an index enabled. If this is the case, indexation will happen solely
// via `BlockConnected` signals until, possibly, the next restart.
m_synced = m_best_block_index.load() == active_chain.Tip();
// Skip pruning check if indexes are not ready to sync (because reindex-chainstate has wiped the chain).
if (!m_synced && g_indexes_ready_to_sync) {
const CBlockIndex* start_block = m_best_block_index.load();
bool synced = start_block == active_chain.Tip();
if (!synced && g_indexes_ready_to_sync) {
bool prune_violation = false;
if (!m_best_block_index) {
if (!start_block) {
// index is not built yet
// make sure we have all block data back to the genesis
prune_violation = m_chainstate->m_blockman.GetFirstStoredBlock(*active_chain.Tip()) != active_chain.Genesis();
@ -119,7 +116,7 @@ bool BaseIndex::Init()
// in case the index has a best block set and is not fully synced
// check if we have the required blocks to continue building the index
else {
const CBlockIndex* block_to_test = m_best_block_index.load();
const CBlockIndex* block_to_test = start_block;
if (!active_chain.Contains(block_to_test)) {
// if the bestblock is not part of the mainchain, find the fork
// and make sure we have all data down to the fork
@ -143,6 +140,16 @@ bool BaseIndex::Init()
return InitError(strprintf(Untranslated("%s best block of the index goes beyond pruned data. Please disable the index or reindex (which will download the whole blockchain again)"), GetName()));
}
}
// Child init
if (!CustomInit(start_block ? std::make_optional(interfaces::BlockKey{start_block->GetBlockHash(), start_block->nHeight}) : std::nullopt)) {
return false;
}
// Note: this will latch to true immediately if the user starts up with an empty
// datadir and an index enabled. If this is the case, indexation will happen solely
// via `BlockConnected` signals until, possibly, the next restart.
m_synced = synced;
return true;
}
@ -408,11 +415,6 @@ bool BaseIndex::Start()
RegisterValidationInterface(this);
if (!Init()) return false;
const CBlockIndex* index = m_best_block_index.load();
if (!CustomInit(index ? std::make_optional(interfaces::BlockKey{index->GetBlockHash(), index->nHeight}) : std::nullopt)) {
return false;
}
m_thread_sync = std::thread(&util::TraceThread, GetName(), [this] { ThreadSync(); });
return true;
}