Fix invalid data column sidecars getting accepted

beacon_node/beacon_chain/src/data_column_verification.rs

@@ -127,6 +127,25 @@ pub enum GossipDataColumnError {
  slot: Slot,
  index: ColumnIndex,
  },
+ /// Data column index must be between 0 and `NUMBER_OF_COLUMNS` (exclusive).
+ ///
+ /// ## Peer scoring
+ ///
+ /// The column sidecar is invalid and the peer is faulty
+ InvalidColumnIndex(u64),
+ /// Data column not expected for a block with empty kzg commitments.
+ ///
+ /// ## Peer scoring
+ ///
+ /// The column sidecar is invalid and the peer is faulty
+ UnexpectedDataColumn,
+ /// The data column length must be equal to the number of commitments/proofs, otherwise the
+ /// sidecar is invalid.
+ ///
+ /// ## Peer scoring
+ ///
+ /// The column sidecar is invalid and the peer is faulty
+ InconsistentCommitmentsOrProofLength,
 }
 
 impl From<BeaconChainError> for GossipDataColumnError {
@@ -367,7 +386,7 @@ pub fn validate_data_column_sidecar_for_gossip<T: BeaconChainTypes>(
  chain: &BeaconChain<T>,
 ) -> Result<GossipVerifiedDataColumn<T>, GossipDataColumnError> {
  let column_slot = data_column.slot();
-
+ verify_data_column_sidecar(&data_column, &chain.spec)?;
  verify_index_matches_subnet(&data_column, subnet, &chain.spec)?;
  verify_sidecar_not_from_future_slot(chain, column_slot)?;
  verify_slot_greater_than_latest_finalized_slot(chain, column_slot)?;
@@ -396,6 +415,26 @@ pub fn validate_data_column_sidecar_for_gossip<T: BeaconChainTypes>(
  })
 }
 
+/// Verify if the data column sidecar is valid.
+fn verify_data_column_sidecar<E: EthSpec>(
+ data_column: &DataColumnSidecar<E>,
+ spec: &ChainSpec,
+) -> Result<(), GossipDataColumnError> {
+ if data_column.index >= spec.number_of_columns as u64 {
+ return Err(GossipDataColumnError::InvalidColumnIndex(data_column.index));
+ }
+ if data_column.kzg_commitments.is_empty() {
+ return Err(GossipDataColumnError::UnexpectedDataColumn);
+ }
+ if data_column.column.len() != data_column.kzg_commitments.len()
+ || data_column.column.len() != data_column.kzg_proofs.len()
+ {
+ return Err(GossipDataColumnError::InconsistentCommitmentsOrProofLength);
+ }
+
+ Ok(())
+}
+
 // Verify that this is the first column sidecar received for the tuple:
 // (block_header.slot, block_header.proposer_index, column_sidecar.index)
 fn verify_is_first_sidecar<T: BeaconChainTypes>(
@@ -613,3 +652,55 @@ fn verify_sidecar_not_from_future_slot<T: BeaconChainTypes>(
  }
  Ok(())
 }
+
+#[cfg(test)]
+mod test {
+ use crate::data_column_verification::{
+ validate_data_column_sidecar_for_gossip, GossipDataColumnError,
+ };
+ use crate::test_utils::BeaconChainHarness;
+ use types::{DataColumnSidecar, EthSpec, ForkName, MainnetEthSpec};
+
+ type E = MainnetEthSpec;
+
+ #[tokio::test]
+ async fn empty_data_column_sidecars_fails_validation() {
+ let spec = ForkName::latest().make_genesis_spec(E::default_spec());
+ let harness = BeaconChainHarness::builder(E::default())
+ .spec(spec.into())
+ .deterministic_keypairs(64)
+ .fresh_ephemeral_store()
+ .mock_execution_layer()
+ .build();
+ harness.advance_slot();
+
+ let slot = harness.get_current_slot();
+ let state = harness.get_current_state();
+ let ((block, _blobs_opt), _state) = harness
+ .make_block_with_modifier(state, slot, |block| {
+ *block.body_mut().blob_kzg_commitments_mut().unwrap() = vec![].into();
+ })
+ .await;
+
+ let index = 0;
+ let column_sidecar = DataColumnSidecar::<E> {
+ index,
+ column: vec![].into(),
+ kzg_commitments: vec![].into(),
+ kzg_proofs: vec![].into(),
+ signed_block_header: block.signed_block_header(),
+ kzg_commitments_inclusion_proof: block
+ .message()
+ .body()
+ .kzg_commitments_merkle_proof()
+ .unwrap(),
+ };
+
+ let result =
+ validate_data_column_sidecar_for_gossip(column_sidecar.into(), index, &harness.chain);
+ assert!(matches!(
+ result.err(),
+ Some(GossipDataColumnError::UnexpectedDataColumn)
+ ));
+ }
+}

beacon_node/network/src/network_beacon_processor/gossip_methods.rs

@@ -711,6 +711,9 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
  | GossipDataColumnError::InvalidSubnetId { .. }
  | GossipDataColumnError::InvalidInclusionProof { .. }
  | GossipDataColumnError::InvalidKzgProof { .. }
+ | GossipDataColumnError::UnexpectedDataColumn
+ | GossipDataColumnError::InvalidColumnIndex(_)
+ | GossipDataColumnError::InconsistentCommitmentsOrProofLength
  | GossipDataColumnError::NotFinalizedDescendant { .. } => {
  debug!(
  self.log,