From 6bd2f7695276e0670666ac64a61a12de45e347c9 Mon Sep 17 00:00:00 2001 From: Marcos Nicolau <76252340+MarcosNicolau@users.noreply.github.com> Date: Wed, 19 Feb 2025 11:05:50 -0300 Subject: [PATCH] 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 --- .github/workflows/ci_l1.yaml | 2 +- crates/common/types/fork_id.rs | 40 ++++++++++++++--------- crates/common/types/genesis.rs | 18 ++++++++-- crates/networking/p2p/rlpx/eth/backend.rs | 20 ++++++++---- 4 files changed, 54 insertions(+), 26 deletions(-) diff --git a/.github/workflows/ci_l1.yaml b/.github/workflows/ci_l1.yaml index d629fabb4a..5147ed84db 100644 --- 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 index 1989e79758..9e8d184c1b 100644 --- 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, 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 = 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 index 7f6b9e5245..be94f9f5c3 100644 --- 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, Vec) { - let block_number_based_forks: Vec = vec![ + pub fn gather_forks(&self, genesis_header: BlockHeader) -> (Vec, Vec) { + let mut block_number_based_forks: Vec = 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 = vec![ + // Remove repeated values + block_number_based_forks.sort(); + block_number_based_forks.dedup(); + + let mut timestamp_based_forks: Vec = 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 index b4f7e4aa44..274c0e8f9c 100644 --- 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