From 76bb6710844d4f683d1681ef738efe9e5880b137 Mon Sep 17 00:00:00 2001 From: Paul Hauner Date: Fri, 9 Aug 2019 11:54:35 +1000 Subject: [PATCH] Fix bug with fork choice, tidy --- beacon_node/beacon_chain/src/beacon_chain.rs | 63 +++++++++---------- beacon_node/beacon_chain/src/fork_choice.rs | 14 ++++- beacon_node/beacon_chain/src/test_utils.rs | 6 +- beacon_node/beacon_chain/tests/tests.rs | 22 ++++--- .../src/per_block_processing/errors.rs | 3 + .../verify_attestation.rs | 11 ++++ 6 files changed, 70 insertions(+), 49 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3d50d701c..81e5bdd65 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -524,7 +524,13 @@ impl BeaconChain { // If it turns out that the attestation was made using the head state, then there // is no need to load a state from the database to process the attestation. - if state.current_epoch() == attestation_head_block.epoch() + // + // Note: use the epoch of the target because it indicates which epoch the + // attestation was created in. You cannot use the epoch of the head block, because + // the block doesn't necessarily need to be in the same epoch as the attestation + // (e.g., if there are skip slots between the epoch the block was created in and + // the epoch for the attestation). + if state.current_epoch() == attestation.data.target.epoch && (state .get_block_root(attestation_head_block.slot) .map(|root| *root == attestation.data.beacon_block_root) @@ -546,7 +552,11 @@ impl BeaconChain { if let Some(outcome) = optional_outcome { outcome } else { - // The state required to verify this attestation must be loaded from the database. + // Use the `data.beacon_block_root` to load the state from the latest non-skipped + // slot preceding the attestations creation. + // + // This state is guaranteed to be in the same chain as the attestation, but it's + // not guaranteed to be from the same slot or epoch as the attestation. let mut state: BeaconState = self .store .get(&attestation_head_block.state_root)? @@ -554,7 +564,21 @@ impl BeaconChain { // Ensure the state loaded from the database matches the state of the attestation // head block. - for _ in state.slot.as_u64()..attestation_head_block.slot.as_u64() { + // + // The state needs to be advanced from the current slot through to the epoch in + // which the attestation was created in. It would be an error to try and use + // `state.get_attestation_data_slot(..)` because the state matching the + // `data.beacon_block_root` isn't necessarily in a nearby epoch to the attestation + // (e.g., if there were lots of skip slots since the head of the chain and the + // epoch creation epoch). + for _ in state.slot.as_u64() + ..attestation + .data + .target + .epoch + .start_slot(T::EthSpec::slots_per_epoch()) + .as_u64() + { per_slot_processing(&mut state, &self.spec)?; } @@ -639,36 +663,6 @@ impl BeaconChain { timer.observe_duration(); result - - /* - if self - .fork_choice - .should_process_attestation(state, &attestation)? - { - // TODO: check validation. - let indexed_attestation = common::get_indexed_attestation(state, &attestation)?; - per_block_processing::is_valid_indexed_attestation( - state, - &indexed_attestation, - &self.spec, - )?; - self.fork_choice.process_attestation(&state, &attestation)?; - } - - let result = self - .op_pool - .insert_attestation(attestation, state, &self.spec); - - timer.observe_duration(); - - if result.is_ok() { - self.metrics.attestation_processing_successes.inc(); - } - - result - .map(|_| AttestationProcessingOutcome::Processed) - .map_err(|e| Error::AttestationValidationError(e)) - */ } /// Accept some deposit and queue it for inclusion in an appropriate block. @@ -735,7 +729,7 @@ impl BeaconChain { return Ok(BlockProcessingOutcome::GenesisBlock); } - let block_root = block.block_header().canonical_root(); + let block_root = block.canonical_root(); if block_root == self.genesis_block_root { return Ok(BlockProcessingOutcome::GenesisBlock); @@ -781,6 +775,7 @@ impl BeaconChain { per_slot_processing(&mut state, &self.spec)?; } + state.build_committee_cache(RelativeEpoch::Previous, &self.spec)?; state.build_committee_cache(RelativeEpoch::Current, &self.spec)?; // Apply the received block to its parent state (which has been transitioned into this diff --git a/beacon_node/beacon_chain/src/fork_choice.rs b/beacon_node/beacon_chain/src/fork_choice.rs index 83d6c335f..6800f61d8 100644 --- a/beacon_node/beacon_chain/src/fork_choice.rs +++ b/beacon_node/beacon_chain/src/fork_choice.rs @@ -19,6 +19,7 @@ pub enum Error { } pub struct ForkChoice { + store: Arc, backend: T::LmdGhost, /// Used for resolving the `0x00..00` alias back to genesis. /// @@ -38,6 +39,7 @@ impl ForkChoice { genesis_block_root: Hash256, ) -> Self { Self { + store: store.clone(), backend: T::LmdGhost::new(store, genesis_block, genesis_block_root), genesis_block_root, } @@ -117,9 +119,19 @@ impl ForkChoice { // // https://github.com/ethereum/eth2.0-specs/blob/v0.7.0/specs/core/0_fork-choice.md for attestation in &block.body.attestations { - self.process_attestation(state, attestation, block)?; + let block = self + .store + .get::>(&attestation.data.beacon_block_root)? + .ok_or_else(|| Error::MissingBlock(attestation.data.beacon_block_root))?; + + self.process_attestation(state, attestation, &block)?; } + // This does not apply a vote to the block, it just makes fork choice aware of the block so + // it can still be identified as the head even if it doesn't have any votes. + // + // A case where a block without any votes can be the head is where it is the only child of + // a block that has the majority of votes applied to it. self.backend.process_block(block, block_root)?; Ok(()) diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index f2ec5a0fd..6997f52ae 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -349,14 +349,12 @@ where agg_sig }; - let attestation = Attestation { + vec.push(Attestation { aggregation_bits, data, custody_bits, signature, - }; - - vec.push(attestation) + }) } } }); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index 8dc4ae6ec..c22f02563 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -342,27 +342,29 @@ fn free_attestations_added_to_fork_choice_some_none() { let state = &harness.chain.head().beacon_state; let fork_choice = &harness.chain.fork_choice; - let validators: Vec = (0..VALIDATOR_COUNT).collect(); - let slots: Vec = validators - .iter() - .map(|&v| { - state - .get_attestation_duties(v, RelativeEpoch::Current) + let validator_slots: Vec<(usize, Slot)> = (0..VALIDATOR_COUNT) + .into_iter() + .map(|validator_index| { + let slot = state + .get_attestation_duties(validator_index, RelativeEpoch::Current) .expect("should get attester duties") .unwrap() - .slot + .slot; + + (validator_index, slot) }) .collect(); - let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); for (validator, slot) in validator_slots.clone() { - let latest_message = fork_choice.latest_message(*validator); + let latest_message = fork_choice.latest_message(validator); if slot <= num_blocks_produced && slot != 0 { assert_eq!( latest_message.unwrap().1, slot, - "Latest message slot should be equal to attester duty." + "Latest message slot for {} should be equal to slot {}.", + validator, + slot ) } else { assert!( diff --git a/eth2/state_processing/src/per_block_processing/errors.rs b/eth2/state_processing/src/per_block_processing/errors.rs index 65179167c..436ec96ce 100644 --- a/eth2/state_processing/src/per_block_processing/errors.rs +++ b/eth2/state_processing/src/per_block_processing/errors.rs @@ -136,6 +136,9 @@ pub enum AttestationInvalid { delay: u64, attestation: Slot, }, + /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the + /// future). + AttestsToFutureState { state: Slot, attestation: Slot }, /// Attestation slot is too far in the past to be included in a block. IncludedTooLate { state: Slot, attestation: Slot }, /// Attestation target epoch does not match the current or previous epoch. diff --git a/eth2/state_processing/src/per_block_processing/verify_attestation.rs b/eth2/state_processing/src/per_block_processing/verify_attestation.rs index 74dbefa23..127d251de 100644 --- a/eth2/state_processing/src/per_block_processing/verify_attestation.rs +++ b/eth2/state_processing/src/per_block_processing/verify_attestation.rs @@ -62,6 +62,17 @@ pub fn verify_attestation_for_state( Invalid::BadShard ); + let attestation_slot = state.get_attestation_data_slot(&data)?; + + // An attestation cannot attest to a state that is later than itself. + verify!( + attestation_slot <= state.slot, + Invalid::AttestsToFutureState { + state: state.slot, + attestation: attestation_slot + } + ); + // Verify the Casper FFG vote and crosslink data. let parent_crosslink = verify_casper_ffg_vote(attestation, state)?;