Two Capella bugfixes (#3749)

* Two Capella bugfixes

* fix payload default check in fork choice

* Revert "fix payload default check in fork choice"

This reverts commit e56fefbd05.

Co-authored-by: realbigsean <sean@sigmaprime.io>

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -4112,38 +4112,30 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             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

beacon_node/beacon_chain/src/execution_payload.rs

@@ -310,7 +310,7 @@ pub fn validate_execution_payload_for_gossip<T: BeaconChainTypes>(
             }
         };
 
-        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())

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<T: EthSpec>(state: &BeaconState<T>) -> 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<T: EthSpec, Payload: AbstractExecPayload<T>>(
     state: &BeaconState<T>,
     body: BeaconBlockBodyRef<T, Payload>,
 ) -> 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

consensus/types/src/execution_payload_header.rs

@@ -103,9 +103,9 @@ impl<T: EthSpec> ExecutionPayloadHeader<T> {
 }
 
 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()
         })
     }

consensus/types/src/payload.rs

@@ -40,8 +40,11 @@ pub trait ExecPayload<T: EthSpec>: Debug + Clone + PartialEq + Hash + TreeHash +
     #[cfg(feature = "withdrawals")]
     fn withdrawals_root(&self) -> Result<Hash256, Error>;
 
-    /// 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<T: EthSpec> ExecPayload<T> for FullPayload<T> {
         }
     }
 
-    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<T: EthSpec> FullPayload<T> {
@@ -338,13 +346,17 @@ impl<'b, T: EthSpec> ExecPayload<T> 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<T: EthSpec> AbstractExecPayload<T> for FullPayload<T> {
@@ -505,11 +517,16 @@ impl<T: EthSpec> ExecPayload<T> for BlindedPayload<T> {
         }
     }
 
-    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<T> 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<T: EthSpec> ExecPayload<T> for $wrapper_type<T> {
             fn block_type() -> BlockType {
                 BlockType::$block_type_variant
             }
 
             fn to_execution_payload_header(&self) -> ExecutionPayloadHeader<T> {
-                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<T>> {
@@ -657,8 +692,8 @@ macro_rules! impl_exec_payload_common {
             }
         }
 
-        impl<T: EthSpec> From<$wrapped_type_full<T>> for $wrapper_type<T> {
-            fn from($wrapped_field: $wrapped_type_full<T>) -> Self {
+        impl<T: EthSpec> From<$wrapped_type<T>> for $wrapper_type<T> {
+            fn from($wrapped_field: $wrapped_type<T>) -> 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,