From 8041370b08a46bf318cf444fdd7883940fafa288 Mon Sep 17 00:00:00 2001 From: Mac L Date: Tue, 5 Apr 2022 18:34:31 +1000 Subject: [PATCH] Remove redundent calls to `get_block` --- beacon_node/beacon_chain/src/beacon_chain.rs | 69 ++++++++----------- .../beacon_chain/src/beacon_proposer_cache.rs | 2 +- beacon_node/http_api/src/block_id.rs | 32 +++++---- beacon_node/http_api/src/lib.rs | 43 ++++++------ beacon_node/http_api/src/proposer_duties.rs | 2 +- beacon_node/http_api/src/state_id.rs | 2 +- beacon_node/http_api/src/sync_committees.rs | 2 +- 7 files changed, 74 insertions(+), 78 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6bf2aebfa6f..fa58ee7f04e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1420,7 +1420,7 @@ impl BeaconChain { }) .collect(); - let execution_optimistic = self.is_optimistic_head(Some(head_block_root))?; + let execution_optimistic = self.is_optimistic_head()?; Ok((duties, dependent_root, execution_optimistic)) }) @@ -2819,7 +2819,7 @@ impl BeaconChain { snapshot_cache.insert( BeaconSnapshot { beacon_state: state, - beacon_block: signed_block, + beacon_block: signed_block.clone(), beacon_block_root: block_root, }, None, @@ -2844,7 +2844,7 @@ impl BeaconChain { event_handler.register(EventKind::Block(SseBlock { slot, block: block_root, - execution_optimistic: self.is_optimistic_block(&block_root)?, + execution_optimistic: self.is_optimistic_block(&signed_block)?, })); } } @@ -3643,8 +3643,7 @@ impl BeaconChain { current_duty_dependent_root, previous_duty_dependent_root, epoch_transition: is_epoch_transition, - execution_optimistic: self - .is_optimistic_head(Some(beacon_block_root))?, + execution_optimistic: self.is_optimistic_head()?, })); } (Err(e), _) | (_, Err(e)) => { @@ -3666,7 +3665,7 @@ impl BeaconChain { new_head_block: beacon_block_root, new_head_state: state_root, epoch: head_slot.epoch(T::EthSpec::slots_per_epoch()), - execution_optimistic: self.is_optimistic_head(Some(current_head.block_root))?, + execution_optimistic: self.is_optimistic_head()?, })); } @@ -3692,7 +3691,7 @@ impl BeaconChain { observed_delay: block_delays.observed, imported_delay: block_delays.imported, set_as_head_delay: block_delays.set_as_head, - execution_optimistic: self.is_optimistic_head(Some(beacon_block_root))?, + execution_optimistic: self.is_optimistic_head()?, })); } } @@ -4098,48 +4097,40 @@ impl BeaconChain { } } - /// Returns the value of `execution_optimistic` for `block_root`. + /// Returns the value of `execution_optimistic` for `block`. /// /// Returns `Ok(false)` if the block is pre-Bellatrix, or has `ExecutionStatus::Valid`. /// Returns `Ok(true)` if the block has `ExecutionStatus::Unknown`. - pub fn is_optimistic_block(&self, block_root: &Hash256) -> Result { - if let Some(block) = self.get_block(block_root)? { - // Check if the block is pre-Bellatrix. - if block.message().execution_payload().is_err() { - Ok(false) - } else { - self.fork_choice - .read() - .is_optimistic_block(block_root) - .map_err(BeaconChainError::ForkChoiceError) - } + pub fn is_optimistic_block( + &self, + block: &SignedBeaconBlock, + ) -> Result { + // Check if the block is pre-Bellatrix. + if block.message().execution_payload().is_err() { + Ok(false) } else { - Err(BeaconChainError::MissingBeaconBlock(*block_root)) + self.fork_choice + .read() + .is_optimistic_block(&block.canonical_root()) + .map_err(BeaconChainError::ForkChoiceError) } } - /// Returns the value of `execution_optimistic` for `head_block_root` which can be provided - /// optionally if available. + /// Returns the value of `execution_optimistic` for the current head block. /// /// Returns `Ok(false)` if the head block is pre-Bellatrix, or has `ExecutionStatus::Valid`. /// Returns `Ok(true)` if the head block has `ExecutionStatus::Unknown`. - pub fn is_optimistic_head( - &self, - head_block_root: Option, - ) -> Result { - let head_block_root = head_block_root.unwrap_or(self.head_info()?.block_root); - if let Some(block) = self.get_block(&head_block_root)? { - // Check if the block is pre-Bellatrix. - if block.message().execution_payload().is_err() { - Ok(false) - } else { - self.fork_choice - .read() - .is_optimistic_block_no_fallback(&head_block_root) - .map_err(BeaconChainError::ForkChoiceError) - } + pub fn is_optimistic_head(&self) -> Result { + let head_info = self.head_info()?; + let head_block_root = head_info.block_root; + // Check if the block is pre-Bellatrix. + if head_info.execution_payload_block_hash.is_none() { + Ok(false) } else { - Err(BeaconChainError::MissingBeaconBlock(head_block_root)) + self.fork_choice + .read() + .is_optimistic_block_no_fallback(&head_block_root) + .map_err(BeaconChainError::ForkChoiceError) } } @@ -4282,7 +4273,7 @@ impl BeaconChain { epoch: new_finalized_checkpoint.epoch, block: new_finalized_checkpoint.root, state: new_finalized_state_root, - execution_optimistic: self.is_optimistic_head(None)?, + execution_optimistic: self.is_optimistic_head()?, })); } } diff --git a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs index 692c19b64b8..1ea3ce7a9e5 100644 --- a/beacon_node/beacon_chain/src/beacon_proposer_cache.rs +++ b/beacon_node/beacon_chain/src/beacon_proposer_cache.rs @@ -153,7 +153,7 @@ pub fn compute_proposer_duties_from_head( .proposer_shuffling_decision_root(chain.genesis_block_root) .map_err(BeaconChainError::from)?; - let execution_optimistic = chain.is_optimistic_head(Some(head.beacon_block_root))?; + let execution_optimistic = chain.is_optimistic_head()?; Ok((indices, dependent_root, execution_optimistic, state.fork())) } diff --git a/beacon_node/http_api/src/block_id.rs b/beacon_node/http_api/src/block_id.rs index 76a3282b1f4..1b4acaad303 100644 --- a/beacon_node/http_api/src/block_id.rs +++ b/beacon_node/http_api/src/block_id.rs @@ -98,26 +98,28 @@ impl BlockId { } } - /// Return the `execution_optimistic` value identified by `self`. - pub fn is_execution_optimistic( + /// Returns the `block` along with the `execution_optimistic` value identified by `self`. + pub fn block_and_is_execution_optimistic( &self, chain: &BeaconChain, - ) -> Result { - match self.0 { + ) -> Result<(SignedBeaconBlock, bool), warp::Rejection> { + let block = self.block(chain)?; + let execution_optimistic = match self.0 { // Genesis block is inherently verified. - CoreBlockId::Genesis => Ok(false), - CoreBlockId::Head => Ok(chain - .is_optimistic_head(None) - .map_err(warp_utils::reject::beacon_chain_error)?), + CoreBlockId::Genesis => false, + CoreBlockId::Head => chain + .is_optimistic_head() + .map_err(warp_utils::reject::beacon_chain_error)?, // Slot, Finalized and Justified are determined based on the current head. - CoreBlockId::Slot(_) | CoreBlockId::Finalized | CoreBlockId::Justified => Ok(chain - .is_optimistic_head(None) - .map_err(warp_utils::reject::beacon_chain_error)?), + CoreBlockId::Slot(_) | CoreBlockId::Finalized | CoreBlockId::Justified => chain + .is_optimistic_head() + .map_err(warp_utils::reject::beacon_chain_error)?, // If the root is explicitly given, we can determine based on fork-choice. - CoreBlockId::Root(block_root) => Ok(chain - .is_optimistic_block(&block_root) - .map_err(warp_utils::reject::beacon_chain_error)?), - } + CoreBlockId::Root(_) => chain + .is_optimistic_block(&block) + .map_err(warp_utils::reject::beacon_chain_error)?, + }; + Ok((block, execution_optimistic)) } } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 558ae3374eb..f7145d48d53 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -897,8 +897,8 @@ pub fn serve( // The value of `execution_optimistic` depends on whether the method used to // compute the response is dependent on the head block. let execution_optimistic = match uses_head { - false => chain.is_optimistic_block(&root), - true => chain.is_optimistic_head(Some(root)), + false => chain.is_optimistic_block(&block), + true => chain.is_optimistic_head(), } .map_err(warp_utils::reject::beacon_chain_error)?; @@ -931,15 +931,14 @@ pub fn serve( .and_then(|block_id: BlockId, chain: Arc>| { blocking_json_task(move || { let root = block_id.root(&chain)?; - let block = BlockId::from_root(root).block(&chain)?; + let (block, execution_optimistic) = + BlockId::from_root(root).block_and_is_execution_optimistic(&chain)?; let canonical = chain .block_root_at_slot(block.slot(), WhenSlotSkipped::None) .map_err(warp_utils::reject::beacon_chain_error)? .map_or(false, |canonical| root == canonical); - let execution_optimistic = block_id.is_execution_optimistic(&chain)?; - let data = api_types::BlockHeaderData { root, canonical, @@ -1199,11 +1198,12 @@ pub fn serve( chain: Arc>, accept_header: Option| { blocking_task(move || { - let block = block_id.block(&chain)?; + let (block, execution_optimistic) = + block_id.block_and_is_execution_optimistic(&chain)?; let fork_name = block .fork_name(&chain.spec) .map_err(inconsistent_fork_rejection)?; - let execution_optimistic = block_id.is_execution_optimistic(&chain)?; + match accept_header { Some(api_types::Accept::Ssz) => Response::builder() .status(200) @@ -1235,12 +1235,13 @@ pub fn serve( .and(warp::path::end()) .and_then(|block_id: BlockId, chain: Arc>| { blocking_json_task(move || { - let execution_optimistic = block_id.is_execution_optimistic(&chain)?; - block_id - .root(&chain) - .map(api_types::RootData::from) - .map(api_types::GenericResponse::from) - .map(|resp| resp.add_execution_optimistic(execution_optimistic)) + let (block, execution_optimistic) = + block_id.block_and_is_execution_optimistic(&chain)?; + + Ok(api_types::GenericResponse::from(api_types::RootData::from( + block.canonical_root(), + )) + .add_execution_optimistic(execution_optimistic)) }) }); @@ -1251,12 +1252,13 @@ pub fn serve( .and(warp::path::end()) .and_then(|block_id: BlockId, chain: Arc>| { blocking_json_task(move || { - let execution_optimistic = block_id.is_execution_optimistic(&chain)?; - block_id - .block(&chain) - .map(|block| block.message().body().attestations().clone()) - .map(api_types::GenericResponse::from) - .map(|resp| resp.add_execution_optimistic(execution_optimistic)) + let (block, execution_optimistic) = + block_id.block_and_is_execution_optimistic(&chain)?; + + Ok( + api_types::GenericResponse::from(block.message().body().attestations().clone()) + .add_execution_optimistic(execution_optimistic), + ) }) }); @@ -1720,7 +1722,8 @@ pub fn serve( None } else if endpoint_version == V2 { BlockId::from_root(root) - .is_execution_optimistic(&chain) + .block_and_is_execution_optimistic(&chain) + .map(|(_, execution_optimistic)| execution_optimistic) .ok() } else { return Err(unsupported_version_rejection(endpoint_version)); diff --git a/beacon_node/http_api/src/proposer_duties.rs b/beacon_node/http_api/src/proposer_duties.rs index e2b5b79fc28..b410766120a 100644 --- a/beacon_node/http_api/src/proposer_duties.rs +++ b/beacon_node/http_api/src/proposer_duties.rs @@ -114,7 +114,7 @@ fn try_proposer_duties_from_cache( }; let execution_optimistic = chain - .is_optimistic_head(Some(head.block_root)) + .is_optimistic_head() .map_err(warp_utils::reject::beacon_chain_error)?; chain diff --git a/beacon_node/http_api/src/state_id.rs b/beacon_node/http_api/src/state_id.rs index e5258d9e544..1c8955dd209 100644 --- a/beacon_node/http_api/src/state_id.rs +++ b/beacon_node/http_api/src/state_id.rs @@ -119,7 +119,7 @@ impl StateId { | CoreStateId::Justified | CoreStateId::Finalized | CoreStateId::Root(_) => chain - .is_optimistic_head(None) + .is_optimistic_head() .map_err(warp_utils::reject::beacon_chain_error), } } diff --git a/beacon_node/http_api/src/sync_committees.rs b/beacon_node/http_api/src/sync_committees.rs index 2c651245ddd..54a3e075d34 100644 --- a/beacon_node/http_api/src/sync_committees.rs +++ b/beacon_node/http_api/src/sync_committees.rs @@ -40,7 +40,7 @@ pub fn sync_committee_duties( // Even when computing duties from state, any block roots pulled using the request epoch are // still dependent on the head. So using `is_optimistic_head` is fine for both cases. let execution_optimistic = chain - .is_optimistic_head(None) + .is_optimistic_head() .map_err(warp_utils::reject::beacon_chain_error)?; // Try using the head's sync committees to satisfy the request. This should be sufficient for