Fix for #7377 Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier #7399

SunnysidedJ · 2025-05-05T15:20:10Z

Issue Addressed

Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier #7377

Proposed Changes

As described in ethereum/consensus-specs#4284

Additional Info

SunnysidedJ · 2025-05-05T15:21:12Z

Status: testing

SunnysidedJ · 2025-05-06T09:27:39Z

Status: review ready

jimmygchen

@SunnysidedJ thanks a lot for this PR!
I'm still reviewing but thought I'd post my comments I have so far. I'll continue review this afternoon.

jimmygchen · 2025-05-07T01:30:47Z

consensus/types/src/data_column_sidecar.rs

@@ -32,6 +34,51 @@ pub struct DataColumnIdentifier {
    pub index: ColumnIndex,
 }


DataColumnIdentifier can be removed

jimmygchen · 2025-05-07T01:37:05Z

beacon_node/network/src/sync/network_context/requests/data_columns_by_root.rs

+        let number_of_columns = spec.number_of_columns as usize;
+        // TODO we aren't handling the case where self.indices > NUMBER_OF_COLUMNS defined by the
+        // spec. Do we do this else where? I think we shall use RuntimeVariableList::new() and
+        // handle errors.


Yes agree that we should use RuntimeVariableList::new() instead, perhaps modify this function to try_into_request and return Result here

beacon_node/lighthouse_network/src/rpc/codec.rs

jimmygchen · 2025-05-07T03:03:36Z

consensus/types/src/data_column_sidecar.rs

