diff --git a/crates/blockchain-tree/src/blockchain_tree.rs b/crates/blockchain-tree/src/blockchain_tree.rs index dcb69c6f1..975f46666 100644 --- a/crates/blockchain-tree/src/blockchain_tree.rs +++ b/crates/blockchain-tree/src/blockchain_tree.rs @@ -1013,7 +1013,6 @@ impl BlockchainTree { self.commit_canonical_to_database(new_canon_chain, &mut durations_recorder)?; } else { // it forks to canonical block that is not the tip. - let canon_fork: BlockNumHash = new_canon_chain.fork_block(); // sanity check if self.block_indices().canonical_hash(&canon_fork.number) != Some(canon_fork.hash) { diff --git a/crates/consensus/beacon/src/engine/mod.rs b/crates/consensus/beacon/src/engine/mod.rs index 74f1f21b5..2d456e5de 100644 --- a/crates/consensus/beacon/src/engine/mod.rs +++ b/crates/consensus/beacon/src/engine/mod.rs @@ -430,7 +430,7 @@ where Err(error) => { if let RethError::Canonical(ref err) = error { if err.is_fatal() { - tracing::error!(target: "consensus::engine", %err, "Encountered fatal error"); + error!(target: "consensus::engine", %err, "Encountered fatal error"); return Err(error) } } @@ -1080,7 +1080,7 @@ where /// /// This returns a [`PayloadStatus`] that represents the outcome of a processed new payload and /// returns an error if an internal error occurred. - #[instrument(level = "trace", skip(self, payload, cancun_fields), fields(block_hash= ?payload.block_hash(), block_number = %payload.block_number(), is_pipeline_idle = %self.sync.is_pipeline_idle()), target = "consensus::engine")] + #[instrument(level = "trace", skip(self, payload, cancun_fields), fields(block_hash = ?payload.block_hash(), block_number = %payload.block_number(), is_pipeline_idle = %self.sync.is_pipeline_idle()), target = "consensus::engine")] fn on_new_payload( &mut self, payload: ExecutionPayload, @@ -1120,7 +1120,30 @@ where // if we're currently syncing and the inserted block is the targeted FCU // head block, we can try to make it canonical. if block_hash == target.head_block_hash { - self.try_make_sync_target_canonical(block_num_hash); + if let Err((_hash, error)) = + self.try_make_sync_target_canonical(block_num_hash) + { + if let RethError::Canonical(ref err) = error { + if err.is_fatal() { + error!(target: "consensus::engine", %err, "Encountered fatal error"); + return Err(BeaconOnNewPayloadError::Internal(Box::new( + error, + ))) + } + } else { + // If we could not make the sync target block canonical, we + // should return the error as an invalid payload status. + return Ok(PayloadStatus::new( + PayloadStatusEnum::Invalid { + validation_error: error.to_string(), + }, + // TODO: return a proper latest valid hash + // + // See: + self.forkchoice_state_tracker.last_valid_head(), + )) + } + } } } // block was successfully inserted, so we can cancel the full block request, if @@ -1148,21 +1171,22 @@ where /// - the versioned hashes passed with the payload do not exactly match transaction /// versioned hashes /// - the block does not contain blob transactions if it is pre-cancun - // This validates the following engine API rule: - // - // 3. Given the expected array of blob versioned hashes client software **MUST** run its - // validation by taking the following steps: - // - // 1. Obtain the actual array by concatenating blob versioned hashes lists - // (`tx.blob_versioned_hashes`) of each [blob - // transaction](https://eips.ethereum.org/EIPS/eip-4844#new-transaction-type) included - // in the payload, respecting the order of inclusion. If the payload has no blob - // transactions the expected array **MUST** be `[]`. - // - // 2. Return `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` - // if the expected and the actual arrays don't match. - // - // This validation **MUST** be instantly run in all cases even during active sync process. + /// + /// This validates the following engine API rule: + /// + /// 3. Given the expected array of blob versioned hashes client software **MUST** run its + /// validation by taking the following steps: + /// + /// 1. Obtain the actual array by concatenating blob versioned hashes lists + /// (`tx.blob_versioned_hashes`) of each [blob + /// transaction](https://eips.ethereum.org/EIPS/eip-4844#new-transaction-type) included + /// in the payload, respecting the order of inclusion. If the payload has no blob + /// transactions the expected array **MUST** be `[]`. + /// + /// 2. Return `{status: INVALID, latestValidHash: null, validationError: errorMessage | null}` + /// if the expected and the actual arrays don't match. + /// + /// This validation **MUST** be instantly run in all cases even during active sync process. fn ensure_well_formed_payload( &self, payload: ExecutionPayload, @@ -1277,6 +1301,7 @@ where let status = self .blockchain .insert_block_without_senders(block.clone(), BlockValidationKind::Exhaustive)?; + let elapsed = start.elapsed(); let mut latest_valid_hash = None; let block = Arc::new(block); @@ -1402,7 +1427,16 @@ where // 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. - self.try_make_sync_target_canonical(downloaded_num_hash); + if let Err((hash, error)) = + self.try_make_sync_target_canonical(downloaded_num_hash) + { + tracing::error!( + target: "consensus::engine", + "Unexpected error while making sync target canonical: {:?}, {:?}", + error, + hash + ) + } } InsertPayloadOk::Inserted(BlockStatus::Disconnected { missing_ancestor: missing_parent, @@ -1484,7 +1518,10 @@ where /// Note: This will not succeed if the sync target has changed since the block download request /// was issued and the new target is still disconnected and additional missing blocks are /// downloaded - fn try_make_sync_target_canonical(&mut self, inserted: BlockNumHash) { + fn try_make_sync_target_canonical( + &mut self, + inserted: BlockNumHash, + ) -> Result<(), (B256, RethError)> { if let Some(target) = self.forkchoice_state_tracker.sync_target_state() { // optimistically try to make the head of the current FCU target canonical, the sync // target might have changed since the block download request was issued @@ -1514,6 +1551,7 @@ where // clear any active block requests self.sync.clear_block_download_requests(); + Ok(()) } Err(err) => { // if we failed to make the FCU's head canonical, because we don't have that @@ -1532,11 +1570,16 @@ where if let Some(target_hash) = ForkchoiceStateHash::find(&target, inserted.hash) .filter(|h| !h.is_head()) { + // TODO: do not ignore this let _ = self.blockchain.make_canonical(target_hash.as_ref()); } } + + Err((target.head_block_hash, err)) } } + } else { + Ok(()) } } diff --git a/crates/interfaces/src/blockchain_tree/error.rs b/crates/interfaces/src/blockchain_tree/error.rs index f5b02ab71..fb41c7b54 100644 --- a/crates/interfaces/src/blockchain_tree/error.rs +++ b/crates/interfaces/src/blockchain_tree/error.rs @@ -4,6 +4,7 @@ use crate::{ consensus::ConsensusError, executor::{BlockExecutionError, BlockValidationError}, provider::ProviderError, + RethError, }; use reth_primitives::{BlockHash, BlockNumber, SealedBlock}; @@ -110,6 +111,11 @@ impl InsertBlockError { Self::new(block, InsertBlockErrorKind::Execution(error)) } + /// Create a new InsertBlockError from a RethError and block. + pub fn from_reth_error(error: RethError, block: SealedBlock) -> Self { + Self::new(block, error.into()) + } + /// Consumes the error and returns the block that resulted in the error #[inline] pub fn into_block(self) -> SealedBlock { @@ -324,11 +330,9 @@ impl InsertBlockErrorKind { } } -// This is a convenience impl to convert from crate::Error to InsertBlockErrorKind, most -impl From for InsertBlockErrorKind { - fn from(err: crate::RethError) -> Self { - use crate::RethError; - +// This is a convenience impl to convert from crate::Error to InsertBlockErrorKind +impl From for InsertBlockErrorKind { + fn from(err: RethError) -> Self { match err { RethError::Execution(err) => InsertBlockErrorKind::Execution(err), RethError::Consensus(err) => InsertBlockErrorKind::Consensus(err),