From 53a22c2fcb9c6208c25dfda5cf26d0cff0a0a0e1 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 23 Nov 2022 18:37:25 +1100 Subject: [PATCH 1/3] Two Capella bugfixes --- beacon_node/beacon_chain/src/beacon_chain.rs | 50 ++++++------- .../beacon_chain/src/execution_payload.rs | 2 +- .../src/per_block_processing.rs | 10 ++- .../types/src/execution_payload_header.rs | 4 +- consensus/types/src/payload.rs | 75 ++++++++++++++----- 5 files changed, 88 insertions(+), 53 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c243d50cb..89ccd96b1 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4112,38 +4112,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 753a19398..d1c4cf12a 100644 --- a/consensus/state_processing/src/per_block_processing.rs +++ b/consensus/state_processing/src/per_block_processing.rs @@ -428,9 +428,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 @@ -438,8 +440,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 6f6b5aa95..37547614d 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, From e56fefbd05811526af4499711045275db366aa09 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 23 Nov 2022 09:42:55 -0500 Subject: [PATCH 2/3] fix payload default check in fork choice --- consensus/fork_choice/src/fork_choice.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 5f4c9931f..88a749b0a 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -402,7 +402,7 @@ where |()| ExecutionStatus::irrelevant(), |message| { let execution_payload = &message.body.execution_payload; - if execution_payload == &<_>::default() { + if execution_payload.is_default_with_zero_roots() { // A default payload does not have execution enabled. ExecutionStatus::irrelevant() } else { From 743347cf047c854b18f0cea1ea872077ac6b92d6 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Wed, 23 Nov 2022 11:18:47 -0500 Subject: [PATCH 3/3] Revert "fix payload default check in fork choice" This reverts commit e56fefbd05811526af4499711045275db366aa09. --- consensus/fork_choice/src/fork_choice.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 88a749b0a..5f4c9931f 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -402,7 +402,7 @@ where |()| ExecutionStatus::irrelevant(), |message| { let execution_payload = &message.body.execution_payload; - if execution_payload.is_default_with_zero_roots() { + if execution_payload == &<_>::default() { // A default payload does not have execution enabled. ExecutionStatus::irrelevant() } else {