From 0a6e4a11d7304ed087d023b898fd030bafc3ed89 Mon Sep 17 00:00:00 2001
From: dknopik <107140945+dknopik@users.noreply.github.com>
Date: Sat, 23 Mar 2024 01:01:11 +0100
Subject: [PATCH] Verify whether validators really are unknown during sync
 committee duty API request (#5174)

* Verify whether validators really are unknown during sync committee duty API request

* Merge branch 'unstable' into fix-4717

* Merge branch 'unstable' into fix-4717

* Merge branch 'unstable' of https://github.com/sigp/lighthouse into fix-4717
---
 beacon_node/beacon_chain/src/beacon_chain.rs |  2 +-
 beacon_node/http_api/src/sync_committees.rs  | 53 ++++++++++++++++++--
 consensus/types/src/beacon_state.rs          |  6 +--
 3 files changed, 54 insertions(+), 7 deletions(-)

diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs
index 1313cf7ec..2d393a90a 100644
--- a/beacon_node/beacon_chain/src/beacon_chain.rs
+++ b/beacon_node/beacon_chain/src/beacon_chain.rs
@@ -2566,7 +2566,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         epoch: Epoch,
         validator_indices: &[u64],
-    ) -> Result<Vec<Option<SyncDuty>>, Error> {
+    ) -> Result<Vec<Result<Option<SyncDuty>, BeaconStateError>>, Error> {
         self.with_head(move |head| {
             head.beacon_state
                 .get_sync_committee_duties(epoch, validator_indices, &self.spec)
diff --git a/beacon_node/http_api/src/sync_committees.rs b/beacon_node/http_api/src/sync_committees.rs
index 8b0c7dc0e..3e5b1dc52 100644
--- a/beacon_node/http_api/src/sync_committees.rs
+++ b/beacon_node/http_api/src/sync_committees.rs
@@ -45,7 +45,12 @@ pub fn sync_committee_duties<T: BeaconChainTypes>(
     // the vast majority of requests. Rather than checking if we think the request will succeed in a
     // way prone to data races, we attempt the request immediately and check the error code.
     match chain.sync_committee_duties_from_head(request_epoch, request_indices) {
-        Ok(duties) => return Ok(convert_to_response(duties, execution_optimistic)),
+        Ok(duties) => {
+            return Ok(convert_to_response(
+                verify_unknown_validators(duties, request_epoch, chain)?,
+                execution_optimistic,
+            ))
+        }
         Err(BeaconChainError::SyncDutiesError(BeaconStateError::SyncCommitteeNotKnown {
             ..
         }))
@@ -64,7 +69,10 @@ pub fn sync_committee_duties<T: BeaconChainTypes>(
         )),
         e => warp_utils::reject::beacon_chain_error(e),
     })?;
-    Ok(convert_to_response(duties, execution_optimistic))
+    Ok(convert_to_response(
+        verify_unknown_validators(duties, request_epoch, chain)?,
+        execution_optimistic,
+    ))
 }
 
 /// Slow path for duties: load a state and use it to compute the duties.
@@ -73,7 +81,7 @@ fn duties_from_state_load<T: BeaconChainTypes>(
     request_indices: &[u64],
     altair_fork_epoch: Epoch,
     chain: &BeaconChain<T>,
-) -> Result<Vec<Option<SyncDuty>>, BeaconChainError> {
+) -> Result<Vec<Result<Option<SyncDuty>, BeaconStateError>>, BeaconChainError> {
     // Determine what the current epoch would be if we fast-forward our system clock by
     // `MAXIMUM_GOSSIP_CLOCK_DISPARITY`.
     //
@@ -121,6 +129,45 @@ fn duties_from_state_load<T: BeaconChainTypes>(
     }
 }
 
+fn verify_unknown_validators<T: BeaconChainTypes>(
+    duties: Vec<Result<Option<SyncDuty>, BeaconStateError>>,
+    request_epoch: Epoch,
+    chain: &BeaconChain<T>,
+) -> Result<Vec<Option<SyncDuty>>, warp::reject::Rejection> {
+    // Lazily load the request_epoch_state, as it is only needed if there are any UnknownValidator
+    let mut request_epoch_state = None;
+
+    duties
+        .into_iter()
+        .map(|res| {
+            res.or_else(|err| {
+                // Make sure the validator is really unknown w.r.t. the request_epoch
+                if let BeaconStateError::UnknownValidator(idx) = err {
+                    let request_epoch_state = match &mut request_epoch_state {
+                        Some(state) => state,
+                        None => request_epoch_state.insert(chain.state_at_slot(
+                            request_epoch.start_slot(T::EthSpec::slots_per_epoch()),
+                            StateSkipConfig::WithoutStateRoots,
+                        )?),
+                    };
+                    request_epoch_state
+                        .get_validator(idx)
+                        .map_err(BeaconChainError::SyncDutiesError)
+                        .map(|_| None)
+                } else {
+                    Err(BeaconChainError::SyncDutiesError(err))
+                }
+            })
+        })
+        .collect::<Result<Vec<_>, _>>()
+        .map_err(|err| match err {
+            BeaconChainError::SyncDutiesError(BeaconStateError::UnknownValidator(idx)) => {
+                warp_utils::reject::custom_bad_request(format!("invalid validator index: {idx}"))
+            }
+            e => warp_utils::reject::beacon_chain_error(e),
+        })
+}
+
 fn convert_to_response(duties: Vec<Option<SyncDuty>>, execution_optimistic: bool) -> SyncDuties {
     api_types::GenericResponse::from(duties.into_iter().flatten().collect::<Vec<_>>())
         .add_execution_optimistic(execution_optimistic)
diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs
index 698c3a657..8bef7281f 100644
--- a/consensus/types/src/beacon_state.rs
+++ b/consensus/types/src/beacon_state.rs
@@ -960,10 +960,10 @@ impl<T: EthSpec> BeaconState<T> {
         epoch: Epoch,
         validator_indices: &[u64],
         spec: &ChainSpec,
-    ) -> Result<Vec<Option<SyncDuty>>, Error> {
+    ) -> Result<Vec<Result<Option<SyncDuty>, Error>>, Error> {
         let sync_committee = self.get_built_sync_committee(epoch, spec)?;
 
-        validator_indices
+        Ok(validator_indices
             .iter()
             .map(|&validator_index| {
                 let pubkey = self.get_validator(validator_index as usize)?.pubkey;
@@ -974,7 +974,7 @@ impl<T: EthSpec> BeaconState<T> {
                     sync_committee,
                 ))
             })
-            .collect()
+            .collect())
     }
 
     /// Get the canonical root of the `latest_block_header`, filling in its state root if necessary.