diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 44baa698a72..1c99c0a9637 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/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 GossipVerifiedBlock { } 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 { diff --git a/consensus/state_processing/src/envelope_processing.rs b/consensus/state_processing/src/envelope_processing.rs index ff3060c7798..96ceeacdd62 100644 --- a/consensus/state_processing/src/envelope_processing.rs +++ b/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 for EnvelopeProcessingError { } } +impl From 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( 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( 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 { diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 37bf4e16ffa..d8323bd8197 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -609,7 +609,9 @@ pub fn apply_parent_execution_payload( 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( Ok(()) } -/// Deposit requests are deliberately unbounded (see the `deposit_requests_greater_than_electra_max` -/// spec test). -pub fn verify_execution_request_list_lengths( - requests: &ExecutionRequestsGloas, -) -> 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 diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index a6bfedd384d..7f80bbaf520 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/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>( ) -> 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>( 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>( - block_body: BeaconBlockBodyRef, -) -> 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::*; diff --git a/consensus/types/src/block/beacon_block.rs b/consensus/types/src/block/beacon_block.rs index 16b8265a597..cdde16f5e2e 100644 --- a/consensus/types/src/block/beacon_block.rs +++ b/consensus/types/src/block/beacon_block.rs @@ -149,7 +149,11 @@ impl> BeaconBlock { bytes: &[u8], fork_name: ForkName, ) -> Result { - 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> BeaconBlock { } } +/// 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>( + block_body: BeaconBlockBodyRef, +) -> 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> BeaconBlockRef<'a, E, Payload> { /// Returns the name of the fork pertaining to `self`. /// diff --git a/consensus/types/src/block/mod.rs b/consensus/types/src/block/mod.rs index d7e58f025f3..e2c3bf16f6c 100644 --- a/consensus/types/src/block/mod.rs +++ b/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, diff --git a/consensus/types/src/execution/execution_payload.rs b/consensus/types/src/execution/execution_payload.rs index b2e5757f419..3835b69ff11 100644 --- a/consensus/types/src/execution/execution_payload.rs +++ b/consensus/types/src/execution/execution_payload.rs @@ -268,7 +268,7 @@ impl ExecutionPayload { impl ForkVersionDecode for ExecutionPayload { /// SSZ decode with explicit fork variant. fn from_ssz_bytes_by_fork(bytes: &[u8], fork_name: ForkName) -> Result { - match fork_name { + let payload = match fork_name { ForkName::Base | ForkName::Altair => Err(ssz::DecodeError::BytesInvalid(format!( "unsupported fork for ExecutionPayload: {fork_name}", ))), @@ -281,10 +281,60 @@ impl ForkVersionDecode for ExecutionPayload { ForkName::Fulu => ExecutionPayloadFulu::from_ssz_bytes(bytes).map(Self::Fulu), ForkName::Gloas => ExecutionPayloadGloas::from_ssz_bytes(bytes).map(Self::Gloas), ForkName::Heze => ExecutionPayloadHeze::from_ssz_bytes(bytes).map(Self::Heze), + }?; + if fork_name.gloas_enabled() { + verify_execution_payload_list_lengths_post_gloas(payload.to_ref())?; } + Ok(payload) } } +/// Verify the lengths of progressive payload lists against the spec's runtime limits. +/// +/// [New in Gloas:EIP7688]: these limits used to be enforced by the SSZ types, but progressive +/// lists are unbounded so both the number of transactions and each transaction's length must +/// be checked explicitly, along with the number of withdrawals. +pub fn verify_execution_payload_list_lengths_post_gloas( + payload: ExecutionPayloadRef<'_, E>, +) -> Result<(), ssz::DecodeError> { + let transactions = payload.transactions(); + let checks = [ + ( + "transactions", + transactions.len(), + E::max_transactions_per_payload(), + ), + ( + "withdrawals", + payload + .withdrawals() + .map(|withdrawals| withdrawals.len()) + .unwrap_or(0), + E::max_withdrawals_per_payload(), + ), + ]; + + for (kind, length, max) in checks { + if length > max { + return Err(ssz::DecodeError::BytesInvalid(format!( + "progressive list {kind} has length {length} > {max}" + ))); + } + } + + let max = E::max_bytes_per_transaction(); + for transaction in transactions.iter() { + let length = transaction.len(); + if length > max { + return Err(ssz::DecodeError::BytesInvalid(format!( + "progressive list transaction has length {length} > {max}" + ))); + } + } + + Ok(()) +} + impl ExecutionPayload { #[allow(clippy::arithmetic_side_effects)] /// Returns the maximum size of an execution payload. diff --git a/consensus/types/src/execution/execution_payload_envelope.rs b/consensus/types/src/execution/execution_payload_envelope.rs index 1984df38f65..280deb7afbe 100644 --- a/consensus/types/src/execution/execution_payload_envelope.rs +++ b/consensus/types/src/execution/execution_payload_envelope.rs @@ -1,11 +1,15 @@ -use crate::execution::{ExecutionPayloadGloas, ExecutionRequestsGloas}; +use crate::execution::{ + ExecutionPayloadGloas, ExecutionPayloadRef, ExecutionRequestsGloas, + verify_execution_payload_list_lengths_post_gloas, + verify_execution_request_list_lengths_post_gloas, +}; use crate::{EthSpec, ForkName, Hash256, SignedRoot, Slot}; use context_deserialize::context_deserialize; use educe::Educe; use fixed_bytes::FixedBytesExtended; use serde::{Deserialize, Serialize}; -use ssz::{BYTES_PER_LENGTH_OFFSET, Encode as SszEncode}; -use ssz_derive::{Decode, Encode}; +use ssz::{BYTES_PER_LENGTH_OFFSET, Decode, Encode as SszEncode}; +use ssz_derive::Encode; use tree_hash_derive::TreeHash; #[cfg_attr( @@ -13,7 +17,7 @@ use tree_hash_derive::TreeHash; derive(arbitrary::Arbitrary), arbitrary(bound = "E: EthSpec") )] -#[derive(Debug, Clone, Serialize, Encode, Decode, Deserialize, TreeHash, Educe)] +#[derive(Debug, Clone, Serialize, Encode, Deserialize, TreeHash, Educe)] #[educe(PartialEq, Hash(bound(E: EthSpec)))] #[context_deserialize(ForkName)] #[serde(bound = "E: EthSpec")] @@ -74,10 +78,124 @@ impl ExecutionPayloadEnvelope { impl SignedRoot for ExecutionPayloadEnvelope {} +impl Decode for ExecutionPayloadEnvelope { + fn is_ssz_fixed_len() -> bool { + false + } + + fn from_ssz_bytes(bytes: &[u8]) -> Result { + let mut builder = ssz::SszDecoderBuilder::new(bytes); + builder.register_type::>()?; + builder.register_type::>()?; + builder.register_type::()?; + builder.register_type::()?; + builder.register_type::()?; + + let mut decoder = builder.build()?; + let envelope = Self { + payload: decoder.decode_next()?, + execution_requests: decoder.decode_next()?, + builder_index: decoder.decode_next()?, + beacon_block_root: decoder.decode_next()?, + parent_beacon_block_root: decoder.decode_next()?, + }; + verify_execution_payload_list_lengths_post_gloas(ExecutionPayloadRef::Gloas( + &envelope.payload, + ))?; + verify_execution_request_list_lengths_post_gloas(&envelope.execution_requests)?; + Ok(envelope) + } +} + #[cfg(test)] mod tests { use super::*; - use crate::MainnetEthSpec; + use crate::{ + ExecutionPayload, ForkVersionDecode, MainnetEthSpec, SignedExecutionPayloadEnvelope, + }; + use bls::Signature; ssz_and_tree_hash_tests!(ExecutionPayloadEnvelope); + + type E = MainnetEthSpec; + + fn assert_payload_decoding_result( + payload: ExecutionPayloadGloas, + expected: Result<(), ssz::DecodeError>, + ) { + assert_eq!( + verify_execution_payload_list_lengths_post_gloas(ExecutionPayloadRef::Gloas(&payload)), + expected + ); + let payload_bytes = payload.as_ssz_bytes(); + for fork in [ForkName::Gloas, ForkName::Heze] { + assert_eq!( + ExecutionPayload::::from_ssz_bytes_by_fork(&payload_bytes, fork).map( + |decoded| { + assert_eq!(decoded.fork_name(), fork); + assert_eq!(decoded.as_ssz_bytes(), payload_bytes); + } + ), + expected + ); + } + + let envelope = ExecutionPayloadEnvelope { + payload, + ..ExecutionPayloadEnvelope::empty() + }; + assert_eq!( + ExecutionPayloadEnvelope::::from_ssz_bytes(&envelope.as_ssz_bytes()) + .map(|decoded| assert_eq!(decoded, envelope)), + expected + ); + let signed_envelope = SignedExecutionPayloadEnvelope { + message: envelope, + signature: Signature::empty(), + }; + assert_eq!( + SignedExecutionPayloadEnvelope::::from_ssz_bytes(&signed_envelope.as_ssz_bytes()) + .map(|decoded| assert_eq!(decoded, signed_envelope)), + expected + ); + } + + #[test] + fn payload_transaction_list_length() { + let max = E::max_transactions_per_payload(); + let mut payload = ExecutionPayloadGloas { + transactions: std::iter::repeat_n(Default::default(), max).collect(), + ..ExecutionPayloadGloas::default() + }; + assert_payload_decoding_result(payload.clone(), Ok(())); + + payload.transactions.push(Default::default()); + assert_payload_decoding_result( + payload, + Err(ssz::DecodeError::BytesInvalid(format!( + "progressive list transactions has length {} > {max}", + max + 1, + ))), + ); + } + + #[test] + fn payload_withdrawal_list_length() { + let max = E::max_withdrawals_per_payload(); + let mut payload = ExecutionPayloadGloas { + withdrawals: std::iter::repeat_n(crate::test_utils::test_arbitrary_instance(), max) + .collect(), + ..ExecutionPayloadGloas::default() + }; + assert_payload_decoding_result(payload.clone(), Ok(())); + + payload.withdrawals.push(payload.withdrawals[0].clone()); + assert_payload_decoding_result( + payload, + Err(ssz::DecodeError::BytesInvalid(format!( + "progressive list withdrawals has length {} > {max}", + max + 1, + ))), + ); + } } diff --git a/consensus/types/src/execution/execution_requests.rs b/consensus/types/src/execution/execution_requests.rs index 642e6181d84..8bddac91776 100644 --- a/consensus/types/src/execution/execution_requests.rs +++ b/consensus/types/src/execution/execution_requests.rs @@ -141,12 +141,55 @@ impl ForkVersionDecode for ExecutionRequests { ExecutionRequestsElectra::from_ssz_bytes(bytes).map(Self::Electra) } ForkName::Gloas | ForkName::Heze => { - ExecutionRequestsGloas::from_ssz_bytes(bytes).map(Self::Gloas) + let requests = ExecutionRequestsGloas::from_ssz_bytes(bytes)?; + verify_execution_request_list_lengths_post_gloas(&requests)?; + Ok(Self::Gloas(requests)) } } } } +/// Verify the lengths of progressive execution request lists against the spec's runtime limits. +/// +/// [New in Gloas:EIP7688]: progressive lists have no type-level limits. Deposit requests are +/// deliberately unbounded (see the `deposit_requests_greater_than_electra_max` spec test). +pub fn verify_execution_request_list_lengths_post_gloas( + requests: &ExecutionRequestsGloas, +) -> Result<(), ssz::DecodeError> { + let checks = [ + ( + "withdrawal_requests", + requests.withdrawals.len(), + E::max_withdrawal_requests_per_payload(), + ), + ( + "consolidation_requests", + requests.consolidations.len(), + E::max_consolidation_requests_per_payload(), + ), + ( + "builder_deposit_requests", + requests.builder_deposits.len(), + E::max_builder_deposit_requests_per_payload(), + ), + ( + "builder_exit_requests", + requests.builder_exits.len(), + E::max_builder_exit_requests_per_payload(), + ), + ]; + + for (kind, length, max) in checks { + if length > max { + return Err(ssz::DecodeError::BytesInvalid(format!( + "progressive list {kind} has length {length} > {max}" + ))); + } + } + + Ok(()) +} + /// Build the EIP-7685 requests list from each request kind's `(request_type, is_empty, ssz_bytes)`. fn build_execution_requests_list(encoded: Vec<(RequestType, bool, Vec)>) -> Vec { encoded @@ -318,7 +361,113 @@ mod electra_tests { #[cfg(test)] mod gloas_tests { use super::*; - use crate::MainnetEthSpec; + use crate::{ExecutionPayloadEnvelope, MainnetEthSpec, SignedExecutionPayloadEnvelope}; + use bls::Signature; ssz_and_tree_hash_tests!(ExecutionRequestsGloas); + + type E = MainnetEthSpec; + + fn assert_decoding_result( + requests: ExecutionRequestsGloas, + expected: Result<(), ssz::DecodeError>, + ) { + assert_eq!( + verify_execution_request_list_lengths_post_gloas(&requests), + expected + ); + for fork in [ForkName::Gloas, ForkName::Heze] { + assert_eq!( + ExecutionRequests::::from_ssz_bytes_by_fork(&requests.as_ssz_bytes(), fork) + .map(|decoded| assert_eq!(decoded, ExecutionRequests::Gloas(requests.clone()))), + expected + ); + } + + let envelope = ExecutionPayloadEnvelope { + execution_requests: requests, + ..ExecutionPayloadEnvelope::empty() + }; + assert_eq!( + ExecutionPayloadEnvelope::::from_ssz_bytes(&envelope.as_ssz_bytes()) + .map(|decoded| assert_eq!(decoded, envelope)), + expected + ); + + let signed_envelope = SignedExecutionPayloadEnvelope { + message: envelope, + signature: Signature::empty(), + }; + assert_eq!( + SignedExecutionPayloadEnvelope::::from_ssz_bytes(&signed_envelope.as_ssz_bytes()) + .map(|decoded| assert_eq!(decoded, signed_envelope)), + expected + ); + } + + macro_rules! request_list_length_test { + ($test:ident, $field:ident, $limit:ident, $kind:literal) => { + #[test] + fn $test() { + let max = E::$limit(); + let request = crate::test_utils::test_arbitrary_instance(); + let mut requests = ExecutionRequestsGloas::default(); + requests.$field = std::iter::repeat_n(request, max).collect(); + assert_decoding_result(requests.clone(), Ok(())); + + requests.$field.push(requests.$field[0].clone()); + assert_decoding_result( + requests, + Err(ssz::DecodeError::BytesInvalid(format!( + "progressive list {} has length {} > {max}", + $kind, + max + 1, + ))), + ); + } + }; + } + + request_list_length_test!( + withdrawal_request_list_length, + withdrawals, + max_withdrawal_requests_per_payload, + "withdrawal_requests" + ); + request_list_length_test!( + consolidation_request_list_length, + consolidations, + max_consolidation_requests_per_payload, + "consolidation_requests" + ); + request_list_length_test!( + builder_deposit_request_list_length, + builder_deposits, + max_builder_deposit_requests_per_payload, + "builder_deposit_requests" + ); + request_list_length_test!( + builder_exit_request_list_length, + builder_exits, + max_builder_exit_requests_per_payload, + "builder_exit_requests" + ); + + #[test] + fn deposit_requests_are_unbounded() { + let requests = ExecutionRequestsGloas { + deposits: std::iter::repeat_n( + crate::test_utils::test_arbitrary_instance(), + E::max_deposit_requests_per_payload() + 1, + ) + .collect(), + ..ExecutionRequestsGloas::default() + }; + assert_decoding_result(requests, Ok(())); + } + + #[test] + fn empty_request_lists() { + assert_decoding_result(ExecutionRequestsGloas::default(), Ok(())); + } } diff --git a/consensus/types/src/execution/mod.rs b/consensus/types/src/execution/mod.rs index 008b7bfaa54..d6b4a830ebe 100644 --- a/consensus/types/src/execution/mod.rs +++ b/consensus/types/src/execution/mod.rs @@ -24,6 +24,7 @@ pub use execution_payload::{ ExecutionPayloadDeneb, ExecutionPayloadElectra, ExecutionPayloadFulu, ExecutionPayloadGloas, ExecutionPayloadHeze, ExecutionPayloadRef, ProgressiveTransactions, ProgressiveWithdrawals, Transaction, Transactions, TransactionsIter, TransactionsRef, WithdrawalsRef, + verify_execution_payload_list_lengths_post_gloas, }; pub use execution_payload_bid::ExecutionPayloadBid; pub use execution_payload_envelope::ExecutionPayloadEnvelope; @@ -39,7 +40,7 @@ pub use execution_proof::{ pub use execution_requests::{ BuilderDepositRequests, BuilderExitRequests, ConsolidationRequests, DepositRequests, ExecutionRequests, ExecutionRequestsElectra, ExecutionRequestsGloas, ExecutionRequestsRef, - RequestType, WithdrawalRequests, + RequestType, WithdrawalRequests, verify_execution_request_list_lengths_post_gloas, }; pub use inclusion_list::{InclusionList, InclusionListCommittee}; pub use payload::{