Fsync during backfill

[michaelsproul]

Description

This issue needs fleshing out, but basically our backfill database writes aren't crash-safe. We write across multiple databases (hot/cold/blobs) during backfill, and unlike in block import, we never fsync. This means the hot DB and cold DB can get out of sync, which is a form of DB corruption.

This is the cause of gaps in the cold DB's block roots array, which is never repaired.

This issue is hit occasionally by users who have crash-killed or OOM'd their node. It might be straight-forward to reproduce now that we know the issue is in backfill. My previous attempts to recreate it involved crashing a synced LH node, which (thankfully) doesn't work.

[NikhilSharmaWe]

@michaelsproul Looking into this.

The failure appears to be in import_historical_block_batch: it writes across the blobs, hot, and cold databases, then advances AnchorInfo, without any sync barriers:

lighthouse/beacon_node/beacon_chain/src/historical_blocks.rs

Lines 251 to 307 in b263df5

	// Write the I/O batches to disk, writing the blocks themselves first, as it's better
	// for the hot DB to contain extra blocks than for the cold DB to point to blocks that
	// do not exist.
	{
	let _span = debug_span!("backfill_write_blobs_db").entered();
	self.store.blobs_db.do_atomically(blob_batch)?;
	}
	{
	let _span = debug_span!("backfill_write_hot_db").entered();
	self.store.hot_db.do_atomically(hot_batch)?;
	}
	{
	let _span = debug_span!("backfill_write_cold_db").entered();
	self.store.cold_db.do_atomically(cold_batch)?;
	}
	let mut anchor_and_blob_batch = Vec::with_capacity(3);
	// Update the blob info.
	if new_oldest_blob_slot != blob_info.oldest_blob_slot
	&& let Some(oldest_blob_slot) = new_oldest_blob_slot
	{
	let new_blob_info = BlobInfo {
	oldest_blob_slot: Some(oldest_blob_slot),
	..blob_info.clone()
	};
	anchor_and_blob_batch.push(
	self.store
	.compare_and_set_blob_info(blob_info, new_blob_info)?,
	);
	}
	// Update the data column info.
	if new_oldest_data_column_slot != data_column_info.oldest_data_column_slot
	&& let Some(oldest_data_column_slot) = new_oldest_data_column_slot
	{
	let new_data_column_info = DataColumnInfo {
	oldest_data_column_slot: Some(oldest_data_column_slot),
	};
	anchor_and_blob_batch.push(
	self.store
	.compare_and_set_data_column_info(data_column_info, new_data_column_info)?,
	);
	}
	// Update the anchor.
	let new_anchor = AnchorInfo {
	oldest_block_slot: prev_block_slot,
	oldest_block_parent: expected_block_root,
	..anchor_info
	};
	let backfill_complete = new_anchor.block_backfill_complete(self.genesis_backfill_slot);
	anchor_and_blob_batch.push(
	self.store
	.compare_and_set_anchor_info(anchor_info, new_anchor)?,
	);
	self.store.hot_db.do_atomically(anchor_and_blob_batch)?;

If the updated oldest_block_slot becomes durable while some corresponding cold BeaconBlockRoots entries do not, restart leaves permanent gaps below the anchor. Backfill will not repair them, since it only fills slots prior to oldest_block_slot.

Freezer migration already implements the correct cross-DB protocol: persist cold data, cold_db.sync(), then update the hot-DB pointer with put_sync:

lighthouse/beacon_node/store/src/hot_cold_store.rs

Lines 3662 to 3699 in b263df5

	// Warning: Critical section. We have to take care not to put any of the two databases in an
	// inconsistent state if the OS process dies at any point during the freezing
	// procedure.
	//
	// Since it is pretty much impossible to be atomic across more than one database, we trade
	// potentially re-doing the migration to copy data to the freezer, for consistency. If we crash
	// after writing all new block & state data to the freezer but before updating the split, then
	// in the worst case we will restart with the old split and re-run the migration.
	store.cold_db.do_atomically(cold_db_block_ops)?;
	store.cold_db.sync()?;
	let new_split = {
	let mut split_guard = store.split.write();
	let latest_split = *split_guard;
	// Detect a situation where the split point is (erroneously) changed from more than one
	// place in code.
	if latest_split.slot != current_split.slot {
	error!(
	previous_split_slot = %current_split.slot,
	current_split_slot = %latest_split.slot,
	"Race condition detected: Split point changed while copying states to the freezer"
	);
	// Assume the freezing procedure will be retried in case this happens.
	return Err(Error::SplitPointModified(
	current_split.slot,
	latest_split.slot,
	));
	}
	// Before updating the in-memory split value, we flush it to disk first, so that should the
	// OS process die at this point, we pick up from the right place after a restart.
	let new_split = Split {
	slot: finalized_state.slot(),
	state_root: finalized_state_root,
	block_root: finalized_block_root,
	};
	store.hot_db.put_sync(&SPLIT_KEY, &new_split)?;

Proposed approach:

1. Retain the existing blobs -> hot -> cold write order from Backfill blob storage fix #5119.
2. Call cold_db.sync() after the cold batch.
3. Persist the anchor (and related metadata) with a synced hot-DB write.

A couple of open questions:

• Do blobs/hot also need a sync each batch, or is cold -> anchor sufficient for this corruption mode?
• Should already-corrupt databases be repaired, or left as re-sync?

Happy to take this if that sounds right; could you assign me?