From 07dfb9fdc43cf63d0312ac64f37dd1f43e6fc537 Mon Sep 17 00:00:00 2001 From: Roman Krasiuk Date: Mon, 27 May 2024 15:36:56 +0200 Subject: [PATCH] chore(engine): tree action on downloaded block (#8409) --- crates/consensus/beacon/src/engine/mod.rs | 158 +++++++++++----------- 1 file changed, 79 insertions(+), 79 deletions(-) diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index c1ef622874..039c14e422 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -1271,84 +1271,6 @@ where Ok(PayloadStatus::new(status, latest_valid_hash)) } - /// Invoked if we successfully downloaded a new block from the network. - /// - /// This will attempt to insert the block into the tree. - /// - /// There are several scenarios: - /// - /// ## [BlockStatus::Valid] - /// - /// The block is connected to the current canonical chain and is valid. - /// If the block is an ancestor of the current forkchoice head, then we can try again to make - /// the chain canonical. - /// - /// ## [BlockStatus::Disconnected] - /// - /// The block is not connected to the canonical chain, and we need to download the missing - /// parent first. - /// - /// ## Insert Error - /// - /// If the insertion into the tree failed, then the block was well-formed (valid hash), but its - /// chain is invalid, which means the FCU that triggered the download is invalid. Here we can - /// stop because there's nothing to do here and the engine needs to wait for another FCU. - fn on_downloaded_block(&mut self, block: SealedBlock) { - let downloaded_num_hash = block.num_hash(); - trace!(target: "consensus::engine", hash=?block.hash(), number=%block.number, "Downloaded full block"); - // check if the block's parent is already marked as invalid - if self.check_invalid_ancestor_with_head(block.parent_hash, block.hash()).is_some() { - // can skip this invalid block - return - } - - match self - .blockchain - .insert_block_without_senders(block, BlockValidationKind::SkipStateRootValidation) - { - Ok(status) => { - match status { - InsertPayloadOk::Inserted(BlockStatus::Valid(_)) => { - // block is connected to the canonical chain and is valid. - // if it's not connected to current canonical head, the state root - // has not been validated. - if let Err((hash, error)) = - self.try_make_sync_target_canonical(downloaded_num_hash) - { - if error.is_fatal() { - error!(target: "consensus::engine", %error, "Encountered fatal error while making sync target canonical: {:?}, {:?}", error, hash); - } else if !error.is_block_hash_not_found() { - debug!( - target: "consensus::engine", - "Unexpected error while making sync target canonical: {:?}, {:?}", - error, - hash - ) - } - } - } - InsertPayloadOk::Inserted(BlockStatus::Disconnected { - missing_ancestor: missing_parent, - }) => { - // block is not connected to the canonical head, we need to download its - // missing branch first - self.on_disconnected_block(downloaded_num_hash, missing_parent); - } - _ => (), - } - } - Err(err) => { - warn!(target: "consensus::engine", %err, "Failed to insert downloaded block"); - if err.kind().is_invalid_block() { - let (block, err) = err.split(); - warn!(target: "consensus::engine", invalid_number=?block.number, invalid_hash=?block.hash(), %err, "Marking block as invalid"); - - self.invalid_headers.insert(block.header); - } - } - } - } - /// This handles downloaded blocks that are shown to be disconnected from the canonical chain. /// /// This mainly compares the missing parent of the downloaded block with the current canonical @@ -1476,7 +1398,15 @@ where ) -> Result { let outcome = match event { EngineSyncEvent::FetchedFullBlock(block) => { - self.on_downloaded_block(block); + trace!(target: "consensus::engine", hash=?block.hash(), number=%block.number, "Downloaded full block"); + // Insert block only if the block's parent is not marked as invalid + if self.check_invalid_ancestor_with_head(block.parent_hash, block.hash()).is_none() + { + let previous_action = self + .blockchain_tree_action + .replace(BlockchainTreeAction::InsertDownloadedPayload { block }); + debug_assert!(previous_action.is_none(), "Pre-existing action found"); + } EngineEventOutcome::Processed } EngineSyncEvent::PipelineStarted(target) => { @@ -1794,6 +1724,55 @@ where trace!(target: "consensus::engine", ?status, "Returning payload status"); let _ = tx.send(Ok(status)); } + + BlockchainTreeAction::InsertDownloadedPayload { block } => { + let downloaded_num_hash = block.num_hash(); + match self.blockchain.insert_block_without_senders( + block, + BlockValidationKind::SkipStateRootValidation, + ) { + Ok(status) => { + match status { + InsertPayloadOk::Inserted(BlockStatus::Valid(_)) => { + // block is connected to the canonical chain and is valid. + // if it's not connected to current canonical head, the state root + // has not been validated. + if let Err((hash, error)) = + self.try_make_sync_target_canonical(downloaded_num_hash) + { + if error.is_fatal() { + error!(target: "consensus::engine", %error, "Encountered fatal error while making sync target canonical: {:?}, {:?}", error, hash); + } else if !error.is_block_hash_not_found() { + debug!( + target: "consensus::engine", + "Unexpected error while making sync target canonical: {:?}, {:?}", + error, + hash + ) + } + } + } + InsertPayloadOk::Inserted(BlockStatus::Disconnected { + missing_ancestor: missing_parent, + }) => { + // block is not connected to the canonical head, we need to download + // its missing branch first + self.on_disconnected_block(downloaded_num_hash, missing_parent); + } + _ => (), + } + } + Err(err) => { + warn!(target: "consensus::engine", %err, "Failed to insert downloaded block"); + if err.kind().is_invalid_block() { + let (block, err) = err.split(); + warn!(target: "consensus::engine", invalid_number=?block.number, invalid_hash=?block.hash(), %err, "Marking block as invalid"); + + self.invalid_headers.insert(block.header); + } + } + } + } }; Ok(EngineEventOutcome::Processed) } @@ -1935,6 +1914,27 @@ enum BlockchainTreeAction { status: PayloadStatus, tx: oneshot::Sender>, }, + /// Action to insert a new block that we successfully downloaded from the network. + /// There are several outcomes for inserting a downloaded block into the tree: + /// + /// ## [BlockStatus::Valid] + /// + /// The block is connected to the current canonical chain and is valid. + /// If the block is an ancestor of the current forkchoice head, then we can try again to + /// make the chain canonical. + /// + /// ## [BlockStatus::Disconnected] + /// + /// The block is not connected to the canonical chain, and we need to download the + /// missing parent first. + /// + /// ## Insert Error + /// + /// If the insertion into the tree failed, then the block was well-formed (valid hash), + /// but its chain is invalid, which means the FCU that triggered the + /// download is invalid. Here we can stop because there's nothing to do here + /// and the engine needs to wait for another FCU. + InsertDownloadedPayload { block: SealedBlock }, } /// Represents outcomes of processing an engine event