From 2fc6dbb02a16706bced7b20c35b3cdd90afda218 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 31 Jan 2019 08:49:50 +1100 Subject: [PATCH] Fix bug with `inclusion_distance` --- .../beacon_chain/src/block_production.rs | 22 +++++-- eth2/types/src/beacon_state/committees.rs | 31 +++++++-- .../src/beacon_state/epoch_processing.rs | 66 ++++++++++--------- .../types/src/beacon_state/slot_processing.rs | 10 --- 4 files changed, 77 insertions(+), 52 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_production.rs b/beacon_node/beacon_chain/src/block_production.rs index 289bc485d..2a9998ccf 100644 --- a/beacon_node/beacon_chain/src/block_production.rs +++ b/beacon_node/beacon_chain/src/block_production.rs @@ -4,6 +4,7 @@ use bls::Signature; use log::debug; use slot_clock::TestingSlotClockError; use types::{ + beacon_state::CommitteesError, readers::{BeaconBlockReader, BeaconStateReader}, BeaconBlock, BeaconBlockBody, BeaconState, Eth1Data, Hash256, }; @@ -13,6 +14,7 @@ pub enum Error { DBError(String), StateTransitionError(TransitionError), PresentSlotIsNone, + CommitteesError(CommitteesError), } impl BeaconChain @@ -50,12 +52,14 @@ where debug!("Finding attesatations for block..."); - let attestations = self - .attestation_aggregator - .read() - .unwrap() - // TODO: advance the parent_state slot. - .get_attestations_for_state(&parent_state, &self.spec); + let attestations = { + let mut next_state = parent_state.clone(); + next_state.per_slot_processing(Hash256::zero(), &self.spec)?; + self.attestation_aggregator + .read() + .unwrap() + .get_attestations_for_state(&next_state, &self.spec) + }; debug!("Found {} attestation(s).", attestations.len()); @@ -111,3 +115,9 @@ impl From for Error { unreachable!(); // Testing clock never throws an error. } } + +impl From for Error { + fn from(e: CommitteesError) -> Error { + Error::CommitteesError(e) + } +} diff --git a/eth2/types/src/beacon_state/committees.rs b/eth2/types/src/beacon_state/committees.rs index ae9ad6fac..cbf2dc054 100644 --- a/eth2/types/src/beacon_state/committees.rs +++ b/eth2/types/src/beacon_state/committees.rs @@ -1,15 +1,28 @@ use crate::{validator_registry::get_active_validator_indices, BeaconState, ChainSpec}; +use log::debug; use std::ops::Range; #[derive(Debug, PartialEq)] pub enum Error { - InvalidSlot(u64, Range), + InvalidEpoch(u64, Range), InsufficientNumberOfValidators, } +macro_rules! ensure { + ($condition: expr, $result: expr) => { + if !$condition { + return Err($result); + } + }; +} + type Result = std::result::Result; impl BeaconState { + pub fn current_epoch(&self, spec: &ChainSpec) -> u64 { + self.slot / spec.epoch_length + } + /// Returns the number of committees per slot. /// /// Note: this is _not_ the committee size. @@ -69,7 +82,7 @@ impl BeaconState { let previous_epoch_range = self.get_current_epoch_boundaries(spec.epoch_length); let current_epoch_range = self.get_current_epoch_boundaries(spec.epoch_length); if !range_contains(¤t_epoch_range, slot) { - return Err(Error::InvalidSlot(slot, current_epoch_range)); + return Err(Error::InvalidEpoch(slot, current_epoch_range)); } */ let epoch = slot / spec.epoch_length; @@ -81,9 +94,17 @@ impl BeaconState { }; let next_epoch = current_epoch + 1; - if !((previous_epoch <= epoch) & (epoch < next_epoch)) { - return Err(Error::InvalidSlot(slot, previous_epoch..current_epoch)); - } + /* + debug!( + "state.slot: {}, slot: {}, current_epoch: {}, previous_epoch: {}, next_epoch: {}", + self.slot, slot, current_epoch, previous_epoch, next_epoch + ); + */ + + ensure!( + (previous_epoch <= epoch) & (epoch < next_epoch), + Error::InvalidEpoch(slot, previous_epoch..current_epoch) + ); let offset = slot % spec.epoch_length; diff --git a/eth2/types/src/beacon_state/epoch_processing.rs b/eth2/types/src/beacon_state/epoch_processing.rs index 042bac518..8b429b967 100644 --- a/eth2/types/src/beacon_state/epoch_processing.rs +++ b/eth2/types/src/beacon_state/epoch_processing.rs @@ -54,10 +54,18 @@ impl BeaconState { total_balance ); + debug!( + "latest_attestations = {:?}", + self.latest_attestations + .iter() + .map(|a| a.data.slot) + .collect::>() + ); + let current_epoch_attestations: Vec<&PendingAttestation> = self .latest_attestations .iter() - .filter(|a| (self.slot - spec.epoch_length <= a.data.slot) && (a.data.slot < self.slot)) + .filter(|a| a.data.slot / spec.epoch_length == self.current_epoch(spec)) .collect(); /* @@ -103,8 +111,7 @@ impl BeaconState { .iter() .filter(|a| { //TODO: ensure these saturating subs are correct. - (self.slot.saturating_sub(2 * spec.epoch_length) <= a.data.slot) - && (a.data.slot < self.slot.saturating_sub(spec.epoch_length)) + a.data.slot / spec.epoch_length == self.current_epoch(spec).saturating_sub(1) }) .collect(); @@ -183,6 +190,11 @@ impl BeaconState { let previous_epoch_head_attesting_balance = self.get_effective_balances(&previous_epoch_head_attester_indices[..], spec); + debug!( + "previous_epoch_head_attester_balance of {} wei.", + previous_epoch_head_attesting_balance + ); + /* * Eth1 Data */ @@ -295,7 +307,8 @@ impl BeaconState { /* * Justification and finalization */ - let epochs_since_finality = (self.slot - self.finalized_slot) / spec.epoch_length; + let epochs_since_finality = + self.slot.saturating_sub(self.finalized_slot) / spec.epoch_length; // TODO: fix this extra map let previous_epoch_justified_attester_indices_hashset: HashSet = @@ -307,6 +320,8 @@ impl BeaconState { let previous_epoch_attester_indices_hashset: HashSet = HashSet::from_iter(previous_epoch_attester_indices.iter().map(|i| *i)); + debug!("{} epochs since finality.", epochs_since_finality); + if epochs_since_finality <= 4 { for index in 0..self.validator_balances.len() { let base_reward = self.base_reward(index, base_reward_quotient, spec); @@ -341,7 +356,7 @@ impl BeaconState { for index in previous_epoch_attester_indices { let base_reward = self.base_reward(index, base_reward_quotient, spec); - let inclusion_distance = self.inclusion_distance(index, spec)?; + let inclusion_distance = self.inclusion_distance(&previous_epoch_attestations, index, spec)?; safe_add_assign!( self.validator_balances[index], @@ -372,7 +387,7 @@ impl BeaconState { for index in previous_epoch_attester_indices { let base_reward = self.base_reward(index, base_reward_quotient, spec); - let inclusion_distance = self.inclusion_distance(index, spec)?; + let inclusion_distance = self.inclusion_distance(&previous_epoch_attestations, index, spec)?; safe_sub_assign!( self.validator_balances[index], @@ -388,7 +403,7 @@ impl BeaconState { * Attestation inclusion */ for index in previous_epoch_attester_indices_hashset { - let inclusion_slot = self.inclusion_slot(index, spec)?; + let inclusion_slot = self.inclusion_slot(&previous_epoch_attestations[..], index, spec)?; let proposer_index = self .get_beacon_proposer_index(inclusion_slot, spec) .map_err(|_| Error::UnableToDetermineProducer)?; @@ -499,7 +514,7 @@ impl BeaconState { self.latest_attestations = self .latest_attestations .iter() - .filter(|a| a.data.slot < self.slot - spec.epoch_length) + .filter(|a| a.data.slot / spec.epoch_length >= self.current_epoch(spec)) .cloned() .collect(); @@ -656,10 +671,11 @@ impl BeaconState { fn inclusion_distance( &self, + attestations: &[&PendingAttestation], validator_index: usize, spec: &ChainSpec, ) -> Result { - let attestation = self.earliest_included_attestation(validator_index, spec)?; + let attestation = self.earliest_included_attestation(attestations, validator_index, spec)?; Ok(attestation .slot_included .saturating_sub(attestation.data.slot)) @@ -667,21 +683,23 @@ impl BeaconState { fn inclusion_slot( &self, + attestations: &[&PendingAttestation], validator_index: usize, spec: &ChainSpec, ) -> Result { - let attestation = self.earliest_included_attestation(validator_index, spec)?; + let attestation = self.earliest_included_attestation(attestations, validator_index, spec)?; Ok(attestation.slot_included) } fn earliest_included_attestation( &self, + attestations: &[&PendingAttestation], validator_index: usize, spec: &ChainSpec, - ) -> Result<&PendingAttestation, InclusionError> { + ) -> Result { let mut included_attestations = vec![]; - for a in &self.latest_attestations { + for (i, a) in attestations.iter().enumerate() { let participants = self.get_attestation_participants(&a.data, &a.aggregation_bitfield, spec)?; if participants @@ -689,29 +707,15 @@ impl BeaconState { .find(|i| **i == validator_index) .is_some() { - included_attestations.push(a); + included_attestations.push(i); } } - Ok(included_attestations + let earliest_attestation_index = included_attestations .iter() - .min_by_key(|a| a.slot_included) - .and_then(|x| Some(*x)) - .ok_or_else(|| InclusionError::NoIncludedAttestations)?) - /* - self.latest_attestations - .iter() - .try_for_each(|a| { - self.get_attestation_participants(&a.data, &a.aggregation_bitfield, spec) - })? - .filter(|participants| { - participants - .iter() - .find(|i| **i == validator_index) - .is_some() - }) - .min_by_key(|a| a.slot_included) - */ + .min_by_key(|i| attestations[**i].slot_included) + .ok_or_else(|| InclusionError::NoIncludedAttestations)?; + Ok(attestations[*earliest_attestation_index].clone()) } fn base_reward( diff --git a/eth2/types/src/beacon_state/slot_processing.rs b/eth2/types/src/beacon_state/slot_processing.rs index 5974596b6..9a82bcdf4 100644 --- a/eth2/types/src/beacon_state/slot_processing.rs +++ b/eth2/types/src/beacon_state/slot_processing.rs @@ -36,19 +36,9 @@ impl BeaconState { if committee.iter().find(|i| **i == validator_index).is_some() { result = Some(Ok((slot, shard))); } - } } result.unwrap() - /* - // TODO: this is a stub; implement it properly. - let validator_index = validator_index as u64; - - let slot = validator_index % spec.epoch_length; - let shard = validator_index % spec.shard_count; - - (slot, shard) - */ } }