Block availability data enum #6866

dapplion · 2025-01-25T14:36:57Z

Issue Addressed

PeerDAS has undergone multiple refactors + the blending with the get_blobs optimization has generated technical debt.

A function signature like this

lighthouse/beacon_node/beacon_chain/src/beacon_chain.rs

Lines 7171 to 7178 in f008b84

    
           fn get_blobs_or_columns_store_op( 
        
               &self, 
        
               block_root: Hash256, 
        
               block_epoch: Epoch, 
        
               blobs: Option<BlobSidecarList<T::EthSpec>>, 
        
               data_columns: Option<DataColumnSidecarList<T::EthSpec>>, 
        
               data_column_recv: Option<oneshot::Receiver<DataColumnSidecarList<T::EthSpec>>>, 
        
           ) -> Result<Option<StoreOp<T::EthSpec>>, String> {

Allows at least the following combination of states:

blobs: Some / None
data_columns: Some / None
data_column_recv: Some / None
Block has data? Yes / No
Block post-PeerDAS? Yes / No

In reality, we don't have that many possible states, only:

NoData: pre-deneb, pre-PeerDAS with 0 blobs or post-PeerDAS with 0 blobs
Blobs(BlobSidecarList<E>): post-Deneb pre-PeerDAS with > 0 blobs
DataColumns(DataColumnSidecarList<E>): post-PeerDAS with > 0 blobs
DataColumnsRecv(oneshot::Receiver<DataColumnSidecarList<E>>): post-PeerDAS with > 0 blobs, but we obtained the columns via reconstruction

^ this are the variants of the new AvailableBlockData enum

So we go from 2^5 states to 4 well-defined. Downstream code benefits nicely from this clarity and I think it makes the whole feature much more maintainable.

Currently is_available returns a bool, and then we construct the available block in make_available. In a way the availability condition is duplicated in both functions. Instead, this PR constructs AvailableBlockData in is_available so the availability conditions are written once

if let Some(block_data) = is_available(..) {
    let available_block = make_available(block_data);
}

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

jimmygchen · 2025-01-30T11:10:20Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

-        let num_received_blobs = self.num_received_blobs();
-        let num_received_columns = self.num_received_data_columns();
+        if spec.is_peer_das_enabled_for_epoch(block.epoch()) {
+            if self.verified_data_columns.len() >= custody_column_count {


We've discussed about this before - we don't allow for duplicates when adding columns to the available cache, so if len() is greater than custody column count, we likely have added non-custody column to the cache, and this would result in:

inconsistent columns in DB

incorrectly identifying a block as available where it isn't (getting some non custody columns, but custody columns may be missing)

With the current approach, we must be able to detect if there's a bug here.

I have added a TODO in the line below to fix this case later. We were not handling this case before and it's out of scope for this PR IMO.

As mentioned in the other comment, our code on unstable right now requires an exact match for either blobs or columns, why change this?

jimmygchen · 2025-01-30T11:15:44Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

-            "received_columns" => num_received_columns,
-            "expected_columns" => expected_columns_msg,
-        );
+                // TODO(das) get only the columns that we need, filter the rest


Is this filter necessary with the current approach?
Is this TODO to account for peer sampling?
Feels like it's inconsistent in the context of subnet sampling though - because we could have enough verified_data_columns but end up storing less than custody_column_count?

I'm not sure what to do in this case. Erroring here is bad as it will prevent block import. This should not happen and points to an internal error. I would rather think how to handle the > cases in a separate PR.

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

jimmygchen · 2025-01-30T11:20:36Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

+        } else {
+            // Before PeerDAS, blobs
+            let num_received_blobs = self.verified_blobs.iter().flatten().count();
+            if num_received_blobs >= num_expected_blobs {


it's definitely a bug if > happens, I'm not sure if we just make it available and store the the "uncommitted blobs"

I think the previous handling is good - and if there's unexpected blob there should be an error AvailabilityCheckError::Unexpected

Before we did not handle the > case, we had a .take() on the blobs iter, we would just ignore the extra blobs

The take() is unnecessary I believe, because we check for duplicates AND exact match before this PR

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

Lines 240 to 244 in a1b7d61

let all_blobs_received = block_kzg_commitments_count_opt

.is_some_and(|num_expected_blobs| num_expected_blobs == num_received_blobs);

let all_columns_received = expected_columns_opt

.is_some_and(|num_expected_columns| num_expected_columns == num_received_columns);

beacon_node/beacon_chain/src/data_availability_checker.rs

jimmygchen · 2025-01-30T12:03:53Z

beacon_node/beacon_chain/src/beacon_chain.rs

+                    "block_root" => %block_root,
+                    "count" => blobs.len(),
+                );
+                Ok(Some(StoreOp::PutBlobs(block_root, blobs)))


Should we keep the is_empty check for both blobs and columns so we dont't store empty values?

AvailableBlockData::Blobs enum by definition must have more than zero blobs. If we do a check here we should error, instead of ignoring the Op

beacon_node/beacon_chain/src/data_availability_checker.rs

beacon_node/beacon_chain/src/early_attester_cache.rs

jimmygchen

Nice, very elegant solution, and much cleaner.
I've added some comments, the main questions I have are the DA checker is_available checks and edge case handling.

mergify · 2025-02-03T14:10:19Z

This pull request has merge conflicts. Could you please resolve them @dapplion? 🙏

…data-enum

jimmygchen · 2025-02-05T06:40:05Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

-            .unwrap_or("unknown".to_string());
+    /// Returns Some if the block has received all its required data for import. The return value
+    /// must be persisted in the DB along with the block.
+    pub fn is_available(


nit on convention: the name is_available implies an immutable method that does not mutate self and return a bool - although I can't think of a better name.

dapplion added 4 commits January 25, 2025 17:26

Use enum for clear block availability states

59ff256

Re-add Component added to data availability checker log

dd03585

Complete is_available

a171ab7

Fix TODOs

190622d

dapplion added the das Data Availability Sampling label Jan 25, 2025

dapplion requested review from jimmygchen and michaelsproul January 25, 2025 14:36

dapplion added the ready-for-review The code is ready for review label Jan 25, 2025

dapplion added 2 commits January 26, 2025 07:30

Complete early attester cache impl

637d615

Fix store tests

b060bab

jimmygchen requested changes Jan 30, 2025

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Jan 30, 2025

jimmygchen requested changes Jan 30, 2025

View reviewed changes

dapplion requested a review from jimmygchen January 31, 2025 23:06

dapplion added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Jan 31, 2025

Review PR

5358dbb

dapplion force-pushed the block-availability-data-enum branch from 22692ee to 5358dbb Compare February 1, 2025 04:09

dapplion mentioned this pull request Feb 1, 2025

Generalize block available time metric for PeerDAS #6850

Open

Merge remote-tracking branch 'sigp/unstable' into block-availability-…

a59dafe

…data-enum

jimmygchen reviewed Feb 5, 2025

View reviewed changes

jimmygchen added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block availability data enum #6866

Block availability data enum #6866

dapplion commented Jan 25, 2025

jimmygchen Jan 30, 2025

dapplion Jan 31, 2025

jimmygchen Feb 6, 2025

jimmygchen Jan 30, 2025

dapplion Jan 31, 2025

jimmygchen Jan 30, 2025

jimmygchen Jan 30, 2025

dapplion Jan 31, 2025

jimmygchen Feb 6, 2025 •

edited

Loading

jimmygchen Jan 30, 2025

dapplion Jan 31, 2025

jimmygchen left a comment

mergify bot commented Feb 3, 2025

jimmygchen Feb 5, 2025

	fn get_blobs_or_columns_store_op(
	&self,
	block_root: Hash256,
	block_epoch: Epoch,
	blobs: Option<BlobSidecarList<T::EthSpec>>,
	data_columns: Option<DataColumnSidecarList<T::EthSpec>>,
	data_column_recv: Option<oneshot::Receiver<DataColumnSidecarList<T::EthSpec>>>,
	) -> Result<Option<StoreOp<T::EthSpec>>, String> {

	let all_blobs_received = block_kzg_commitments_count_opt
	.is_some_and(\|num_expected_blobs\| num_expected_blobs == num_received_blobs);

	let all_columns_received = expected_columns_opt
	.is_some_and(\|num_expected_columns\| num_expected_columns == num_received_columns);

Block availability data enum #6866

Are you sure you want to change the base?

Block availability data enum #6866

Conversation

dapplion commented Jan 25, 2025

Issue Addressed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmygchen Feb 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmygchen left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 3, 2025

Choose a reason for hiding this comment

jimmygchen Feb 6, 2025 •

edited

Loading