Remove CGC from data_availability checker #7033

dapplion · 2025-02-24T23:40:03Z

Issue Addressed

Part of Implement basic validator data column custody #6767

Validator custody makes the CGC and set of sampling columns dynamic. Right now this information is stored twice:

in the data availability checker
in the network globals

If that state becomes dynamic we must make sure it is in sync updating it twice, or guarding it behind a mutex. However, I noted that we don't really have to keep the CGC inside the data availability checker. All consumers can actually read it from the network globals, and we can update make_available to read the expected count of data columns from the block.

dapplion · 2025-02-24T23:42:34Z

beacon_node/beacon_chain/src/block_verification_types.rs

@@ -104,6 +109,8 @@ impl<E: EthSpec> RpcBlock<E> {
        Self {
            block_root,
            block: RpcBlockInner::Block(block),
+            // Block has zero columns
+            custody_columns_count: 0,


An RPC blocks without columns will never read this value

dapplion · 2025-02-24T23:43:48Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

        if received_column_count >= total_column_count {
            return ReconstructColumnsDecision::No("all columns received");
        }
+        // Only supernodes receive >= 50% of columns


Removed the "not required for full node" condition as it's redundant and would force us to pipe the custody group here somehow

I had a quick look and didn't see any existing check to ensure this wasn't being called on a non-full node. Why do you say it's redundant?

Could you not do this check with:

if let Some(custody_columns_count) = pending_components .executed_block .as_ref() .map(|diet_block| diet_block.custody_columns_count()) { if custody_columns_count != total_column_count { return ReconstructColumnsDecision::No("not required for full node"); } }

What you suggest will cause weird behavior:

if block is there: reconstruction happens only if custody_columns_count = 100%

if block is not there: reconstruction happens only if custody_columns_count = 50%

Block being there or not before attempting to reconstruction depends on network conditions and timings.

dapplion · 2025-02-24T23:44:47Z

beacon_node/beacon_chain/src/historical_blocks.rs

        let mut expected_block_root = anchor_info.oldest_block_parent;
        let mut prev_block_slot = anchor_info.oldest_block_slot;
        let mut new_oldest_blob_slot = blob_info.oldest_blob_slot;
        let mut new_oldest_data_column_slot = data_column_info.oldest_data_column_slot;

-        let mut blob_batch = Vec::<KeyValueStoreOp>::with_capacity(blob_batch_size);
+        let mut blob_batch = Vec::<KeyValueStoreOp>::new();


Using with_capacity here feels like a micro-optimization unless I'm missing something. Dropped it as we don't have access to the column count here anymore

dapplion · 2025-02-24T23:45:05Z

beacon_node/beacon_chain/src/test_utils.rs

@@ -815,6 +815,11 @@ where
        (0..self.validator_keypairs.len()).collect()
    }

+    pub fn get_sampling_column_count(&self) -> usize {
+        // Default column custody count
+        8


This should have no effect on tests I believe

dapplion · 2025-02-24T23:45:52Z

beacon_node/network/src/sync/tests/range.rs

+                block,
+                columns.clone(),
+                // TODO
+                columns.len(),


Assumes the CGC value is equal to all the columns received, which makes sense. We can change it later if we want to do more complex tests

ethDreamer · 2025-03-08T22:33:31Z

beacon_node/beacon_chain/src/block_verification_types.rs

@@ -145,13 +152,16 @@ impl<E: EthSpec> RpcBlock<E> {
        Ok(Self {
            block_root,
            block: inner,
+            // Block is before PeerDAS
+            custody_columns_count: 0,
        })
    }



might be worth having consistent terminology for:

Blocks

Post-Deneb Blocks (blocks with blobs)

Post-PeerDAS Blocks (blocks with custody columns)

Not sure what terminology that would be, but if we don't already have one / can't think of one, we should at least rename the above new function to something like new_with_blobs or something.

I want to get rid of RpcBlock completely, so this assignment here will become irrelevant. Instead have a chain segment as an enum as you suggest

ethDreamer

some tests still need to be fixed

ethDreamer · 2025-03-08T22:37:32Z

beacon_node/network/src/sync/block_sidecar_coupling.rs

+                    Some(block_root),
+                    block,
+                    custody_columns,
+                    expects_custody_columns.len(),


The expects_custody_columns field on RangeBlockComponentsRequest could use a documentation comment

ethDreamer · 2025-03-10T19:00:42Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

        if received_column_count >= total_column_count {
            return ReconstructColumnsDecision::No("all columns received");
        }
+        // Only supernodes receive >= 50% of columns


I had a quick look and didn't see any existing check to ensure this wasn't being called on a non-full node. Why do you say it's redundant?

Could you not do this check with:

if let Some(custody_columns_count) = pending_components .executed_block .as_ref() .map(|diet_block| diet_block.custody_columns_count()) { if custody_columns_count != total_column_count { return ReconstructColumnsDecision::No("not required for full node"); } }

ethDreamer · 2025-03-10T19:12:44Z

beacon_node/lighthouse_network/src/types/globals.rs

@@ -33,6 +33,8 @@ pub struct NetworkGlobals<E: EthSpec> {
    /// The computed sampling subnets and columns is stored to avoid re-computing.
    pub sampling_subnets: HashSet<DataColumnSubnetId>,
    pub sampling_columns: HashSet<ColumnIndex>,
+    /// Constant custody group count (CGC) set at startup
+    custody_group_count: u64,


You think it might be worth using one naming convention (like custody_group_count) everywhere instead of also using custody_columns_count?

Also this isn't behind any RwLock or anything but it was my understanding that this could change as the node synced more columns / added more validators? Or is that just not implemented yet?

not implemented yet

dapplion added the das Data Availability Sampling label Feb 24, 2025

dapplion requested a review from jxs as a code owner February 24, 2025 23:40

dapplion added the ready-for-review The code is ready for review label Feb 24, 2025

Remove CGC from data_availability checker

629d054

dapplion force-pushed the dedup-cgc-block branch from 32018b4 to 629d054 Compare February 24, 2025 23:40

dapplion commented Feb 24, 2025

View reviewed changes

Update tests

b3edc66

dapplion requested review from jimmygchen and ethDreamer February 25, 2025 20:21

dapplion mentioned this pull request Feb 25, 2025

Implement basic validator data column custody #6767

Open

6 tasks

ethDreamer reviewed Mar 8, 2025

View reviewed changes

ethDreamer requested changes Mar 10, 2025

View reviewed changes

ethDreamer 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 Mar 11, 2025

Add comment

320a406

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 Mar 12, 2025

dapplion added 2 commits March 12, 2025 02:46

Merge remote-tracking branch 'sigp/unstable' into dedup-cgc-block

061c5a0

Update tests

e0a7357

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove CGC from data_availability checker #7033

Remove CGC from data_availability checker #7033

dapplion commented Feb 24, 2025

dapplion Feb 24, 2025

dapplion Feb 24, 2025

ethDreamer Mar 10, 2025

dapplion Mar 12, 2025

dapplion Feb 24, 2025

dapplion Feb 24, 2025

dapplion Feb 24, 2025

ethDreamer Mar 8, 2025

dapplion Mar 9, 2025 •

edited

Loading

ethDreamer left a comment

ethDreamer Mar 8, 2025

dapplion Mar 12, 2025

ethDreamer Mar 10, 2025

ethDreamer Mar 10, 2025

ethDreamer Mar 10, 2025

dapplion Mar 12, 2025

Remove CGC from data_availability checker #7033

Are you sure you want to change the base?

Remove CGC from data_availability checker #7033

Conversation

dapplion commented Feb 24, 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

dapplion Mar 9, 2025 • edited Loading

Choose a reason for hiding this comment

ethDreamer left a 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

dapplion Mar 9, 2025 •

edited

Loading