Simplify, fix bugs, add tests for chain iters

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -244,15 +244,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     ///
     /// 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(
+    pub fn rev_iter_block_roots(&self) -> ReverseBlockRootIterator<T::EthSpec, T::Store> {
-        &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);
+        let iter = BlockRootsIterator::owned(self.store.clone(), state.clone());
 
         ReverseBlockRootIterator::new((block_root, block_slot), iter)
     }
@@ -267,15 +264,12 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     ///
     /// 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_state_roots(
+    pub fn rev_iter_state_roots(&self) -> ReverseStateRootIterator<T::EthSpec, T::Store> {
-        &self,
-        slot: Slot,
-    ) -> ReverseStateRootIterator<T::EthSpec, T::Store> {
         let state = &self.head().beacon_state;
         let state_root = self.head().beacon_state_root;
         let state_slot = state.slot;
 
-        let iter = StateRootsIterator::owned(self.store.clone(), state.clone(), slot);
+        let iter = StateRootsIterator::owned(self.store.clone(), state.clone());
 
         ReverseStateRootIterator::new((state_root, state_slot), iter)
     }
@@ -448,9 +442,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     pub fn produce_attestation_data(&self, shard: u64) -> Result<AttestationData, Error> {
         let state = self.state.read();
         let head_block_root = self.head().beacon_block_root;
-        let head_block_slot = self.head().beacon_block.slot;
 
-        self.produce_attestation_data_for_block(shard, head_block_root, head_block_slot, &*state)
+        self.produce_attestation_data_for_block(shard, head_block_root, &*state)
     }
 
     /// Produce an `AttestationData` that attests to the chain denoted by `block_root` and `state`.
@@ -461,39 +454,19 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         shard: u64,
         head_block_root: Hash256,
-        head_block_slot: Slot,
         state: &BeaconState<T::EthSpec>,
     ) -> Result<AttestationData, Error> {
         // Collect some metrics.
         self.metrics.attestation_production_requests.inc();
         let timer = self.metrics.attestation_production_times.start_timer();
 
-        let slots_per_epoch = T::EthSpec::slots_per_epoch();
-        let current_epoch_start_slot = state.current_epoch().start_slot(slots_per_epoch);
-
         // The `target_root` is the root of the first block of the current epoch.
-        //
+        let target_root = self
-        // The `state` does not know the root of the block for it's current slot (it only knows
+            .rev_iter_block_roots()
-        // about blocks from prior slots). This creates an edge-case when the state is on the first
+            .find(|(_root, slot)| *slot % T::EthSpec::slots_per_epoch() == 0)
-        // slot of the epoch -- we're unable to obtain the `target_root` because it is not a prior
+            .map(|(root, _slot)| root)
-        // root.
+            .ok_or_else(|| Error::UnableToFindTargetRoot(self.head().beacon_state.slot))?;
-        //
+
-        // This edge case is handled in two ways:
-        //
-        // - If the head block is on the same slot as the state, we use it's root.
-        // - Otherwise, assume the current slot has been skipped and use the block root from the
-        // prior slot.
-        //
-        // For all other cases, we simply read the `target_root` from `state.latest_block_roots`.
-        let target_root = if state.slot == current_epoch_start_slot {
-            if head_block_slot == current_epoch_start_slot {
-                head_block_root
-            } else {
-                *state.get_block_root(current_epoch_start_slot - 1)?
-            }
-        } else {
-            *state.get_block_root(current_epoch_start_slot)?
-        };
         let target = Checkpoint {
             epoch: state.current_epoch(),
             root: target_root,
@@ -523,7 +496,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         })
     }
 
-    /// Accept a new attestation from the network.
+    /// Accept a new, potentially invalid attestation from the network.
     ///
     /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation
     /// if possible.
@@ -614,7 +587,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         &self,
         attestation: Attestation<T::EthSpec>,
         state: &BeaconState<T::EthSpec>,
-        _head_block: &BeaconBlock<T::EthSpec>,
+        block: &BeaconBlock<T::EthSpec>,
     ) -> Result<AttestationProcessingOutcome, Error> {
         self.metrics.attestation_processing_requests.inc();
         let timer = self.metrics.attestation_processing_times.start_timer();

beacon_node/beacon_chain/src/test_utils.rs

@@ -194,7 +194,7 @@ where
             if let BlockProcessingOutcome::Processed { block_root } = outcome {
                 head_block_root = Some(block_root);
 
-                self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot);
+                self.add_free_attestations(&attestation_strategy, &new_state, block_root);
             } else {
                 panic!("block should be successfully processed: {:?}", outcome);
             }
