Refactor head accessor methods

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -508,11 +508,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             ));
         }
 
-        let local_head = self.head()?;
+        let local_head = self.head_snapshot();
 
         let iter = self.store.forwards_block_roots_iterator(
             start_slot,
-            local_head.beacon_state,
+            local_head.beacon_state.clone_with(CloneConfig::none()),
             local_head.beacon_block_root,
             &self.spec,
         )?;
@@ -612,12 +612,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         start_slot: Slot,
     ) -> Result<impl Iterator<Item = Result<(Hash256, Slot), Error>> + '_, Error> {
-        let local_head = self.head()?;
+        let local_head = self.head_snapshot();
 
         let iter = self.store.forwards_state_roots_iterator(
             start_slot,
             local_head.beacon_state_root(),
-            local_head.beacon_state,
+            local_head.beacon_state.clone_with(CloneConfig::none()),
             &self.spec,
         )?;
 
@@ -967,52 +967,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         Ok(self.store.get_state(state_root, slot)?)
     }
 
-    /// Returns a `Checkpoint` representing the head block and state. Contains the "best block";
-    /// the head of the canonical `BeaconChain`.
-    ///
-    /// It is important to note that the `beacon_state` returned may not match the present slot. It
-    /// is the state as it was when the head block was received, which could be some slots prior to
-    /// now.
-    pub fn head(&self) -> Result<BeaconSnapshot<T::EthSpec>, Error> {
-        self.with_head(|head| Ok(head.clone_with(CloneConfig::committee_caches_only())))
-    }
-
-    /// Apply a function to the canonical head without cloning it.
-    pub fn with_head<U, E>(
-        &self,
-        f: impl FnOnce(&BeaconSnapshot<T::EthSpec>) -> Result<U, E>,
-    ) -> Result<U, E>
-    where
-        E: From<Error>,
-    {
-        let head_lock = self.canonical_head.cached_head();
-        f(&head_lock.snapshot)
-    }
-
-    /// Returns the beacon block root at the head of the canonical chain.
-    ///
-    /// See `Self::head` for more information.
-    pub fn head_beacon_block_root(&self) -> Result<Hash256, Error> {
-        self.with_head(|s| Ok(s.beacon_block_root))
-    }
-
-    /// Returns the beacon block at the head of the canonical chain.
-    ///
-    /// See `Self::head` for more information.
-    pub fn head_beacon_block(&self) -> Result<Arc<SignedBeaconBlock<T::EthSpec>>, Error> {
-        self.with_head(|s| Ok(s.beacon_block.clone()))
-    }
-
-    /// Returns the beacon state at the head of the canonical chain.
-    ///
-    /// See `Self::head` for more information.
-    pub fn head_beacon_state(&self) -> Result<BeaconState<T::EthSpec>, Error> {
-        self.with_head(|s| {
-            Ok(s.beacon_state
-                .clone_with(CloneConfig::committee_caches_only()))
-        })
-    }
-
     /// Return the sync committee at `slot + 1` from the canonical chain.
     ///
     /// This is useful when dealing with sync committee messages, because messages are signed
