From df90caeb34243812d4fa91890c8cf871b87de6d7 Mon Sep 17 00:00:00 2001
From: Dan Cline <6798349+Rjected@users.noreply.github.com>
Date: Mon, 5 Jun 2023 18:24:29 -0400
Subject: [PATCH] fix: ignore forkchoice updates to old head (#2952)

Co-authored-by: Georgios Konstantopoulos <me@gakonst.com>
---
 crates/consensus/beacon/src/engine/mod.rs    | 23 +++++++++++++-------
 crates/interfaces/src/blockchain_tree/mod.rs |  5 +++++
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs
index 5a3144630d..a394d91af0 100644
--- a/crates/consensus/beacon/src/engine/mod.rs
+++ b/crates/consensus/beacon/src/engine/mod.rs
@@ -490,9 +490,6 @@ where
             // node's fully synced, clear pending requests
             self.sync.clear_full_block_requests();
 
-            // new VALID update that moved the canonical chain forward
-            let _ = self.update_canon_chain(&state);
-
             let tip_number = self.blockchain.canonical_tip().number;
             if self.sync.has_reached_max_block(tip_number) {
                 return true
@@ -534,14 +531,24 @@ where
 
         let status = match self.blockchain.make_canonical(&state.head_block_hash) {
             Ok(outcome) => {
-                let header = outcome.into_header();
-                debug!(target: "consensus::engine", hash=?state.head_block_hash, number=header.number, "canonicalized new head");
+                if !outcome.is_already_canonical() {
+                    debug!(target: "consensus::engine", hash=?state.head_block_hash, number=outcome.header().number, "canonicalized new head");
+
+                    // new VALID update that moved the canonical chain forward
+                    let _ = self.update_canon_chain(&state);
+                } else {
+                    debug!(target: "consensus::engine", fcu_head_num=?outcome.header().number, current_head_num=?self.blockchain.canonical_tip().number, "Ignoring beacon update to old head");
+                }
 
                 if let Some(attrs) = attrs {
                     // the CL requested to build a new payload on top of this new VALID head
-                    let payload_response =
-                        self.process_payload_attributes(attrs, header.unseal(), state);
-                    trace!(target: "consensus::engine", status = ?payload_response, ?state, "Returning forkchoice status ");
+                    let payload_response = self.process_payload_attributes(
+                        attrs,
+                        outcome.into_header().unseal(),
+                        state,
+                    );
+
+                    trace!(target: "consensus::engine", status = ?payload_response, ?state, "Returning forkchoice status");
                     return Ok(payload_response)
                 }
 
diff --git a/crates/interfaces/src/blockchain_tree/mod.rs b/crates/interfaces/src/blockchain_tree/mod.rs
index a695a53838..6a63de4775 100644
--- a/crates/interfaces/src/blockchain_tree/mod.rs
+++ b/crates/interfaces/src/blockchain_tree/mod.rs
@@ -108,6 +108,11 @@ impl CanonicalOutcome {
             CanonicalOutcome::Committed { head } => head,
         }
     }
+
+    /// Returns true if the block was already canonical.
+    pub fn is_already_canonical(&self) -> bool {
+        matches!(self, CanonicalOutcome::AlreadyCanonical { .. })
+    }
 }
 
 /// From Engine API spec, block inclusion can be valid, accepted or invalid.