fix(l1): added checks to newpayload and forkchoiceupdated (#2831)

**Motivation**

This should fix some sync and inconsistent behaviour problems with the
hive engine tests. The problem was happening when a sync process had
begun, and a block from that sync process entered the server. Some
checks for that scenario have been added.

Also made some tests in the CI easier to read and edit, while adding a
couple of them.

**Description**

- Made the CI tests have 1 by 1.
- Added some fixed CI tests to "Paris Engine tests" and "Engine
withdrawal tests"
- Added a check to forkchoiceupdatet for the body to be present before
forking.
- Added a check to execute_payload for the body of the parent to be
present before executing.

Fixes some tests in #1285

Author: fedacking

.github/workflows/pr-main_l1.yaml

@@ -160,11 +160,53 @@ jobs:
             ethrex_flags: ""
           - 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|Invalid PayloadAttributes|Unknown HeadBlockHash|Valid NewPayload->ForkchoiceUpdated|Invalid NewPayload, ParentHash|Bad Hash on NewPayload|In-Order Consecutive Payload Execution|Re-Org Back to Canonical Chain" # |Invalid P9 -> flaky
+            test_pattern: "engine-api/RPC|
+              Bad Hash on NewPayload|
+              Build Payload|
+              Fork ID|
+              In-Order Consecutive Payload Execution|
+              Inconsistent|
+              Invalid Missing Ancestor ReOrg|
+              Invalid NewPayload|
+              Invalid PayloadAttributes|
+              Multiple New Payloads|
+              NewPayload with|
+              ParentHash equals BlockHash on NewPayload|
+              Payload Build|
+              PrevRandao Opcode Transactions|
+              Re-Execute Payload|
+              Re-Org Back|
+              Re-org to Previously Validated Sidechain Payload|
+              RPC:|
+              Safe Re-Org|
+              Suggested Fee|
+              Transaction Re-Org|
+              Unique Payload ID|
+              Unknown|
+              Valid NewPayload->ForkchoiceUpdated" # |Invalid P9 -> flaky
             ethrex_flags: ""
           - name: "Engine withdrawal tests"
             simulation: ethereum/engine
-            test_pattern: "engine-withdrawals/engine-withdrawals test loader|GetPayloadV2 Block Value|Sync after 2 blocks - Withdrawals on Genesis|Max Initcode Size|Pre-Merge Fork Number > 0|Empty Withdrawals|Corrupted Block Hash Payload|Withdrawals Fork on Block 2|Withdrawals Fork on Block 3|GetPayloadBodies|Withdrawals Fork on Block 1 - 1 Block Re-Org|Withdrawals Fork on Block 1 - 8 Block Re-Org NewPayload|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]"
+            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]"
             ethrex_flags: ""
           - name: "Sync full"
             simulation: ethereum/sync

crates/blockchain/blockchain.rs

@@ -76,11 +76,21 @@ impl Blockchain {
         block: &Block,
     ) -> Result<(BlockExecutionResult, Vec<AccountUpdate>), ChainError> {
         // Validate if it can be the new head and find the parent
-        let Ok(parent_header) = find_parent_header(&block.header, &self.storage) else {
-            // If the parent is not present, we store it as pending.
-            self.storage.add_pending_block(block.clone()).await?;
-            return Err(ChainError::ParentNotFound);
+        // Feda (#2831): We search for the entire block because during full/batch sync
+        // we can have the header without the body indicating it's still syncing.
+        let parent = self
+            .storage
+            .get_block_by_hash(block.header.parent_hash)
+            .await?;
+        let parent_header = match parent {
+            Some(parent_block) => parent_block.header,
+            None => {
+                // If the parent is not present, we store it as pending.
+                self.storage.add_pending_block(block.clone()).await?;
+                return Err(ChainError::ParentNotFound);
+            }
         };
+
         let chain_config = self.storage.get_chain_config()?;
 
         // Validate the block pre-execution
@@ -231,16 +241,18 @@ impl Blockchain {
             }
             // for the first block, we need to query the store
             let parent_header = if i == 0 {
-                let Ok(parent_header) = find_parent_header(&block.header, &self.storage) else {
-                    return Err((
-                        ChainError::ParentNotFound,
-                        Some(BatchBlockProcessingFailure {
-                            failed_block_hash: block.hash(),
-                            last_valid_hash,
-                        }),
-                    ));
-                };
-                parent_header
+                match find_parent_header(&block.header, &self.storage) {
+                    Ok(parent_header) => parent_header,
+                    Err(error) => {
+                        return Err((
+                            error,
+                            Some(BatchBlockProcessingFailure {
+                                failed_block_hash: block.hash(),
+                                last_valid_hash,
+                            }),
+                        ))
+                    }
+                }
             } else {
                 // for the subsequent ones, the parent is the previous block
                 blocks[i - 1].header.clone()
@@ -557,8 +569,8 @@ pub async fn latest_canonical_block_hash(storage: &Store) -> Result<H256, ChainE
     )))
 }
 
-/// Validates if the provided block could be the new head of the chain, and returns the
-/// parent_header in that case. If not found, the new block is saved as pending.
+/// Searchs the header of the parent block header. If the parent header is missing,
+/// Returns a ChainError::ParentNotFound. If the storage has an error it propagates it
 pub fn find_parent_header(
     block_header: &BlockHeader,
     storage: &Store,

crates/blockchain/fork_choice.rs

@@ -40,19 +40,23 @@ pub async fn apply_fork_choice(
         None
     };
 
-    let head_res = store.get_block_header_by_hash(head_hash)?;
+    // Feda (#2831): We search for the entire block because during full/batch sync
+    // we can have the header without the body indicating it's still syncing.
+    let head_block_res = store.get_block_by_hash(head_hash).await?;
+
+    let Some(head_block) = head_block_res else {
+        return Err(InvalidForkChoice::Syncing);
+    };
 
     if !safe_hash.is_zero() {
-        check_order(&safe_res, &head_res)?;
+        check_order(safe_res.as_ref(), Some(&head_block.header))?;
     }
 
     if !finalized_hash.is_zero() && !safe_hash.is_zero() {
-        check_order(&finalized_res, &safe_res)?;
+        check_order(finalized_res.as_ref(), safe_res.as_ref())?;
     }
 
-    let Some(head) = head_res else {
-        return Err(InvalidForkChoice::Syncing);
-    };
+    let head = head_block.header;
 
     let latest = store.get_latest_block_number().await?;
 
@@ -130,8 +134,8 @@ pub async fn apply_fork_choice(
 
 // Checks that block 1 is prior to block 2 and that if the second is present, the first one is too.
 fn check_order(
-    block_1: &Option<BlockHeader>,
-    block_2: &Option<BlockHeader>,
+    block_1: Option<&BlockHeader>,
+    block_2: Option<&BlockHeader>,
 ) -> Result<(), InvalidForkChoice> {
     // We don't need to perform the check if the hashes are null
     match (block_1, block_2) {

crates/networking/rpc/eth/block.rs

@@ -312,7 +312,7 @@ impl RpcHandler for GetBlobBaseFee {
             _ => return Err(RpcErr::Internal("Could not get block header".to_owned())),
         };
         let parent_header = match find_parent_header(&header, &context.storage) {
-            Ok(header) => header,
+            Ok(option_header) => option_header,
             Err(error) => return Err(RpcErr::Internal(error.to_string())),
         };