From 04bef689e33c0f3f4288a96d90bd06f91e0eacea Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Sat, 10 Aug 2019 17:47:34 +1000 Subject: [PATCH] Fix attestation prod. target roots change --- beacon_node/beacon_chain/src/beacon_chain.rs | 34 ++++++++++++++++---- beacon_node/beacon_chain/src/test_utils.rs | 30 +++++++++++------ beacon_node/beacon_chain/tests/tests.rs | 1 + 3 files changed, 50 insertions(+), 15 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c92a05a728..7488a7795e 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -450,8 +450,9 @@ 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, &*state) + self.produce_attestation_data_for_block(shard, head_block_root, head_block_slot, &*state) } /// Produce an `AttestationData` that attests to the chain denoted by `block_root` and `state`. @@ -462,18 +463,39 @@ 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. - 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))?; + // + // 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 = Checkpoint { epoch: state.current_epoch(), diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index 6997f52aec..298c637dbd 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); + self.add_free_attestations(&attestation_strategy, &new_state, block_root, slot); } else { panic!("block should be successfully processed: {:?}", outcome); } @@ -282,14 +282,20 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, + head_block_slot: Slot, ) { - self.get_free_attestations(attestation_strategy, state, head_block_root) - .into_iter() - .for_each(|attestation| { - self.chain - .process_attestation(attestation) - .expect("should process attestation"); - }); + 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"); + }); } /// Generates a `Vec` for some attestation strategy and head_block. @@ -298,6 +304,7 @@ where attestation_strategy: &AttestationStrategy, state: &BeaconState, head_block_root: Hash256, + head_block_slot: Slot, ) -> Vec> { let spec = &self.spec; let fork = &state.fork; @@ -322,7 +329,12 @@ where if attesting_validators.contains(validator_index) { let data = self .chain - .produce_attestation_data_for_block(cc.shard, head_block_root, state) + .produce_attestation_data_for_block( + cc.shard, + head_block_root, + head_block_slot, + 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 c22f025639..22b667f159 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -395,6 +395,7 @@ 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();