From 8677b9e9cc61fd6792fb2ae4f18ee7be92b4d9da Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sun, 17 Mar 2019 21:07:19 +1100 Subject: [PATCH] Fix bug with epoch caches, add tests --- .../src/per_block_processing.rs | 6 +- .../verify_slashable_attestation.rs | 2 +- .../per_block_processing/verify_transfer.rs | 2 +- eth2/types/src/beacon_state.rs | 4 +- eth2/types/src/beacon_state/epoch_cache.rs | 2 +- eth2/types/src/beacon_state/tests.rs | 54 ++++++++++++++++- eth2/types/src/relative_epoch.rs | 58 +++++++++++++++++++ 7 files changed, 120 insertions(+), 8 deletions(-) diff --git a/eth2/state_processing/src/per_block_processing.rs b/eth2/state_processing/src/per_block_processing.rs index c6b22fa75..78cf927f5 100644 --- a/eth2/state_processing/src/per_block_processing.rs +++ b/eth2/state_processing/src/per_block_processing.rs @@ -32,7 +32,7 @@ const VERIFY_DEPOSIT_MERKLE_PROOFS: bool = false; /// Returns `Ok(())` if the block is valid and the state was successfully updated. Otherwise /// returns an error describing why the block was invalid or how the function failed to execute. /// -/// Spec v0.4.0 +/// Spec v0.5.0 pub fn per_block_processing( state: &mut BeaconState, block: &BeaconBlock, @@ -47,7 +47,7 @@ pub fn per_block_processing( /// Returns `Ok(())` if the block is valid and the state was successfully updated. Otherwise /// returns an error describing why the block was invalid or how the function failed to execute. /// -/// Spec v0.4.0 +/// Spec v0.5.0 pub fn per_block_processing_without_verifying_block_signature( state: &mut BeaconState, block: &BeaconBlock, @@ -62,7 +62,7 @@ pub fn per_block_processing_without_verifying_block_signature( /// Returns `Ok(())` if the block is valid and the state was successfully updated. Otherwise /// returns an error describing why the block was invalid or how the function failed to execute. /// -/// Spec v0.4.0 +/// Spec v0.5.0 fn per_block_processing_signature_optional( mut state: &mut BeaconState, block: &BeaconBlock, diff --git a/eth2/state_processing/src/per_block_processing/verify_slashable_attestation.rs b/eth2/state_processing/src/per_block_processing/verify_slashable_attestation.rs index f0d371043..aa9a32196 100644 --- a/eth2/state_processing/src/per_block_processing/verify_slashable_attestation.rs +++ b/eth2/state_processing/src/per_block_processing/verify_slashable_attestation.rs @@ -10,7 +10,7 @@ use types::*; /// /// Returns `Ok(())` if the `SlashableAttestation` is valid, otherwise indicates the reason for invalidity. /// -/// Spec v0.4.0 +/// Spec v0.5.0 pub fn verify_slashable_attestation( state: &BeaconState, slashable_attestation: &SlashableAttestation, diff --git a/eth2/state_processing/src/per_block_processing/verify_transfer.rs b/eth2/state_processing/src/per_block_processing/verify_transfer.rs index 546760fd0..f873cd850 100644 --- a/eth2/state_processing/src/per_block_processing/verify_transfer.rs +++ b/eth2/state_processing/src/per_block_processing/verify_transfer.rs @@ -94,7 +94,7 @@ pub fn verify_transfer( /// /// Does not check that the transfer is valid, however checks for overflow in all actions. /// -/// Spec v0.4.0 +/// Spec v0.5.0 pub fn execute_transfer( state: &mut BeaconState, transfer: &Transfer, diff --git a/eth2/types/src/beacon_state.rs b/eth2/types/src/beacon_state.rs index a90f09759..8999d8be8 100644 --- a/eth2/types/src/beacon_state.rs +++ b/eth2/types/src/beacon_state.rs @@ -279,7 +279,9 @@ impl BeaconState { fn cache(&self, relative_epoch: RelativeEpoch, spec: &ChainSpec) -> Result<&EpochCache, Error> { let cache = &self.caches[self.cache_index(relative_epoch)]; - if cache.initialized_epoch == Some(self.slot.epoch(spec.slots_per_epoch)) { + let epoch = relative_epoch.into_epoch(self.slot.epoch(spec.slots_per_epoch)); + + if cache.initialized_epoch == Some(epoch) { Ok(cache) } else { Err(Error::EpochCacheUninitialized(relative_epoch)) diff --git a/eth2/types/src/beacon_state/epoch_cache.rs b/eth2/types/src/beacon_state/epoch_cache.rs index 0759a7617..4436972f1 100644 --- a/eth2/types/src/beacon_state/epoch_cache.rs +++ b/eth2/types/src/beacon_state/epoch_cache.rs @@ -159,7 +159,7 @@ impl EpochCrosslinkCommittees { let epoch_start_slot = self.epoch.start_slot(spec.slots_per_epoch); let epoch_end_slot = self.epoch.end_slot(spec.slots_per_epoch); - if (epoch_start_slot < slot) && (slot <= epoch_end_slot) { + if (epoch_start_slot <= slot) && (slot <= epoch_end_slot) { let index = slot - epoch_start_slot; self.crosslink_committees.get(index.as_usize()) } else { diff --git a/eth2/types/src/beacon_state/tests.rs b/eth2/types/src/beacon_state/tests.rs index 6c10ebe86..dc16a013b 100644 --- a/eth2/types/src/beacon_state/tests.rs +++ b/eth2/types/src/beacon_state/tests.rs @@ -1,5 +1,57 @@ #![cfg(test)] - use super::*; +use crate::test_utils::*; ssz_tests!(BeaconState); + +/// Test that +/// +/// 1. Using the cache before it's built fails. +/// 2. Using the cache after it's build passes. +/// 3. Using the cache after it's dropped fails. +fn test_cache_initialization<'a>( + state: &'a mut BeaconState, + relative_epoch: RelativeEpoch, + spec: &ChainSpec, +) { + let slot = relative_epoch + .into_epoch(state.slot.epoch(spec.slots_per_epoch)) + .start_slot(spec.slots_per_epoch); + + // Assuming the cache isn't already built, assert that a call to a cache-using function fails. + assert_eq!( + state.get_beacon_proposer_index(slot, relative_epoch, spec), + Err(BeaconStateError::EpochCacheUninitialized(relative_epoch)) + ); + + // Build the cache. + state.build_epoch_cache(relative_epoch, spec).unwrap(); + + // Assert a call to a cache-using function passes. + let _ = state + .get_beacon_proposer_index(slot, relative_epoch, spec) + .unwrap(); + + // Drop the cache. + state.drop_cache(relative_epoch); + + // Assert a call to a cache-using function fail. + assert_eq!( + state.get_beacon_proposer_index(slot, relative_epoch, spec), + Err(BeaconStateError::EpochCacheUninitialized(relative_epoch)) + ); +} + +#[test] +fn cache_initialization() { + let spec = ChainSpec::few_validators(); + let (mut state, _keypairs) = + TestingBeaconStateBuilder::from_default_keypairs_file_if_exists(16, &spec).build(); + + state.slot = (spec.genesis_epoch + 1).start_slot(spec.slots_per_epoch); + + test_cache_initialization(&mut state, RelativeEpoch::Previous, &spec); + test_cache_initialization(&mut state, RelativeEpoch::Current, &spec); + test_cache_initialization(&mut state, RelativeEpoch::NextWithRegistryChange, &spec); + test_cache_initialization(&mut state, RelativeEpoch::NextWithoutRegistryChange, &spec); +} diff --git a/eth2/types/src/relative_epoch.rs b/eth2/types/src/relative_epoch.rs index 943936605..6c135b1a6 100644 --- a/eth2/types/src/relative_epoch.rs +++ b/eth2/types/src/relative_epoch.rs @@ -74,3 +74,61 @@ impl RelativeEpoch { ) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn test_into_epoch() { + let base = Epoch::new(10); + + assert_eq!(RelativeEpoch::Current.into_epoch(base), base); + assert_eq!(RelativeEpoch::Previous.into_epoch(base), base - 1); + assert_eq!( + RelativeEpoch::NextWithRegistryChange.into_epoch(base), + base + 1 + ); + assert_eq!( + RelativeEpoch::NextWithoutRegistryChange.into_epoch(base), + base + 1 + ); + } + + #[test] + fn from_epoch() { + let base = Epoch::new(10); + + assert_eq!( + RelativeEpoch::from_epoch(base, base - 1), + Ok(RelativeEpoch::Previous) + ); + assert_eq!( + RelativeEpoch::from_epoch(base, base), + Ok(RelativeEpoch::Current) + ); + assert_eq!( + RelativeEpoch::from_epoch(base, base + 1), + Err(RelativeEpochError::AmbiguiousNextEpoch) + ); + } + + #[test] + fn from_slot() { + let spec = ChainSpec::foundation(); + let base = Epoch::new(10).start_slot(spec.slots_per_epoch); + + assert_eq!( + RelativeEpoch::from_slot(base, base - 1, &spec), + Ok(RelativeEpoch::Previous) + ); + assert_eq!( + RelativeEpoch::from_slot(base, base, &spec), + Ok(RelativeEpoch::Current) + ); + assert_eq!( + RelativeEpoch::from_slot(base, base + spec.slots_per_epoch, &spec), + Err(RelativeEpochError::AmbiguiousNextEpoch) + ); + } +}