Fix tests and block rewards API

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -69,7 +69,7 @@ use slog::{crit, debug, error, info, trace, warn, Logger};
 use slot_clock::SlotClock;
 use ssz::Encode;
 use state_processing::{
-    common::{get_attesting_indices, get_indexed_attestation},
+    common::{get_attesting_indices_from_state, get_indexed_attestation},
     per_block_processing,
     per_block_processing::{
         errors::AttestationValidationError, verify_attestation_for_block_inclusion,
@@ -3280,12 +3280,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             metrics::start_timer(&metrics::BLOCK_PRODUCTION_UNAGGREGATED_TIMES);
         for attestation in self.naive_aggregation_pool.read().iter() {
             let import = |attestation: &Attestation<T::EthSpec>| {
-                let committee =
+                let attesting_indices = get_attesting_indices_from_state(&state, &attestation)?;
-                    state.get_beacon_committee(attestation.data.slot, attestation.data.index)?;
-                let attesting_indices = get_attesting_indices::<T::EthSpec>(
-                    committee.committee,
-                    &attestation.aggregation_bits,
-                )?;
                 self.op_pool
                     .insert_attestation(attestation.clone(), attesting_indices)
             };

beacon_node/beacon_chain/src/block_reward.rs

@@ -1,7 +1,10 @@
 use crate::{BeaconChain, BeaconChainError, BeaconChainTypes};
 use eth2::lighthouse::{AttestationRewards, BlockReward, BlockRewardMeta};
-use operation_pool::{AttMaxCover, MaxCover};
+use operation_pool::{AttMaxCover, MaxCover, RewardCache, SplitAttestation};
-use state_processing::per_block_processing::altair::sync_committee::compute_sync_aggregate_rewards;
+use state_processing::{
+    common::get_attesting_indices_from_state,
+    per_block_processing::altair::sync_committee::compute_sync_aggregate_rewards,
+};
 use types::{BeaconBlockRef, BeaconState, EthSpec, ExecPayload, Hash256};
 
 impl<T: BeaconChainTypes> BeaconChain<T> {
@@ -12,20 +15,36 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         state: &BeaconState<T::EthSpec>,
         include_attestations: bool,
     ) -> Result<BlockReward, BeaconChainError> {
-        // FIXME(sproul): make an AttestationRef?
-        unimplemented!()
-        /*
         if block.slot() != state.slot() {
             return Err(BeaconChainError::BlockRewardSlotError);
         }
 
+        // FIXME(sproul): pass this in
+        let mut reward_cache = RewardCache::default();
+        reward_cache.update(state)?;
+
         let total_active_balance = state.get_total_active_balance()?;
-        let mut per_attestation_rewards = block
+
+        let split_attestations = block
             .body()
             .attestations()
             .iter()
             .map(|att| {
-                AttMaxCover::new(att, state, total_active_balance, &self.spec)
+                let attesting_indices = get_attesting_indices_from_state(state, att)?;
+                Ok(SplitAttestation::new(att.clone(), attesting_indices))
+            })
+            .collect::<Result<Vec<_>, BeaconChainError>>()?;
+
+        let mut per_attestation_rewards = split_attestations
+            .iter()
+            .map(|att| {
+                AttMaxCover::new(
+                    att.as_ref(),
+                    state,
+                    &reward_cache,
+                    total_active_balance,
+                    &self.spec,
+                )
                 .ok_or(BeaconChainError::BlockRewardAttestationError)
             })
             .collect::<Result<Vec<_>, _>>()?;
@@ -37,7 +56,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             let latest_att = &updated[i];
 
             for att in to_update {
-                att.update_covering_set(latest_att.object(), latest_att.covering_set());
+                att.update_covering_set(latest_att.intermediate(), latest_att.covering_set());
             }
         }
 
@@ -109,6 +128,5 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
             attestation_rewards,
             sync_committee_rewards,
         })
-        */
     }
 }

beacon_node/operation_pool/src/attestation_storage.rs

@@ -27,6 +27,13 @@ pub struct CompactIndexedAttestation<T: EthSpec> {
     pub signature: AggregateSignature,
 }
 
+#[derive(Debug)]
+pub struct SplitAttestation<T: EthSpec> {
+    pub checkpoint: CheckpointKey,
+    pub data: CompactAttestationData,
+    pub indexed: CompactIndexedAttestation<T>,
+}
+
 #[derive(Debug, Clone)]
 pub struct AttestationRef<'a, T: EthSpec> {
     pub checkpoint: &'a CheckpointKey,
@@ -44,30 +51,37 @@ pub struct AttestationDataMap<T: EthSpec> {
     attestations: HashMap<CompactAttestationData, Vec<CompactIndexedAttestation<T>>>,
 }
 
-fn split<T: EthSpec>(
+impl<T: EthSpec> SplitAttestation<T> {
-    attestation: Attestation<T>,
+    pub fn new(attestation: Attestation<T>, attesting_indices: Vec<u64>) -> Self {
-    attesting_indices: Vec<u64>,
+        let checkpoint = CheckpointKey {
-) -> (
-    CheckpointKey,
-    CompactAttestationData,
-    CompactIndexedAttestation<T>,
-) {
-    let checkpoint_key = CheckpointKey {
             source: attestation.data.source,
             target_epoch: attestation.data.target.epoch,
         };
-    let attestation_data = CompactAttestationData {
+        let data = CompactAttestationData {
             slot: attestation.data.slot,
             index: attestation.data.index,
             beacon_block_root: attestation.data.beacon_block_root,
             target_root: attestation.data.target.root,
         };
-    let indexed_attestation = CompactIndexedAttestation {
+        let indexed = CompactIndexedAttestation {
             attesting_indices,
             aggregation_bits: attestation.aggregation_bits,
             signature: attestation.signature,
         };
-    (checkpoint_key, attestation_data, indexed_attestation)
+        Self {
+            checkpoint,
+            data,
+            indexed,
+        }
+    }
+
+    pub fn as_ref(&self) -> AttestationRef<T> {
+        AttestationRef {
+            checkpoint: &self.checkpoint,
+            data: &self.data,
+            indexed: &self.indexed,
+        }
+    }
 }
 
 impl<'a, T: EthSpec> AttestationRef<'a, T> {
@@ -129,16 +143,19 @@ impl<T: EthSpec> CompactIndexedAttestation<T> {
 
 impl<T: EthSpec> AttestationMap<T> {
     pub fn insert(&mut self, attestation: Attestation<T>, attesting_indices: Vec<u64>) {
-        let (checkpoint_key, attestation_data, indexed_attestation) =
+        let SplitAttestation {
-            split(attestation, attesting_indices);
+            checkpoint,
+            data,
+            indexed,
+        } = SplitAttestation::new(attestation, attesting_indices);
 
         let attestation_map = self
             .checkpoint_map
-            .entry(checkpoint_key)
+            .entry(checkpoint)
             .or_insert_with(AttestationDataMap::default);
         let attestations = attestation_map
             .attestations
-            .entry(attestation_data)
+            .entry(data)
             .or_insert_with(Vec::new);
 
         // Greedily aggregate the attestation with all existing attestations.
@@ -146,16 +163,16 @@ impl<T: EthSpec> AttestationMap<T> {
         // aggregation.
         let mut aggregated = false;
         for existing_attestation in attestations.iter_mut() {
-            if existing_attestation.signers_disjoint_from(&indexed_attestation) {
+            if existing_attestation.signers_disjoint_from(&indexed) {
-                existing_attestation.aggregate(&indexed_attestation);
+                existing_attestation.aggregate(&indexed);
                 aggregated = true;
-            } else if *existing_attestation == indexed_attestation {
+            } else if *existing_attestation == indexed {
                 aggregated = true;
             }
         }
 
         if !aggregated {
-            attestations.push(indexed_attestation);
+            attestations.push(indexed);
         }
     }
 

beacon_node/operation_pool/src/lib.rs

@@ -9,7 +9,7 @@ mod reward_cache;
 mod sync_aggregate_id;
 
 pub use attestation::AttMaxCover;
-pub use attestation_storage::AttestationRef;
+pub use attestation_storage::{AttestationRef, SplitAttestation};
 pub use max_cover::MaxCover;
 pub use persistence::{PersistedOperationPool, PersistedOperationPoolAltair};
 pub use reward_cache::RewardCache;
@@ -21,8 +21,7 @@ use max_cover::maximum_cover;
 use parking_lot::{RwLock, RwLockWriteGuard};
 use state_processing::per_block_processing::errors::AttestationValidationError;
 use state_processing::per_block_processing::{
-    get_slashable_indices_modular, verify_attestation_for_block_inclusion, verify_exit,
+    get_slashable_indices_modular, verify_exit, VerifySignatures,
-    VerifySignatures,
 };
 use state_processing::SigVerifiedOp;
 use std::collections::{hash_map::Entry, HashMap, HashSet};
@@ -633,7 +632,7 @@ mod release_tests {
         test_spec, BeaconChainHarness, EphemeralHarnessType, RelativeSyncCommittee,
     };
     use lazy_static::lazy_static;
-    use state_processing::VerifyOperation;
+    use state_processing::{common::get_attesting_indices_from_state, VerifyOperation};
     use std::collections::BTreeSet;
     use types::consts::altair::SYNC_COMMITTEE_SUBNET_COUNT;
     use types::*;
@@ -664,7 +663,10 @@ mod release_tests {
     fn attestation_test_state<E: EthSpec>(
         num_committees: usize,
     ) -> (BeaconChainHarness<EphemeralHarnessType<E>>, ChainSpec) {
-        let spec = test_spec::<E>();
+        let mut spec = test_spec::<E>();
+
+        // FIXME(sproul): make this modular?
+        spec.altair_fork_epoch = Some(Epoch::new(0));
 
         let num_validators =
             num_committees * E::slots_per_epoch() as usize * spec.target_committee_size;
@@ -804,14 +806,12 @@ mod release_tests {
         );
 
         for (atts, _) in attestations {
-            for att in atts.into_iter() {
+            for (att, _) in atts {
-                op_pool
+                let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap();
-                    .insert_attestation(att.0, &state.fork(), state.genesis_validators_root(), spec)
+                op_pool.insert_attestation(att, attesting_indices).unwrap();
-                    .unwrap();
             }
         }
 
-        assert_eq!(op_pool.attestations.read().len(), committees.len());
         assert_eq!(op_pool.num_attestations(), committees.len());
 
         // Before the min attestation inclusion delay, get_attestations shouldn't return anything.
@@ -877,17 +877,11 @@ mod release_tests {
 
         for (_, aggregate) in attestations {
             let att = aggregate.unwrap().message.aggregate;
+            let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap();
             op_pool
-                .insert_attestation(
+                .insert_attestation(att.clone(), attesting_indices.clone())
-                    att.clone(),
-                    &state.fork(),
-                    state.genesis_validators_root(),
-                    spec,
-                )
-                .unwrap();
-            op_pool
-                .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec)
                 .unwrap();
+            op_pool.insert_attestation(att, attesting_indices).unwrap();
         }
 
         assert_eq!(op_pool.num_attestations(), committees.len());
@@ -971,16 +965,17 @@ mod release_tests {
                 .collect::<Vec<_>>();
 
             for att in aggs1.into_iter().chain(aggs2.into_iter()) {
-                op_pool
+                let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap();
-                    .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec)
+                op_pool.insert_attestation(att, attesting_indices).unwrap();
-                    .unwrap();
             }
         }
 
         // The attestations should get aggregated into two attestations that comprise all
         // validators.
-        assert_eq!(op_pool.attestations.read().len(), committees.len());
+        let stats = op_pool.attestation_stats();
-        assert_eq!(op_pool.num_attestations(), 2 * committees.len());
+        assert_eq!(stats.num_attestation_data, committees.len());
+        assert_eq!(stats.num_attestations, 2 * committees.len());
+        assert_eq!(stats.max_aggregates_per_data, 2);
     }
 
     /// Create a bunch of attestations signed by a small number of validators, and another
@@ -1042,9 +1037,8 @@ mod release_tests {
                 .collect::<Vec<_>>();
 
             for att in aggs {
-                op_pool
+                let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap();
-                    .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec)
+                op_pool.insert_attestation(att, attesting_indices).unwrap();
-                    .unwrap();
             }
         };
 
@@ -1059,12 +1053,13 @@ mod release_tests {
         let num_small = target_committee_size / small_step_size;
         let num_big = target_committee_size / big_step_size;
 
-        assert_eq!(op_pool.attestations.read().len(), committees.len());
+        let stats = op_pool.attestation_stats();
+        assert_eq!(stats.num_attestation_data, committees.len());
         assert_eq!(
-            op_pool.num_attestations(),
+            stats.num_attestations,
             (num_small + num_big) * committees.len()
         );
-        assert!(op_pool.num_attestations() > max_attestations);
+        assert!(stats.num_attestations > max_attestations);
 
         *state.slot_mut() += spec.min_attestation_inclusion_delay;
         let best_attestations = op_pool
@@ -1137,9 +1132,8 @@ mod release_tests {
                 .collect::<Vec<_>>();
 
             for att in aggs {
-                op_pool
+                let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap();
-                    .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec)
+                op_pool.insert_attestation(att, attesting_indices).unwrap();
-                    .unwrap();
             }
         };
 
@@ -1154,7 +1148,10 @@ mod release_tests {
         let num_small = target_committee_size / small_step_size;
         let num_big = target_committee_size / big_step_size;
 
-        assert_eq!(op_pool.attestations.read().len(), committees.len());
+        assert_eq!(
+            op_pool.attestation_stats().num_attestation_data,
+            committees.len()
+        );
         assert_eq!(
             op_pool.num_attestations(),
             (num_small + num_big) * committees.len()
@@ -1174,9 +1171,19 @@ mod release_tests {
         // Used for asserting that rewards are in decreasing order.
         let mut prev_reward = u64::max_value();
 
-        for att in &best_attestations {
+        let mut reward_cache = RewardCache::default();
-            let mut fresh_validators_rewards =
+        reward_cache.update(&state).unwrap();
-                AttMaxCover::new(att, &state, total_active_balance, spec)
+
+        for att in best_attestations {
+            let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap();
+            let split_attestation = SplitAttestation::new(att, attesting_indices);
+            let mut fresh_validators_rewards = AttMaxCover::new(
+                split_attestation.as_ref(),
+                &state,
+                &reward_cache,
+                total_active_balance,
+                spec,
+            )
             .unwrap()
             .fresh_validators_rewards;
 

beacon_node/operation_pool/src/reward_cache.rs

@@ -1,6 +1,6 @@
 use crate::OpPoolError;
 use bitvec::vec::BitVec;
-use types::{BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, ParticipationFlags};
+use types::{BeaconState, BeaconStateError, Epoch, EthSpec, Hash256, ParticipationFlags, Slot};
 
 #[derive(Debug, Clone)]
 struct Initialization {
@@ -49,15 +49,35 @@ impl RewardCache {
         }
     }
 
+    // Determine the "marker" block root to store in `self.init` for a given `slot`.
+    //
+    // For simplicity at genesis we return the zero hash, which will cause one unnecessary
+    // re-calculation.
+    fn marker_block_root<E: EthSpec>(
+        state: &BeaconState<E>,
+        slot: Slot,
+    ) -> Result<Hash256, OpPoolError> {
+        if slot == 0 {
+            Ok(Hash256::zero())
+        } else {
+            Ok(*state
+                .get_block_root(slot)
+                .map_err(OpPoolError::RewardCacheGetBlockRoot)?)
+        }
+    }
+
     /// Update the cache.
     pub fn update<E: EthSpec>(&mut self, state: &BeaconState<E>) -> Result<(), OpPoolError> {
+        if state.previous_epoch_participation().is_err() {
+            return Ok(());
+        }
+
         let current_epoch = state.current_epoch();
-        let prev_epoch_last_block_root = *state
+        let prev_epoch_last_block_root = Self::marker_block_root(
-            .get_block_root(state.previous_epoch().start_slot(E::slots_per_epoch()))
+            state,
-            .map_err(OpPoolError::RewardCacheGetBlockRoot)?;
+            state.previous_epoch().start_slot(E::slots_per_epoch()),
-        let latest_block_root = *state
+        )?;
-            .get_block_root(state.slot() - 1)
+        let latest_block_root = Self::marker_block_root(state, state.slot() - 1)?;
-            .map_err(OpPoolError::RewardCacheGetBlockRoot)?;
 
         // If the `state` is from a new epoch or a different fork with a different last epoch block,
         // then update the effective balance cache (the effective balances are liable to have

consensus/state_processing/src/common/get_attesting_indices.rs

@@ -1,8 +1,6 @@
 use types::*;
 
 /// Returns validator indices which participated in the attestation, sorted by increasing index.
-///
-/// Spec v0.12.1
 pub fn get_attesting_indices<T: EthSpec>(
     committee: &[usize],
     bitlist: &BitList<T::MaxValidatorsPerCommittee>,
@@ -23,3 +21,12 @@ pub fn get_attesting_indices<T: EthSpec>(
 
     Ok(indices)
 }
+
+/// Shortcut for getting the attesting indices while fetching the committee from the state's cache.
+pub fn get_attesting_indices_from_state<T: EthSpec>(
+    state: &BeaconState<T>,
+    att: &Attestation<T>,
+) -> Result<Vec<u64>, BeaconStateError> {
+    let committee = state.get_beacon_committee(att.data.slot, att.data.index)?;
+    get_attesting_indices::<T>(&committee.committee, &att.aggregation_bits)
+}

consensus/state_processing/src/common/mod.rs

@@ -10,7 +10,7 @@ pub mod base;
 
 pub use deposit_data_tree::DepositDataTree;
 pub use get_attestation_participation::get_attestation_participation_flag_indices;
-pub use get_attesting_indices::get_attesting_indices;
+pub use get_attesting_indices::{get_attesting_indices, get_attesting_indices_from_state};
 pub use get_indexed_attestation::get_indexed_attestation;
 pub use initiate_validator_exit::initiate_validator_exit;
 pub use slash_validator::slash_validator;