Partial columns cleanup (#9321)

#8314 left a few ugly potentially panicking location behind - all of them believed to be unreachable, but this PR fixes them regardless for good hygiene.


  Update to `ethereum_ssz 0.10.4` for two new helpers: `not_inplace` and `clone_zeroed`.

Remove remaining `expect` and `todo!` in favour of these helpers and one new fallible (but practically infallible) method.


Co-Authored-By: Daniel Knopik <daniel@dknopik.de>

Cargo.lock

@@ -3282,9 +3282,9 @@ dependencies = [
 
 [[package]]
 name = "ethereum_ssz"
-version = "0.10.3"
+version = "0.10.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "368a4a4e4273b0135111fe9464e35465067766a8f664615b5a86338b73864407"
+checksum = "e462875ad8693755ea8913d6e905715c76ea4836e2254e18c9cf0f7a8f8c2a13"
 dependencies = [
  "alloy-primitives",
  "arbitrary",

beacon_node/beacon_chain/src/data_column_verification.rs

@@ -1220,7 +1220,16 @@ pub fn validate_partial_data_column_sidecar_for_gossip<T: BeaconChainTypes>(
                 header,
             };
         }
-        Err(MissingCellsError::UnexpectedError(e)) => todo!("handle unexpected error {:?}", e),
+        Err(MissingCellsError::UnexpectedError(e)) => {
+            return PartialColumnVerificationResult::ErrWithValidHeader {
+                err: GossipDataColumnError::InternalError(format!(
+                    "An unexpected error occurred while validating partial data columns: {:?}",
+                    e
+                ))
+                .into(),
+                header,
+            };
+        }
     };
 
     // We do not have to check block related data here, as we create the verifiable column from

beacon_node/http_api/src/publish_blocks.rs

@@ -524,11 +524,15 @@ pub(crate) fn publish_column_sidecars<T: BeaconChainTypes>(
         if chain.config.enable_partial_columns
             && let DataColumnSidecar::Fulu(fulu_data_col) = data_col.as_ref()
         {
-            let mut partial = fulu_data_col.to_partial();
-            if let Some(header) = partial.sidecar.header.take() {
-                partial_header = Some(header);
+            match fulu_data_col.to_partial() {
+                Ok(mut partial) => {
+                    if let Some(header) = partial.sidecar.header.take() {
+                        partial_header = Some(header);
+                    }
+                    partial_columns.push(Arc::new(partial));
+                }
+                Err(err) => crit!(?err, "Could not convert from full to partial"),
             }
-            partial_columns.push(Arc::new(partial));
         }
 
         let subnet = DataColumnSubnetId::from_column_index(*data_col.index(), &chain.spec);

beacon_node/lighthouse_network/src/types/partial.rs

@@ -9,7 +9,7 @@ use std::sync::Arc;
 use tracing::{debug, error};
 use types::core::{EthSpec, Hash256};
 use types::data::{
-    CellBitmap, PartialDataColumn, PartialDataColumnHeader, PartialDataColumnPartsMetadata,
+    PartialDataColumn, PartialDataColumnHeader, PartialDataColumnPartsMetadata,
     PartialDataColumnSidecar, PartialDataColumnSidecarRef,
 };
 
@@ -32,12 +32,8 @@ impl<E: EthSpec> OutgoingPartialColumn<E> {
         header_sent_set: HeaderSentSet,
     ) -> Self {
         // For now, always request all cells
-        let mut requests = partial_column.sidecar.cells_present_bitmap.clone();
-        for idx in 0..requests.len() {
-            requests
-                .set(idx, true)
-                .expect("Bound asserted via `len` above");
-        }
+        let mut requests = partial_column.sidecar.cells_present_bitmap.clone_zeroed();
+        requests.not_inplace();
         let metadata = PartialDataColumnPartsMetadata::<E> {
             available: partial_column.sidecar.cells_present_bitmap.clone(),
             requests,
@@ -45,10 +41,7 @@ impl<E: EthSpec> OutgoingPartialColumn<E> {
         .into();
 
         let header_message = PartialDataColumnSidecarRef {
-            cells_present_bitmap: CellBitmap::<E>::with_capacity(
-                partial_column.sidecar.cells_present_bitmap.len(),
-            )
-            .expect("Taking length from bitmap with same bound"),
+            cells_present_bitmap: partial_column.sidecar.cells_present_bitmap.clone_zeroed(),
             column: vec![],
             kzg_proofs: vec![],
             header: Some(header).into(),
@@ -210,7 +203,7 @@ impl<E: EthSpec> Partial for OutgoingPartialColumn<E> {
                 let send = self
                     .partial_column
                     .sidecar
-                    .filter(|idx| want.get(idx).expect("Bound checked above"))
+                    .filter(|idx| want.get(idx).unwrap_or(false))
                     .map_err(|err| {
                         error!(?err, "Unexpected error filtering sidecar");
                         PartialError::InvalidFormat
@@ -262,6 +255,7 @@ mod tests {
     use fixed_bytes::FixedBytesExtended;
     use libp2p::identity::Keypair;
     use ssz_types::FixedVector;
+    use types::CellBitmap;
     use types::block::{BeaconBlockHeader, SignedBeaconBlockHeader};
     use types::core::{MinimalEthSpec, Slot};
     use types::data::PartialDataColumnHeader;

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -1381,16 +1381,20 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
                 &[&data_column_index.to_string()],
             );
 
-            let mut column = col.to_partial();
-            let header = column.sidecar.header.take();
-            if let Some(header) = header {
-                self.send_network_message(NetworkMessage::PublishPartialColumns {
-                    columns: vec![Arc::new(column)],
-                    header: Arc::new(header),
-                });
-            } else {
-                crit!("Converting from full to partial yielded headerless partial")
-            };
+            match col.to_partial() {
+                Ok(mut column) => {
+                    let header = column.sidecar.header.take();
+                    if let Some(header) = header {
+                        self.send_network_message(NetworkMessage::PublishPartialColumns {
+                            columns: vec![Arc::new(column)],
+                            header: Arc::new(header),
+                        });
+                    } else {
+                        crit!("Converting from full to partial yielded headerless partial")
+                    };
+                }
+                Err(err) => crit!(?err, "Could not convert from full to partial"),
+            }
         }
 
         let result = self

consensus/types/src/data/data_column_sidecar.rs

@@ -250,19 +250,16 @@ impl<E: EthSpec> DataColumnSidecarFulu<E> {
     }
 
     /// Convert this full data column into a verifiable partial data column.
-    pub fn to_partial(&self) -> PartialDataColumn<E> {
+    /// Note: This is not expected to ever fail.
+    pub fn to_partial(&self) -> Result<PartialDataColumn<E>, PartialDataColumnSidecarError> {
         let cell_count = self.column.len();
-        let mut bitmap =
-            CellBitmap::<E>::with_capacity(cell_count).expect("our column has the same bound");
-        for idx in 0..cell_count {
-            bitmap
-                .set(idx, true)
-                .expect("The correct size is initialized right above");
-        }
+        let mut bitmap = CellBitmap::<E>::with_capacity(cell_count)
+            .map_err(|_| PartialDataColumnSidecarError::UnexpectedBounds)?;
+        bitmap.not_inplace();
 
         let block_root = self.block_root();
 
-        PartialDataColumn {
+        Ok(PartialDataColumn {
             block_root,
             index: self.index,
             sidecar: PartialDataColumnSidecar {
@@ -276,7 +273,7 @@ impl<E: EthSpec> DataColumnSidecarFulu<E> {
                 })
                 .into(),
             },
-        }
+        })
     }
 }