From 4f98609beec359e19ba9fbb949f0ea964ef126f6 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 7 Jul 2022 16:18:34 +1000 Subject: [PATCH] Fix tests and block rewards API --- beacon_node/beacon_chain/src/beacon_chain.rs | 9 +- beacon_node/beacon_chain/src/block_reward.rs | 38 ++++++--- .../operation_pool/src/attestation_storage.rs | 81 +++++++++++------- beacon_node/operation_pool/src/lib.rs | 85 ++++++++++--------- .../operation_pool/src/reward_cache.rs | 34 ++++++-- .../src/common/get_attesting_indices.rs | 11 ++- consensus/state_processing/src/common/mod.rs | 2 +- 7 files changed, 162 insertions(+), 98 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 16820f08f3..df7c9cfeab 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/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 BeaconChain { metrics::start_timer(&metrics::BLOCK_PRODUCTION_UNAGGREGATED_TIMES); for attestation in self.naive_aggregation_pool.read().iter() { let import = |attestation: &Attestation| { - let committee = - state.get_beacon_committee(attestation.data.slot, attestation.data.index)?; - let attesting_indices = get_attesting_indices::( - committee.committee, - &attestation.aggregation_bits, - )?; + let attesting_indices = get_attesting_indices_from_state(&state, &attestation)?; self.op_pool .insert_attestation(attestation.clone(), attesting_indices) }; diff --git a/beacon_node/beacon_chain/src/block_reward.rs b/beacon_node/beacon_chain/src/block_reward.rs index 790ad743b2..478c21add8 100644 --- a/beacon_node/beacon_chain/src/block_reward.rs +++ b/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 state_processing::per_block_processing::altair::sync_committee::compute_sync_aggregate_rewards; +use operation_pool::{AttMaxCover, MaxCover, RewardCache, SplitAttestation}; +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 BeaconChain { @@ -12,21 +15,37 @@ impl BeaconChain { state: &BeaconState, include_attestations: bool, ) -> Result { - // 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) - .ok_or(BeaconChainError::BlockRewardAttestationError) + let attesting_indices = get_attesting_indices_from_state(state, att)?; + Ok(SplitAttestation::new(att.clone(), attesting_indices)) + }) + .collect::, 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::, _>>()?; @@ -37,7 +56,7 @@ impl BeaconChain { 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 BeaconChain { attestation_rewards, sync_committee_rewards, }) - */ } } diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 3e3f6f1b77..56e2240e7b 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -27,6 +27,13 @@ pub struct CompactIndexedAttestation { pub signature: AggregateSignature, } +#[derive(Debug)] +pub struct SplitAttestation { + pub checkpoint: CheckpointKey, + pub data: CompactAttestationData, + pub indexed: CompactIndexedAttestation, +} + #[derive(Debug, Clone)] pub struct AttestationRef<'a, T: EthSpec> { pub checkpoint: &'a CheckpointKey, @@ -44,30 +51,37 @@ pub struct AttestationDataMap { attestations: HashMap>>, } -fn split( - attestation: Attestation, - attesting_indices: Vec, -) -> ( - CheckpointKey, - CompactAttestationData, - CompactIndexedAttestation, -) { - let checkpoint_key = CheckpointKey { - source: attestation.data.source, - target_epoch: attestation.data.target.epoch, - }; - let attestation_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 { - attesting_indices, - aggregation_bits: attestation.aggregation_bits, - signature: attestation.signature, - }; - (checkpoint_key, attestation_data, indexed_attestation) +impl SplitAttestation { + pub fn new(attestation: Attestation, attesting_indices: Vec) -> Self { + let checkpoint = CheckpointKey { + source: attestation.data.source, + target_epoch: attestation.data.target.epoch, + }; + 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 = CompactIndexedAttestation { + attesting_indices, + aggregation_bits: attestation.aggregation_bits, + signature: attestation.signature, + }; + Self { + checkpoint, + data, + indexed, + } + } + + pub fn as_ref(&self) -> AttestationRef { + AttestationRef { + checkpoint: &self.checkpoint, + data: &self.data, + indexed: &self.indexed, + } + } } impl<'a, T: EthSpec> AttestationRef<'a, T> { @@ -129,16 +143,19 @@ impl CompactIndexedAttestation { impl AttestationMap { pub fn insert(&mut self, attestation: Attestation, attesting_indices: Vec) { - let (checkpoint_key, attestation_data, indexed_attestation) = - split(attestation, attesting_indices); + let SplitAttestation { + 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 AttestationMap { // aggregation. let mut aggregated = false; for existing_attestation in attestations.iter_mut() { - if existing_attestation.signers_disjoint_from(&indexed_attestation) { - existing_attestation.aggregate(&indexed_attestation); + if existing_attestation.signers_disjoint_from(&indexed) { + 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); } } diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 27dcf89f70..0003841db4 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/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, - VerifySignatures, + get_slashable_indices_modular, verify_exit, 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( num_committees: usize, ) -> (BeaconChainHarness>, ChainSpec) { - let spec = test_spec::(); + let mut spec = test_spec::(); + + // 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() { - op_pool - .insert_attestation(att.0, &state.fork(), state.genesis_validators_root(), spec) - .unwrap(); + for (att, _) in atts { + let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap(); + op_pool.insert_attestation(att, attesting_indices).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( - att.clone(), - &state.fork(), - state.genesis_validators_root(), - spec, - ) - .unwrap(); - op_pool - .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec) + .insert_attestation(att.clone(), attesting_indices.clone()) .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::>(); for att in aggs1.into_iter().chain(aggs2.into_iter()) { - op_pool - .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec) - .unwrap(); + let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap(); + op_pool.insert_attestation(att, attesting_indices).unwrap(); } } // The attestations should get aggregated into two attestations that comprise all // validators. - assert_eq!(op_pool.attestations.read().len(), committees.len()); - assert_eq!(op_pool.num_attestations(), 2 * committees.len()); + let stats = op_pool.attestation_stats(); + 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::>(); for att in aggs { - op_pool - .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec) - .unwrap(); + let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap(); + op_pool.insert_attestation(att, attesting_indices).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::>(); for att in aggs { - op_pool - .insert_attestation(att, &state.fork(), state.genesis_validators_root(), spec) - .unwrap(); + let attesting_indices = get_attesting_indices_from_state(&state, &att).unwrap(); + op_pool.insert_attestation(att, attesting_indices).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,11 +1171,21 @@ 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 fresh_validators_rewards = - AttMaxCover::new(att, &state, total_active_balance, spec) - .unwrap() - .fresh_validators_rewards; + let mut reward_cache = RewardCache::default(); + reward_cache.update(&state).unwrap(); + + 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; // Remove validators covered by previous attestations. fresh_validators_rewards diff --git a/beacon_node/operation_pool/src/reward_cache.rs b/beacon_node/operation_pool/src/reward_cache.rs index b278cb14f0..451806d2d4 100644 --- a/beacon_node/operation_pool/src/reward_cache.rs +++ b/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( + state: &BeaconState, + slot: Slot, + ) -> Result { + if slot == 0 { + Ok(Hash256::zero()) + } else { + Ok(*state + .get_block_root(slot) + .map_err(OpPoolError::RewardCacheGetBlockRoot)?) + } + } + /// Update the cache. pub fn update(&mut self, state: &BeaconState) -> Result<(), OpPoolError> { + if state.previous_epoch_participation().is_err() { + return Ok(()); + } + let current_epoch = state.current_epoch(); - let prev_epoch_last_block_root = *state - .get_block_root(state.previous_epoch().start_slot(E::slots_per_epoch())) - .map_err(OpPoolError::RewardCacheGetBlockRoot)?; - let latest_block_root = *state - .get_block_root(state.slot() - 1) - .map_err(OpPoolError::RewardCacheGetBlockRoot)?; + let prev_epoch_last_block_root = Self::marker_block_root( + state, + state.previous_epoch().start_slot(E::slots_per_epoch()), + )?; + let latest_block_root = Self::marker_block_root(state, state.slot() - 1)?; // 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 diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 3367b8b064..4cb2f2e0e5 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/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( committee: &[usize], bitlist: &BitList, @@ -23,3 +21,12 @@ pub fn get_attesting_indices( Ok(indices) } + +/// Shortcut for getting the attesting indices while fetching the committee from the state's cache. +pub fn get_attesting_indices_from_state( + state: &BeaconState, + att: &Attestation, +) -> Result, BeaconStateError> { + let committee = state.get_beacon_committee(att.data.slot, att.data.index)?; + get_attesting_indices::(&committee.committee, &att.aggregation_bits) +} diff --git a/consensus/state_processing/src/common/mod.rs b/consensus/state_processing/src/common/mod.rs index 334a293ed5..8a2e2439bb 100644 --- a/consensus/state_processing/src/common/mod.rs +++ b/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;