@@ -1107,7 +1061,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         slot: Slot,
         config: StateSkipConfig,
     ) -> Result<BeaconState<T::EthSpec>, Error> {
-        let head_state = self.head()?.beacon_state;
+        let head_state = self.head_beacon_state_cloned();
 
         match slot.cmp(&head_state.slot()) {
             Ordering::Equal => Ok(head_state),

beacon_node/beacon_chain/src/builder.rs

@@ -796,9 +796,7 @@ where
             validator_monitor: RwLock::new(validator_monitor),
         };
 
-        let head = beacon_chain
-            .head()
-            .map_err(|e| format!("Failed to get head: {:?}", e))?;
+        let head = beacon_chain.head_snapshot();
 
         // Prime the attester cache with the head state.
         beacon_chain
@@ -1001,10 +999,10 @@ mod test {
             .build()
             .expect("should build");
 
-        let head = chain.head().expect("should get head");
+        let head = chain.head_snapshot();
 
-        let state = head.beacon_state;
-        let block = head.beacon_block;
+        let state = &head.beacon_state;
+        let block = &head.beacon_block;
 
         assert_eq!(state.slot(), Slot::new(0), "should start from genesis");
         assert_eq!(

beacon_node/beacon_chain/src/canonical_head.rs

@@ -310,6 +310,14 @@ impl<T: BeaconChainTypes> CanonicalHead<T> {
         self.cached_head.read().clone()
     }
 
+    /// Access a write-lock for the cached head.
+    ///
+    /// This function is **not safe** to be public. See the module-level documentation for more
+    /// information about protecting from deadlocks.
+    fn cached_head_read_lock(&self) -> RwLockReadGuard<CachedHead<T::EthSpec>> {
+        self.cached_head.read()
+    }
+
     /// Access a write-lock for the cached head.
     ///
     /// This function is **not safe** to be public. See the module-level documentation for more
@@ -330,6 +338,66 @@ impl<T: BeaconChainTypes> CanonicalHead<T> {
 }
 
 impl<T: BeaconChainTypes> BeaconChain<T> {
+    /// Contains the "best block"; the head of the canonical `BeaconChain`.
+    ///
+    /// It is important to note that the `snapshot.beacon_state` returned may not match the present slot. It
+    /// is the state as it was when the head block was received, which could be some slots prior to
+    /// now.
+    pub fn head(&self) -> CachedHead<T::EthSpec> {
+        self.canonical_head.cached_head()
+    }
+
+    /// Apply a function to an `Arc`-clone of the canonical head snapshot.
+    ///
+    /// This method is a relic from an old implementation where the canonical head was not behind
+    /// an `Arc` and the canonical head lock had to be held whenever it was read. This method is
+    /// fine to be left here, it just seems a bit weird.
+    pub fn with_head<U, E>(
+        &self,
+        f: impl FnOnce(&BeaconSnapshot<T::EthSpec>) -> Result<U, E>,
+    ) -> Result<U, E>
+    where
+        E: From<Error>,
+    {
+        let head_snapshot = self.head_snapshot();
+        f(&head_snapshot)
+    }
+
+    /// Returns the beacon block root at the head of the canonical chain.
+    ///
+    /// See `Self::head` for more information.
+    pub fn head_beacon_block_root(&self) -> Hash256 {
+        self.canonical_head
+            .cached_head_read_lock()
+            .snapshot
+            .beacon_block_root
+    }
+
+    /// Returns a `Arc` of the `BeaconSnapshot` at the head of the canonical chain.
+    ///
+    /// See `Self::head` for more information.
+    pub fn head_snapshot(&self) -> Arc<BeaconSnapshot<T::EthSpec>> {
+        self.canonical_head.cached_head_read_lock().snapshot.clone()
+    }
+
+    /// Returns the beacon block at the head of the canonical chain.
+    ///
+    /// See `Self::head` for more information.
+    pub fn head_beacon_block(&self) -> Arc<SignedBeaconBlock<T::EthSpec>> {
+        self.head_snapshot().beacon_block.clone()
+    }
+
+    /// Returns a clone of the beacon state at the head of the canonical chain.
+    ///
+    /// Cloning the head state is expensive and should generally be avoided outside of tests.
+    ///
+    /// See `Self::head` for more information.
+    pub fn head_beacon_state_cloned(&self) -> BeaconState<T::EthSpec> {
+        self.head_snapshot()
+            .beacon_state
+            .clone_with(CloneConfig::committee_caches_only())
+    }
+
     /// Execute the fork choice algorithm and enthrone the result as the canonical head.
     ///
     /// This method replaces the old `BeaconChain::fork_choice` method.

beacon_node/beacon_chain/src/snapshot_cache.rs

@@ -385,7 +385,7 @@ mod test {
     fn get_snapshot(i: u64) -> BeaconSnapshot<MainnetEthSpec> {
         let spec = MainnetEthSpec::default_spec();
 
-        let beacon_state = get_harness().chain.head_beacon_state().unwrap();
+        let beacon_state = get_harness().chain.head_beacon_state_cloned();
 
         let signed_beacon_block = SignedBeaconBlock::from_block(
             BeaconBlock::empty(&spec),

beacon_node/beacon_chain/src/test_utils.rs

@@ -514,13 +514,16 @@ where
     }
 
     pub fn get_current_state(&self) -> BeaconState<E> {
-        self.chain.head().unwrap().beacon_state
+        self.chain.head_beacon_state_cloned()
     }
 
     pub fn get_current_state_and_root(&self) -> (BeaconState<E>, Hash256) {
-        let head = self.chain.head().unwrap();
+        let head = self.chain.head_snapshot();
         let state_root = head.beacon_state_root();
-        (head.beacon_state, state_root)
+        (
+            head.beacon_state.clone_with_only_committee_caches(),
+            state_root,
+        )
     }
 
     pub fn head_slot(&self) -> Slot {
@@ -1205,12 +1208,7 @@ where
     }
 
     pub fn make_proposer_slashing(&self, validator_index: u64) -> ProposerSlashing {
-        let mut block_header_1 = self
-            .chain
-            .head_beacon_block()
-            .unwrap()
-            .message()
-            .block_header();
+        let mut block_header_1 = self.chain.head_beacon_block().message().block_header();
         block_header_1.proposer_index = validator_index;
 
         let mut block_header_2 = block_header_1.clone();
@@ -1737,12 +1735,7 @@ where
         honest_fork_blocks: usize,
         faulty_fork_blocks: usize,
     ) -> (Hash256, Hash256) {
-        let initial_head_slot = self
-            .chain
-            .head()
-            .expect("should get head")
-            .beacon_block
-            .slot();
+        let initial_head_slot = self.chain.head_snapshot().beacon_block.slot();
 
         // Move to the next slot so we may produce some more blocks on the head.
         self.advance_slot();

beacon_node/beacon_chain/tests/attestation_production.rs

@@ -183,7 +183,7 @@ async fn early_attester_cache_old_request() {
         )
         .await;
 
-    let head = harness.chain.head().unwrap();
+    let head = harness.chain.head_snapshot();
     assert_eq!(head.beacon_block.slot(), 2);
     let head_proto_block = harness
         .chain

beacon_node/beacon_chain/tests/attestation_verification.rs

@@ -56,7 +56,7 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness<EphemeralHarnessTyp
 fn get_valid_unaggregated_attestation<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
 ) -> (Attestation<T::EthSpec>, usize, usize, SecretKey, SubnetId) {
-    let head = chain.head().expect("should get head");
+    let head = chain.head_snapshot();
     let current_slot = chain.slot().expect("should get slot");
 
     let mut valid_attestation = chain
@@ -106,7 +106,8 @@ fn get_valid_aggregated_attestation<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
     aggregate: Attestation<T::EthSpec>,
 ) -> (SignedAggregateAndProof<T::EthSpec>, usize, SecretKey) {
-    let state = &chain.head().expect("should get head").beacon_state;
+    let head = chain.head_snapshot();
+    let state = &head.beacon_state;
     let current_slot = chain.slot().expect("should get slot");
 
     let committee = state
@@ -155,7 +156,8 @@ fn get_non_aggregator<T: BeaconChainTypes>(
     chain: &BeaconChain<T>,
     aggregate: &Attestation<T::EthSpec>,
 ) -> (usize, SecretKey) {
-    let state = &chain.head().expect("should get head").beacon_state;
+    let head = chain.head_snapshot();
+    let state = &head.beacon_state;
     let current_slot = chain.slot().expect("should get slot");
 
     let committee = state
@@ -514,8 +516,7 @@ async fn aggregated_gossip_verification() {
                 let committee_len = tester
                     .harness
                     .chain
-                    .head()
-                    .unwrap()
+                    .head_snapshot()
                     .beacon_state
                     .get_beacon_committee(tester.slot(), a.message.aggregate.data.index)
                     .expect("should get committees")
@@ -688,8 +689,7 @@ async fn unaggregated_gossip_verification() {
                 a.data.index = tester
                     .harness
                     .chain
-                    .head()
-                    .unwrap()
+                    .head_snapshot()
                     .beacon_state
                     .get_committee_count_at_slot(a.data.slot)
                     .unwrap()

beacon_node/beacon_chain/tests/merge.rs

@@ -70,8 +70,7 @@ async fn merge_with_terminal_block_hash_override() {
     assert!(
         harness
             .chain
-            .head()
-            .unwrap()
+            .head_snapshot()
             .beacon_block
             .as_merge()
             .is_ok(),
@@ -82,7 +81,7 @@ async fn merge_with_terminal_block_hash_override() {
     for i in 0..E::slots_per_epoch() * 3 {
         harness.extend_slots(1).await;
 
-        let block = harness.chain.head().unwrap().beacon_block;
+        let block = &harness.chain.head_snapshot().beacon_block;
 
         let execution_payload = block.message().body().execution_payload().unwrap().clone();
         if i == 0 {
@@ -118,15 +117,15 @@ async fn base_altair_merge_with_terminal_block_after_fork() {
      * Start with the base fork.
      */
 
-    assert!(harness.chain.head().unwrap().beacon_block.as_base().is_ok());
+    assert!(harness.chain.head_snapshot().beacon_block.as_base().is_ok());
 
     /*
      * Do the Altair fork.
      */
 
     harness.extend_to_slot(altair_fork_slot).await;
 
-    let altair_head = harness.chain.head().unwrap().beacon_block;
+    let altair_head = &harness.chain.head_snapshot().beacon_block;
     assert!(altair_head.as_altair().is_ok());
     assert_eq!(altair_head.slot(), altair_fork_slot);
 
@@ -136,7 +135,7 @@ async fn base_altair_merge_with_terminal_block_after_fork() {
 
     harness.extend_to_slot(merge_fork_slot).await;
 
-    let merge_head = harness.chain.head().unwrap().beacon_block;
+    let merge_head = &harness.chain.head_snapshot().beacon_block;
     assert!(merge_head.as_merge().is_ok());
     assert_eq!(merge_head.slot(), merge_fork_slot);
     assert_eq!(
@@ -150,7 +149,7 @@ async fn base_altair_merge_with_terminal_block_after_fork() {
 
     harness.extend_slots(1).await;
 
-    let one_after_merge_head = harness.chain.head().unwrap().beacon_block;
+    let one_after_merge_head = &harness.chain.head_snapshot().beacon_block;
     assert_eq!(
         *one_after_merge_head
             .message()
@@ -177,7 +176,7 @@ async fn base_altair_merge_with_terminal_block_after_fork() {
     for _ in 0..4 {
         harness.extend_slots(1).await;
 
-        let block = harness.chain.head().unwrap().beacon_block;
+        let block = &harness.chain.head_snapshot().beacon_block;
         execution_payloads.push(block.message().body().execution_payload().unwrap().clone());
     }