Fix data columns not persisting for PeerDAS due to a getBlobs race condition

[jimmygchen]

Issue Addressed

This PR fixes a race condition with getBlobs in PeerDAS, causing data columns not to be persisted on block import.

This bug was discovered on a local devnet. The sequence of events below:

1. A new block is received on gossip. Fetch engine blobs is triggerd before block verification completes (optimisation from Fetch blobs from EL prior to block verification #6600).
2. Blobs is received from the EL, the node starts computing data columns, and passes a receiver to process_availability.
3. If this happens before block is verified, the receiver gets dropped.
4. After the block is verified, we import the block, however at this point we've lost the receiver, and data column is not persisted. We're hitting this code branch:
   

   lighthouse/beacon_node/beacon_chain/src/beacon_chain.rs

   Lines 3929 to 3931 in fa6c4c0

   	// No data columns present and compute data columns task was not spawned.
   	// Could either be no blobs in the block or before PeerDAS activation.
   	(None, None) => None,

getBlobsSidecars endpoint shows that both supernode and fullnode are missing columns

# fullnode
$ curl http://127.0.0.1:64385/eth/v1/beacon/blob_sidecars/148
{"code":500,"message":"INTERNAL_SERVER_ERROR: Insufficient data columns to reconstruct blobs: required 64, but only 0 were found.","stacktraces":[]}%

# fullnode
$ curl http://127.0.0.1:64505/eth/v1/beacon/blob_sidecars/148
{"code":500,"message":"INTERNAL_SERVER_ERROR: Insufficient data columns to reconstruct blobs: required 64, but only 8 were found.","stacktraces":[]}%

# supernode
$ curl http://127.0.0.1:64341/eth/v1/beacon/blob_sidecars/148
{"code":500,"message":"INTERNAL_SERVER_ERROR: Insufficient data columns to reconstruct blobs: required 64, but only 0 were found.","stacktraces":[]}%

Proposed Changes

After some discussion with @michaelsproul , we think the optimisation #6600 is useful to keep, as it enables data columns to be computed and propagated to the network sooner. The solution proposed is to store the data column Receiver in the DataAvailaiblityChecker (new field in BlockImportData), so it's always available on block import.

TODO

• Add some tests to cover the race condition if possible.

[michaelsproul]

These clones where we delete the receiver are OK because we should be using the receiver the first time it is returned inside an available block, right?

[jimmygchen]

Yes exactly - I added some docs to the function but I still think don't think it's immediately obvious:

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

Lines 42 to 44 in aabb5c1

	/// Clones the `PendingComponent` without cloning `data_column_recv`, as `Receiver` is not cloneable.
	/// This should only be used when the receiver is no longer needed.
	pub fn clone_without_column_recv(&self) -> Self {

Added some more comments above this line in 4173135:

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

Lines 552 to 554 in 4173135

	// We keep the pending components in the availability cache during block import (#5845).
	// `data_column_recv` is returned as part of the available block and is no longer needed here.
	write_lock.put(block_root, pending_components.clone_without_column_recv());

[dapplion]

If data_column_recv is already Some means we have double reconstruction happening. Should we mind that case or do something?

[jimmygchen]

Yes, I chatted with Michael about this - so currently it's possible any of these three happens at the same time

• gossip block triggering EL getBlobs and column computation
• rpc block triggering EL getBlobs and column computation
• gossip/rpc column triggering column reconstruction (only one computation at a time)

I initially wanted to address these altogether, but it got a bit messy and I decided to leave this issue to a separate PR and to keep this PR small , as I wanted to get this PR merged first and start interop testing.

[jimmygchen]

Created issue #6764

[dapplion]

Looks good to me, just a minor comment. Thanks for the docs throughout

[dapplion]

LGTM

[jimmygchen]

@mergify queue

[mergify]

queue

✅ The pull request has been merged automatically

Details

The pull request has been merged automatically at dd7591f