From 7c134a7504d2e8ff8b8cdd7d20459f96abde04a9 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Thu, 8 Aug 2019 16:47:24 +1000 Subject: [PATCH] Simplify, fix bugs, add tests for chain iters --- beacon_node/beacon_chain/src/beacon_chain.rs | 53 ++++----------- beacon_node/beacon_chain/src/test_utils.rs | 32 +++------ beacon_node/beacon_chain/tests/tests.rs | 68 +++++++++++++++++++- beacon_node/store/src/iter.rs | 30 ++++----- eth2/lmd_ghost/src/reduced_tree.rs | 6 +- 5 files changed, 106 insertions(+), 83 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 60b65c95b..e8dcd50ab 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -244,15 +244,12 @@ impl BeaconChain { /// /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot /// returned may be earlier than the wall-clock slot. - pub fn rev_iter_block_roots( - &self, - slot: Slot, - ) -> ReverseBlockRootIterator { + pub fn rev_iter_block_roots(&self) -> ReverseBlockRootIterator { let state = &self.head().beacon_state; let block_root = self.head().beacon_block_root; let block_slot = state.slot; - let iter = BlockRootsIterator::owned(self.store.clone(), state.clone(), slot); + let iter = BlockRootsIterator::owned(self.store.clone(), state.clone()); ReverseBlockRootIterator::new((block_root, block_slot), iter) } @@ -267,15 +264,12 @@ impl BeaconChain { /// /// Because this iterator starts at the `head` of the chain (viz., the best block), the first slot /// returned may be earlier than the wall-clock slot. - pub fn rev_iter_state_roots( - &self, - slot: Slot, - ) -> ReverseStateRootIterator { + pub fn rev_iter_state_roots(&self) -> ReverseStateRootIterator { let state = &self.head().beacon_state; let state_root = self.head().beacon_state_root; let state_slot = state.slot; - let iter = StateRootsIterator::owned(self.store.clone(), state.clone(), slot); + let iter = StateRootsIterator::owned(self.store.clone(), state.clone()); ReverseStateRootIterator::new((state_root, state_slot), iter) } @@ -448,9 +442,8 @@ impl BeaconChain { pub fn produce_attestation_data(&self, shard: u64) -> Result { let state = self.state.read(); let head_block_root = self.head().beacon_block_root; - let head_block_slot = self.head().beacon_block.slot; - self.produce_attestation_data_for_block(shard, head_block_root, head_block_slot, &*state) + self.produce_attestation_data_for_block(shard, head_block_root, &*state) } /// Produce an `AttestationData` that attests to the chain denoted by `block_root` and `state`. @@ -461,39 +454,19 @@ impl BeaconChain { &self, shard: u64, head_block_root: Hash256, - head_block_slot: Slot, state: &BeaconState, ) -> Result { // Collect some metrics. self.metrics.attestation_production_requests.inc(); let timer = self.metrics.attestation_production_times.start_timer(); - let slots_per_epoch = T::EthSpec::slots_per_epoch(); - let current_epoch_start_slot = state.current_epoch().start_slot(slots_per_epoch); - // The `target_root` is the root of the first block of the current epoch. - // - // The `state` does not know the root of the block for it's current slot (it only knows - // about blocks from prior slots). This creates an edge-case when the state is on the first - // slot of the epoch -- we're unable to obtain the `target_root` because it is not a prior - // root. - // - // This edge case is handled in two ways: - // - // - If the head block is on the same slot as the state, we use it's root. - // - Otherwise, assume the current slot has been skipped and use the block root from the - // prior slot. - // - // For all other cases, we simply read the `target_root` from `state.latest_block_roots`. - let target_root = if state.slot == current_epoch_start_slot { - if head_block_slot == current_epoch_start_slot { - head_block_root - } else { - *state.get_block_root(current_epoch_start_slot - 1)? - } - } else { - *state.get_block_root(current_epoch_start_slot)? - }; + let target_root = self + .rev_iter_block_roots() + .find(|(_root, slot)| *slot % T::EthSpec::slots_per_epoch() == 0) + .map(|(root, _slot)| root) + .ok_or_else(|| Error::UnableToFindTargetRoot(self.head().beacon_state.slot))?; + let target = Checkpoint { epoch: state.current_epoch(), root: target_root, @@ -523,7 +496,7 @@ impl BeaconChain { }) } - /// Accept a new attestation from the network. + /// Accept a new, potentially invalid attestation from the network. /// /// If valid, the attestation is added to the `op_pool` and aggregated with another attestation /// if possible. @@ -614,7 +587,7 @@ impl BeaconChain { &self, attestation: Attestation, state: &BeaconState, - _head_block: &BeaconBlock, + block: &BeaconBlock, ) -> Result { self.metrics.attestation_processing_requests.inc(); let timer = self.metrics.attestation_processing_times.start_timer(); diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 293d3b9b9..f2ec5a0fd 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -194,7 +194,7 @@ where if let BlockProcessingOutcome::Processed { block_root } = outcome { head_block_root = Some(block_root); - self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot); + self.add_free_attestations(&attestation_strategy, &new_state, block_root); } else { panic!("block should be successfully processed: {:?}", outcome); } @@ -209,7 +209,7 @@ where fn get_state_at_slot(&self, state_slot: Slot) -> BeaconState { let state_root = self .chain - .rev_iter_state_roots(self.chain.head().beacon_state.slot - 1) + .rev_iter_state_roots() .find(|(_hash, slot)| *slot == state_slot) .map(|(hash, _slot)| hash) .expect("could not find state root"); @@ -282,20 +282,14 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, - head_block_slot: Slot, ) { - self.get_free_attestations( - attestation_strategy, - state, - head_block_root, - head_block_slot, - ) - .into_iter() - .for_each(|attestation| { - self.chain - .process_attestation(attestation) - .expect("should process attestation"); - }); + self.get_free_attestations(attestation_strategy, state, head_block_root) + .into_iter() + .for_each(|attestation| { + self.chain + .process_attestation(attestation) + .expect("should process attestation"); + }); } /// Generates a `Vec` for some attestation strategy and head_block. @@ -304,7 +298,6 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, - head_block_slot: Slot, ) -> Vec> { let spec = &self.spec; let fork = &state.fork; @@ -329,12 +322,7 @@ where if attesting_validators.contains(validator_index) { let data = self .chain - .produce_attestation_data_for_block( - cc.shard, - head_block_root, - head_block_slot, - state, - ) + .produce_attestation_data_for_block(cc.shard, head_block_root, state) .expect("should produce attestation data"); let mut aggregation_bits = BitList::with_capacity(committee_size).unwrap(); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index d286aaec0..8dc4ae6ec 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -32,6 +32,73 @@ fn get_harness(validator_count: usize) -> BeaconChainHarness = harness.chain.rev_iter_block_roots().collect(); + let state_roots: Vec<(Hash256, Slot)> = harness.chain.rev_iter_state_roots().collect(); + + assert_eq!( + block_roots.len(), + state_roots.len(), + "should be an equal amount of block and state roots" + ); + + assert!( + block_roots.iter().any(|(_root, slot)| *slot == 0), + "should contain genesis block root" + ); + assert!( + state_roots.iter().any(|(_root, slot)| *slot == 0), + "should contain genesis state root" + ); + + assert_eq!( + block_roots.len(), + num_blocks_produced as usize + 1, + "should contain all produced blocks, plus the genesis block" + ); + + block_roots.windows(2).for_each(|x| { + assert_eq!( + x[1].1, + x[0].1 - 1, + "block root slots should be decreasing by one" + ) + }); + state_roots.windows(2).for_each(|x| { + assert_eq!( + x[1].1, + x[0].1 - 1, + "state root slots should be decreasing by one" + ) + }); + + let head = &harness.chain.head(); + + assert_eq!( + *block_roots.first().expect("should have some block roots"), + (head.beacon_block_root, head.beacon_block.slot), + "first block root and slot should be for the head block" + ); + + assert_eq!( + *state_roots.first().expect("should have some state roots"), + (head.beacon_state_root, head.beacon_state.slot), + "first state root and slot should be for the head state" + ); +} + #[test] fn chooses_fork() { let harness = get_harness(VALIDATOR_COUNT); @@ -326,7 +393,6 @@ fn attestations_with_increasing_slots() { &AttestationStrategy::AllValidators, &harness.chain.head().beacon_state, harness.chain.head().beacon_block_root, - harness.chain.head().beacon_block.slot, )); harness.advance_slot(); diff --git a/beacon_node/store/src/iter.rs b/beacon_node/store/src/iter.rs index c4e557b2d..84bf3759f 100644 --- a/beacon_node/store/src/iter.rs +++ b/beacon_node/store/src/iter.rs @@ -20,7 +20,7 @@ impl<'a, U: Store, E: EthSpec> AncestorIter> for fn try_iter_ancestor_roots(&self, store: Arc) -> Option> { let state = store.get::>(&self.state_root).ok()??; - Some(BlockRootsIterator::owned(store, state, self.slot)) + Some(BlockRootsIterator::owned(store, state)) } } @@ -32,19 +32,19 @@ pub struct StateRootsIterator<'a, T: EthSpec, U> { } impl<'a, T: EthSpec, U: Store> StateRootsIterator<'a, T, U> { - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { + pub fn new(store: Arc, beacon_state: &'a BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Borrowed(beacon_state), - slot: start_slot + 1, } } - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { + pub fn owned(store: Arc, beacon_state: BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Owned(beacon_state), - slot: start_slot + 1, } } } @@ -88,16 +88,16 @@ pub struct BlockIterator<'a, T: EthSpec, U> { impl<'a, T: EthSpec, U: Store> BlockIterator<'a, T, U> { /// Create a new iterator over all blocks in the given `beacon_state` and prior states. - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { + pub fn new(store: Arc, beacon_state: &'a BeaconState) -> Self { Self { - roots: BlockRootsIterator::new(store, beacon_state, start_slot), + roots: BlockRootsIterator::new(store, beacon_state), } } /// Create a new iterator over all blocks in the given `beacon_state` and prior states. - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { + pub fn owned(store: Arc, beacon_state: BeaconState) -> Self { Self { - roots: BlockRootsIterator::owned(store, beacon_state, start_slot), + roots: BlockRootsIterator::owned(store, beacon_state), } } } @@ -128,20 +128,20 @@ pub struct BlockRootsIterator<'a, T: EthSpec, U> { impl<'a, T: EthSpec, U: Store> BlockRootsIterator<'a, T, U> { /// Create a new iterator over all block roots in the given `beacon_state` and prior states. - pub fn new(store: Arc, beacon_state: &'a BeaconState, start_slot: Slot) -> Self { + pub fn new(store: Arc, beacon_state: &'a BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Borrowed(beacon_state), - slot: start_slot + 1, } } /// Create a new iterator over all block roots in the given `beacon_state` and prior states. - pub fn owned(store: Arc, beacon_state: BeaconState, start_slot: Slot) -> Self { + pub fn owned(store: Arc, beacon_state: BeaconState) -> Self { Self { store, + slot: beacon_state.slot, beacon_state: Cow::Owned(beacon_state), - slot: start_slot + 1, } } } @@ -218,7 +218,7 @@ mod test { state_b.state_roots[0] = state_a_root; store.put(&state_a_root, &state_a).unwrap(); - let iter = BlockRootsIterator::new(store.clone(), &state_b, state_b.slot - 1); + let iter = BlockRootsIterator::new(store.clone(), &state_b); assert!( iter.clone().find(|(_root, slot)| *slot == 0).is_some(), @@ -267,7 +267,7 @@ mod test { store.put(&state_a_root, &state_a).unwrap(); store.put(&state_b_root, &state_b).unwrap(); - let iter = StateRootsIterator::new(store.clone(), &state_b, state_b.slot - 1); + let iter = StateRootsIterator::new(store.clone(), &state_b); assert!( iter.clone().find(|(_root, slot)| *slot == 0).is_some(), diff --git a/eth2/lmd_ghost/src/reduced_tree.rs b/eth2/lmd_ghost/src/reduced_tree.rs index 5d7074804..9668620b7 100644 --- a/eth2/lmd_ghost/src/reduced_tree.rs +++ b/eth2/lmd_ghost/src/reduced_tree.rs @@ -611,11 +611,7 @@ where let block = self.get_block(child)?; let state = self.get_state(block.state_root)?; - Ok(BlockRootsIterator::owned( - self.store.clone(), - state, - block.slot - 1, - )) + Ok(BlockRootsIterator::owned(self.store.clone(), state)) } /// Verify the integrity of `self`. Returns `Ok(())` if the tree has integrity, otherwise returns `Err(description)`.