builder gas limit & some refactoring (#6583)

* Cache gas_limit

* Payload Parameters Refactor

* Enforce Proposer Gas Limit

* Fixed and Added New Tests

* Fix Beacon Chain Tests

beacon_node/execution_layer/src/lib.rs

@@ -28,7 +28,7 @@ use sensitive_url::SensitiveUrl;
 use serde::{Deserialize, Serialize};
 use slog::{crit, debug, error, info, warn, Logger};
 use slot_clock::SlotClock;
-use std::collections::HashMap;
+use std::collections::{hash_map::Entry, HashMap};
 use std::fmt;
 use std::future::Future;
 use std::io::Write;
@@ -319,10 +319,52 @@ impl<E: EthSpec, Payload: AbstractExecPayload<E>> BlockProposalContents<E, Paylo
     }
 }
 
+// This just groups together a bunch of parameters that commonly
+// get passed around together in calls to get_payload.
+#[derive(Clone, Copy)]
+pub struct PayloadParameters<'a> {
+    pub parent_hash: ExecutionBlockHash,
+    pub parent_gas_limit: u64,
+    pub proposer_gas_limit: Option<u64>,
+    pub payload_attributes: &'a PayloadAttributes,
+    pub forkchoice_update_params: &'a ForkchoiceUpdateParameters,
+    pub current_fork: ForkName,
+}
+
 #[derive(Clone, PartialEq)]
 pub struct ProposerPreparationDataEntry {
     update_epoch: Epoch,
     preparation_data: ProposerPreparationData,
+    gas_limit: Option<u64>,
+}
+
+impl ProposerPreparationDataEntry {
+    pub fn update(&mut self, updated: Self) -> bool {
+        let mut changed = false;
+        // Update `gas_limit` if `updated.gas_limit` is `Some` and:
+        // - `self.gas_limit` is `None`, or
+        // - both are `Some` but the values differ.
+        if let Some(updated_gas_limit) = updated.gas_limit {
+            if self.gas_limit != Some(updated_gas_limit) {
+                self.gas_limit = Some(updated_gas_limit);
+                changed = true;
+            }
+        }
+
+        // Update `update_epoch` if it differs
+        if self.update_epoch != updated.update_epoch {
+            self.update_epoch = updated.update_epoch;
+            changed = true;
+        }
+
+        // Update `preparation_data` if it differs
+        if self.preparation_data != updated.preparation_data {
+            self.preparation_data = updated.preparation_data;
+            changed = true;
+        }
+
+        changed
+    }
 }
 
 #[derive(Hash, PartialEq, Eq)]
@@ -711,23 +753,29 @@ impl<E: EthSpec> ExecutionLayer<E> {
     }
 
     /// Updates the proposer preparation data provided by validators
