Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(engine): is_fork header traversal #11368

Merged
merged 3 commits into from
Oct 1, 2024
+49 −61
Merged

fix(engine): is_fork header traversal #11368

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc43d6e
fix(engine): `is_fork` header traversal
rkrasiuk Oct 1, 2024
f09fa29
rm finalized hash
rkrasiuk Oct 1, 2024
5047dcf
docs
rkrasiuk Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
110 changes: 49 additions & 61 deletions crates/engine/tree/src/tree/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<B256>,
},
}

Expand Down Expand Up @@ -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,
}));
}

Expand All @@ -842,18 +827,14 @@ 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<B256>,
) -> ProviderResult<Option<NewCanonicalChain>> {
fn on_new_head(&self, new_head: B256) -> ProviderResult<Option<NewCanonicalChain>> {
// 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)
};

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;
Expand Down Expand Up @@ -889,6 +870,25 @@ 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());
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);
}
}
mattsse marked this conversation as resolved.
Show resolved Hide resolved

// 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;
Expand All @@ -905,7 +905,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);
}
Expand All @@ -927,28 +927,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<B256>) -> ProviderResult<bool> {
fn is_fork(&self, target_hash: B256) -> ProviderResult<bool> {
// 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
}
Comment on lines 934 to +941
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this loop is now safe and cuts off at the canonical head which is correct because this is the section we need to check here.

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)
}

Expand Down Expand Up @@ -1022,11 +1025,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);

Expand Down Expand Up @@ -1355,8 +1355,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<B256>) -> 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);
}

Expand All @@ -1376,8 +1376,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) => {
Expand Down Expand Up @@ -1747,12 +1747,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) => {
Expand Down Expand Up @@ -2060,14 +2055,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");
Expand Down Expand Up @@ -2258,13 +2250,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)
Expand Down Expand Up @@ -3400,7 +3388,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);
Expand All @@ -3409,7 +3397,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);
Expand Down Expand Up @@ -3470,7 +3458,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);
Expand All @@ -3487,7 +3475,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();
}
}

Expand Down Expand Up @@ -3912,7 +3900,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]
Expand Down
Loading