From b55b58b3c6d71eab13805ed736c5579d7b68d9da Mon Sep 17 00:00:00 2001
From: Pawan Dhananjay <pawandhananjay@gmail.com>
Date: Wed, 24 Jan 2024 04:05:24 +0530
Subject: [PATCH] Fix indices filter in blobs_sidecar http endpoint (#5118)

* add get blobs unit test

* Use a multi_key_query for blob_sidecar indices

* Fix test

* Remove env_logger

---------

Co-authored-by: realbigsean <seananderson33@GMAIL.com>
---
 beacon_node/http_api/src/lib.rs     |  5 +--
 beacon_node/http_api/tests/tests.rs | 54 +++++++++++++++++++++++++++++
 common/eth2/src/lib.rs              | 12 ++++++-
 3 files changed, 68 insertions(+), 3 deletions(-)

diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs
index 3777e6142..1594668e5 100644
--- a/beacon_node/http_api/src/lib.rs
+++ b/beacon_node/http_api/src/lib.rs
@@ -1704,18 +1704,19 @@ pub fn serve<T: BeaconChainTypes>(
         .and(warp::path("beacon"))
         .and(warp::path("blob_sidecars"))
         .and(block_id_or_err)
-        .and(warp::query::<api_types::BlobIndicesQuery>())
         .and(warp::path::end())
+        .and(multi_key_query::<api_types::BlobIndicesQuery>())
         .and(task_spawner_filter.clone())
         .and(chain_filter.clone())
         .and(warp::header::optional::<api_types::Accept>("accept"))
         .then(
             |block_id: BlockId,
-             indices: api_types::BlobIndicesQuery,
+             indices_res: Result<api_types::BlobIndicesQuery, warp::Rejection>,
              task_spawner: TaskSpawner<T::EthSpec>,
              chain: Arc<BeaconChain<T>>,
              accept_header: Option<api_types::Accept>| {
                 task_spawner.blocking_response_task(Priority::P1, move || {
+                    let indices = indices_res?;
                     let blob_sidecar_list_filtered =
                         block_id.blob_sidecar_list_filtered(indices, &chain)?;
                     match accept_header {
diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs
index 4e0b4d761..933f98661 100644
--- a/beacon_node/http_api/tests/tests.rs
+++ b/beacon_node/http_api/tests/tests.rs
@@ -1587,6 +1587,39 @@ impl ApiTester {
         self
     }
 
+    pub async fn test_get_blob_sidecars(self, use_indices: bool) -> Self {
+        let block_id = BlockId(CoreBlockId::Finalized);
+        let (block_root, _, _) = block_id.root(&self.chain).unwrap();
+        let (block, _, _) = block_id.full_block(&self.chain).await.unwrap();
+        let num_blobs = block.num_expected_blobs();
+        let blob_indices = if use_indices {
+            Some(
+                (0..num_blobs.saturating_sub(1) as u64)
+                    .into_iter()
+                    .collect::<Vec<_>>(),
+            )
+        } else {
+            None
+        };
+        let result = match self
+            .client
+            .get_blobs::<E>(CoreBlockId::Root(block_root), blob_indices.as_deref())
+            .await
+        {
+            Ok(result) => result.unwrap().data,
+            Err(e) => panic!("query failed incorrectly: {e:?}"),
+        };
+
+        assert_eq!(
+            result.len(),
+            blob_indices.map_or(num_blobs, |indices| indices.len())
+        );
+        let expected = block.slot();
+        assert_eq!(result.get(0).unwrap().slot(), expected);
+
+        self
+    }
+
     pub async fn test_beacon_blocks_attestations(self) -> Self {
         for block_id in self.interesting_block_ids() {
             let result = self
@@ -6291,6 +6324,27 @@ async fn builder_works_post_deneb() {
         .await;
 }
 
+#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
+async fn get_blob_sidecars() {
+    let mut config = ApiTesterConfig {
+        retain_historic_states: false,
+        spec: E::default_spec(),
+    };
+    config.spec.altair_fork_epoch = Some(Epoch::new(0));
+    config.spec.bellatrix_fork_epoch = Some(Epoch::new(0));
+    config.spec.capella_fork_epoch = Some(Epoch::new(0));
+    config.spec.deneb_fork_epoch = Some(Epoch::new(0));
+
+    ApiTester::new_from_config(config)
+        .await
+        .test_post_beacon_blocks_valid()
+        .await
+        .test_get_blob_sidecars(false)
+        .await
+        .test_get_blob_sidecars(true)
+        .await;
+}
+
 #[tokio::test(flavor = "multi_thread", worker_threads = 2)]
 async fn post_validator_liveness_epoch() {
     ApiTester::new()
diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs
index bed5044b4..16801be8e 100644
--- a/common/eth2/src/lib.rs
+++ b/common/eth2/src/lib.rs
@@ -1064,8 +1064,18 @@ impl BeaconNodeHttpClient {
     pub async fn get_blobs<T: EthSpec>(
         &self,
         block_id: BlockId,
+        indices: Option<&[u64]>,
     ) -> Result<Option<GenericResponse<BlobSidecarList<T>>>, Error> {
-        let path = self.get_blobs_path(block_id)?;
+        let mut path = self.get_blobs_path(block_id)?;
+        if let Some(indices) = indices {
+            let indices_string = indices
+                .iter()
+                .map(|i| i.to_string())
+                .collect::<Vec<_>>()
+                .join(",");
+            path.query_pairs_mut()
+                .append_pair("indices", &indices_string);
+        }
         let Some(response) = self.get_response(path, |b| b).await.optional()? else {
             return Ok(None);
         };