-    pub async fn update_proposer_preparation(
-        &self,
-        update_epoch: Epoch,
-        preparation_data: &[ProposerPreparationData],
-    ) {
+    pub async fn update_proposer_preparation<'a, I>(&self, update_epoch: Epoch, proposer_data: I)
+    where
+        I: IntoIterator<Item = (&'a ProposerPreparationData, &'a Option<u64>)>,
+    {
         let mut proposer_preparation_data = self.proposer_preparation_data().await;
-        for preparation_entry in preparation_data {
+
+        for (preparation_entry, gas_limit) in proposer_data {
             let new = ProposerPreparationDataEntry {
                 update_epoch,
                 preparation_data: preparation_entry.clone(),
+                gas_limit: *gas_limit,
             };
 
-            let existing =
-                proposer_preparation_data.insert(preparation_entry.validator_index, new.clone());
-
-            if existing != Some(new) {
-                metrics::inc_counter(&metrics::EXECUTION_LAYER_PROPOSER_DATA_UPDATED);
+            match proposer_preparation_data.entry(preparation_entry.validator_index) {
+                Entry::Occupied(mut entry) => {
+                    if entry.get_mut().update(new) {
+                        metrics::inc_counter(&metrics::EXECUTION_LAYER_PROPOSER_DATA_UPDATED);
+                    }
+                }
+                Entry::Vacant(entry) => {
+                    entry.insert(new);
+                    metrics::inc_counter(&metrics::EXECUTION_LAYER_PROPOSER_DATA_UPDATED);
+                }
             }
         }
     }
@@ -809,6 +857,13 @@ impl<E: EthSpec> ExecutionLayer<E> {
         }
     }
 
+    pub async fn get_proposer_gas_limit(&self, proposer_index: u64) -> Option<u64> {
+        self.proposer_preparation_data()
+            .await
+            .get(&proposer_index)
+            .and_then(|entry| entry.gas_limit)
+    }
+
     /// Maps to the `engine_getPayload` JSON-RPC call.
     ///
     /// However, it will attempt to call `self.prepare_payload` if it cannot find an existing
@@ -818,14 +873,10 @@ impl<E: EthSpec> ExecutionLayer<E> {
     ///
     /// The result will be returned from the first node that returns successfully. No more nodes
     /// will be contacted.
-    #[allow(clippy::too_many_arguments)]
     pub async fn get_payload(
         &self,
-        parent_hash: ExecutionBlockHash,
-        payload_attributes: &PayloadAttributes,
-        forkchoice_update_params: ForkchoiceUpdateParameters,
+        payload_parameters: PayloadParameters<'_>,
         builder_params: BuilderParams,
-        current_fork: ForkName,
         spec: &ChainSpec,
         builder_boost_factor: Option<u64>,
         block_production_version: BlockProductionVersion,
@@ -833,11 +884,8 @@ impl<E: EthSpec> ExecutionLayer<E> {
         let payload_result_type = match block_production_version {
             BlockProductionVersion::V3 => match self
                 .determine_and_fetch_payload(
-                    parent_hash,
-                    payload_attributes,
-                    forkchoice_update_params,
+                    payload_parameters,
                     builder_params,
-                    current_fork,
                     builder_boost_factor,
                     spec,
                 )
@@ -857,25 +905,11 @@ impl<E: EthSpec> ExecutionLayer<E> {
                     &metrics::EXECUTION_LAYER_REQUEST_TIMES,
                     &[metrics::GET_BLINDED_PAYLOAD],
                 );
-                self.determine_and_fetch_payload(
-                    parent_hash,
-                    payload_attributes,
-                    forkchoice_update_params,
-                    builder_params,
-                    current_fork,
-                    None,
-                    spec,
-                )
-                .await?
+                self.determine_and_fetch_payload(payload_parameters, builder_params, None, spec)
+                    .await?
             }
             BlockProductionVersion::FullV2 => self
-                .get_full_payload_with(
-                    parent_hash,
-                    payload_attributes,
-                    forkchoice_update_params,
-                    current_fork,
-                    noop,
-                )
+                .get_full_payload_with(payload_parameters, noop)
                 .await
                 .and_then(GetPayloadResponseType::try_into)
                 .map(ProvenancedPayload::Local)?,
@@ -922,17 +956,15 @@ impl<E: EthSpec> ExecutionLayer<E> {
     async fn fetch_builder_and_local_payloads(
         &self,
         builder: &BuilderHttpClient,
-        parent_hash: ExecutionBlockHash,
         builder_params: &BuilderParams,
-        payload_attributes: &PayloadAttributes,
-        forkchoice_update_params: ForkchoiceUpdateParameters,
-        current_fork: ForkName,
+        payload_parameters: PayloadParameters<'_>,
     ) -> (
         Result<Option<ForkVersionedResponse<SignedBuilderBid<E>>>, builder_client::Error>,
         Result<GetPayloadResponse<E>, Error>,
     ) {
         let slot = builder_params.slot;
         let pubkey = &builder_params.pubkey;
+        let parent_hash = payload_parameters.parent_hash;
 
         info!(
             self.log(),
@@ -950,17 +982,12 @@ impl<E: EthSpec> ExecutionLayer<E> {
                     .await
             }),
             timed_future(metrics::GET_BLINDED_PAYLOAD_LOCAL, async {
-                self.get_full_payload_caching(
-                    parent_hash,
-                    payload_attributes,
-                    forkchoice_update_params,
-                    current_fork,
-                )
-                .await
-                .and_then(|local_result_type| match local_result_type {
-                    GetPayloadResponseType::Full(payload) => Ok(payload),
-                    GetPayloadResponseType::Blinded(_) => Err(Error::PayloadTypeMismatch),
-                })
+                self.get_full_payload_caching(payload_parameters)
+                    .await
+                    .and_then(|local_result_type| match local_result_type {
+                        GetPayloadResponseType::Full(payload) => Ok(payload),
+                        GetPayloadResponseType::Blinded(_) => Err(Error::PayloadTypeMismatch),
+                    })
             })
         );
 
@@ -984,26 +1011,17 @@ impl<E: EthSpec> ExecutionLayer<E> {
         (relay_result, local_result)
     }
 
-    #[allow(clippy::too_many_arguments)]
     async fn determine_and_fetch_payload(
         &self,
-        parent_hash: ExecutionBlockHash,
-        payload_attributes: &PayloadAttributes,
-        forkchoice_update_params: ForkchoiceUpdateParameters,
+        payload_parameters: PayloadParameters<'_>,
         builder_params: BuilderParams,
-        current_fork: ForkName,
         builder_boost_factor: Option<u64>,
         spec: &ChainSpec,
     ) -> Result<ProvenancedPayload<BlockProposalContentsType<E>>, Error> {
         let Some(builder) = self.builder() else {
             // no builder.. return local payload
             return self
-                .get_full_payload_caching(
-                    parent_hash,
-                    payload_attributes,
-                    forkchoice_update_params,
-                    current_fork,
-                )
+                .get_full_payload_caching(payload_parameters)
                 .await
                 .and_then(GetPayloadResponseType::try_into)
                 .map(ProvenancedPayload::Local);
@@ -1034,26 +1052,15 @@ impl<E: EthSpec> ExecutionLayer<E> {
                 ),
             }
             return self
-                .get_full_payload_caching(
-                    parent_hash,
-                    payload_attributes,
-                    forkchoice_update_params,
-                    current_fork,
-                )
+                .get_full_payload_caching(payload_parameters)
                 .await
                 .and_then(GetPayloadResponseType::try_into)
                 .map(ProvenancedPayload::Local);
         }
 
+        let parent_hash = payload_parameters.parent_hash;
         let (relay_result, local_result) = self
-            .fetch_builder_and_local_payloads(
-                builder.as_ref(),
-                parent_hash,
-                &builder_params,
-                payload_attributes,
-                forkchoice_update_params,
-                current_fork,
-            )
+            .fetch_builder_and_local_payloads(builder.as_ref(), &builder_params, payload_parameters)
             .await;
 
         match (relay_result, local_result) {
@@ -1118,14 +1125,9 @@ impl<E: EthSpec> ExecutionLayer<E> {
                 );
 
                 // check relay payload validity
-                if let Err(reason) = verify_builder_bid(
-                    &relay,
-                    parent_hash,
-                    payload_attributes,
-                    Some(local.block_number()),
-                    current_fork,
-                    spec,
-                ) {
+                if let Err(reason) =
+                    verify_builder_bid(&relay, payload_parameters, Some(local.block_number()), spec)
+                {
                     // relay payload invalid -> return local
                     metrics::inc_counter_vec(
                         &metrics::EXECUTION_LAYER_GET_PAYLOAD_BUILDER_REJECTIONS,
@@ -1202,14 +1204,7 @@ impl<E: EthSpec> ExecutionLayer<E> {
                     "parent_hash" => ?parent_hash,
                 );
 
-                match verify_builder_bid(
-                    &relay,
-                    parent_hash,
-                    payload_attributes,
-                    None,
-                    current_fork,
-                    spec,
-                ) {
+                match verify_builder_bid(&relay, payload_parameters, None, spec) {
                     Ok(()) => Ok(ProvenancedPayload::try_from(relay.data.message)?),
                     Err(reason) => {
                         metrics::inc_counter_vec(
@@ -1234,32 +1229,28 @@ impl<E: EthSpec> ExecutionLayer<E> {
     /// Get a full payload and cache its result in the execution layer's payload cache.
     async fn get_full_payload_caching(
         &self,
-        parent_hash: ExecutionBlockHash,
-        payload_attributes: &PayloadAttributes,
-        forkchoice_update_params: ForkchoiceUpdateParameters,
-        current_fork: ForkName,
+        payload_parameters: PayloadParameters<'_>,
     ) -> Result<GetPayloadResponseType<E>, Error> {
-        self.get_full_payload_with(
-            parent_hash,
-            payload_attributes,
-            forkchoice_update_params,
-            current_fork,
-            Self::cache_payload,
-        )
-        .await
+        self.get_full_payload_with(payload_parameters, Self::cache_payload)
+            .await
     }
 
     async fn get_full_payload_with(
         &self,
-        parent_hash: ExecutionBlockHash,
-        payload_attributes: &PayloadAttributes,
-        forkchoice_update_params: ForkchoiceUpdateParameters,
-        current_fork: ForkName,
+        payload_parameters: PayloadParameters<'_>,
         cache_fn: fn(
             &ExecutionLayer<E>,
             PayloadContentsRefTuple<E>,
         ) -> Option<FullPayloadContents<E>>,
     ) -> Result<GetPayloadResponseType<E>, Error> {
+        let PayloadParameters {
+            parent_hash,
+            payload_attributes,
+            forkchoice_update_params,
+            current_fork,
+            ..
+        } = payload_parameters;
+
         self.engine()
             .request(move |engine| async move {
                 let payload_id = if let Some(id) = engine
@@ -1984,6 +1975,10 @@ enum InvalidBuilderPayload {
         payload: Option<Hash256>,
         expected: Option<Hash256>,
     },
+    GasLimitMismatch {
+        payload: u64,
+        expected: u64,
+    },
 }
 
 impl fmt::Display for InvalidBuilderPayload {
@@ -2022,19 +2017,51 @@ impl fmt::Display for InvalidBuilderPayload {
                     opt_string(expected)
                 )
             }
+            InvalidBuilderPayload::GasLimitMismatch { payload, expected } => {
+                write!(f, "payload gas limit was {} not {}", payload, expected)
+            }
         }
     }
 }
 
+/// Calculate the expected gas limit for a block.
+pub fn expected_gas_limit(
+    parent_gas_limit: u64,
+    target_gas_limit: u64,
+    spec: &ChainSpec,
+) -> Option<u64> {
+    // Calculate the maximum gas limit difference allowed safely
+    let max_gas_limit_difference = parent_gas_limit
+        .checked_div(spec.gas_limit_adjustment_factor)
+        .and_then(|result| result.checked_sub(1))
+        .unwrap_or(0);
+
+    // Adjust the gas limit safely
+    if target_gas_limit > parent_gas_limit {
+        let gas_diff = target_gas_limit.saturating_sub(parent_gas_limit);
+        parent_gas_limit.checked_add(std::cmp::min(gas_diff, max_gas_limit_difference))
+    } else {
+        let gas_diff = parent_gas_limit.saturating_sub(target_gas_limit);
+        parent_gas_limit.checked_sub(std::cmp::min(gas_diff, max_gas_limit_difference))
+    }
+}
+
 /// Perform some cursory, non-exhaustive validation of the bid returned from the builder.
 fn verify_builder_bid<E: EthSpec>(
     bid: &ForkVersionedResponse<SignedBuilderBid<E>>,
-    parent_hash: ExecutionBlockHash,
-    payload_attributes: &PayloadAttributes,
+    payload_parameters: PayloadParameters<'_>,
     block_number: Option<u64>,
-    current_fork: ForkName,
     spec: &ChainSpec,
 ) -> Result<(), Box<InvalidBuilderPayload>> {
+    let PayloadParameters {
+        parent_hash,
+        payload_attributes,
+        current_fork,
+        parent_gas_limit,
+        proposer_gas_limit,
+        ..
+    } = payload_parameters;
+
     let is_signature_valid = bid.data.verify_signature(spec);
     let header = &bid.data.message.header();
 
@@ -2050,6 +2077,8 @@ fn verify_builder_bid<E: EthSpec>(
         .cloned()
         .map(|withdrawals| Withdrawals::<E>::from(withdrawals).tree_hash_root());
     let payload_withdrawals_root = header.withdrawals_root().ok();
+    let expected_gas_limit = proposer_gas_limit
+        .and_then(|target_gas_limit| expected_gas_limit(parent_gas_limit, target_gas_limit, spec));
 
     if header.parent_hash() != parent_hash {
         Err(Box::new(InvalidBuilderPayload::ParentHash {
@@ -2086,6 +2115,14 @@ fn verify_builder_bid<E: EthSpec>(
             payload: payload_withdrawals_root,
             expected: expected_withdrawals_root,
         }))
+    } else if expected_gas_limit
+        .map(|gas_limit| header.gas_limit() != gas_limit)
+        .unwrap_or(false)
+    {
+        Err(Box::new(InvalidBuilderPayload::GasLimitMismatch {
+            payload: header.gas_limit(),
+            expected: expected_gas_limit.unwrap_or(0),
+        }))
     } else {
         Ok(())
     }
@@ -2138,6 +2175,27 @@ mod test {
             .await;
     }
 
+    #[tokio::test]
+    async fn test_expected_gas_limit() {
+        let spec = ChainSpec::mainnet();
+        assert_eq!(
+            expected_gas_limit(30_000_000, 30_000_000, &spec),
+            Some(30_000_000)
+        );
+        assert_eq!(
+            expected_gas_limit(30_000_000, 40_000_000, &spec),
+            Some(30_029_295)
+        );
+        assert_eq!(
+            expected_gas_limit(30_029_295, 40_000_000, &spec),
+            Some(30_058_619)
+        );
+        assert_eq!(
+            expected_gas_limit(30_058_619, 30_000_000, &spec),
+            Some(30_029_266)
+        );
+    }
+
     #[tokio::test]
     async fn test_forked_terminal_block() {
         let runtime = TestRuntime::default();