From 5a7903a3773ae5c640a0004ef2f2ddec77322dfb Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Mon, 19 Aug 2019 16:15:55 +1000 Subject: [PATCH] Improve BeaconState safe accessors And fix a bug in the compact committees accessor. --- .../src/per_epoch_processing.rs | 45 +++----- eth2/types/src/beacon_state.rs | 104 +++++++++++------- 2 files changed, 81 insertions(+), 68 deletions(-) diff --git a/eth2/state_processing/src/per_epoch_processing.rs b/eth2/state_processing/src/per_epoch_processing.rs index 8d6153aea..71d8b20da 100644 --- a/eth2/state_processing/src/per_epoch_processing.rs +++ b/eth2/state_processing/src/per_epoch_processing.rs @@ -221,42 +221,29 @@ pub fn process_final_updates( // Update start shard. state.start_shard = state.next_epoch_start_shard(spec)?; - // This is a hack to allow us to update index roots and slashed balances for the next epoch. - // - // The indentation here is to make it obvious where the weird stuff happens. - { - state.slot += 1; - - // Set active index root - let index_epoch = next_epoch + spec.activation_exit_delay; - let indices_list = VariableList::::from( - state.get_active_validator_indices(index_epoch), - ); - state.set_active_index_root( - index_epoch, - Hash256::from_slice(&indices_list.tree_hash_root()), - spec, - )?; - - // Reset slashings - state.set_slashings(next_epoch, 0)?; - - // Set randao mix - state.set_randao_mix(next_epoch, *state.get_randao_mix(current_epoch)?)?; - - state.slot -= 1; - } + // Set active index root + let index_epoch = next_epoch + spec.activation_exit_delay; + let indices_list = VariableList::::from( + state.get_active_validator_indices(index_epoch), + ); + state.set_active_index_root( + index_epoch, + Hash256::from_slice(&indices_list.tree_hash_root()), + spec, + )?; // Set committees root - // Note: we do this out-of-order w.r.t. to the spec, because we don't want the slot to be - // incremented. It's safe because the updates to slashings and the RANDAO mix (above) don't - // affect this. state.set_compact_committee_root( next_epoch, get_compact_committees_root(state, RelativeEpoch::Next, spec)?, - spec, )?; + // Reset slashings + state.set_slashings(next_epoch, 0)?; + + // Set randao mix + state.set_randao_mix(next_epoch, *state.get_randao_mix(current_epoch)?)?; + // Set historical root accumulator if next_epoch.as_u64() % (T::SlotsPerHistoricalRoot::to_u64() / T::slots_per_epoch()) == 0 { let historical_batch = state.historical_batch(); diff --git a/eth2/types/src/beacon_state.rs b/eth2/types/src/beacon_state.rs index d312316f3..5b00f08b7 100644 --- a/eth2/types/src/beacon_state.rs +++ b/eth2/types/src/beacon_state.rs @@ -60,6 +60,22 @@ pub enum Error { SszTypesError(ssz_types::Error), } +/// Control whether an epoch-indexed field can be indexed at the next epoch or not. +#[derive(Debug, PartialEq, Clone, Copy)] +enum AllowNextEpoch { + True, + False, +} + +impl AllowNextEpoch { + fn upper_bound_of(self, current_epoch: Epoch) -> Epoch { + match self { + AllowNextEpoch::True => current_epoch + 1, + AllowNextEpoch::False => current_epoch, + } + } +} + /// The state of the `BeaconChain` at some slot. /// /// Spec v0.8.0 @@ -108,12 +124,12 @@ where pub start_shard: u64, pub randao_mixes: FixedVector, #[compare_fields(as_slice)] - active_index_roots: FixedVector, + pub active_index_roots: FixedVector, #[compare_fields(as_slice)] - compact_committees_roots: FixedVector, + pub compact_committees_roots: FixedVector, // Slashings - slashings: FixedVector, + pub slashings: FixedVector, // Attestations pub previous_epoch_attestations: VariableList, T::MaxPendingAttestations>, @@ -459,12 +475,16 @@ impl BeaconState { /// Safely obtains the index for `randao_mixes` /// - /// Spec v0.8.0 - fn get_randao_mix_index(&self, epoch: Epoch) -> Result { + /// Spec v0.8.1 + fn get_randao_mix_index( + &self, + epoch: Epoch, + allow_next_epoch: AllowNextEpoch, + ) -> Result { let current_epoch = self.current_epoch(); let len = T::EpochsPerHistoricalVector::to_u64(); - if epoch + len > current_epoch && epoch <= current_epoch { + if current_epoch < epoch + len && epoch <= allow_next_epoch.upper_bound_of(current_epoch) { Ok(epoch.as_usize() % len as usize) } else { Err(Error::EpochOutOfBounds) @@ -492,7 +512,7 @@ impl BeaconState { /// /// Spec v0.8.1 pub fn get_randao_mix(&self, epoch: Epoch) -> Result<&Hash256, Error> { - let i = self.get_randao_mix_index(epoch)?; + let i = self.get_randao_mix_index(epoch, AllowNextEpoch::False)?; Ok(&self.randao_mixes[i]) } @@ -500,21 +520,29 @@ impl BeaconState { /// /// Spec v0.8.1 pub fn set_randao_mix(&mut self, epoch: Epoch, mix: Hash256) -> Result<(), Error> { - let i = self.get_randao_mix_index(epoch)?; + let i = self.get_randao_mix_index(epoch, AllowNextEpoch::True)?; self.randao_mixes[i] = mix; Ok(()) } /// Safely obtains the index for `active_index_roots`, given some `epoch`. /// + /// If `allow_next_epoch` is `True`, then we allow an _extra_ one epoch of lookahead. + /// /// Spec v0.8.1 - fn get_active_index_root_index(&self, epoch: Epoch, spec: &ChainSpec) -> Result { + fn get_active_index_root_index( + &self, + epoch: Epoch, + spec: &ChainSpec, + allow_next_epoch: AllowNextEpoch, + ) -> Result { let current_epoch = self.current_epoch(); let lookahead = spec.activation_exit_delay; let lookback = self.active_index_roots.len() as u64 - lookahead; + let epoch_upper_bound = allow_next_epoch.upper_bound_of(current_epoch) + lookahead; - if epoch + lookback > current_epoch && current_epoch + lookahead >= epoch { + if current_epoch < epoch + lookback && epoch <= epoch_upper_bound { Ok(epoch.as_usize() % self.active_index_roots.len()) } else { Err(Error::EpochOutOfBounds) @@ -525,7 +553,7 @@ impl BeaconState { /// /// Spec v0.8.1 pub fn get_active_index_root(&self, epoch: Epoch, spec: &ChainSpec) -> Result { - let i = self.get_active_index_root_index(epoch, spec)?; + let i = self.get_active_index_root_index(epoch, spec, AllowNextEpoch::False)?; Ok(self.active_index_roots[i]) } @@ -538,7 +566,7 @@ impl BeaconState { index_root: Hash256, spec: &ChainSpec, ) -> Result<(), Error> { - let i = self.get_active_index_root_index(epoch, spec)?; + let i = self.get_active_index_root_index(epoch, spec, AllowNextEpoch::True)?; self.active_index_roots[i] = index_root; Ok(()) } @@ -552,19 +580,17 @@ impl BeaconState { /// Safely obtains the index for `compact_committees_roots`, given some `epoch`. /// - /// Spec v0.8.0 + /// Spec v0.8.1 fn get_compact_committee_root_index( &self, epoch: Epoch, - spec: &ChainSpec, + allow_next_epoch: AllowNextEpoch, ) -> Result { let current_epoch = self.current_epoch(); + let len = T::EpochsPerHistoricalVector::to_u64(); - let lookahead = spec.activation_exit_delay; - let lookback = self.compact_committees_roots.len() as u64 - lookahead; - - if epoch + lookback > current_epoch && current_epoch + lookahead >= epoch { - Ok(epoch.as_usize() % self.compact_committees_roots.len()) + if current_epoch < epoch + len && epoch <= allow_next_epoch.upper_bound_of(current_epoch) { + Ok(epoch.as_usize() % len as usize) } else { Err(Error::EpochOutOfBounds) } @@ -572,26 +598,21 @@ impl BeaconState { /// Return the `compact_committee_root` at a recent `epoch`. /// - /// Spec v0.8.0 - pub fn get_compact_committee_root( - &self, - epoch: Epoch, - spec: &ChainSpec, - ) -> Result { - let i = self.get_compact_committee_root_index(epoch, spec)?; + /// Spec v0.8.1 + pub fn get_compact_committee_root(&self, epoch: Epoch) -> Result { + let i = self.get_compact_committee_root_index(epoch, AllowNextEpoch::False)?; Ok(self.compact_committees_roots[i]) } /// Set the `compact_committee_root` at a recent `epoch`. /// - /// Spec v0.8.0 + /// Spec v0.8.1 pub fn set_compact_committee_root( &mut self, epoch: Epoch, index_root: Hash256, - spec: &ChainSpec, ) -> Result<(), Error> { - let i = self.get_compact_committee_root_index(epoch, spec)?; + let i = self.get_compact_committee_root_index(epoch, AllowNextEpoch::True)?; self.compact_committees_roots[i] = index_root; Ok(()) } @@ -642,14 +663,19 @@ impl BeaconState { /// Safely obtain the index for `slashings`, given some `epoch`. /// - /// Spec v0.8.0 - fn get_slashings_index(&self, epoch: Epoch) -> Result { + /// Spec v0.8.1 + fn get_slashings_index( + &self, + epoch: Epoch, + allow_next_epoch: AllowNextEpoch, + ) -> Result { // We allow the slashings vector to be accessed at any cached epoch at or before - // the current epoch. - if epoch <= self.current_epoch() - && epoch + T::EpochsPerSlashingsVector::to_u64() >= self.current_epoch() + 1 + // the current epoch, or the next epoch if `AllowNextEpoch::True` is passed. + let current_epoch = self.current_epoch(); + if current_epoch < epoch + T::EpochsPerSlashingsVector::to_u64() + && epoch <= allow_next_epoch.upper_bound_of(current_epoch) { - Ok((epoch.as_u64() % T::EpochsPerSlashingsVector::to_u64()) as usize) + Ok(epoch.as_usize() % T::EpochsPerSlashingsVector::to_usize()) } else { Err(Error::EpochOutOfBounds) } @@ -664,17 +690,17 @@ impl BeaconState { /// Get the total slashed balances for some epoch. /// - /// Spec v0.8.0 + /// Spec v0.8.1 pub fn get_slashings(&self, epoch: Epoch) -> Result { - let i = self.get_slashings_index(epoch)?; + let i = self.get_slashings_index(epoch, AllowNextEpoch::False)?; Ok(self.slashings[i]) } /// Set the total slashed balances for some epoch. /// - /// Spec v0.8.0 + /// Spec v0.8.1 pub fn set_slashings(&mut self, epoch: Epoch, value: u64) -> Result<(), Error> { - let i = self.get_slashings_index(epoch)?; + let i = self.get_slashings_index(epoch, AllowNextEpoch::True)?; self.slashings[i] = value; Ok(()) }