From 1565c57ae6fa8d4be6288b25b107e12f365e7a55 Mon Sep 17 00:00:00 2001
From: andri lim <jangko128@gmail.com>
Date: Sat, 1 Jun 2024 20:26:12 +0700
Subject: [PATCH] Fix engine_forkchoiceUpdated bug (#2274)

* fCU Ignore update to old head

* Only enable terminal PoW block conditions for fCUV1

* Typo fix: TDD -> TTD

* Add link to Shanghai fCUV2 specification

* Add link to Paris fCUV1 specification
---
 nimbus/beacon/api_handler/api_forkchoice.nim | 55 +++++++++++---------
 1 file changed, 31 insertions(+), 24 deletions(-)

diff --git a/nimbus/beacon/api_handler/api_forkchoice.nim b/nimbus/beacon/api_handler/api_forkchoice.nim
index ea865fade..cf61b6050 100644
--- a/nimbus/beacon/api_handler/api_forkchoice.nim
+++ b/nimbus/beacon/api_handler/api_forkchoice.nim
@@ -102,38 +102,45 @@ proc forkchoiceUpdated*(ben: BeaconEngineRef,
 
   # Block is known locally, just sanity check that the beacon client does not
   # attempt to push us back to before the merge.
-  let blockNumber = header.blockNumber.truncate(uint64)
-  if header.difficulty > 0.u256 or blockNumber ==  0'u64:
-    var
-      td, ptd: DifficultyInt
-      ttd = com.ttd.get(high(common.BlockNumber))
+  #
+  # Disable terminal PoW block conditions validation for fCUV2 and later.
+  # https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.4/src/engine/shanghai.md#specification-1
+  if apiVersion == Version.V1:
+    let blockNumber = header.blockNumber.truncate(uint64)
+    if header.difficulty > 0.u256 or blockNumber ==  0'u64:
+      var
+        td, ptd: DifficultyInt
+        ttd = com.ttd.get(high(common.BlockNumber))
 
-    if not db.getTd(blockHash, td) or (blockNumber > 0'u64 and not db.getTd(header.parentHash, ptd)):
-      error "TDs unavailable for TTD check",
-        number = blockNumber,
-        hash = blockHash.short,
-        td = td,
-        parent = header.parentHash.short,
-        ptd = ptd
-      return simpleFCU(PayloadExecutionStatus.invalid, "TDs unavailable for TDD check")
+      if not db.getTd(blockHash, td) or (blockNumber > 0'u64 and not db.getTd(header.parentHash, ptd)):
+        error "TDs unavailable for TTD check",
+          number = blockNumber,
+          hash = blockHash.short,
+          td = td,
+          parent = header.parentHash.short,
+          ptd = ptd
+        return simpleFCU(PayloadExecutionStatus.invalid, "TDs unavailable for TTD check")
 
-    if td < ttd or (blockNumber > 0'u64 and ptd > ttd):
-      notice "Refusing beacon update to pre-merge",
-        number = blockNumber,
-        hash = blockHash.short,
-        diff = header.difficulty,
-        ptd = ptd,
-        ttd = ttd
+      if td < ttd or (blockNumber > 0'u64 and ptd > ttd):
+        notice "Refusing beacon update to pre-merge",
+          number = blockNumber,
+          hash = blockHash.short,
+          diff = header.difficulty,
+          ptd = ptd,
+          ttd = ttd
 
-      return invalidFCU("Refusing beacon update to pre-merge")
+        return invalidFCU("Refusing beacon update to pre-merge")
 
   # If the head block is already in our canonical chain, the beacon client is
   # probably resyncing. Ignore the update.
+  # See point 2 of fCUV1 specification
+  # https://github.com/ethereum/execution-apis/blob/v1.0.0-beta.4/src/engine/paris.md#specification-1
   var canonHash: common.Hash256
   if db.getBlockHash(header.blockNumber, canonHash) and canonHash == blockHash:
-    # TODO should this be possible?
-    # If we allow these types of reorgs, we will do lots and lots of reorgs during sync
-    notice "Reorg to previous block", blockHash
+    notice "Ignoring beacon update to old head",
+      blockHash=blockHash.short,
+      blockNumber=header.blockNumber
+    return validFCU(none(PayloadID), blockHash)
 
   chain.setCanonical(header).isOkOr:
     return invalidFCU(error, com, header)