chore(engine): refactor pipeline outcome processing #7692
Conversation
rkrasiuk
added
C-debt
| 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 | ||
| ) |
There was a problem hiding this comment.
BlockchainTreeError::BlockHashNotFoundInChain is never fatal
|
|
||
| let new_head = outcome.into_header(); | ||
| debug!(target: "consensus::engine", hash=?new_head.hash(), number=new_head.number, "Canonicalized new head"); | ||
| let Some(target) = self.forkchoice_state_tracker.sync_target_state() else { return Ok(()) }; |
There was a problem hiding this comment.
this only flattens if let Some(_) { ... } else { return Ok(()) } to let Some(_) = else { return Ok(()) }; ...
| // Terminate the sync early if it's reached the maximum user-configured block. | ||
| SyncEventOutcome::ReachedMaxBlock |
There was a problem hiding this comment.
returns Ok(SyncEventOutcome::ReachedMaxBlock) instead of Some(Ok(()))
| EngineSyncEvent::PipelineFinished { result, reached_max_block } => { | ||
| trace!(target: "consensus::engine", ?result, ?reached_max_block, "Pipeline finished"); | ||
| // Any pipeline error at this point is fatal. | ||
| let ctrl = result?; |
There was a problem hiding this comment.
any pipeline error is considered fatal, same as before
| let newest_finalized = self | ||
| .forkchoice_state_tracker | ||
| .sync_target_state() | ||
| .map(|s| s.finalized_block_hash) | ||
| .and_then(|h| self.blockchain.buffered_header_by_hash(h)) | ||
| .map(|header| header.number); |
There was a problem hiding this comment.
this unnecessarily retrieved optional self.forkchoice_state_tracker.sync_target_state(), but we already retrieved and ensured that it's some up above
| let pipeline_target = if let (Some(progress), Some(finalized_number)) = | ||
| (ctrl.block_number(), newest_finalized) | ||
| { | ||
| // Determines whether or not we should run the pipeline again, in case the | ||
| // new gap is large enough to warrant running the pipeline. | ||
| self.can_pipeline_sync_to_finalized(progress, finalized_number, None) | ||
| } else { | ||
| None | ||
| }; |
There was a problem hiding this comment.
if let (Some(), Some()) { /* invoke fn that returns an option */ } else { None } is simply refactored to .zip().and_then(...)
|
Preemptive hive run, because why not |
|
hive results are unchanged |
Description
Refactor and simplify the processing of pipeline outcome.
Remove the anti-pattern of returning
Option<Result<(), _>>where the only use case for returningSome(Ok(()))was to indicate that the pipeline has reached the max block.Gets rid of
match resultstatements and uses residual instead.