@@ -209,7 +209,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()
             .find(|(_hash, slot)| *slot == state_slot)
             .map(|(hash, _slot)| hash)
             .expect("could not find state root");
@@ -282,20 +282,14 @@ where
         attestation_strategy: &AttestationStrategy,
         state: &BeaconState<E>,
         head_block_root: Hash256,
-        head_block_slot: Slot,
     ) {
-        self.get_free_attestations(
+        self.get_free_attestations(attestation_strategy, state, head_block_root)
-            attestation_strategy,
+            .into_iter()
-            state,
+            .for_each(|attestation| {
-            head_block_root,
+                self.chain
-            head_block_slot,
+                    .process_attestation(attestation)
-        )
+                    .expect("should process attestation");
-        .into_iter()
+            });
-        .for_each(|attestation| {
-            self.chain
-                .process_attestation(attestation)
-                .expect("should process attestation");
-        });
     }
 
     /// Generates a `Vec<Attestation>` for some attestation strategy and head_block.
@@ -304,7 +298,6 @@ where
         attestation_strategy: &AttestationStrategy,
         state: &BeaconState<E>,
         head_block_root: Hash256,
-        head_block_slot: Slot,
     ) -> Vec<Attestation<E>> {
         let spec = &self.spec;
         let fork = &state.fork;
@@ -329,12 +322,7 @@ where
                     if attesting_validators.contains(validator_index) {
                         let data = self
                             .chain
-                            .produce_attestation_data_for_block(
+                            .produce_attestation_data_for_block(cc.shard, head_block_root, state)
-                                cc.shard,
-                                head_block_root,
-                                head_block_slot,
-                                state,
-                            )
                             .expect("should produce attestation data");
 
                         let mut aggregation_bits = BitList::with_capacity(committee_size).unwrap();

beacon_node/beacon_chain/tests/tests.rs

@@ -32,6 +32,73 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness<TestForkChoice, Min
     harness
 }
 
+#[test]
+fn iterators() {
+    let num_blocks_produced = MinimalEthSpec::slots_per_epoch() * 2 - 1;
+
+    let harness = get_harness(VALIDATOR_COUNT);
+
+    harness.extend_chain(
+        num_blocks_produced as usize,
+        BlockStrategy::OnCanonicalHead,
+        // No need to produce attestations for this test.
+        AttestationStrategy::SomeValidators(vec![]),
+    );
+
+    let block_roots: Vec<(Hash256, Slot)> = harness.chain.rev_iter_block_roots().collect();
+    let state_roots: Vec<(Hash256, Slot)> = harness.chain.rev_iter_state_roots().collect();
+
+    assert_eq!(
+        block_roots.len(),
+        state_roots.len(),
+        "should be an equal amount of block and state roots"
+    );
+
+    assert!(
+        block_roots.iter().any(|(_root, slot)| *slot == 0),
+        "should contain genesis block root"
+    );
+    assert!(
+        state_roots.iter().any(|(_root, slot)| *slot == 0),
+        "should contain genesis state root"
+    );
+
+    assert_eq!(
+        block_roots.len(),
+        num_blocks_produced as usize + 1,
+        "should contain all produced blocks, plus the genesis block"
+    );
+
+    block_roots.windows(2).for_each(|x| {
+        assert_eq!(
+            x[1].1,
+            x[0].1 - 1,
+            "block root slots should be decreasing by one"
+        )
+    });
+    state_roots.windows(2).for_each(|x| {
+        assert_eq!(
+            x[1].1,
+            x[0].1 - 1,
+            "state root slots should be decreasing by one"
+        )
+    });
+
+    let head = &harness.chain.head();
+
+    assert_eq!(
+        *block_roots.first().expect("should have some block roots"),
+        (head.beacon_block_root, head.beacon_block.slot),
+        "first block root and slot should be for the head block"
+    );
+
+    assert_eq!(
+        *state_roots.first().expect("should have some state roots"),
+        (head.beacon_state_root, head.beacon_state.slot),
+        "first state root and slot should be for the head state"
+    );
+}
+
 #[test]
 fn chooses_fork() {
     let harness = get_harness(VALIDATOR_COUNT);
@@ -326,7 +393,6 @@ fn attestations_with_increasing_slots() {
             &AttestationStrategy::AllValidators,
             &harness.chain.head().beacon_state,
             harness.chain.head().beacon_block_root,
-            harness.chain.head().beacon_block.slot,
         ));
 
         harness.advance_slot();

beacon_node/store/src/iter.rs

@@ -20,7 +20,7 @@ impl<'a, U: Store, E: EthSpec> AncestorIter<U, BlockRootsIterator<'a, E, U>> for
     fn try_iter_ancestor_roots(&self, store: Arc<U>) -> Option<BlockRootsIterator<'a, E, U>> {
         let state = store.get::<BeaconState<E>>(&self.state_root).ok()??;
 
-        Some(BlockRootsIterator::owned(store, state, self.slot))
+        Some(BlockRootsIterator::owned(store, state))
     }
 }
 
