Only allow one data column computation at a time for a given block

[jimmygchen]

Description

See this comment for context.

Currently we allow for reconstruction and multiple column computation for the same block to happen at the same time, this is wasteful and we can avoid this.

In PendingComponents, we track reconstruction with a bool reconstruction_started and we track blob to column computation with a data_column_recv. We should be able to replace the reconstruction_started usage with data_column_recv as well, and check if data_column_recv is Some before triggering either 1/ blob to column computation or 2/ column reconstruction.

lighthouse/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

Lines 23 to 39 in 4173135

	/// This represents the components of a partially available block
	///
	/// The blobs are all gossip and kzg verified.
	/// The block has completed all verifications except the availability check.
	pub struct PendingComponents<E: EthSpec> {
	pub block_root: Hash256,
	pub verified_blobs: FixedVector<Option<KzgVerifiedBlob<E>>, E::MaxBlobsPerBlock>,
	pub verified_data_columns: Vec<KzgVerifiedCustodyDataColumn<E>>,
	pub executed_block: Option<DietAvailabilityPendingExecutedBlock<E>>,
	pub reconstruction_started: bool,
	/// Receiver for data columns that are computed asynchronously;
	///
	/// If `data_column_recv` is `Some`, it means data column computation or reconstruction has been
	/// started. This can happen either via engine blobs fetching or data column reconstruction
	/// (triggered when >= 50% columns are received via gossip).
	pub data_column_recv: Option<oneshot::Receiver<DataColumnSidecarList<E>>>,
	}

[SunnysidedJ]

Hi, let me try working on this issue!

[jimmygchen]

Hi @SunnysidedJ

Thanks for helping out! - FYI with the recent proposals to offload proof computation from the beacon node, it's likely that we will get rid of the data_column_recv in order to reduce complexity, as the computation is no longer a long running task - see this comment here: #7117 (comment)

I think there will still be some race condition, but this may not be too big of an issue - I'll have to think about it a bit more. The change in #7117 will likely affect this, however we can't merge it yet because the spec changes for cell proof computation hasn't been finalised. It might be best to work on another issue for now.

One upcoming change that's kinda related is an optimisation to trigger getBlobs when the node receive a data_column_sidecar from gossip. Currently we only do this when we receive a block:

lighthouse/beacon_node/network/src/network_beacon_processor/gossip_methods.rs

Lines 1555 to 1562 in cbf1c04

	self.executor.spawn(
	async move {
	self_clone
	.fetch_engine_blobs_and_publish(block_clone, block_root, publish_blobs)
	.await
	},
	"fetch_blobs_gossip",
	);

However we could also trigger getBlobs as soon as we receive a valid data column - and if we do this, we'll have to handle concurrency as well (in case multiple columns triggered it simultaneously)

See the spec change here:
https://github.com/ethereum/consensus-specs/pull/4183/files#diff-81ad0f2992fd2a384c30b338ff6760b5b538a2f056b829db2d036ce42ba3090eR210

I haven't created an issue for this yet, but can do a writeup tomorrow if you're interested.

[SunnysidedJ]

Hi @SunnysidedJ

Thanks for helping out! - FYI with the recent proposals to offload proof computation from the beacon node, it's likely that we will get rid of the data_column_recv in order to reduce complexity, as the computation is no longer a long running task - see this comment here: #7117 (comment)

I think there will still be some race condition, but this may not be too big of an issue - I'll have to think about it a bit more. The change in #7117 will likely affect this, however we can't merge it yet because the spec changes for cell proof computation hasn't been finalised. It might be best to work on another issue for now.

One upcoming change that's kinda related is an optimisation to trigger getBlobs when the node receive a data_column_sidecar from gossip. Currently we only do this when we receive a block:

lighthouse/beacon_node/network/src/network_beacon_processor/gossip_methods.rs

Lines 1555 to 1562 in cbf1c04

self.executor.spawn(
async move {
self_clone
.fetch_engine_blobs_and_publish(block_clone, block_root, publish_blobs)
.await
},
"fetch_blobs_gossip",
);
However we could also trigger getBlobs as soon as we receive a valid data column - and if we do this, we'll have to handle concurrency as well (in case multiple columns triggered it simultaneously)

See the spec change here: https://github.com/ethereum/consensus-specs/pull/4183/files#diff-81ad0f2992fd2a384c30b338ff6760b5b538a2f056b829db2d036ce42ba3090eR210

I haven't created an issue for this yet, but can do a writeup tomorrow if you're interested.

Hi @jimmygchen, sure. I'd like to have a go, and it'd be appreciated if you could do a writeup at your earliest convenience.

[jimmygchen]

@SunnysidedJ #7212 let me know if you have any questions!

[jimmygchen]

The implementation has changed a lot since this was originally written and there main changes affecting this are:

• engine_getBlobs now returns proofs and compute_cells is very cheap and this code path is no longer a problem
• reconstruction are now queued and the queue size is 1, so it's very unlikely that we perform reconstruction multiple times for a single block.

Closing this now as it's no longer relevant.