Move progressive list checks into BeaconBlock::from_ssz_bytes

beacon_node/beacon_chain/src/block_verification.rs

@@ -81,9 +81,6 @@ use slot_clock::SlotClock;
 use ssz::Encode;
 use ssz_derive::{Decode, Encode};
 use state_processing::per_block_processing::errors::IntoWithIndex;
-use state_processing::per_block_processing::{
-    process_operations::verify_operation_list_lengths, verify_execution_request_list_lengths,
-};
 use state_processing::{
     AllCaches, BlockProcessingError, BlockSignatureStrategy, ConsensusContext,
     GloasVerificationContext, SlotProcessingError, VerifyBlockRoot,
@@ -105,6 +102,7 @@ use types::{
     BeaconBlockRef, BeaconState, BeaconStateError, BlobsList, ChainSpec, DataColumnSidecarList,
     Epoch, EthSpec, ExecutionBlockHash, FullPayload, Hash256, InconsistentFork, KzgProofs,
     RelativeEpoch, SignedBeaconBlock, SignedBeaconBlockHeader, Slot, data::DataColumnSidecarError,
+    verify_execution_request_list_lengths_post_gloas,
 };
 
 /// Maximum block slot number. Block with slots bigger than this constant will NOT be processed.
@@ -902,9 +900,8 @@ impl<T: BeaconChainTypes> GossipVerifiedBlock<T> {
         }
 
         if let Ok(parent_execution_requests) = block.message().body().parent_execution_requests() {
-            verify_operation_list_lengths(block.message().body())
-                .map_err(BlockError::PerBlockProcessingError)?;
-            verify_execution_request_list_lengths(parent_execution_requests)
+            verify_execution_request_list_lengths_post_gloas(parent_execution_requests)
+                .map_err(BlockProcessingError::from)
                 .map_err(BlockError::PerBlockProcessingError)?;
             let deposits_len = block.message().body().deposits().len();
             if deposits_len > 0 {

consensus/state_processing/src/envelope_processing.rs

@@ -1,10 +1,13 @@
 use crate::VerifySignatures;
 use crate::per_block_processing::compute_timestamp_at_slot;
 use safe_arith::ArithError;
+use ssz::DecodeError;
 use tree_hash::TreeHash;
 use types::{
-    BeaconState, BeaconStateError, BuilderIndex, ChainSpec, EthSpec, ExecutionBlockHash, Hash256,
-    SignedExecutionPayloadEnvelope, Slot,
+    BeaconState, BeaconStateError, BuilderIndex, ChainSpec, EthSpec, ExecutionBlockHash,
+    ExecutionPayloadRef, Hash256, SignedExecutionPayloadEnvelope, Slot,
+    verify_execution_payload_list_lengths_post_gloas,
+    verify_execution_request_list_lengths_post_gloas,
 };
 
 macro_rules! envelope_verify {
@@ -21,6 +24,7 @@ pub enum EnvelopeProcessingError {
     BadSignature,
     BeaconStateError(BeaconStateError),
     ArithError(ArithError),
+    SszDecodeError(DecodeError),
     /// Envelope doesn't match latest beacon block header
     LatestBlockHeaderMismatch {
         envelope_root: Hash256,
@@ -93,6 +97,12 @@ impl From<ArithError> for EnvelopeProcessingError {
     }
 }
 
+impl From<DecodeError> for EnvelopeProcessingError {
+    fn from(e: DecodeError) -> Self {
+        EnvelopeProcessingError::SszDecodeError(e)
+    }
+}
+
 /// Verifies a `SignedExecutionPayloadEnvelope` against the beacon state.
 ///
 /// This function performs pure verification with no state mutation. The execution requests
@@ -116,6 +126,11 @@ pub fn verify_execution_payload_envelope<E: EthSpec>(
     let envelope = &signed_envelope.message;
     let payload = &envelope.payload;
 
+    // These limits are also checked during SSZ decoding, but check again here for defense in
+    // depth in case the envelope bypassed decoding.
+    verify_execution_payload_list_lengths_post_gloas(ExecutionPayloadRef::Gloas(payload))?;
+    verify_execution_request_list_lengths_post_gloas(&envelope.execution_requests)?;
+
     // Verify consistency with the beacon block.
     // Use a copy of the header with state_root filled in, matching the spec's approach.
     let mut header = state.latest_block_header().clone();
@@ -233,6 +248,83 @@ pub fn verify_execution_payload_envelope<E: EthSpec>(
     Ok(())
 }
 
+#[cfg(test)]
+mod progressive_list_tests {
+    use super::*;
+    use crate::BlockProcessingError;
+    use crate::per_block_processing::apply_parent_execution_payload;
+    use beacon_chain::test_utils::BeaconChainHarness;
+    use bls::Signature;
+    use types::{ExecutionPayloadEnvelope, ForkName, MinimalEthSpec};
+
+    #[tokio::test]
+    async fn reject_oversized_lists_without_ssz_decoding() {
+        type E = MinimalEthSpec;
+        let spec = ForkName::Gloas.make_genesis_spec(E::default_spec());
+        let harness = BeaconChainHarness::builder(E::default())
+            .spec(spec.into())
+            .deterministic_keypairs(8)
+            .fresh_ephemeral_store()
+            .mock_execution_layer()
+            .build();
+        let mut state = harness.get_current_state();
+        let mut envelope = SignedExecutionPayloadEnvelope {
+            message: ExecutionPayloadEnvelope::empty(),
+            signature: Signature::empty(),
+        };
+        let max = E::max_withdrawal_requests_per_payload();
+        envelope.message.execution_requests.withdrawals = std::iter::repeat_n(
+            types::test_utils::test_arbitrary_instance(),
+            max.saturating_add(1),
+        )
+        .collect();
+        let expected = DecodeError::BytesInvalid(format!(
+            "progressive list withdrawal_requests has length {} > {max}",
+            max.saturating_add(1),
+        ));
+        assert!(matches!(
+            verify_execution_payload_envelope(
+                &state,
+                &envelope,
+                VerifySignatures::False,
+                Hash256::default(),
+                &harness.spec,
+            ),
+            Err(EnvelopeProcessingError::SszDecodeError(error)) if error == expected
+        ));
+        assert!(matches!(
+            apply_parent_execution_payload(
+                &mut state,
+                &envelope.message.execution_requests,
+                &harness.spec,
+            ),
+            Err(BlockProcessingError::SszDecodeError(error)) if error == expected
+        ));
+
+        envelope.message.execution_requests = Default::default();
+        let max = E::max_withdrawals_per_payload();
+        envelope.message.payload.withdrawals = std::iter::repeat_n(
+            types::test_utils::test_arbitrary_instance(),
+            max.saturating_add(1),
+        )
+        .collect();
+        let expected = DecodeError::BytesInvalid(format!(
+            "progressive list withdrawals has length {} > {max}",
+            max.saturating_add(1),
+        ));
+        assert!(matches!(
+            verify_execution_payload_envelope(
+                &state,
+                &envelope,
+                VerifySignatures::False,
+                Hash256::default(),
+                &harness.spec,
+            ),
+            Err(EnvelopeProcessingError::SszDecodeError(error)) if error == expected
+        ));
+    }
+}
+
 #[cfg(not(debug_assertions))]
 #[cfg(test)]
 mod tests {

consensus/state_processing/src/per_block_processing.rs

@@ -609,7 +609,9 @@ pub fn apply_parent_execution_payload<E: EthSpec>(
     let parent_slot = parent_bid.slot;
     let parent_epoch = parent_slot.epoch(E::slots_per_epoch());
 
-    verify_execution_request_list_lengths(requests)?;
+    // These limits are also checked during SSZ decoding, but check again here for defense in
+    // depth in case the requests bypassed decoding.
+    verify_execution_request_list_lengths_post_gloas(requests)?;
 
     // Process execution requests from the parent's payload
     process_operations::process_deposit_requests(state, &requests.deposits, spec)?;
@@ -656,42 +658,6 @@ pub fn apply_parent_execution_payload<E: EthSpec>(
     Ok(())
 }
 
-/// Deposit requests are deliberately unbounded (see the `deposit_requests_greater_than_electra_max`
-/// spec test).
-pub fn verify_execution_request_list_lengths<E: EthSpec>(
-    requests: &ExecutionRequestsGloas<E>,
-) -> Result<(), BlockProcessingError> {
-    let checks = [
-        (
-            "withdrawal_requests",
-            requests.withdrawals.len(),
-            E::MaxWithdrawalRequestsPerPayload::to_usize(),
-        ),
-        (
-            "consolidation_requests",
-            requests.consolidations.len(),
-            E::MaxConsolidationRequestsPerPayload::to_usize(),
-        ),
-        (
-            "builder_deposit_requests",
-            requests.builder_deposits.len(),
-            E::MaxBuilderDepositRequestsPerPayload::to_usize(),
-        ),
-        (
-            "builder_exit_requests",
-            requests.builder_exits.len(),
-            E::MaxBuilderExitRequestsPerPayload::to_usize(),
-        ),
-    ];
-    for (kind, length, max) in checks {
-        block_verify!(
-            length <= max,
-            BlockProcessingError::OperationListTooLong { kind, length, max }
-        );
-    }
-    Ok(())
-}
-
 /// Spec: `settle_builder_payment`.
 ///
 /// Moves a pending payment from `builder_pending_payments[payment_index]` into

consensus/state_processing/src/per_block_processing/process_operations.rs

@@ -8,6 +8,7 @@ use crate::per_block_processing::errors::{BlockProcessingError, ExitInvalid, Int
 use crate::per_block_processing::verify_payload_attestation::verify_payload_attestation;
 use ssz_types::FixedVector;
 use typenum::U33;
+use types::block::verify_operation_list_lengths_post_gloas;
 use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR};
 use types::consts::gloas::PAYLOAD_BUILDER_VERSION;
 use types::is_builder_withdrawal_credential;
@@ -22,8 +23,11 @@ pub fn process_operations<E: EthSpec, Payload: AbstractExecPayload<E>>(
 ) -> Result<(), BlockProcessingError> {
     // [New in Gloas:EIP7688] The operation lists are `ProgressiveList`s without type-level
     // limits, so the spec's per-block limits are enforced at runtime instead.
+    //
+    // These limits are also checked when decoding the block, but we check again here for defense
+    // in depth.
     if state.fork_name_unchecked().gloas_enabled() {
-        verify_operation_list_lengths(block_body)?;
+        verify_operation_list_lengths_post_gloas(block_body)?;
     }
 
     process_proposer_slashings(
@@ -87,62 +91,6 @@ pub fn process_operations<E: EthSpec, Payload: AbstractExecPayload<E>>(
     Ok(())
 }
 
-/// Verify the lengths of the (progressive) operation lists against the spec's runtime limits.
-///
-/// [New in Gloas:EIP7688]: these limits used to be enforced by the SSZ types, but
-/// `ProgressiveList` is unbounded so they must be checked explicitly.
-pub fn verify_operation_list_lengths<E: EthSpec, Payload: AbstractExecPayload<E>>(
-    block_body: BeaconBlockBodyRef<E, Payload>,
-) -> Result<(), BlockProcessingError> {
-    let checks: [(&str, usize, usize); 6] = [
-        (
-            "proposer_slashings",
-            block_body.proposer_slashings().len(),
-            E::MaxProposerSlashings::to_usize(),
-        ),
-        (
-            "attester_slashings",
-            block_body.attester_slashings_len(),
-            E::MaxAttesterSlashingsElectra::to_usize(),
-        ),
-        (
-            "attestations",
-            block_body.attestations_len(),
-            E::MaxAttestationsElectra::to_usize(),
-        ),
-        (
-            "voluntary_exits",
-            block_body.voluntary_exits().len(),
-            E::MaxVoluntaryExits::to_usize(),
-        ),
-        (
-            "bls_to_execution_changes",
-            block_body
-                .bls_to_execution_changes()
-                .map(|changes| changes.len())
-                .unwrap_or(0),
-            E::MaxBlsToExecutionChanges::to_usize(),
-        ),
-        (
-            "payload_attestations",
-            block_body
-                .payload_attestations()
-                .map(|atts| atts.len())
-                .unwrap_or(0),
-            E::MaxPayloadAttestations::to_usize(),
-        ),
-    ];
-
-    for (kind, length, max) in checks {
-        block_verify!(
-            length <= max,
-            BlockProcessingError::OperationListTooLong { kind, length, max }
-        );
-    }
-
-    Ok(())
-}
-
 pub mod base {
     use super::*;
 

consensus/types/src/block/beacon_block.rs

@@ -149,7 +149,11 @@ impl<E: EthSpec, Payload: AbstractExecPayload<E>> BeaconBlock<E, Payload> {
         bytes: &[u8],
         fork_name: ForkName,
     ) -> Result<Self, ssz::DecodeError> {
-        Ok(map_fork_name!(fork_name, Self, <_>::from_ssz_bytes(bytes)?))
+        let block = map_fork_name!(fork_name, Self, <_>::from_ssz_bytes(bytes)?);
+        if block.to_ref().fork_name_unchecked().gloas_enabled() {
+            verify_operation_list_lengths_post_gloas(block.body())?;
+        }
+        Ok(block)
     }
 
     /// Try decoding each beacon block variant in sequence.
@@ -231,6 +235,63 @@ impl<E: EthSpec, Payload: AbstractExecPayload<E>> BeaconBlock<E, Payload> {
     }
 }
 
+/// Verify the lengths of the (progressive) operation lists against the spec's runtime limits.
+///
+/// [New in Gloas:EIP7688]: these limits used to be enforced by the SSZ types, but
+/// `ProgressiveList` is unbounded so they must be checked explicitly.
+pub fn verify_operation_list_lengths_post_gloas<E: EthSpec, Payload: AbstractExecPayload<E>>(
+    block_body: BeaconBlockBodyRef<E, Payload>,
+) -> Result<(), ssz::DecodeError> {
+    let checks: [(&str, usize, usize); 6] = [
+        (
+            "proposer_slashings",
+            block_body.proposer_slashings().len(),
+            E::MaxProposerSlashings::to_usize(),
+        ),
+        (
+            "attester_slashings",
+            block_body.attester_slashings_len(),
+            E::MaxAttesterSlashingsElectra::to_usize(),
+        ),
+        (
+            "attestations",
+            block_body.attestations_len(),
+            E::MaxAttestationsElectra::to_usize(),
+        ),
+        (
+            "voluntary_exits",
+            block_body.voluntary_exits().len(),
+            E::MaxVoluntaryExits::to_usize(),
+        ),
+        (
+            "bls_to_execution_changes",
+            block_body
+                .bls_to_execution_changes()
+                .map(|changes| changes.len())
+                .unwrap_or(0),
+            E::MaxBlsToExecutionChanges::to_usize(),
+        ),
+        (
+            "payload_attestations",
+            block_body
+                .payload_attestations()
+                .map(|atts| atts.len())
+                .unwrap_or(0),
+            E::MaxPayloadAttestations::to_usize(),
+        ),
+    ];
+
+    for (kind, length, max) in checks {
+        if length > max {
+            return Err(ssz::DecodeError::BytesInvalid(format!(
+                "progressive list {kind} has length {length} > {max}"
+            )));
+        }
+    }
+
+    Ok(())
+}
+
 impl<'a, E: EthSpec, Payload: AbstractExecPayload<E>> BeaconBlockRef<'a, E, Payload> {
     /// Returns the name of the fork pertaining to `self`.
     ///

consensus/types/src/block/mod.rs

@@ -8,6 +8,7 @@ pub use beacon_block::{
     BeaconBlock, BeaconBlockAltair, BeaconBlockBase, BeaconBlockBellatrix, BeaconBlockCapella,
     BeaconBlockDeneb, BeaconBlockElectra, BeaconBlockFulu, BeaconBlockGloas, BeaconBlockHeze,
     BeaconBlockRef, BeaconBlockRefMut, BlindedBeaconBlock, BlockImportSource, EmptyBlock,
+    verify_operation_list_lengths_post_gloas,
 };
 pub use beacon_block_body::{
     BLOB_KZG_COMMITMENTS_INDEX, BeaconBlockBody, BeaconBlockBodyAltair, BeaconBlockBodyBase,