Fix post-merge checkpoint sync (#3065)

## Issue Addressed

This address an issue which was preventing checkpoint-sync.

When the node starts from checkpoint sync, the head block and the finalized block are the same value. We did not respect this when sending a `forkchoiceUpdated` (fcU) call to the EL and were expecting fork choice to hold the *finalized ancestor of the head* and returning an error when it didn't.

This PR uses *only fork choice* for sending fcU updates. This is actually quite nice and avoids some atomicity issues between `chain.canonical_head` and `chain.fork_choice`. Now, whenever `chain.fork_choice.get_head` returns a value we also cache the values required for the next fcU call.

## TODO

- [x] ~~Blocked on #3043~~
- [x] Ensure there isn't a warn message at startup.

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -3952,56 +3952,45 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         // of the head. I.e., we will never communicate a past head after communicating a later
         // one.
         //
-        // There are two "deadlock warnings" in this function. The downside of this nice ordering
+        // There is a "deadlock warning" in this function. The downside of this nice ordering is the
-        // is the potential for deadlock. I would advise against any other use of
+        // potential for deadlock. I would advise against any other use of
         // `execution_engine_forkchoice_lock` apart from the one here.
         let forkchoice_lock = execution_layer.execution_engine_forkchoice_lock().await;
 
         // Deadlock warning:
         //
-        // We are taking the `self.canonical_head` lock whilst holding the `forkchoice_lock`. This
+        // We are taking the `self.fork_choice` lock whilst holding the `forkchoice_lock`. This
         // is intentional, since it allows us to ensure a consistent ordering of messages to the
         // execution layer.
-        let head = self.head_info()?;
+        let (head_block_root, head_hash, finalized_hash) =
-        let head_block_root = head.block_root;
+            if let Some(params) = self.fork_choice.read().get_forkchoice_update_parameters() {
-        let head_execution_block_hash = if let Some(hash) = head
+                if let Some(head_hash) = params.head_hash {
-            .execution_payload_block_hash
+                    (
-            .filter(|h| *h != ExecutionBlockHash::zero())
+                        params.head_root,
-        {
+                        head_hash,
-            hash
+                        params
-        } else {
+                            .finalized_hash
-            // Don't send fork choice updates to the execution layer before the transition block.
+                            .unwrap_or_else(ExecutionBlockHash::zero),
-            return Ok(());
+                    )
-        };
+                } else {
-
+                    // The head block does not have an execution block hash, there is no need to
-        let finalized_root = if head.finalized_checkpoint.root == Hash256::zero() {
+                    // send an update to the EL.
-            // De-alias `0x00..00` to the genesis block root.
+                    return Ok(());
-            self.genesis_block_root
+                }
-        } else {
+            } else {
-            head.finalized_checkpoint.root
+                warn!(
-        };
+                    self.log,
-        // Deadlock warning:
+                    "Missing forkchoice params";
-        //
+                    "msg" => "please report this non-critical bug"
-        // The same as above, but the lock on `self.fork_choice`.
+                );
-        let finalized_execution_block_hash = self
+                return Ok(());
-            .fork_choice
+            };
-            .read()
-            .get_block(&finalized_root)
-            .ok_or(Error::FinalizedBlockMissingFromForkChoice(finalized_root))?
-            .execution_status
-            .block_hash()
-            .unwrap_or_else(ExecutionBlockHash::zero);
 
         let forkchoice_updated_response = self
             .execution_layer
             .as_ref()
             .ok_or(Error::ExecutionLayerMissing)?
-            .notify_forkchoice_updated(
+            .notify_forkchoice_updated(head_hash, finalized_hash, current_slot, head_block_root)
-                head_execution_block_hash,
-                finalized_execution_block_hash,
-                current_slot,
-                head_block_root,
-            )
             .await
             .map_err(Error::ExecutionForkChoiceUpdateFailed);
 

consensus/fork_choice/src/fork_choice.rs

@@ -241,6 +241,14 @@ pub enum AttestationFromBlock {
     False,
 }
 
+/// Parameters which are cached between calls to `Self::get_head`.
+#[derive(Clone, Copy)]
+pub struct ForkchoiceUpdateParameters {
+    pub head_root: Hash256,
+    pub head_hash: Option<ExecutionBlockHash>,
+    pub finalized_hash: Option<ExecutionBlockHash>,
+}
+
 /// Provides an implementation of "Ethereum 2.0 Phase 0 -- Beacon Chain Fork Choice":
 ///
 /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#ethereum-20-phase-0----beacon-chain-fork-choice
@@ -258,6 +266,8 @@ pub struct ForkChoice<T, E> {
     proto_array: ProtoArrayForkChoice,
     /// Attestations that arrived at the current slot and must be queued for later processing.
     queued_attestations: Vec<QueuedAttestation>,
+    /// Stores a cache of the values required to be sent to the execution layer.
+    forkchoice_update_parameters: Option<ForkchoiceUpdateParameters>,
     _phantom: PhantomData<E>,
 }
 
@@ -332,6 +342,7 @@ where
             fc_store,
             proto_array,
             queued_attestations: vec![],
+            forkchoice_update_parameters: None,
             _phantom: PhantomData,
         })
     }
@@ -349,10 +360,20 @@ where
             fc_store,
             proto_array,
             queued_attestations,
+            forkchoice_update_parameters: None,
             _phantom: PhantomData,
         }
     }
 
+    /// Returns cached information that can be used to issue a `forkchoiceUpdated` message to an
+    /// execution engine.
+    ///
+    /// These values are updated each time `Self::get_head` is called. May return `None` if
+    /// `Self::get_head` has not yet been called.
+    pub fn get_forkchoice_update_parameters(&self) -> Option<ForkchoiceUpdateParameters> {
+        self.forkchoice_update_parameters
+    }
+
     /// Returns the block root of an ancestor of `block_root` at the given `slot`. (Note: `slot` refers
     /// to the block that is *returned*, not the one that is supplied.)
     ///
@@ -414,15 +435,29 @@ where
 
         let store = &mut self.fc_store;
 
-        self.proto_array
+        let head_root = self.proto_array.find_head::<E>(
-            .find_head::<E>(
+            *store.justified_checkpoint(),
-                *store.justified_checkpoint(),
+            *store.finalized_checkpoint(),
-                *store.finalized_checkpoint(),
+            store.justified_balances(),
-                store.justified_balances(),
+            store.proposer_boost_root(),
-                store.proposer_boost_root(),
+            spec,
-                spec,
+        )?;
-            )
+
-            .map_err(Into::into)
+        // Cache some values for the next forkchoiceUpdate call to the execution layer.
+        let head_hash = self
+            .get_block(&head_root)
+            .and_then(|b| b.execution_status.block_hash());
+        let finalized_root = self.finalized_checkpoint().root;
+        let finalized_hash = self
+            .get_block(&finalized_root)
+            .and_then(|b| b.execution_status.block_hash());
+        self.forkchoice_update_parameters = Some(ForkchoiceUpdateParameters {
+            head_root,
+            head_hash,
+            finalized_hash,
+        });
+
+        Ok(head_root)
     }
 
     /// Returns `true` if the given `store` should be updated to set
@@ -1049,6 +1084,7 @@ where
             fc_store,
             proto_array,
             queued_attestations: persisted.queued_attestations,
+            forkchoice_update_parameters: None,
             _phantom: PhantomData,
         })
     }