diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 31d52deae..9bcf8a0d6 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -985,11 +985,8 @@ impl BeaconChain { })? .ok_or(Error::BlockHashMissingFromExecutionLayer(exec_block_hash))?; - //FIXME(sean) avoid the clone by comparing refs to headers (`as_execution_payload_header` method ?) - let full_payload: FullPayload = execution_payload.clone().into(); - // Verify payload integrity. - let header_from_payload = full_payload.to_execution_payload_header(); + let header_from_payload = ExecutionPayloadHeader::from(execution_payload.to_ref()); if header_from_payload != execution_payload_header { for txn in execution_payload.transactions() { debug!( diff --git a/beacon_node/lighthouse_network/src/rpc/config.rs b/beacon_node/lighthouse_network/src/rpc/config.rs index 871fa644e..e89d45850 100644 --- a/beacon_node/lighthouse_network/src/rpc/config.rs +++ b/beacon_node/lighthouse_network/src/rpc/config.rs @@ -67,6 +67,7 @@ pub struct OutboundRateLimiterConfig { pub(super) goodbye_quota: Quota, pub(super) blocks_by_range_quota: Quota, pub(super) blocks_by_root_quota: Quota, + pub(super) blobs_by_range_quota: Quota, } impl OutboundRateLimiterConfig { @@ -77,6 +78,8 @@ impl OutboundRateLimiterConfig { pub const DEFAULT_BLOCKS_BY_RANGE_QUOTA: Quota = Quota::n_every(methods::MAX_REQUEST_BLOCKS, 10); pub const DEFAULT_BLOCKS_BY_ROOT_QUOTA: Quota = Quota::n_every(128, 10); + pub const DEFAULT_BLOBS_BY_RANGE_QUOTA: Quota = + Quota::n_every(methods::MAX_REQUEST_BLOBS_SIDECARS, 10); } impl Default for OutboundRateLimiterConfig { @@ -88,6 +91,7 @@ impl Default for OutboundRateLimiterConfig { goodbye_quota: Self::DEFAULT_GOODBYE_QUOTA, blocks_by_range_quota: Self::DEFAULT_BLOCKS_BY_RANGE_QUOTA, blocks_by_root_quota: Self::DEFAULT_BLOCKS_BY_ROOT_QUOTA, + blobs_by_range_quota: Self::DEFAULT_BLOBS_BY_RANGE_QUOTA, } } } @@ -111,6 +115,7 @@ impl Debug for OutboundRateLimiterConfig { .field("goodbye", fmt_q!(&self.goodbye_quota)) .field("blocks_by_range", fmt_q!(&self.blocks_by_range_quota)) .field("blocks_by_root", fmt_q!(&self.blocks_by_root_quota)) + .field("blobs_by_range", fmt_q!(&self.blobs_by_range_quota)) .finish() } } @@ -129,7 +134,6 @@ impl FromStr for OutboundRateLimiterConfig { let mut goodbye_quota = None; let mut blocks_by_range_quota = None; let mut blocks_by_root_quota = None; - // TODO(eip4844): use this blob quota let mut blobs_by_range_quota = None; for proto_def in s.split(';') { let ProtocolQuota { protocol, quota } = proto_def.parse()?; @@ -154,6 +158,8 @@ impl FromStr for OutboundRateLimiterConfig { .unwrap_or(Self::DEFAULT_BLOCKS_BY_RANGE_QUOTA), blocks_by_root_quota: blocks_by_root_quota .unwrap_or(Self::DEFAULT_BLOCKS_BY_ROOT_QUOTA), + blobs_by_range_quota: blobs_by_range_quota + .unwrap_or(Self::DEFAULT_BLOBS_BY_RANGE_QUOTA), }) } } diff --git a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs index 451c6206f..61e9b46a9 100644 --- a/beacon_node/lighthouse_network/src/rpc/self_limiter.rs +++ b/beacon_node/lighthouse_network/src/rpc/self_limiter.rs @@ -60,6 +60,7 @@ impl SelfRateLimiter { goodbye_quota, blocks_by_range_quota, blocks_by_root_quota, + blobs_by_range_quota, } = config; let limiter = RateLimiter::builder() @@ -69,6 +70,7 @@ impl SelfRateLimiter { .set_quota(Protocol::Goodbye, goodbye_quota) .set_quota(Protocol::BlocksByRange, blocks_by_range_quota) .set_quota(Protocol::BlocksByRoot, blocks_by_root_quota) + .set_quota(Protocol::BlobsByRange, blobs_by_range_quota) // Manually set the LightClientBootstrap quota, since we use the same rate limiter for // inbound and outbound requests, and the LightClientBootstrap is an only inbound // protocol. diff --git a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs index bbf2c1caa..709302eec 100644 --- a/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs +++ b/consensus/state_processing/src/per_block_processing/block_signature_verifier.rs @@ -348,8 +348,7 @@ where &mut self, block: &'a SignedBeaconBlock, ) -> Result<()> { - // FIXME(capella): to improve performance we might want to decompress the withdrawal pubkeys - // in parallel. + // To improve performance we might want to decompress the withdrawal pubkeys in parallel. if let Ok(bls_to_execution_changes) = block.message().body().bls_to_execution_changes() { for bls_to_execution_change in bls_to_execution_changes { self.sets.push(bls_execution_change_signature_set( diff --git a/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs b/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs index 34700a33e..15a856c40 100644 --- a/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs +++ b/consensus/state_processing/src/per_block_processing/verify_bls_to_execution_change.rs @@ -37,10 +37,9 @@ pub fn verify_bls_to_execution_change( Invalid::NonBlsWithdrawalCredentials ); + // Re-hashing the pubkey isn't necessary during block replay, so we may want to skip that in + // future. let pubkey_hash = hash(address_change.from_bls_pubkey.as_serialized()); - - // FIXME: Should this check be put inside the verify_signatures.is_true() condition? - // I believe that's used for fuzzing so this is a Mehdi question.. verify!( validator.withdrawal_credentials.as_bytes().get(1..) == pubkey_hash.get(1..), Invalid::WithdrawalCredentialsMismatch diff --git a/consensus/types/src/beacon_block_body.rs b/consensus/types/src/beacon_block_body.rs index 28c9213d1..07c8f898b 100644 --- a/consensus/types/src/beacon_block_body.rs +++ b/consensus/types/src/beacon_block_body.rs @@ -279,7 +279,7 @@ impl From>> voluntary_exits, sync_aggregate, execution_payload: BlindedPayloadMerge { - execution_payload_header: From::from(execution_payload.clone()), + execution_payload_header: From::from(&execution_payload), }, }, Some(execution_payload), @@ -320,7 +320,7 @@ impl From>> voluntary_exits, sync_aggregate, execution_payload: BlindedPayloadCapella { - execution_payload_header: From::from(execution_payload.clone()), + execution_payload_header: From::from(&execution_payload), }, bls_to_execution_changes, }, @@ -363,7 +363,7 @@ impl From>> voluntary_exits, sync_aggregate, execution_payload: BlindedPayloadEip4844 { - execution_payload_header: From::from(execution_payload.clone()), + execution_payload_header: From::from(&execution_payload), }, bls_to_execution_changes, blob_kzg_commitments, @@ -414,7 +414,7 @@ impl BeaconBlockBodyMerge> { voluntary_exits: voluntary_exits.clone(), sync_aggregate: sync_aggregate.clone(), execution_payload: BlindedPayloadMerge { - execution_payload_header: From::from(execution_payload.clone()), + execution_payload_header: execution_payload.into(), }, } } @@ -447,7 +447,7 @@ impl BeaconBlockBodyCapella> { voluntary_exits: voluntary_exits.clone(), sync_aggregate: sync_aggregate.clone(), execution_payload: BlindedPayloadCapella { - execution_payload_header: From::from(execution_payload.clone()), + execution_payload_header: execution_payload.into(), }, bls_to_execution_changes: bls_to_execution_changes.clone(), } @@ -482,7 +482,7 @@ impl BeaconBlockBodyEip4844> { voluntary_exits: voluntary_exits.clone(), sync_aggregate: sync_aggregate.clone(), execution_payload: BlindedPayloadEip4844 { - execution_payload_header: From::from(execution_payload.clone()), + execution_payload_header: execution_payload.into(), }, bls_to_execution_changes: bls_to_execution_changes.clone(), blob_kzg_commitments: blob_kzg_commitments.clone(), diff --git a/consensus/types/src/execution_payload.rs b/consensus/types/src/execution_payload.rs index 16b277835..6e055d0a7 100644 --- a/consensus/types/src/execution_payload.rs +++ b/consensus/types/src/execution_payload.rs @@ -35,7 +35,9 @@ pub type Withdrawals = VariableList::MaxWithdrawal arbitrary(bound = "T: EthSpec") ), cast_error(ty = "Error", expr = "BeaconStateError::IncorrectStateVariant"), - partial_getter_error(ty = "Error", expr = "BeaconStateError::IncorrectStateVariant") + partial_getter_error(ty = "Error", expr = "BeaconStateError::IncorrectStateVariant"), + map_into(FullPayload, BlindedPayload), + map_ref_into(ExecutionPayloadHeader) )] #[derive( Debug, Clone, Serialize, Encode, Deserialize, TreeHash, Derivative, arbitrary::Arbitrary, diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 695c0cfdf..4dc79ddc9 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -159,40 +159,40 @@ impl ExecutionPayloadHeaderCapella { } } -impl From> for ExecutionPayloadHeaderMerge { - fn from(payload: ExecutionPayloadMerge) -> Self { +impl<'a, T: EthSpec> From<&'a ExecutionPayloadMerge> for ExecutionPayloadHeaderMerge { + fn from(payload: &'a ExecutionPayloadMerge) -> Self { Self { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, state_root: payload.state_root, receipts_root: payload.receipts_root, - logs_bloom: payload.logs_bloom, + logs_bloom: payload.logs_bloom.clone(), prev_randao: payload.prev_randao, block_number: payload.block_number, gas_limit: payload.gas_limit, gas_used: payload.gas_used, timestamp: payload.timestamp, - extra_data: payload.extra_data, + extra_data: payload.extra_data.clone(), base_fee_per_gas: payload.base_fee_per_gas, block_hash: payload.block_hash, transactions_root: payload.transactions.tree_hash_root(), } } } -impl From> for ExecutionPayloadHeaderCapella { - fn from(payload: ExecutionPayloadCapella) -> Self { +impl<'a, T: EthSpec> From<&'a ExecutionPayloadCapella> for ExecutionPayloadHeaderCapella { + fn from(payload: &'a ExecutionPayloadCapella) -> Self { Self { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, state_root: payload.state_root, receipts_root: payload.receipts_root, - logs_bloom: payload.logs_bloom, + logs_bloom: payload.logs_bloom.clone(), prev_randao: payload.prev_randao, block_number: payload.block_number, gas_limit: payload.gas_limit, gas_used: payload.gas_used, timestamp: payload.timestamp, - extra_data: payload.extra_data, + extra_data: payload.extra_data.clone(), base_fee_per_gas: payload.base_fee_per_gas, block_hash: payload.block_hash, transactions_root: payload.transactions.tree_hash_root(), @@ -200,20 +200,21 @@ impl From> for ExecutionPayloadHeaderCape } } } -impl From> for ExecutionPayloadHeaderEip4844 { - fn from(payload: ExecutionPayloadEip4844) -> Self { + +impl<'a, T: EthSpec> From<&'a ExecutionPayloadEip4844> for ExecutionPayloadHeaderEip4844 { + fn from(payload: &'a ExecutionPayloadEip4844) -> Self { Self { parent_hash: payload.parent_hash, fee_recipient: payload.fee_recipient, state_root: payload.state_root, receipts_root: payload.receipts_root, - logs_bloom: payload.logs_bloom, + logs_bloom: payload.logs_bloom.clone(), prev_randao: payload.prev_randao, block_number: payload.block_number, gas_limit: payload.gas_limit, gas_used: payload.gas_used, timestamp: payload.timestamp, - extra_data: payload.extra_data, + extra_data: payload.extra_data.clone(), base_fee_per_gas: payload.base_fee_per_gas, excess_data_gas: payload.excess_data_gas, block_hash: payload.block_hash, @@ -223,31 +224,33 @@ impl From> for ExecutionPayloadHeaderEip4 } } -impl From> for ExecutionPayloadHeader { - fn from(payload: ExecutionPayloadMerge) -> Self { - Self::Merge(ExecutionPayloadHeaderMerge::from(payload)) +// These impls are required to work around an inelegance in `to_execution_payload_header`. +// They only clone headers so they should be relatively cheap. +impl<'a, T: EthSpec> From<&'a Self> for ExecutionPayloadHeaderMerge { + fn from(payload: &'a Self) -> Self { + payload.clone() } } -impl From> for ExecutionPayloadHeader { - fn from(payload: ExecutionPayloadCapella) -> Self { - Self::Capella(ExecutionPayloadHeaderCapella::from(payload)) +impl<'a, T: EthSpec> From<&'a Self> for ExecutionPayloadHeaderCapella { + fn from(payload: &'a Self) -> Self { + payload.clone() } } -impl From> for ExecutionPayloadHeader { - fn from(payload: ExecutionPayloadEip4844) -> Self { - Self::Eip4844(ExecutionPayloadHeaderEip4844::from(payload)) +impl<'a, T: EthSpec> From<&'a Self> for ExecutionPayloadHeaderEip4844 { + fn from(payload: &'a Self) -> Self { + payload.clone() } } -impl From> for ExecutionPayloadHeader { - fn from(payload: ExecutionPayload) -> Self { - match payload { - ExecutionPayload::Merge(payload) => Self::from(payload), - ExecutionPayload::Capella(payload) => Self::from(payload), - ExecutionPayload::Eip4844(payload) => Self::from(payload), - } +impl<'a, T: EthSpec> From> for ExecutionPayloadHeader { + fn from(payload: ExecutionPayloadRef<'a, T>) -> Self { + map_execution_payload_ref_into_execution_payload_header!( + &'a _, + payload, + |inner, cons| cons(inner.into()) + ) } } diff --git a/consensus/types/src/payload.rs b/consensus/types/src/payload.rs index 9b7d3417f..cc22bc3ab 100644 --- a/consensus/types/src/payload.rs +++ b/consensus/types/src/payload.rs @@ -4,6 +4,7 @@ use serde::de::DeserializeOwned; use serde::{Deserialize, Serialize}; use ssz::{Decode, Encode}; use ssz_derive::{Decode, Encode}; +use std::borrow::Cow; use std::convert::TryFrom; use std::fmt::Debug; use std::hash::Hash; @@ -90,15 +91,15 @@ pub trait AbstractExecPayload: type Merge: OwnedExecPayload + Into - + From> + + for<'a> From>> + TryFrom>; type Capella: OwnedExecPayload + Into - + From> + + for<'a> From>> + TryFrom>; type Eip4844: OwnedExecPayload + Into - + From> + + for<'a> From>> + TryFrom>; fn default_at_fork(fork_name: ForkName) -> Result; @@ -150,31 +151,21 @@ pub struct FullPayload { impl From> for ExecutionPayload { fn from(full_payload: FullPayload) -> Self { - match full_payload { - FullPayload::Merge(payload) => ExecutionPayload::Merge(payload.execution_payload), - FullPayload::Capella(payload) => ExecutionPayload::Capella(payload.execution_payload), - FullPayload::Eip4844(payload) => ExecutionPayload::Eip4844(payload.execution_payload), - } + map_full_payload_into_execution_payload!(full_payload, move |payload, cons| { + cons(payload.execution_payload) + }) } } impl<'a, T: EthSpec> From> for ExecutionPayload { fn from(full_payload_ref: FullPayloadRef<'a, T>) -> Self { - match full_payload_ref { - FullPayloadRef::Merge(payload) => { - ExecutionPayload::Merge(payload.execution_payload.clone()) - } - FullPayloadRef::Capella(payload) => { - ExecutionPayload::Capella(payload.execution_payload.clone()) - } - FullPayloadRef::Eip4844(payload) => { - ExecutionPayload::Eip4844(payload.execution_payload.clone()) - } - } + map_full_payload_ref!(&'a _, full_payload_ref, move |payload, cons| { + cons(payload); + payload.execution_payload.clone().into() + }) } } -// FIXME: can this be implemented as Deref or Clone somehow? impl<'a, T: EthSpec> From> for FullPayload { fn from(full_payload_ref: FullPayloadRef<'a, T>) -> Self { map_full_payload_ref!(&'a _, full_payload_ref, move |payload, cons| { @@ -189,11 +180,12 @@ impl ExecPayload for FullPayload { BlockType::Full } - fn to_execution_payload_header(&self) -> ExecutionPayloadHeader { - let payload = map_full_payload_into_execution_payload!(self.clone(), |inner, cons| { - cons(inner.execution_payload) - }); - ExecutionPayloadHeader::from(payload) + fn to_execution_payload_header<'a>(&'a self) -> ExecutionPayloadHeader { + map_full_payload_ref!(&'a _, self.to_ref(), move |inner, cons| { + cons(inner); + let exec_payload_ref: ExecutionPayloadRef<'a, T> = From::from(&inner.execution_payload); + ExecutionPayloadHeader::from(exec_payload_ref) + }) } fn parent_hash<'a>(&'a self) -> ExecutionBlockHash { @@ -404,17 +396,9 @@ impl AbstractExecPayload for FullPayload { impl From> for FullPayload { fn from(execution_payload: ExecutionPayload) -> Self { - match execution_payload { - ExecutionPayload::Merge(execution_payload) => { - Self::Merge(FullPayloadMerge { execution_payload }) - } - ExecutionPayload::Capella(execution_payload) => { - Self::Capella(FullPayloadCapella { execution_payload }) - } - ExecutionPayload::Eip4844(execution_payload) => { - Self::Eip4844(FullPayloadEip4844 { execution_payload }) - } - } + map_execution_payload_into_full_payload!(execution_payload, |inner, cons| { + cons(inner.into()) + }) } } @@ -666,6 +650,7 @@ macro_rules! impl_exec_payload_common { $wrapped_field:ident, // execution_payload_header | execution_payload $fork_variant:ident, // Merge | Merge $block_type_variant:ident, // Blinded | Full + $is_default_with_empty_roots:block, $f:block, $g:block) => { impl ExecPayload for $wrapper_type { @@ -675,7 +660,7 @@ macro_rules! impl_exec_payload_common { fn to_execution_payload_header(&self) -> ExecutionPayloadHeader { ExecutionPayloadHeader::$fork_variant($wrapped_type_header::from( - self.$wrapped_field.clone(), + &self.$wrapped_field, )) } @@ -712,15 +697,8 @@ macro_rules! impl_exec_payload_common { } fn is_default_with_empty_roots(&self) -> bool { - // FIXME: is there a better way than ignoring this lint? - // This is necessary because the first invocation of this macro might expand to: - // self.execution_payload_header == ExecutionPayloadHeaderMerge::from(ExecutionPayloadMerge::default()) - // but the second invocation might expand to: - // self.execution_payload == ExecutionPayloadMerge::from(ExecutionPayloadMerge::default()) - #[allow(clippy::cmp_owned)] - { - self.$wrapped_field == $wrapped_type::from($wrapped_type_full::default()) - } + let f = $is_default_with_empty_roots; + f(self) } fn transactions(&self) -> Option<&Transactions> { @@ -755,6 +733,12 @@ macro_rules! impl_exec_payload_for_fork { execution_payload_header, $fork_variant, // Merge Blinded, + { + |wrapper: &$wrapper_type_header| { + wrapper.execution_payload_header + == $wrapped_type_header::from(&$wrapped_type_full::default()) + } + }, { |_| { None } }, { let c: for<'a> fn(&'a $wrapper_type_header) -> Result = @@ -788,7 +772,7 @@ macro_rules! impl_exec_payload_for_fork { fn default() -> Self { Self { execution_payload_header: $wrapped_type_header::from( - $wrapped_type_full::default(), + &$wrapped_type_full::default(), ), } } @@ -806,11 +790,11 @@ macro_rules! impl_exec_payload_for_fork { } } - // FIXME(sproul): consider adding references to these From impls - impl From<$wrapped_type_full> for $wrapper_type_header { - fn from(execution_payload: $wrapped_type_full) -> Self { + // BlindedPayload* from CoW reference to ExecutionPayload* (hopefully just a reference). + impl<'a, T: EthSpec> From>> for $wrapper_type_header { + fn from(execution_payload: Cow<'a, $wrapped_type_full>) -> Self { Self { - execution_payload_header: $wrapped_type_header::from(execution_payload), + execution_payload_header: $wrapped_type_header::from(&*execution_payload), } } } @@ -825,6 +809,11 @@ macro_rules! impl_exec_payload_for_fork { execution_payload, $fork_variant, // Merge Full, + { + |wrapper: &$wrapper_type_full| { + wrapper.execution_payload == $wrapped_type_full::default() + } + }, { let c: for<'a> fn(&'a $wrapper_type_full) -> Option<&'a Transactions> = |payload: &$wrapper_type_full| Some(&payload.execution_payload.transactions); @@ -848,6 +837,15 @@ macro_rules! impl_exec_payload_for_fork { } } + // FullPayload * from CoW reference to ExecutionPayload* (hopefully already owned). + impl<'a, T: EthSpec> From>> for $wrapper_type_full { + fn from(execution_payload: Cow<'a, $wrapped_type_full>) -> Self { + Self { + execution_payload: $wrapped_type_full::from(execution_payload.into_owned()), + } + } + } + impl TryFrom> for $wrapper_type_full { type Error = Error; fn try_from(_: ExecutionPayloadHeader) -> Result { @@ -915,11 +913,12 @@ impl AbstractExecPayload for BlindedPayload { impl From> for BlindedPayload { fn from(payload: ExecutionPayload) -> Self { - match payload { - ExecutionPayload::Merge(payload) => BlindedPayload::Merge(payload.into()), - ExecutionPayload::Capella(payload) => BlindedPayload::Capella(payload.into()), - ExecutionPayload::Eip4844(payload) => BlindedPayload::Eip4844(payload.into()), - } + // This implementation is a bit wasteful in that it discards the payload body. + // Required by the top-level constraint on AbstractExecPayload but could maybe be loosened + // in future. + map_execution_payload_into_blinded_payload!(payload, |inner, cons| cons(From::from( + Cow::Owned(inner) + ))) } }