diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index cfb4eafd5..32a0e5ac5 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4157,38 +4157,30 @@ impl BeaconChain { return Ok(()); } - #[cfg(feature = "withdrawals")] - let head_state = &self.canonical_head.cached_head().snapshot.beacon_state; #[cfg(feature = "withdrawals")] let withdrawals = match self.spec.fork_name_at_epoch(prepare_epoch) { - ForkName::Base | ForkName::Altair | ForkName::Merge => { - None - }, - ForkName::Capella | ForkName::Eip4844 => match &head_state { - &BeaconState::Capella(_) | &BeaconState::Eip4844(_) => { - // The head_state is already BeaconState::Capella or later - // FIXME(mark) - // Might implement caching here in the future.. - Some(get_expected_withdrawals(head_state, &self.spec)) - } - &BeaconState::Base(_) | &BeaconState::Altair(_) | &BeaconState::Merge(_) => { - // We are the Capella transition block proposer, need advanced state - let mut prepare_state = self - .state_at_slot(prepare_slot, StateSkipConfig::WithoutStateRoots) - .or_else(|e| { - error!(self.log, "Capella Transition Proposer"; "Error Advancing State: " => ?e); - Err(e) - })?; - // FIXME(mark) - // Might implement caching here in the future.. - Some(get_expected_withdrawals(&prepare_state, &self.spec)) - } - }, - }.transpose().or_else(|e| { - error!(self.log, "Error preparing beacon proposer"; "while calculating expected withdrawals" => ?e); + ForkName::Base | ForkName::Altair | ForkName::Merge => None, + ForkName::Capella | ForkName::Eip4844 => { + // We must use the advanced state because balances can change at epoch boundaries + // and balances affect withdrawals. + // FIXME(mark) + // Might implement caching here in the future.. + let prepare_state = self + .state_at_slot(prepare_slot, StateSkipConfig::WithoutStateRoots) + .or_else(|e| { + error!(self.log, "State advance for withdrawals failed"; "error" => ?e); + Err(e) + })?; + Some(get_expected_withdrawals(&prepare_state, &self.spec)) + } + } + .transpose() + .or_else(|e| { + error!(self.log, "Error preparing beacon proposer"; "error" => ?e); Err(e) - }).map(|withdrawals_opt| withdrawals_opt.map(|w| w.into())) - .map_err(Error::PrepareProposerFailed)?; + }) + .map(|withdrawals_opt| withdrawals_opt.map(|w| w.into())) + .map_err(Error::PrepareProposerFailed)?; let payload_attributes = PayloadAttributes::V2(PayloadAttributesV2 { timestamp: self diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index bf920a6da..85aedc659 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -310,7 +310,7 @@ pub fn validate_execution_payload_for_gossip( } }; - if is_merge_transition_complete || !execution_payload.is_default() { + if is_merge_transition_complete || !execution_payload.is_default_with_empty_roots() { let expected_timestamp = chain .slot_clock .start_of(block.slot()) diff --git a/consensus/state_processing/src/per_block_processing.rs b/consensus/state_processing/src/per_block_processing.rs index 40fdcdd19..8fa964ffc 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -431,9 +431,11 @@ pub fn process_execution_payload<'payload, T: EthSpec, Payload: AbstractExecPayl /// repeaetedly write code to treat these errors as false. /// https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#is_merge_transition_complete pub fn is_merge_transition_complete(state: &BeaconState) -> bool { + // We must check defaultness against the payload header with 0x0 roots, as that's what's meant + // by `ExecutionPayloadHeader()` in the spec. state .latest_execution_payload_header() - .map(|header| !header.is_default()) + .map(|header| !header.is_default_with_zero_roots()) .unwrap_or(false) } /// https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#is_merge_transition_block @@ -441,8 +443,12 @@ pub fn is_merge_transition_block>( state: &BeaconState, body: BeaconBlockBodyRef, ) -> bool { + // For execution payloads in blocks (which may be headers) we must check defaultness against + // the payload with `transactions_root` equal to the tree hash of the empty list. body.execution_payload() - .map(|payload| !is_merge_transition_complete(state) && !payload.is_default()) + .map(|payload| { + !is_merge_transition_complete(state) && !payload.is_default_with_empty_roots() + }) .unwrap_or(false) } /// https://github.com/ethereum/consensus-specs/blob/dev/specs/bellatrix/beacon-chain.md#is_execution_enabled diff --git a/consensus/types/src/execution_payload_header.rs b/consensus/types/src/execution_payload_header.rs index 7c4d9af1a..a9708153c 100644 --- a/consensus/types/src/execution_payload_header.rs +++ b/consensus/types/src/execution_payload_header.rs @@ -103,9 +103,9 @@ impl ExecutionPayloadHeader { } impl<'a, T: EthSpec> ExecutionPayloadHeaderRef<'a, T> { - pub fn is_default(self) -> bool { + pub fn is_default_with_zero_roots(self) -> bool { map_execution_payload_header_ref!(&'a _, self, |inner, cons| { - let _ = cons(inner); + cons(inner); *inner == Default::default() }) } diff --git a/consensus/types/src/payload.rs b/consensus/types/src/payload.rs index 3081dd1cb..2507a9f0e 100644 --- a/consensus/types/src/payload.rs +++ b/consensus/types/src/payload.rs @@ -40,8 +40,11 @@ pub trait ExecPayload: Debug + Clone + PartialEq + Hash + TreeHash + #[cfg(feature = "withdrawals")] fn withdrawals_root(&self) -> Result; - /// Is this a default payload? (pre-merge) - fn is_default(&self) -> bool; + /// Is this a default payload with 0x0 roots for transactions and withdrawals? + fn is_default_with_zero_roots(&self) -> bool; + + /// Is this a default payload with the hash of the empty list for transactions and withdrawals? + fn is_default_with_empty_roots(&self) -> bool; } /// `ExecPayload` functionality the requires ownership. @@ -241,12 +244,17 @@ impl ExecPayload for FullPayload { } } - fn is_default<'a>(&'a self) -> bool { + fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_full_payload_ref!(&'a _, self.to_ref(), move |payload, cons| { cons(payload); payload.execution_payload == <_>::default() }) } + + fn is_default_with_empty_roots<'a>(&'a self) -> bool { + // For full payloads the empty/zero distinction does not exist. + self.is_default_with_zero_roots() + } } impl FullPayload { @@ -338,13 +346,17 @@ impl<'b, T: EthSpec> ExecPayload for FullPayloadRef<'b, T> { } } - // TODO: can this function be optimized? - fn is_default<'a>(&'a self) -> bool { + fn is_default_with_zero_roots<'a>(&'a self) -> bool { map_full_payload_ref!(&'a _, self, move |payload, cons| { cons(payload); payload.execution_payload == <_>::default() }) } + + fn is_default_with_empty_roots(&self) -> bool { + // For full payloads the empty/zero distinction does not exist. + self.is_default_with_zero_roots() + } } impl AbstractExecPayload for FullPayload { @@ -505,11 +517,16 @@ impl ExecPayload for BlindedPayload { } } - fn is_default<'a>(&'a self) -> bool { - map_blinded_payload_ref!(&'a _, self.to_ref(), move |payload, cons| { - cons(payload); - payload.execution_payload_header == <_>::default() - }) + fn is_default_with_zero_roots<'a>(&'a self) -> bool { + self.to_ref().is_default_with_zero_roots() + } + + // For blinded payloads we must check "defaultness" against the default `ExecutionPayload` + // which has been blinded into an `ExecutionPayloadHeader`, NOT against the default + // `ExecutionPayloadHeader` which has a zeroed out `transactions_root`. The transactions root + // should be the root of the empty list. + fn is_default_with_empty_roots(&self) -> bool { + self.to_ref().is_default_with_empty_roots() } } @@ -591,24 +608,38 @@ impl<'b, T: EthSpec> ExecPayload for BlindedPayloadRef<'b, T> { } } - // TODO: can this function be optimized? - fn is_default<'a>(&'a self) -> bool { - map_blinded_payload_ref!(&'a _, self, move |payload, cons| { + fn is_default_with_zero_roots<'a>(&'a self) -> bool { + map_blinded_payload_ref!(&'b _, self, move |payload, cons| { cons(payload); payload.execution_payload_header == <_>::default() }) } + + fn is_default_with_empty_roots<'a>(&'a self) -> bool { + map_blinded_payload_ref!(&'b _, self, move |payload, cons| { + cons(payload); + payload.is_default_with_empty_roots() + }) + } } macro_rules! impl_exec_payload_common { - ($wrapper_type:ident, $wrapped_type_full:ident, $wrapped_header_type:ident, $wrapped_field:ident, $fork_variant:ident, $block_type_variant:ident, $f:block, $g:block) => { + ($wrapper_type:ident, + $wrapped_type:ident, + $wrapped_type_full:ident, + $wrapped_type_header:ident, + $wrapped_field:ident, + $fork_variant:ident, + $block_type_variant:ident, + $f:block, + $g:block) => { impl ExecPayload for $wrapper_type { fn block_type() -> BlockType { BlockType::$block_type_variant } fn to_execution_payload_header(&self) -> ExecutionPayloadHeader { - ExecutionPayloadHeader::$fork_variant($wrapped_header_type::from( + ExecutionPayloadHeader::$fork_variant($wrapped_type_header::from( self.$wrapped_field.clone(), )) } @@ -641,8 +672,12 @@ macro_rules! impl_exec_payload_common { self.$wrapped_field.gas_limit } - fn is_default(&self) -> bool { - self.$wrapped_field == $wrapped_type_full::default() + fn is_default_with_zero_roots(&self) -> bool { + self.$wrapped_field == $wrapped_type::default() + } + + fn is_default_with_empty_roots(&self) -> bool { + self.$wrapped_field == $wrapped_type::from($wrapped_type_full::default()) } fn transactions(&self) -> Option<&Transactions> { @@ -657,8 +692,8 @@ macro_rules! impl_exec_payload_common { } } - impl From<$wrapped_type_full> for $wrapper_type { - fn from($wrapped_field: $wrapped_type_full) -> Self { + impl From<$wrapped_type> for $wrapper_type { + fn from($wrapped_field: $wrapped_type) -> Self { Self { $wrapped_field } } } @@ -672,6 +707,7 @@ macro_rules! impl_exec_payload_for_fork { impl_exec_payload_common!( $wrapper_type_header, $wrapped_type_header, + $wrapped_type_full, $wrapped_type_header, execution_payload_header, $fork_variant, @@ -741,6 +777,7 @@ macro_rules! impl_exec_payload_for_fork { impl_exec_payload_common!( $wrapper_type_full, $wrapped_type_full, + $wrapped_type_full, $wrapped_type_header, execution_payload, $fork_variant,