fix(l1): revert "fix hive ReOrg tests by requesting headers from new to old" (#4784)

Reverts #4676
Related to #4577

This reverts commit f552509, which was
causing problems. Commented tests are from #4673

Author: MegaRedHand

.github/workflows/pr-main_l1.yaml

@@ -203,7 +203,7 @@ jobs:
             ethrex_flags: ""
           - name: "Engine withdrawal tests"
             simulation: ethereum/engine
-            test_pattern: "engine-withdrawals"
+            test_pattern: "engine-withdrawals/Corrupted Block Hash Payload|Empty Withdrawals|engine-withdrawals test loader|GetPayloadBodies|GetPayloadV2 Block Value|Max Initcode Size|Sync after 2 blocks - Withdrawals on Genesis|Withdraw many accounts|Withdraw to a single account|Withdraw to two accounts|Withdraw zero amount|Withdraw many accounts|Withdrawals Fork on Block 1 - 1 Block Re-Org|Withdrawals Fork on Block 1 - 8 Block Re-Org NewPayload|Withdrawals Fork on Block 2|Withdrawals Fork on Block 3|Withdrawals Fork on Block 8 - 10 Block Re-Org NewPayload|Withdrawals Fork on Canonical Block 8 / Side Block 7 - 10 Block Re-Org [^S]|Withdrawals Fork on Canonical Block 8 / Side Block 9 - 10 Block Re-Org [^S]"
           - name: "Sync full"
             simulation: ethereum/sync
             test_pattern: ""

crates/networking/p2p/sync.rs

@@ -57,12 +57,6 @@ const BYTECODE_CHUNK_SIZE: usize = 50_000;
 /// that are unlikely to be re-orged.
 const MISSING_SLOTS_PERCENTAGE: f64 = 0.8;
 
-/// Searching for pending blocks to include during syncing can potentially slow down block processing if we have a large chain of pending blocks.
-/// For example, if we are syncing from genesis, it might take a while until this chain connects, but the searching for pending blocks loop will
-/// happen each time we process a batch of blocks, and can potentially take long, that's why we just limit this check to 32 blocks.
-/// Eventually we will implement some in-memory structure to make this check easy.
-const PENDING_BLOCKS_RETRIEVAL_LIMIT: usize = 32;
-
 #[cfg(feature = "sync-test")]
 lazy_static::lazy_static! {
     static ref EXECUTE_BATCH_SIZE: usize = std::env::var("EXECUTE_BATCH_SIZE").map(|var| var.parse().expect("Execute batch size environmental variable is not a number")).unwrap_or(EXECUTE_BATCH_SIZE_DEFAULT);
@@ -343,16 +337,6 @@ impl Syncer {
             current_head, sync_head
         );
 
-        // Try syncing backwards from the sync_head to find a common ancestor
-        if self
-            .synced_new_to_old(&mut block_sync_state, sync_head, store)
-            .await?
-        {
-            return Ok(());
-        }
-        // synced_new_to_old returns true in case syncing was finished from NewToOld or if the requested headers were None
-        // if sync_finished is false that means we are more than 1024 blocks behind so, for now, we go back to syncing as it follows.
-        // TODO: Have full syncing always be from NewToOld, issue: https://github.com/lambdaclass/ethrex/issues/4717
         loop {
             debug!("Sync Log 1: In Full Sync");
             debug!(
@@ -364,7 +348,7 @@ impl Syncer {
                 block_sync_state.current_blocks.len()
             );
 
-            debug!("Requesting Block Headers from OldToNew from current_head {current_head}");
+            debug!("Requesting Block Headers from {current_head}");
 
             let Some(mut block_headers) = self
                 .peers
@@ -387,7 +371,6 @@ impl Syncer {
                 Some(header) => (header.hash(), header.number),
                 None => continue,
             };
-
             // TODO(#2126): This is just a temporary solution to avoid a bug where the sync would get stuck
             // on a loop when the target head is not found, i.e. on a reorg with a side-chain.
             if first_block_hash == last_block_hash
@@ -443,88 +426,9 @@ impl Syncer {
                 break;
             };
         }
-
         Ok(())
     }
 
-    /// Tries to perform syncing going backwards from the sync_head with one batch of requested headers.
-    /// This is to cover the case where we are on a sidechain and the peer doesn't have our current_head
-    /// so when requesting headers from our current_head on we get None and we never get to finish syncing.
-    /// For more context go to the PR https://github.com/lambdaclass/ethrex/pull/4676
-    ///
-    /// # Returns
-    ///
-    /// Returns an error if the sync fails at any given step and aborts all active processes
-    /// otherwise returns true whether syncing was finished or in case the request of headers returned None,
-    /// otherwise returns false, this means we couldn't find a common ancestor within the requested headers,
-    /// which in turn means the chain is more than 1024 blocks behind.
-    async fn synced_new_to_old(
-        &mut self,
-        block_sync_state: &mut FullBlockSyncState,
-        sync_head: H256,
-        store: Store,
-    ) -> Result<bool, SyncError> {
-        debug!("Sync Log 1: In Full Sync");
-        debug!(
-            "Sync Log 3: State current headers len {}",
-            block_sync_state.current_headers.len()
-        );
-        debug!(
-            "Sync Log 4: State current blocks len {}",
-            block_sync_state.current_blocks.len()
-        );
-
-        debug!("Requesting Block Headers from NewToOld from sync_head {sync_head}");
-
-        // Get oldest pending block to use in the request for headers
-        let mut requested_header = sync_head;
-        while let Some(block) = store.get_pending_block(requested_header).await? {
-            requested_header = block.header.parent_hash;
-        }
-
-        let Some(mut block_headers) = self
-            .peers
-            .request_block_headers_from_hash(requested_header, BlockRequestOrder::NewToOld)
-            .await?
-        else {
-            // sync_head or sync_head parent was not found
-            warn!("Sync failed to find target block header, aborting");
-            debug!("Sync Log 8: Sync failed to find target block header, aborting");
-            return Ok(true);
-        };
-
-        debug!("Sync Log 9: Received {} block headers", block_headers.len());
-
-        let mut found_common_ancestor = false;
-        for i in 0..block_headers.len() {
-            if store
-                .get_block_by_hash(block_headers[i].hash())
-                .await?
-                .is_some()
-            {
-                block_headers.drain(i..);
-                found_common_ancestor = true;
-                break;
-            }
-        }
-
-        if found_common_ancestor {
-            block_headers.reverse();
-            block_sync_state
-                .process_incoming_headers(
-                    block_headers,
-                    sync_head,
-                    true, // sync_head_found is true because of the NewToOld headers request
-                    self.blockchain.clone(),
-                    self.peers.clone(),
-                    self.cancel_token.clone(),
-                )
-                .await?;
-            return Ok(true);
-        }
-        Ok(false)
-    }
-
     /// Executes the given blocks and stores them
     /// If sync_head_found is true, they will be executed one by one
     /// If sync_head_found is false, they will be executed in a single batch
@@ -718,32 +622,16 @@ impl FullBlockSyncState {
         self.current_blocks.extend(blocks);
         // }
 
-        // We check if we have pending blocks we didn't request that are needed for syncing
-        // Then we add it in current_blocks for execution.
-        let mut pending_block_to_sync = vec![];
-        let mut last_header_to_sync = sync_head;
-        let mut pending_blocks_retieved = 0;
-        while let Some(block) = self.store.get_pending_block(last_header_to_sync).await? {
-            let block_parent = block.header.parent_hash;
-            if self
-                .current_blocks
-                .last()
-                .is_some_and(|block| block.hash() == block_parent)
-            {
-                pending_block_to_sync.push(block);
-                sync_head_found = true;
-                pending_block_to_sync.reverse();
-                self.current_blocks.extend(pending_block_to_sync);
-                break;
-            }
-            pending_block_to_sync.push(block);
-            last_header_to_sync = block_parent;
-            if pending_blocks_retieved > PENDING_BLOCKS_RETRIEVAL_LIMIT {
-                break;
+        // If we have the sync_head as a pending block from a new_payload request and its parent_hash matches the hash of the latest received header
+        // we set the sync_head as found. Then we add it in current_blocks for execution.
+        if let Some(block) = self.store.get_pending_block(sync_head).await? {
+            if let Some(last_block) = self.current_blocks.last() {
+                if last_block.hash() == block.header.parent_hash {
+                    self.current_blocks.push(block);
+                    sync_head_found = true;
+                }
             }
-            pending_blocks_retieved += 1;
         }
-
         // Execute full blocks
         // while self.current_blocks.len() >= *EXECUTE_BATCH_SIZE
         //     || (!self.current_blocks.is_empty() && sync_head_found)

tooling/reorgs/src/main.rs

@@ -33,17 +33,19 @@ async fn main() {
     info!("");
 
     run_test(&cmd_path, no_reorgs_full_sync_smoke_test).await;
-    run_test(&cmd_path, test_reorg_back_to_base).await;
-    // This test is flaky 50% of the time, check that it runs correctly 30 times in a row
-    // TODO(#4775): make it deterministic
-    for _ in 0..30 {
-        run_test(&cmd_path, test_chain_split).await;
-    }
-    run_test(&cmd_path, test_one_block_reorg_and_back).await;
-    run_test(&cmd_path, test_reorg_back_to_base_with_common_ancestor).await;
-    run_test(&cmd_path, test_storage_slots_reorg).await;
 
-    run_test(&cmd_path, test_many_blocks_reorg).await;
+    // TODO: uncomment once #4676 is fixed
+    // run_test(&cmd_path, test_reorg_back_to_base).await;
+    // // This test is flaky 50% of the time, check that it runs correctly 30 times in a row
+    // // TODO(#4775): make it deterministic
+    // for _ in 0..30 {
+    //     run_test(&cmd_path, test_chain_split).await;
+    // }
+    // run_test(&cmd_path, test_one_block_reorg_and_back).await;
+    // run_test(&cmd_path, test_reorg_back_to_base_with_common_ancestor).await;
+    // run_test(&cmd_path, test_storage_slots_reorg).await;
+
+    // run_test(&cmd_path, test_many_blocks_reorg).await;
 }
 
 async fn get_ethrex_version(cmd_path: &Path) -> String {
@@ -102,6 +104,7 @@ async fn no_reorgs_full_sync_smoke_test(simulator: Arc<Mutex<Simulator>>) {
     node1.update_forkchoice(&base_chain).await;
 }
 
+#[expect(unused)]
 async fn test_reorg_back_to_base(simulator: Arc<Mutex<Simulator>>) {
     let mut simulator = simulator.lock().await;
 
@@ -121,6 +124,7 @@ async fn test_reorg_back_to_base(simulator: Arc<Mutex<Simulator>>) {
     node0.update_forkchoice(&base_chain).await;
 }
 
+#[expect(unused)]
 async fn test_reorg_back_to_base_with_common_ancestor(simulator: Arc<Mutex<Simulator>>) {
     let mut simulator = simulator.lock().await;
 
@@ -145,6 +149,7 @@ async fn test_reorg_back_to_base_with_common_ancestor(simulator: Arc<Mutex<Simul
     node0.update_forkchoice(&base_chain).await;
 }
 
+#[expect(unused)]
 async fn test_chain_split(simulator: Arc<Mutex<Simulator>>) {
     let mut simulator = simulator.lock().await;
 
@@ -167,6 +172,7 @@ async fn test_chain_split(simulator: Arc<Mutex<Simulator>>) {
     node0.update_forkchoice(&base_chain).await;
 }
 
+#[expect(unused)]
 async fn test_one_block_reorg_and_back(simulator: Arc<Mutex<Simulator>>) {
     let mut simulator = simulator.lock().await;
     let signer: Signer = LocalSigner::new(
@@ -237,6 +243,7 @@ async fn test_one_block_reorg_and_back(simulator: Arc<Mutex<Simulator>>) {
     assert_eq!(new_balance, initial_balance);
 }
 
+#[expect(unused)]
 async fn test_many_blocks_reorg(simulator: Arc<Mutex<Simulator>>) {
     let mut simulator = simulator.lock().await;
     let signer: Signer = LocalSigner::new(
@@ -308,6 +315,7 @@ async fn test_many_blocks_reorg(simulator: Arc<Mutex<Simulator>>) {
     assert_eq!(new_balance, initial_balance + transfer_amount);
 }
 
+#[expect(unused)]
 async fn test_storage_slots_reorg(simulator: Arc<Mutex<Simulator>>) {
     let mut simulator = simulator.lock().await;
     // Initcode for deploying a contract that receives two `bytes32` parameters and sets `storage[param0] = param1`