+        )?
+        .into_iter()
+        .map(|bytes| {
+            // Split manually: first 32 bytes = block_root and next 4 bytes = vector tag


I think it would be a bit cleaner to use SszDecoderBuilder here to decode, and call into a separate DataColumnByRootIdentifier decode function

Something like this:

let mut builder = ssz::SszDecoderBuilder::new(&bytes); builder.register_type::<Hash256>()?; builder.register_anonymous_variable_length_item()?; let mut decoder = builder.build()?; let block_root = decoder.decode_next()?; let indices = decoder.decode_next_with(|bytes| DataColumnsByRootIdentifier::from_ssz_bytes(bytes, num_columns))?; Ok(DataColumnsByRootIdentifier { block_root, indices, })

jimmygchen

I've added some more comments, let me know what you think. Thanks!

jimmygchen · 2025-05-07T04:13:05Z

consensus/types/src/chain_spec.rs

@@ -1781,7 +1793,10 @@ fn default_max_blobs_by_root_request() -> usize {
 }

 fn default_data_columns_by_root_request() -> usize {
-    max_data_columns_by_root_request_common(default_max_request_data_column_sidecars())
+    max_data_columns_by_root_request_common(
+        default_max_request_data_column_sidecars(),


this should be default_max_request_blocks()?

jimmygchen · 2025-05-07T04:13:18Z

consensus/types/src/chain_spec.rs

@@ -2083,6 +2098,7 @@ impl Config {
            max_blobs_by_root_request: max_blobs_by_root_request_common(max_request_blob_sidecars),
            max_data_columns_by_root_request: max_data_columns_by_root_request_common(
                max_request_data_column_sidecars,


default_max_request_blocks()

jimmygchen · 2025-05-07T04:17:07Z

beacon_node/lighthouse_network/src/rpc/methods.rs

 }

 impl DataColumnsByRootRequest {
-    pub fn new(data_column_ids: Vec<DataColumnIdentifier>, spec: &ChainSpec) -> Self {
+    pub fn new(data_column_ids: Vec<DataColumnsByRootIdentifier>, spec: &ChainSpec) -> Self {
        let data_column_ids = RuntimeVariableList::from_vec(
            data_column_ids,
            spec.max_request_data_column_sidecars as usize,


This should be MAX_REQUEST_BLOCKS_DENEB

jimmygchen · 2025-05-07T04:21:36Z

beacon_node/network/src/network_beacon_processor/rpc_methods.rs

+        for data_column_ids_by_root in request.data_column_ids.as_slice() {
+            match self.chain.get_data_columns_checking_all_caches(
+                data_column_ids_by_root.block_root,
+                &Vec::from(data_column_ids_by_root.indices.clone()),


I think we can do data_column_ids_by_root.indices.as_slice() here to avoid cloning?

jimmygchen · 2025-05-07T04:25:27Z

beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs

@@ -405,16 +405,18 @@ impl<T: BeaconChainTypes> DataAvailabilityCheckerInner<T> {
    }

    /// Fetch a data column from the cache without affecting the LRU ordering


Suggested change

/// Fetch a data column from the cache without affecting the LRU ordering

/// Fetch data columns of a given `block_root` from the cache without affecting the LRU ordering

jimmygchen · 2025-05-07T04:28:33Z

beacon_node/beacon_chain/src/beacon_chain.rs

+        } else if let Some(all_cols) = self.early_attester_cache.get_data_columns(block_root) {
+            Ok(Some(all_cols))
+        } else {
+            self.get_data_columns(&block_root)


We probably want to avoid loading all data columns from disk and just load the required one here.

jimmygchen · 2025-05-07T04:57:45Z

beacon_node/beacon_chain/src/beacon_chain.rs

+            Ok(Some(all_cols))
+        } else {
+            self.get_data_columns(&block_root)
+        };


Doesn't look like we need to wrap it with Result here and unwrap it below.

We could simplify this to something like

let columns = if let Some(all_cols) = self .data_availability_checker .get_data_columns(block_root)? { Some(all_cols) } else if let Some(all_cols) = self.early_attester_cache.get_data_columns(block_root) { Some(all_cols) } else { self.get_data_columns(&block_root)? };

and then return Ok(columns) after filtering.

jimmygchen · 2025-05-07T04:58:26Z

beacon_node/beacon_chain/src/beacon_chain.rs

+                    .iter()
+                    .filter(|col| indices.contains(&col.index))
+                    .cloned()
+                    .collect(),


you could do into_iter to avoid having to clone the elements

initial commit, under test

5afeb1f

SunnysidedJ changed the title ~~initial commit, under test~~ Fix for #7377 Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier May 5, 2025

jimmygchen added das Data Availability Sampling ready-for-review The code is ready for review labels May 5, 2025

jimmygchen assigned pawanjay176 May 5, 2025

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 May 6, 2025

test complete

1302909

SunnysidedJ marked this pull request as ready for review May 6, 2025 09:27

SunnysidedJ requested a review from jxs as a code owner May 6, 2025 09:27

jimmygchen 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 May 7, 2025

jimmygchen assigned jimmygchen and unassigned pawanjay176 May 7, 2025

jimmygchen requested changes May 7, 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 May 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for #7377 Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier #7399

Fix for #7377 Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier #7399

SunnysidedJ commented May 5, 2025

SunnysidedJ commented May 5, 2025

SunnysidedJ commented May 6, 2025

jimmygchen left a comment

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen left a comment

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

jimmygchen May 7, 2025

		@@ -32,6 +34,51 @@ pub struct DataColumnIdentifier {
		pub index: ColumnIndex,
		}

		@@ -405,16 +405,18 @@ impl<T: BeaconChainTypes> DataAvailabilityCheckerInner<T> {
		}

		/// Fetch a data column from the cache without affecting the LRU ordering

	/// Fetch a data column from the cache without affecting the LRU ordering
	/// Fetch data columns of a given `block_root` from the cache without affecting the LRU ordering

Fix for #7377 Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier #7399

Are you sure you want to change the base?

Fix for #7377 Update DataColumnSidecarsByRoot request to use DataColumnsByRootIdentifier #7399

Conversation

SunnysidedJ commented May 5, 2025

Issue Addressed

Proposed Changes

Additional Info

SunnysidedJ commented May 5, 2025

SunnysidedJ commented May 6, 2025

jimmygchen 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

jimmygchen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment