Revert "Tidy ancestor iterators"

This reverts commit 5079c25bb2.

Accidental push to master.. my bad!

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1,7 +1,6 @@
 use crate::checkpoint::CheckPoint;
 use crate::errors::{BeaconChainError as Error, BlockProductionError};
 use crate::fork_choice::{Error as ForkChoiceError, ForkChoice};
-use crate::iter::{ReverseBlockRootIterator, ReverseStateRootIterator};
 use crate::metrics::Metrics;
 use crate::persisted_beacon_chain::{PersistedBeaconChain, BEACON_CHAIN_DB_KEY};
 use lmd_ghost::LmdGhost;
@@ -20,7 +19,7 @@ use state_processing::{
     per_slot_processing, BlockProcessingError,
 };
 use std::sync::Arc;
-use store::iter::{BlockRootsIterator, StateRootsIterator};
+use store::iter::{BestBlockRootsIterator, BlockIterator, BlockRootsIterator, StateRootsIterator};
 use store::{Error as DBError, Store};
 use tree_hash::TreeHash;
 use types::*;
@@ -217,53 +216,45 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
         Ok(headers?)
     }
-
+    /// Iterate in reverse (highest to lowest slot) through all blocks from the block at `slot`
-    /// Iterates through all the `BeaconBlock` roots and slots, first returning
+    /// through to the genesis block.
-    /// `self.head().beacon_block` then all prior blocks until either genesis or if the database
-    /// fails to return a prior block.
     ///
-    /// Returns duplicate roots for skip-slots.
+    /// Returns `None` for headers prior to genesis or when there is an error reading from `Store`.
     ///
-    /// Iterator returns `(Hash256, Slot)`.
+    /// Contains duplicate headers when skip slots are encountered.
-    ///
+    pub fn rev_iter_blocks(&self, slot: Slot) -> BlockIterator<T::EthSpec, T::Store> {
-    /// ## Note
+        BlockIterator::owned(self.store.clone(), self.state.read().clone(), slot)
-    ///
-    /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot
-    /// returned may be earlier than the wall-clock slot.
-    pub fn rev_iter_block_roots(
-        &self,
-        slot: Slot,
-    ) -> ReverseBlockRootIterator<T::EthSpec, T::Store> {
-        let state = &self.head().beacon_state;
-        let block_root = self.head().beacon_block_root;
-        let block_slot = state.slot;
-
-        let iter = BlockRootsIterator::owned(self.store.clone(), state.clone(), slot);
-
-        ReverseBlockRootIterator::new((block_root, block_slot), iter)
     }
 
-    /// Iterates through all the `BeaconState` roots and slots, first returning
+    /// Iterates in reverse (highest to lowest slot) through all block roots from `slot` through to
-    /// `self.head().beacon_state` then all prior states until either genesis or if the database
+    /// genesis.
-    /// fails to return a prior state.
     ///
-    /// Iterator returns `(Hash256, Slot)`.
+    /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`.
     ///
-    /// ## Note
+    /// Contains duplicate roots when skip slots are encountered.
+    pub fn rev_iter_block_roots(&self, slot: Slot) -> BlockRootsIterator<T::EthSpec, T::Store> {
+        BlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), slot)
+    }
+
+    /// Iterates in reverse (highest to lowest slot) through all block roots from largest
+    /// `slot <= beacon_state.slot` through to genesis.
     ///
-    /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot
+    /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`.
-    /// returned may be earlier than the wall-clock slot.
+    ///
-    pub fn rev_iter_state_roots(
+    /// Contains duplicate roots when skip slots are encountered.
+    pub fn rev_iter_best_block_roots(
         &self,
         slot: Slot,
-    ) -> ReverseStateRootIterator<T::EthSpec, T::Store> {
+    ) -> BestBlockRootsIterator<T::EthSpec, T::Store> {
-        let state = &self.head().beacon_state;
+        BestBlockRootsIterator::owned(self.store.clone(), self.state.read().clone(), slot)
-        let state_root = self.head().beacon_state_root;
+    }
-        let state_slot = state.slot;
 
-        let iter = StateRootsIterator::owned(self.store.clone(), state.clone(), slot);
+    /// Iterates in reverse (highest to lowest slot) through all state roots from `slot` through to
-
+    /// genesis.
-        ReverseStateRootIterator::new((state_root, state_slot), iter)
+    ///
+    /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`.
+    pub fn rev_iter_state_roots(&self, slot: Slot) -> StateRootsIterator<T::EthSpec, T::Store> {
+        StateRootsIterator::owned(self.store.clone(), self.state.read().clone(), slot)
     }
 
     /// Returns the block at the given root, if any.
@@ -280,10 +271,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
 
     /// Returns a read-lock guarded `BeaconState` which is the `canonical_head` that has been
     /// updated to match the current slot clock.
-    pub fn speculative_state(&self) -> Result<RwLockReadGuard<BeaconState<T::EthSpec>>, Error> {
+    pub fn current_state(&self) -> RwLockReadGuard<BeaconState<T::EthSpec>> {
-        // TODO: ensure the state has done a catch-up.
+        self.state.read()
-
-        Ok(self.state.read())
     }
 
     /// Returns a read-lock guarded `CheckPoint` struct for reading the head (as chosen by the

beacon_node/beacon_chain/src/fork_choice.rs

@@ -52,7 +52,7 @@ impl<T: BeaconChainTypes> ForkChoice<T> {
         // been justified for at least 1 epoch ... If no such descendant exists,
         // set justified_head to finalized_head.
         let (start_state, start_block_root, start_block_slot) = {
-            let state = &chain.head().beacon_state;
+            let state = chain.current_state();
 
             let (block_root, block_slot) =
                 if state.current_epoch() + 1 > state.current_justified_checkpoint.epoch {

beacon_node/beacon_chain/src/iter.rs

@@ -1,48 +0,0 @@
-use store::iter::{BlockRootsIterator, StateRootsIterator};
-use types::{Hash256, Slot};
-
-pub type ReverseBlockRootIterator<'a, E, S> =
-    ReverseHashAndSlotIterator<BlockRootsIterator<'a, E, S>>;
-pub type ReverseStateRootIterator<'a, E, S> =
-    ReverseHashAndSlotIterator<StateRootsIterator<'a, E, S>>;
-
-pub type ReverseHashAndSlotIterator<I> = ReverseChainIterator<(Hash256, Slot), I>;
-
-/// Provides a wrapper for an iterator that returns a given `T` before it starts returning results of
-/// the `Iterator`.
-pub struct ReverseChainIterator<T, I> {
-    first_value_used: bool,
-    first_value: T,
-    iter: I,
-}
-
-impl<T, I> ReverseChainIterator<T, I>
-where
-    T: Sized,
-    I: Iterator<Item = T> + Sized,
-{
-    pub fn new(first_value: T, iter: I) -> Self {
-        Self {
-            first_value_used: false,
-            first_value,
-            iter,
-        }
-    }
-}
-
-impl<T, I> Iterator for ReverseChainIterator<T, I>
-where
-    T: Clone,
-    I: Iterator<Item = T>,
-{
-    type Item = T;
-
-    fn next(&mut self) -> Option<Self::Item> {
-        if self.first_value_used {
-            self.iter.next()
-        } else {
-            self.first_value_used = true;
-            Some(self.first_value.clone())
-        }
-    }
-}

beacon_node/beacon_chain/src/lib.rs

@@ -2,7 +2,6 @@ mod beacon_chain;
 mod checkpoint;
 mod errors;
 mod fork_choice;
-mod iter;
 mod metrics;
 mod persisted_beacon_chain;
 pub mod test_utils;

beacon_node/beacon_chain/src/test_utils.rs

@@ -198,7 +198,7 @@ where
     fn get_state_at_slot(&self, state_slot: Slot) -> BeaconState<E> {
         let state_root = self
             .chain
-            .rev_iter_state_roots(self.chain.head().beacon_state.slot - 1)
+            .rev_iter_state_roots(self.chain.current_state().slot - 1)
             .find(|(_hash, slot)| *slot == state_slot)
             .map(|(hash, _slot)| hash)
             .expect("could not find state root");

beacon_node/network/src/sync/simple_sync.rs

@@ -266,7 +266,7 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
 
     fn root_at_slot(&self, target_slot: Slot) -> Option<Hash256> {
         self.chain
-            .rev_iter_block_roots(target_slot)
+            .rev_iter_best_block_roots(target_slot)
             .take(1)
             .find(|(_root, slot)| *slot == target_slot)
             .map(|(root, _slot)| root)
@@ -280,8 +280,6 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         req: BeaconBlockRootsRequest,
         network: &mut NetworkContext<T::EthSpec>,
     ) {
-        let state = &self.chain.head().beacon_state;
-
         debug!(
             self.log,
             "BlockRootsRequest";
@@ -292,8 +290,8 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
 
         let mut roots: Vec<BlockRootSlot> = self
             .chain
-            .rev_iter_block_roots(std::cmp::min(req.start_slot + req.count, state.slot))
+            .rev_iter_best_block_roots(req.start_slot + req.count)
-            .take_while(|(_root, slot)| req.start_slot <= *slot)
+            .take(req.count as usize)
             .map(|(block_root, slot)| BlockRootSlot { slot, block_root })
             .collect();
 
@@ -304,7 +302,7 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
                 "peer" => format!("{:?}", peer_id),
                 "msg" => "Failed to return all requested hashes",
                 "start_slot" => req.start_slot,
-                "current_slot" => self.chain.present_slot(),
+                "current_slot" => self.chain.current_state().slot,
                 "requested" => req.count,
                 "returned" => roots.len(),
             );
@@ -391,8 +389,6 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         req: BeaconBlockHeadersRequest,
         network: &mut NetworkContext<T::EthSpec>,
     ) {
-        let state = &self.chain.head().beacon_state;
-
         debug!(
             self.log,
             "BlockHeadersRequest";
@@ -403,10 +399,13 @@ impl<T: BeaconChainTypes> SimpleSync<T> {
         let count = req.max_headers;
 
         // Collect the block roots.
+        //
+        // Instead of using `chain.rev_iter_blocks` we collect the roots first. This avoids
+        // unnecessary block deserialization when `req.skip_slots > 0`.
         let mut roots: Vec<Hash256> = self
             .chain
-            .rev_iter_block_roots(std::cmp::min(req.start_slot + count, state.slot))
+            .rev_iter_best_block_roots(req.start_slot + count)
-            .take_while(|(_root, slot)| req.start_slot <= *slot)
+            .take(count as usize)
             .map(|(root, _slot)| root)
             .collect();
 

beacon_node/rest_api/src/beacon_node.rs

@@ -54,7 +54,7 @@ fn get_version(_req: Request<Body>) -> APIResult {
 fn get_genesis_time<T: BeaconChainTypes + 'static>(req: Request<Body>) -> APIResult {
     let beacon_chain = req.extensions().get::<Arc<BeaconChain<T>>>().unwrap();
     let gen_time = {
-        let state = &beacon_chain.head().beacon_state;
+        let state = beacon_chain.current_state();
         state.genesis_time
     };
     let body = Body::from(

beacon_node/rpc/src/attestation.rs

@@ -40,11 +40,7 @@ impl<T: BeaconChainTypes> AttestationService for AttestationServiceInstance<T> {
         // verify the slot, drop lock on state afterwards
         {
             let slot_requested = req.get_slot();
-            // TODO: this whole module is legacy and not maintained well.
+            let state = &self.chain.current_state();
-            let state = &self
-                .chain
-                .speculative_state()
-                .expect("This is legacy code and should be removed");
 
             // Start by performing some checks
             // Check that the AttestationData is for the current slot (otherwise it will not be valid)

beacon_node/rpc/src/validator.rs

@@ -29,11 +29,7 @@ impl<T: BeaconChainTypes> ValidatorService for ValidatorServiceInstance<T> {
         trace!(self.log, "RPC request"; "endpoint" => "GetValidatorDuties", "epoch" => req.get_epoch());
 
         let spec = &self.chain.spec;
-        // TODO: this whole module is legacy and not maintained well.
+        let state = &self.chain.current_state();
-        let state = &self
-            .chain
-            .speculative_state()
-            .expect("This is legacy code and should be removed");
         let epoch = Epoch::from(req.get_epoch());
         let mut resp = GetDutiesResponse::new();
         let resp_validators = resp.mut_active_validators();

beacon_node/store/src/iter.rs

@@ -4,23 +4,20 @@ use std::sync::Arc;
 use types::{BeaconBlock, BeaconState, BeaconStateError, EthSpec, Hash256, Slot};
 
 /// Implemented for types that have ancestors (e.g., blocks, states) that may be iterated over.
-///
-/// ## Note
-///
-/// It is assumed that all ancestors for this object are stored in the database. If this is not the
-/// case, the iterator will start returning `None` prior to genesis.
 pub trait AncestorIter<U: Store, I: Iterator> {
     /// Returns an iterator over the roots of the ancestors of `self`.
     fn try_iter_ancestor_roots(&self, store: Arc<U>) -> Option<I>;
 }
 
-impl<'a, U: Store, E: EthSpec> AncestorIter<U, BlockRootsIterator<'a, E, U>> for BeaconBlock<E> {
+impl<'a, U: Store, E: EthSpec> AncestorIter<U, BestBlockRootsIterator<'a, E, U>>
+    for BeaconBlock<E>
+{
     /// Iterates across all the prior block roots of `self`, starting at the most recent and ending
     /// at genesis.
-    fn try_iter_ancestor_roots(&self, store: Arc<U>) -> Option<BlockRootsIterator<'a, E, U>> {
+    fn try_iter_ancestor_roots(&self, store: Arc<U>) -> Option<BestBlockRootsIterator<'a, E, U>> {
         let state = store.get::<BeaconState<E>>(&self.state_root).ok()??;
 
-        Some(BlockRootsIterator::owned(store, state, self.slot))
+        Some(BestBlockRootsIterator::owned(store, state, self.slot))
     }
 }
 
@@ -119,6 +116,11 @@ impl<'a, T: EthSpec, U: Store> Iterator for BlockIterator<'a, T, U> {
 /// exhausted.
 ///
 /// Returns `None` for roots prior to genesis or when there is an error reading from `Store`.
+///
+/// ## Notes
+///
+/// See [`BestBlockRootsIterator`](struct.BestBlockRootsIterator.html), which has different
+/// `start_slot` logic.
 #[derive(Clone)]
 pub struct BlockRootsIterator<'a, T: EthSpec, U> {
     store: Arc<U>,
@@ -178,6 +180,104 @@ impl<'a, T: EthSpec, U: Store> Iterator for BlockRootsIterator<'a, T, U> {
     }
 }
 
+/// Iterates backwards through block roots with `start_slot` highest possible value
+/// `<= beacon_state.slot`.
+///
+/// The distinction between `BestBlockRootsIterator` and `BlockRootsIterator` is:
+///
+/// - `BestBlockRootsIterator` uses best-effort slot. When `start_slot` is greater than the latest available block root
+/// on `beacon_state`, returns `Some(root, slot)` where `slot` is the latest available block
+/// root.
+/// - `BlockRootsIterator` is strict about `start_slot`. When `start_slot` is greater than the latest available block root
+/// on `beacon_state`, returns  `None`.
+///
+/// This is distinct from `BestBlockRootsIterator`.
+///
+/// Uses the `block_roots` field of `BeaconState` to as the source of block roots and will
+/// perform a lookup on the `Store` for a prior `BeaconState` if `block_roots` has been
+/// exhausted.
+///
+/// Returns `None` for roots prior to genesis or when there is an error reading from `Store`.
+#[derive(Clone)]
+pub struct BestBlockRootsIterator<'a, T: EthSpec, U> {
+    store: Arc<U>,
+    beacon_state: Cow<'a, BeaconState<T>>,
+    slot: Slot,
+}
+
+impl<'a, T: EthSpec, U: Store> BestBlockRootsIterator<'a, T, U> {
+    /// Create a new iterator over all block roots in the given `beacon_state` and prior states.
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>, start_slot: Slot) -> Self {
+        let mut slot = start_slot;
+        if slot >= beacon_state.slot {
+            // Slot may be too high.
+            slot = beacon_state.slot;
+            if beacon_state.get_block_root(slot).is_err() {
+                slot -= 1;
+            }
+        }
+
+        Self {
+            store,
+            beacon_state: Cow::Borrowed(beacon_state),
+            slot: slot + 1,
+        }
+    }
+
+    /// Create a new iterator over all block roots in the given `beacon_state` and prior states.
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>, start_slot: Slot) -> Self {
+        let mut slot = start_slot;
+        if slot >= beacon_state.slot {
+            // Slot may be too high.
+            slot = beacon_state.slot;
+            // TODO: Use a function other than `get_block_root` as this will always return `Err()`
+            // for slot = state.slot.
+            if beacon_state.get_block_root(slot).is_err() {
+                slot -= 1;
+            }
+        }
+
+        Self {
+            store,
+            beacon_state: Cow::Owned(beacon_state),
+            slot: slot + 1,
+        }
+    }
+}
+
+impl<'a, T: EthSpec, U: Store> Iterator for BestBlockRootsIterator<'a, T, U> {
+    type Item = (Hash256, Slot);
+
+    fn next(&mut self) -> Option<Self::Item> {
+        if self.slot == 0 {
+            // End of Iterator
+            return None;
+        }
+
+        self.slot -= 1;
+
+        match self.beacon_state.get_block_root(self.slot) {
+            Ok(root) => Some((*root, self.slot)),
+            Err(BeaconStateError::SlotOutOfBounds) => {
+                // Read a `BeaconState` from the store that has access to prior historical root.
+                let beacon_state: BeaconState<T> = {
+                    // Load the earliest state from disk.
+                    let new_state_root = self.beacon_state.get_oldest_state_root().ok()?;
+
+                    self.store.get(&new_state_root).ok()?
+                }?;
+
+                self.beacon_state = Cow::Owned(beacon_state);
+
+                let root = self.beacon_state.get_block_root(self.slot).ok()?;
+
+                Some((*root, self.slot))
+            }
+            _ => None,
+        }
+    }
+}
+
 #[cfg(test)]
 mod test {
     use super::*;
@@ -237,6 +337,49 @@ mod test {
         }
     }
 
+    #[test]
+    fn best_block_root_iter() {
+        let store = Arc::new(MemoryStore::open());
+        let slots_per_historical_root = MainnetEthSpec::slots_per_historical_root();
+
+        let mut state_a: BeaconState<MainnetEthSpec> = get_state();
+        let mut state_b: BeaconState<MainnetEthSpec> = get_state();
+
+        state_a.slot = Slot::from(slots_per_historical_root);
+        state_b.slot = Slot::from(slots_per_historical_root * 2);
+
+        let mut hashes = (0..).into_iter().map(|i| Hash256::from(i));
+
+        for root in &mut state_a.block_roots[..] {
+            *root = hashes.next().unwrap()
+        }
+        for root in &mut state_b.block_roots[..] {
+            *root = hashes.next().unwrap()
+        }
+
+        let state_a_root = hashes.next().unwrap();
+        state_b.state_roots[0] = state_a_root;
+        store.put(&state_a_root, &state_a).unwrap();
+
+        let iter = BestBlockRootsIterator::new(store.clone(), &state_b, state_b.slot);
+
+        assert!(
+            iter.clone().find(|(_root, slot)| *slot == 0).is_some(),
+            "iter should contain zero slot"
+        );
+
+        let mut collected: Vec<(Hash256, Slot)> = iter.collect();
+        collected.reverse();
+
+        let expected_len = 2 * MainnetEthSpec::slots_per_historical_root();
+
+        assert_eq!(collected.len(), expected_len);
+
+        for i in 0..expected_len {
+            assert_eq!(collected[i].0, Hash256::from(i as u64));
+        }
+    }
+
     #[test]
     fn state_root_iter() {
         let store = Arc::new(MemoryStore::open());

eth2/lmd_ghost/tests/test.rs

@@ -10,7 +10,7 @@ use lmd_ghost::{LmdGhost, ThreadSafeReducedTree as BaseThreadSafeReducedTree};
 use rand::{prelude::*, rngs::StdRng};
 use std::sync::Arc;
 use store::{
-    iter::{AncestorIter, BlockRootsIterator},
+    iter::{AncestorIter, BestBlockRootsIterator},
     MemoryStore, Store,
 };
 use types::{BeaconBlock, EthSpec, Hash256, MinimalEthSpec, Slot};
@@ -159,7 +159,7 @@ fn get_ancestor_roots<E: EthSpec, U: Store>(
         .expect("block should exist")
         .expect("store should not error");
 
-    <BeaconBlock<TestEthSpec> as AncestorIter<_, BlockRootsIterator<TestEthSpec, _>>>::try_iter_ancestor_roots(
+    <BeaconBlock<TestEthSpec> as AncestorIter<_, BestBlockRootsIterator<TestEthSpec, _>>>::try_iter_ancestor_roots(
         &block, store,
     )
     .expect("should be able to create ancestor iter")