fix(l1): hive engine-cancun suite ForkId tests (#1976)

**Motivation** Fix failing tests in the engine-cancun hive suite. **Description** This update fixes all tests matching the pattern `ForkId: *` in the `engine-cancun` suite. The issue resulted from not filtering out duplicate fork values and failing to exclude forks that occurred before the genesis block. Advances on #1285
lambdaclass · Feb 19, 2025 · 6bd2f76 · 6bd2f76
1 parent 8ddabd9
commit 6bd2f76
Show file tree

Hide file tree

Showing 4 changed files with 54 additions and 26 deletions.
diff --git a/.github/workflows/ci_l1.yaml b/.github/workflows/ci_l1.yaml
@@ -168,7 +168,7 @@ jobs:
             test_pattern: engine-(auth|exchange-capabilities)/
           - name: "Cancun Engine tests"
             simulation: ethereum/engine
-            test_pattern: "engine-cancun/Blob Transactions On Block 1|Blob Transaction Ordering, Single|Blob Transaction Ordering, Multiple Accounts|Replace Blob Transactions|Parallel Blob Transactions|ForkchoiceUpdatedV3|ForkchoiceUpdatedV2|ForkchoiceUpdated Version|GetPayload|NewPayloadV3 After Cancun|NewPayloadV3 Before Cancun|NewPayloadV3 Versioned Hashes|Incorrect BlobGasUsed|ParentHash equals BlockHash|RPC:|in ForkchoiceState|Unknown SafeBlockHash|Unknown FinalizedBlockHash|Unique|Re-Execute Payload|Multiple New Payloads|NewPayload with|Build Payload with|Re-org to Previously|Safe Re-Org to Side Chain|Transaction Re-Org|Re-Org Back into Canonical Chain, Depth=5|Suggested Fee Recipient Test|PrevRandao Opcode|Fork ID: Genesis=0|Fork ID: Genesis=1, Cancun=0|Fork ID: Genesis=1, Cancun=2 |Fork ID: Genesis=1, Cancun=2, BlocksBeforePeering=1|Fork ID: Genesis=1, Cancun=2, Shanghai=[^1]|Pre-Merge"
+            test_pattern: "engine-cancun/Blob Transactions On Block 1|Blob Transaction Ordering, Single|Blob Transaction Ordering, Multiple Accounts|Replace Blob Transactions|Parallel Blob Transactions|ForkchoiceUpdatedV3|ForkchoiceUpdatedV2|ForkchoiceUpdated Version|GetPayload|NewPayloadV3 After Cancun|NewPayloadV3 Before Cancun|NewPayloadV3 Versioned Hashes|Incorrect BlobGasUsed|ParentHash equals BlockHash|RPC:|in ForkchoiceState|Unknown SafeBlockHash|Unknown FinalizedBlockHash|Unique|Re-Execute Payload|Multiple New Payloads|NewPayload with|Build Payload with|Re-org to Previously|Safe Re-Org to Side Chain|Transaction Re-Org|Re-Org Back into Canonical Chain, Depth=5|Suggested Fee Recipient Test|PrevRandao Opcode|Fork ID: *"
           - name: "Paris Engine tests"
             simulation: ethereum/engine
             test_pattern: "engine-api/RPC|Re-org to Previously Validated Sidechain Payload|Re-Org Back into Canonical Chain, Depth=5|Safe Re-Org|Transaction Re-Org|Inconsistent|Suggested Fee|PrevRandao Opcode Transactions|Fork ID|Unknown SafeBlockHash|Unknown FinalizedBlockHash|Unique Payload ID|Re-Execute Payload|Multiple New Payloads|NewPayload with|Payload Build|Build Payload"

diff --git a/crates/common/types/fork_id.rs b/crates/common/types/fork_id.rs
@@ -9,7 +9,7 @@ use ethrex_rlp::{
 use ethereum_types::H32;
 use tracing::debug;
 
-use super::{BlockHash, BlockNumber, ChainConfig};
+use super::{BlockHash, BlockHeader, BlockNumber, ChainConfig};
 
 // See https://github.com/ethereum/go-ethereum/blob/530adfc8e3ef9c8b6356facecdec10b30fb81d7d/core/forkid/forkid.go#L51
 const TIMESTAMP_THRESHOLD: u64 = 1438269973;
@@ -23,11 +23,14 @@ pub struct ForkId {
 impl ForkId {
     pub fn new(
         chain_config: ChainConfig,
-        genesis_hash: BlockHash,
+        genesis_header: BlockHeader,
         head_timestamp: u64,
         head_block_number: u64,
     ) -> Self {
-        let (block_number_based_forks, timestamp_based_forks) = chain_config.gather_forks();
+        let genesis_hash = genesis_header.compute_block_hash();
+        let (block_number_based_forks, timestamp_based_forks) =
+            chain_config.gather_forks(genesis_header);
+
         let mut fork_next;
         let mut hasher = Hasher::new();
         // Calculate the starting checksum from the genesis hash
@@ -60,9 +63,11 @@ impl ForkId {
         latest_block_number: u64,
         head_timestamp: u64,
         chain_config: ChainConfig,
-        genesis_hash: BlockHash,
+        genesis_header: BlockHeader,
     ) -> bool {
-        let (block_number_based_forks, timestamp_based_forks) = chain_config.gather_forks();
+        let genesis_hash = genesis_header.compute_block_hash();
+        let (block_number_based_forks, timestamp_based_forks) =
+            chain_config.gather_forks(genesis_header);
 
         // Determine whether to compare the remote fork_next using a block number or a timestamp.
         let head = if head_timestamp >= TIMESTAMP_THRESHOLD {
@@ -220,17 +225,22 @@ mod tests {
     fn assert_test_cases(
         test_cases: Vec<TestCase>,
         chain_config: ChainConfig,
-        genesis_hash: BlockHash,
+        genesis_header: BlockHeader,
     ) {
         for test_case in test_cases {
-            let fork_id = ForkId::new(chain_config, genesis_hash, test_case.time, test_case.head);
+            let fork_id = ForkId::new(
+                chain_config,
+                genesis_header.clone(),
+                test_case.time,
+                test_case.head,
+            );
             assert_eq!(
                 fork_id.is_valid(
                     test_case.fork_id,
                     test_case.head,
                     test_case.time,
                     chain_config,
-                    genesis_hash
+                    genesis_header.clone()
                 ),
                 test_case.is_valid
             )
@@ -244,7 +254,7 @@ mod tests {
         let genesis_reader = BufReader::new(genesis_file);
         let genesis: Genesis =
             serde_json::from_reader(genesis_reader).expect("Failed to read genesis file");
-        let genesis_hash = genesis.get_block().hash();
+        let genesis_header = genesis.get_block().header;
 
         // See https://github.com/ethereum/go-ethereum/blob/4d94bd83b20ce430e435f3107f29632c627cfb26/core/forkid/forkid_test.go#L98
         let test_cases: Vec<TestCase> = vec![
@@ -303,17 +313,17 @@ mod tests {
                 is_valid: true,
             },
         ];
-        assert_test_cases(test_cases, genesis.config, genesis_hash);
+        assert_test_cases(test_cases, genesis.config, genesis_header);
     }
 
-    fn get_sepolia_genesis() -> (Genesis, BlockHash) {
+    fn get_sepolia_genesis() -> (Genesis, BlockHeader) {
         let genesis_file = std::fs::File::open("../../cmd/ethrex/networks/sepolia/genesis.json")
             .expect("Failed to open genesis file");
         let genesis_reader = BufReader::new(genesis_file);
         let genesis: Genesis =
             serde_json::from_reader(genesis_reader).expect("Failed to read genesis file");
-        let genesis_hash = genesis.get_block().hash();
-        (genesis, genesis_hash)
+        let genesis_header = genesis.get_block().header;
+        (genesis, genesis_header)
     }
     #[test]
     fn sepolia_test_cases() {
@@ -417,14 +427,14 @@ mod tests {
             local_head_block_number,
             0,
             ChainConfig::default(),
-            BlockHash::random(),
+            BlockHeader::default(),
         );
         let result_b = local_b.is_valid(
             remote,
             local_head_block_number,
             0,
             ChainConfig::default(),
-            BlockHash::random(),
+            BlockHeader::default(),
         );
         assert!(!result_a);
         assert!(!result_b);

diff --git a/crates/common/types/genesis.rs b/crates/common/types/genesis.rs
@@ -235,8 +235,8 @@ impl ChainConfig {
         self.get_fork(block_timestamp)
     }
 
-    pub fn gather_forks(&self) -> (Vec<u64>, Vec<u64>) {
-        let block_number_based_forks: Vec<u64> = vec![
+    pub fn gather_forks(&self, genesis_header: BlockHeader) -> (Vec<u64>, Vec<u64>) {
+        let mut block_number_based_forks: Vec<u64> = vec![
             self.homestead_block,
             if self.dao_fork_support {
                 self.dao_fork_block
@@ -261,7 +261,11 @@ impl ChainConfig {
         .flatten()
         .collect();
 
-        let timestamp_based_forks: Vec<u64> = vec![
+        // Remove repeated values
+        block_number_based_forks.sort();
+        block_number_based_forks.dedup();
+
+        let mut timestamp_based_forks: Vec<u64> = vec![
             self.shanghai_time,
             self.cancun_time,
             self.prague_time,
@@ -271,6 +275,14 @@ impl ChainConfig {
         .flatten()
         .collect();
 
+        // Remove repeated values
+        timestamp_based_forks.sort();
+        timestamp_based_forks.dedup();
+
+        // Filter forks before genesis
+        block_number_based_forks.retain(|block_number| *block_number != 0);
+        timestamp_based_forks.retain(|block_timestamp| *block_timestamp > genesis_header.timestamp);
+
         (block_number_based_forks, timestamp_based_forks)
     }
 }

diff --git a/crates/networking/p2p/rlpx/eth/backend.rs b/crates/networking/p2p/rlpx/eth/backend.rs
@@ -21,7 +21,12 @@ pub fn get_status(storage: &Store, eth_version: u32) -> Result<StatusMessage, RL
 
     let genesis = genesis_header.compute_block_hash();
     let block_hash = block_header.compute_block_hash();
-    let fork_id = ForkId::new(chain_config, genesis, block_header.timestamp, block_number);
+    let fork_id = ForkId::new(
+        chain_config,
+        genesis_header,
+        block_header.timestamp,
+        block_number,
+    );
     Ok(StatusMessage {
         eth_version,
         network_id,
@@ -43,14 +48,14 @@ pub fn validate_status(
     let genesis_header = storage
         .get_block_header(0)?
         .ok_or(RLPxError::NotFound("Genesis Block".to_string()))?;
+    let genesis_hash = genesis_header.compute_block_hash();
     let latest_block_number = storage.get_latest_block_number()?;
     let latest_block_header = storage
         .get_block_header(latest_block_number)?
         .ok_or(RLPxError::NotFound(format!("Block {latest_block_number}")))?;
-    let genesis = genesis_header.compute_block_hash();
     let fork_id = ForkId::new(
         chain_config,
-        genesis,
+        genesis_header.clone(),
         latest_block_header.timestamp,
         latest_block_number,
     );
@@ -68,7 +73,7 @@ pub fn validate_status(
         ));
     }
     //Check Genesis
-    if msg_data.genesis != genesis {
+    if msg_data.genesis != genesis_hash {
         return Err(RLPxError::HandshakeError(
             "Genesis does not match".to_string(),
         ));
@@ -79,7 +84,7 @@ pub fn validate_status(
         latest_block_number,
         latest_block_header.timestamp,
         chain_config,
-        genesis,
+        genesis_header,
     ) {
         return Err(RLPxError::HandshakeError("Invalid Fork Id".to_string()));
     }
@@ -115,8 +120,9 @@ mod tests {
             .expect("Failed to add genesis block to DB");
         let config = genesis.config;
         let total_difficulty = U256::from(config.terminal_total_difficulty.unwrap_or_default());
-        let genesis_hash = genesis.get_block().hash();
-        let fork_id = ForkId::new(config, genesis_hash, 2707305664, 123);
+        let genesis_header = genesis.get_block().header;
+        let genesis_hash = genesis_header.compute_block_hash();
+        let fork_id = ForkId::new(config, genesis_header, 2707305664, 123);
 
         let eth_version = 68;
         let message = StatusMessage {