From fc43d6eb738a6776c9695c15074073aafbe94610 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Tue, 1 Oct 2024 16:34:07 +0200 Subject: [PATCH 1/3] fix(engine): `is_fork` header traversal --- crates/engine/tree/src/tree/mod.rs | 41 ++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 13 deletions(-) diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index 4731aee2998d..4ac665752a79 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -853,7 +853,7 @@ where }; let new_head_number = new_head_block.block.number; - let current_canonical_number = self.state.tree_state.current_canonical_head.number; + let mut current_canonical_number = self.state.tree_state.current_canonical_head.number; let mut new_chain = vec![new_head_block.clone()]; let mut current_hash = new_head_block.block.parent_hash; @@ -889,6 +889,18 @@ where let mut old_chain = Vec::new(); let mut old_hash = self.state.tree_state.current_canonical_head.hash; + while current_canonical_number > current_number { + if let Some(block) = self.executed_block_by_hash(old_hash)? { + old_chain.push(block.clone()); + old_hash = block.block.header.parent_hash; + current_canonical_number -= 1; + } else { + // This shouldn't happen as we're walking back the canonical chain + warn!(target: "engine::tree", current_hash=?old_hash, "Canonical block not found in TreeState"); + return Ok(None); + } + } + while old_hash != current_hash { if let Some(block) = self.executed_block_by_hash(old_hash)? { old_hash = block.block.header.parent_hash; @@ -927,28 +939,31 @@ where /// extension of the canonical chain. /// * walking back from the current head to verify that the target hash is not already part of /// the canonical chain. - fn is_fork(&self, target_hash: B256, finalized_hash: Option) -> ProviderResult { + fn is_fork(&self, target_hash: B256, _finalized_hash: Option) -> ProviderResult { // verify that the given hash is not part of an extension of the canon chain. + let canonical_head = self.state.tree_state.canonical_head(); let mut current_hash = target_hash; while let Some(current_block) = self.sealed_header_by_hash(current_hash)? { - if current_block.hash() == self.state.tree_state.canonical_block_hash() { + if current_block.hash() == canonical_head.hash { return Ok(false) } + // We already passed the canonical head + if current_block.number <= canonical_head.number { + break + } current_hash = current_block.parent_hash; } - // verify that the given hash is not already part of the canon chain - current_hash = self.state.tree_state.canonical_block_hash(); - while let Some(current_block) = self.sealed_header_by_hash(current_hash)? { - if Some(current_hash) == finalized_hash { - return Ok(true) - } + // verify that the given hash is not already part of canonical chain stored in memory + if self.canonical_in_memory_state.header_by_hash(target_hash).is_some() { + return Ok(false) + } - if current_block.hash() == target_hash { - return Ok(false) - } - current_hash = current_block.parent_hash; + // verify that the given hash is not already part of persisted canonical chain + if self.provider.block_number(target_hash)?.is_some() { + return Ok(false) } + Ok(true) } From f09fa299fae6383de52339c9bc240e26efd7c1b1 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Tue, 1 Oct 2024 21:02:06 +0200 Subject: [PATCH 2/3] rm finalized hash --- crates/engine/tree/src/tree/mod.rs | 64 +++++++----------------------- 1 file changed, 15 insertions(+), 49 deletions(-) diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index 4ac665752a79..969ab4487b67 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -453,8 +453,6 @@ pub enum TreeAction { MakeCanonical { /// The sync target head hash sync_target_head: B256, - /// The sync target finalized hash - sync_target_finalized: Option, }, } @@ -814,22 +812,9 @@ where let mut outcome = TreeOutcome::new(status); if outcome.outcome.is_valid() && self.is_sync_target_head(block_hash) { - // NOTE: if we are in this branch, `is_sync_target_head` has returned true, - // meaning a sync target state exists, so we can safely unwrap - let sync_target = self - .state - .forkchoice_state_tracker - .sync_target_state() - .expect("sync target must exist"); - - // if the hash is zero then we should act like there is no finalized hash - let sync_target_finalized = (!sync_target.finalized_block_hash.is_zero()) - .then_some(sync_target.finalized_block_hash); - // if the block is valid and it is the sync target head, make it canonical outcome = outcome.with_event(TreeEvent::TreeAction(TreeAction::MakeCanonical { sync_target_head: block_hash, - sync_target_finalized, })); } @@ -842,11 +827,7 @@ where /// /// Note: This does not update the tracked state and instead returns the new chain based on the /// given head. - fn on_new_head( - &self, - new_head: B256, - finalized_block: Option, - ) -> ProviderResult> { + fn on_new_head(&self, new_head: B256) -> ProviderResult> { // get the executed new head block let Some(new_head_block) = self.state.tree_state.blocks_by_hash.get(&new_head) else { return Ok(None) @@ -917,7 +898,7 @@ where } if let Some(block) = self.executed_block_by_hash(current_hash)? { - if self.is_fork(block.block.hash(), finalized_block)? { + if self.is_fork(block.block.hash())? { current_hash = block.block.parent_hash; new_chain.push(block); } @@ -939,7 +920,7 @@ where /// extension of the canonical chain. /// * walking back from the current head to verify that the target hash is not already part of /// the canonical chain. - fn is_fork(&self, target_hash: B256, _finalized_hash: Option) -> ProviderResult { + fn is_fork(&self, target_hash: B256) -> ProviderResult { // verify that the given hash is not part of an extension of the canon chain. let canonical_head = self.state.tree_state.canonical_head(); let mut current_hash = target_hash; @@ -1037,11 +1018,8 @@ where return Ok(valid_outcome(state.head_block_hash)) } - let finalized_block_opt = - (!state.finalized_block_hash.is_zero()).then_some(state.finalized_block_hash); - // 2. ensure we can apply a new chain update for the head block - if let Some(chain_update) = self.on_new_head(state.head_block_hash, finalized_block_opt)? { + if let Some(chain_update) = self.on_new_head(state.head_block_hash)? { let tip = chain_update.tip().header.clone(); self.on_canonical_chain_update(chain_update); @@ -1370,8 +1348,8 @@ where /// Attempts to make the given target canonical. /// /// This will update the tracked canonical in memory state and do the necessary housekeeping. - fn make_canonical(&mut self, target: B256, finalized: Option) -> ProviderResult<()> { - if let Some(chain_update) = self.on_new_head(target, finalized)? { + fn make_canonical(&mut self, target: B256) -> ProviderResult<()> { + if let Some(chain_update) = self.on_new_head(target)? { self.on_canonical_chain_update(chain_update); } @@ -1391,8 +1369,8 @@ where fn on_tree_event(&mut self, event: TreeEvent) -> ProviderResult<()> { match event { TreeEvent::TreeAction(action) => match action { - TreeAction::MakeCanonical { sync_target_head, sync_target_finalized } => { - self.make_canonical(sync_target_head, sync_target_finalized)?; + TreeAction::MakeCanonical { sync_target_head } => { + self.make_canonical(sync_target_head)?; } }, TreeEvent::BackfillAction(action) => { @@ -1762,12 +1740,7 @@ where if self.is_sync_target_head(child_num_hash.hash) && matches!(res, InsertPayloadOk2::Inserted(BlockStatus2::Valid)) { - // we are using the sync target here because we're trying to make the sync - // target canonical - let sync_target_finalized = - self.state.forkchoice_state_tracker.sync_target_finalized(); - - self.make_canonical(child_num_hash.hash, sync_target_finalized)?; + self.make_canonical(child_num_hash.hash)?; } } Err(err) => { @@ -2075,14 +2048,11 @@ where Ok(InsertPayloadOk2::Inserted(BlockStatus2::Valid)) => { if self.is_sync_target_head(block_num_hash.hash) { trace!(target: "engine::tree", "appended downloaded sync target block"); - let sync_target_finalized = - self.state.forkchoice_state_tracker.sync_target_finalized(); // we just inserted the current sync target block, we can try to make it // canonical return Ok(Some(TreeEvent::TreeAction(TreeAction::MakeCanonical { sync_target_head: block_num_hash.hash, - sync_target_finalized, }))) } trace!(target: "engine::tree", "appended downloaded block"); @@ -2273,13 +2243,9 @@ where self.state.tree_state.insert_executed(executed); self.metrics.engine.executed_blocks.set(self.state.tree_state.block_count() as f64); - // we are checking that this is a fork block compared to the current `SYNCING` forkchoice - // state. - let finalized = self.state.forkchoice_state_tracker.sync_target_finalized(); - // emit insert event let elapsed = start.elapsed(); - let engine_event = if self.is_fork(block_hash, finalized)? { + let engine_event = if self.is_fork(block_hash)? { BeaconConsensusEngineEvent::ForkBlockAdded(sealed_block, elapsed) } else { BeaconConsensusEngineEvent::CanonicalBlockAdded(sealed_block, elapsed) @@ -3415,7 +3381,7 @@ mod tests { test_harness.tree.state.tree_state.insert_executed(fork_block_5.clone()); // normal (non-reorg) case - let result = test_harness.tree.on_new_head(blocks[4].block.hash(), None).unwrap(); + let result = test_harness.tree.on_new_head(blocks[4].block.hash()).unwrap(); assert!(matches!(result, Some(NewCanonicalChain::Commit { .. }))); if let Some(NewCanonicalChain::Commit { new }) = result { assert_eq!(new.len(), 2); @@ -3424,7 +3390,7 @@ mod tests { } // reorg case - let result = test_harness.tree.on_new_head(fork_block_5.block.hash(), None).unwrap(); + let result = test_harness.tree.on_new_head(fork_block_5.block.hash()).unwrap(); assert!(matches!(result, Some(NewCanonicalChain::Reorg { .. }))); if let Some(NewCanonicalChain::Reorg { new, old }) = result { assert_eq!(new.len(), 3); @@ -3485,7 +3451,7 @@ mod tests { let mut expected_new = Vec::new(); for block in &chain_b { // reorg to chain from block b - let result = test_harness.tree.on_new_head(block.block.hash(), None).unwrap(); + let result = test_harness.tree.on_new_head(block.block.hash()).unwrap(); assert_matches!(result, Some(NewCanonicalChain::Reorg { .. })); expected_new.push(block); @@ -3502,7 +3468,7 @@ mod tests { } // set last block of chain a as canonical head - test_harness.tree.on_new_head(chain_a.last().unwrap().hash(), None).unwrap(); + test_harness.tree.on_new_head(chain_a.last().unwrap().hash()).unwrap(); } } @@ -3927,7 +3893,7 @@ mod tests { test_harness.check_canon_head(chain_b_tip_hash); // verify that chain A is now considered a fork - assert!(test_harness.tree.is_fork(chain_a.last().unwrap().hash(), None).unwrap()); + assert!(test_harness.tree.is_fork(chain_a.last().unwrap().hash()).unwrap()); } #[tokio::test] From 5047dcfd8c16ae0d8f107d8f318d411ffa26ce64 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Tue, 1 Oct 2024 21:37:49 +0200 Subject: [PATCH 3/3] docs --- crates/engine/tree/src/tree/mod.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/crates/engine/tree/src/tree/mod.rs b/crates/engine/tree/src/tree/mod.rs index 969ab4487b67..1c79fa3ef88f 100644 --- a/crates/engine/tree/src/tree/mod.rs +++ b/crates/engine/tree/src/tree/mod.rs @@ -870,6 +870,8 @@ where let mut old_chain = Vec::new(); let mut old_hash = self.state.tree_state.current_canonical_head.hash; + // If the canonical chain is ahead of the new chain, + // gather all blocks until new head number. while current_canonical_number > current_number { if let Some(block) = self.executed_block_by_hash(old_hash)? { old_chain.push(block.clone()); @@ -882,6 +884,11 @@ where } } + // Both new and old chain pointers are now at the same height. + debug_assert_eq!(current_number, current_canonical_number); + + // Walk both chains from specified hashes at same height until + // a common ancestor (fork block) is reached. while old_hash != current_hash { if let Some(block) = self.executed_block_by_hash(old_hash)? { old_hash = block.block.header.parent_hash;