@@ -32,19 +32,19 @@ pub struct StateRootsIterator<'a, T: EthSpec, U> {
 }
 
 impl<'a, T: EthSpec, U: Store> StateRootsIterator<'a, T, U> {
-    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Borrowed(beacon_state),
-            slot: start_slot + 1,
         }
     }
 
-    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Owned(beacon_state),
-            slot: start_slot + 1,
         }
     }
 }
@@ -88,16 +88,16 @@ pub struct BlockIterator<'a, T: EthSpec, U> {
 
 impl<'a, T: EthSpec, U: Store> BlockIterator<'a, T, U> {
     /// Create a new iterator over all blocks in the given `beacon_state` and prior states.
-    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>) -> Self {
         Self {
-            roots: BlockRootsIterator::new(store, beacon_state, start_slot),
+            roots: BlockRootsIterator::new(store, beacon_state),
         }
     }
 
     /// Create a new iterator over all blocks in the given `beacon_state` and prior states.
-    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>, start_slot: Slot) -> Self {
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>) -> Self {
         Self {
-            roots: BlockRootsIterator::owned(store, beacon_state, start_slot),
+            roots: BlockRootsIterator::owned(store, beacon_state),
         }
     }
 }
@@ -128,20 +128,20 @@ pub struct BlockRootsIterator<'a, T: EthSpec, U> {
 
 impl<'a, T: EthSpec, U: Store> BlockRootsIterator<'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 {
+    pub fn new(store: Arc<U>, beacon_state: &'a BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Borrowed(beacon_state),
-            slot: start_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 {
+    pub fn owned(store: Arc<U>, beacon_state: BeaconState<T>) -> Self {
         Self {
             store,
+            slot: beacon_state.slot,
             beacon_state: Cow::Owned(beacon_state),
-            slot: start_slot + 1,
         }
     }
 }
@@ -218,7 +218,7 @@ mod test {
         state_b.state_roots[0] = state_a_root;
         store.put(&state_a_root, &state_a).unwrap();
 
-        let iter = BlockRootsIterator::new(store.clone(), &state_b, state_b.slot - 1);
+        let iter = BlockRootsIterator::new(store.clone(), &state_b);
 
         assert!(
             iter.clone().find(|(_root, slot)| *slot == 0).is_some(),
@@ -267,7 +267,7 @@ mod test {
         store.put(&state_a_root, &state_a).unwrap();
         store.put(&state_b_root, &state_b).unwrap();
 
-        let iter = StateRootsIterator::new(store.clone(), &state_b, state_b.slot - 1);
+        let iter = StateRootsIterator::new(store.clone(), &state_b);
 
         assert!(
             iter.clone().find(|(_root, slot)| *slot == 0).is_some(),

eth2/lmd_ghost/src/reduced_tree.rs

@@ -611,11 +611,7 @@ where
         let block = self.get_block(child)?;
         let state = self.get_state(block.state_root)?;
 
-        Ok(BlockRootsIterator::owned(
+        Ok(BlockRootsIterator::owned(self.store.clone(), state))
-            self.store.clone(),
-            state,
-            block.slot - 1,
-        ))
     }
 
     /// Verify the integrity of `self`. Returns `Ok(())` if the tree has integrity, otherwise returns `Err(description)`.