From 106d44930788f8c31b79b754ab1fcb9d767ff405 Mon Sep 17 00:00:00 2001
From: Federico Gimenez <fgimenez@users.noreply.github.com>
Date: Tue, 21 May 2024 16:24:15 +0200
Subject: [PATCH] fix: disambiguate use of next when validating ForkId (#8320)

---
 crates/ethereum-forks/src/forkid.rs | 92 ++++++++++++++++++++++++++---
 1 file changed, 84 insertions(+), 8 deletions(-)

diff --git a/crates/ethereum-forks/src/forkid.rs b/crates/ethereum-forks/src/forkid.rs
index ee4edb8bd..b0aba0d5a 100644
--- a/crates/ethereum-forks/src/forkid.rs
+++ b/crates/ethereum-forks/src/forkid.rs
@@ -21,6 +21,7 @@ use std::{
 use thiserror::Error;
 
 const CRC_32_IEEE: Crc<u32> = Crc::<u32>::new(&CRC_32_ISO_HDLC);
+const TIMESTAMP_BEFORE_ETHEREUM_MAINNET: u64 = 1_300_000_000;
 
 /// `CRC32` hash of all previous forks starting from genesis block.
 #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
@@ -313,16 +314,25 @@ impl ForkFilter {
                 return Ok(())
             }
 
-            // We check if this fork is time-based or block number-based
-            // NOTE: This is a bit hacky but I'm unsure how else we can figure out when to use
-            // timestamp vs when to use block number..
-            let head_block_or_time = match self.cache.epoch_start {
-                ForkFilterKey::Block(_) => self.head.number,
-                ForkFilterKey::Time(_) => self.head.timestamp,
+            let is_incompatible = if self.head.number < TIMESTAMP_BEFORE_ETHEREUM_MAINNET {
+                // When the block number is less than an old timestamp before Ethereum mainnet,
+                // we check if this fork is time-based or block number-based by estimating that,
+                // if fork_id.next is bigger than the old timestamp, we are dealing with a
+                // timestamp, otherwise with a block.
+                (fork_id.next > TIMESTAMP_BEFORE_ETHEREUM_MAINNET &&
+                    self.head.timestamp >= fork_id.next) ||
+                    (fork_id.next <= TIMESTAMP_BEFORE_ETHEREUM_MAINNET &&
+                        self.head.number >= fork_id.next)
+            } else {
+                // Extra safety check to future-proof for when Ethereum has over a billion blocks.
+                let head_block_or_time = match self.cache.epoch_start {
+                    ForkFilterKey::Block(_) => self.head.number,
+                    ForkFilterKey::Time(_) => self.head.timestamp,
+                };
+                head_block_or_time >= fork_id.next
             };
 
-            //... compare local head to FORK_NEXT.
-            return if head_block_or_time >= fork_id.next {
+            return if is_incompatible {
                 // 1a) A remotely announced but remotely not passed block is already passed locally,
                 // disconnect, since the chains are incompatible.
                 Err(ValidationError::LocalIncompatibleOrStale {
@@ -588,6 +598,72 @@ mod tests {
             filter.validate(remote),
             Err(ValidationError::LocalIncompatibleOrStale { local: filter.current(), remote })
         );
+
+        // Block far in the future (block number bigger than TIMESTAMP_BEFORE_ETHEREUM_MAINNET), not
+        // compatible.
+        filter
+            .set_head(Head { number: TIMESTAMP_BEFORE_ETHEREUM_MAINNET + 1, ..Default::default() });
+        let remote = ForkId {
+            hash: ForkHash(hex!("668db0af")),
+            next: TIMESTAMP_BEFORE_ETHEREUM_MAINNET + 1,
+        };
+        assert_eq!(
+            filter.validate(remote),
+            Err(ValidationError::LocalIncompatibleOrStale { local: filter.current(), remote })
+        );
+
+        // Block far in the future (block number bigger than TIMESTAMP_BEFORE_ETHEREUM_MAINNET),
+        // compatible.
+        filter
+            .set_head(Head { number: TIMESTAMP_BEFORE_ETHEREUM_MAINNET + 1, ..Default::default() });
+        let remote = ForkId {
+            hash: ForkHash(hex!("668db0af")),
+            next: TIMESTAMP_BEFORE_ETHEREUM_MAINNET + 2,
+        };
+        assert_eq!(filter.validate(remote), Ok(()));
+
+        // block number smaller than TIMESTAMP_BEFORE_ETHEREUM_MAINNET and
+        // fork_id.next > TIMESTAMP_BEFORE_ETHEREUM_MAINNET && self.head.timestamp >= fork_id.next,
+        // not compatible.
+        filter.set_head(Head {
+            number: TIMESTAMP_BEFORE_ETHEREUM_MAINNET - 1,
+            timestamp: TIMESTAMP_BEFORE_ETHEREUM_MAINNET + 2,
+            ..Default::default()
+        });
+        let remote = ForkId {
+            hash: ForkHash(hex!("668db0af")),
+            next: TIMESTAMP_BEFORE_ETHEREUM_MAINNET + 1,
+        };
+        assert_eq!(
+            filter.validate(remote),
+            Err(ValidationError::LocalIncompatibleOrStale { local: filter.current(), remote })
+        );
+
+        // block number smaller than TIMESTAMP_BEFORE_ETHEREUM_MAINNET and
+        // fork_id.next <= TIMESTAMP_BEFORE_ETHEREUM_MAINNET && self.head.number >= fork_id.next,
+        // not compatible.
+        filter
+            .set_head(Head { number: TIMESTAMP_BEFORE_ETHEREUM_MAINNET - 1, ..Default::default() });
+        let remote = ForkId {
+            hash: ForkHash(hex!("668db0af")),
+            next: TIMESTAMP_BEFORE_ETHEREUM_MAINNET - 2,
+        };
+        assert_eq!(
+            filter.validate(remote),
+            Err(ValidationError::LocalIncompatibleOrStale { local: filter.current(), remote })
+        );
+
+        // block number smaller than TIMESTAMP_BEFORE_ETHEREUM_MAINNET and
+        // !((fork_id.next > TIMESTAMP_BEFORE_ETHEREUM_MAINNET && self.head.timestamp >=
+        // fork_id.next) || (fork_id.next <= TIMESTAMP_BEFORE_ETHEREUM_MAINNET && self.head.number
+        // >= fork_id.next)), compatible.
+        filter
+            .set_head(Head { number: TIMESTAMP_BEFORE_ETHEREUM_MAINNET - 2, ..Default::default() });
+        let remote = ForkId {
+            hash: ForkHash(hex!("668db0af")),
+            next: TIMESTAMP_BEFORE_ETHEREUM_MAINNET - 1,
+        };
+        assert_eq!(filter.validate(remote), Ok(()));
     }
 
     #[test]