diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3b66e9d142b..7c2c056adb5 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -50,8 +50,8 @@ use crate::{metrics, BeaconChainError}; use eth2::types::{ EventKind, SseBlock, SseChainReorg, SseFinalizedCheckpoint, SseHead, SseLateHead, SyncDuty, }; +use fork_choice::{AttestationFromBlock, ForkChoice}; use execution_layer::ExecutionLayer; -use fork_choice::ForkChoice; use futures::channel::mpsc::Sender; use itertools::process_results; use itertools::Itertools; @@ -1698,7 +1698,11 @@ impl BeaconChain { self.fork_choice .write() - .on_attestation(self.slot()?, verified.indexed_attestation()) + .on_attestation( + self.slot()?, + verified.indexed_attestation(), + AttestationFromBlock::False, + ) .map_err(Into::into) } @@ -2470,7 +2474,11 @@ impl BeaconChain { let indexed_attestation = get_indexed_attestation(committee.committee, attestation) .map_err(|e| BlockError::BeaconChainError(e.into()))?; - match fork_choice.on_attestation(current_slot, &indexed_attestation) { + match fork_choice.on_attestation( + current_slot, + &indexed_attestation, + AttestationFromBlock::True, + ) { Ok(()) => Ok(()), // Ignore invalid attestations whilst importing attestations from a block. The // block might be very old and therefore the attestations useless to fork choice. diff --git a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs index e8513c63881..26859631cbe 100644 --- a/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs +++ b/beacon_node/beacon_chain/src/beacon_fork_choice_store.rs @@ -344,8 +344,8 @@ where pub struct PersistedForkChoiceStore { balances_cache: BalancesCache, time: Slot, - finalized_checkpoint: Checkpoint, - justified_checkpoint: Checkpoint, + pub finalized_checkpoint: Checkpoint, + pub justified_checkpoint: Checkpoint, justified_balances: Vec, best_justified_checkpoint: Checkpoint, } diff --git a/beacon_node/beacon_chain/src/schema_change.rs b/beacon_node/beacon_chain/src/schema_change.rs index d08dd3d6005..5e30e8aae44 100644 --- a/beacon_node/beacon_chain/src/schema_change.rs +++ b/beacon_node/beacon_chain/src/schema_change.rs @@ -119,6 +119,11 @@ pub fn migrate_schema( .map_err(StoreError::SchemaMigrationError)?; }; + migrate_schema_6::update_store_justified_checkpoint::( + &mut persisted_fork_choice, + ) + .map_err(StoreError::SchemaMigrationError)?; + // Store the converted fork choice store under the same key. db.put_item::(&FORK_CHOICE_DB_KEY, &persisted_fork_choice)?; } diff --git a/beacon_node/beacon_chain/src/schema_change/migrate_schema_6.rs b/beacon_node/beacon_chain/src/schema_change/migrate_schema_6.rs index 437060b8db9..021103727d4 100644 --- a/beacon_node/beacon_chain/src/schema_change/migrate_schema_6.rs +++ b/beacon_node/beacon_chain/src/schema_change/migrate_schema_6.rs @@ -21,6 +21,31 @@ use store::Error as StoreError; // selector. four_byte_option_impl!(four_byte_option_usize, usize); +pub(crate) fn update_store_justified_checkpoint( + persisted_fork_choice: &mut PersistedForkChoice, +) -> Result<(), String> { + let bytes = persisted_fork_choice.fork_choice.proto_array_bytes.clone(); + let container = SszContainer::from_ssz_bytes(bytes.as_slice()).unwrap(); + let mut fork_choice: ProtoArrayForkChoice = container.into(); + + let justified_checkpoint = fork_choice + .core_proto_array() + .nodes + .iter() + .find_map(|node| { + (node.finalized_checkpoint + == Some(persisted_fork_choice.fork_choice_store.finalized_checkpoint)) + .then(|| node.justified_checkpoint) + .flatten() + }) + .ok_or("Proto node with current finalized checkpoint not found")?; + + fork_choice.core_proto_array_mut().justified_checkpoint = justified_checkpoint; + persisted_fork_choice.fork_choice.proto_array_bytes = fork_choice.as_bytes(); + persisted_fork_choice.fork_choice_store.justified_checkpoint = justified_checkpoint; + Ok(()) +} + pub(crate) fn update_with_reinitialized_fork_choice( persisted_fork_choice: &mut PersistedForkChoice, db: Arc>, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index edd30981be2..a4db518dd7e 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -218,6 +218,15 @@ fn dequeue_attestations( std::mem::replace(queued_attestations, remaining) } +/// Denotes whether an attestation we are processing received from a block or from gossip. +/// Equivalent to the `is_from_block` bool in: +/// +/// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#validate_on_attestation +pub enum AttestationFromBlock { + True, + False, +} + /// Provides an implementation of "Ethereum 2.0 Phase 0 -- Beacon Chain Fork Choice": /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#ethereum-20-phase-0----beacon-chain-fork-choice @@ -537,25 +546,9 @@ where if state.finalized_checkpoint().epoch > self.fc_store.finalized_checkpoint().epoch { self.fc_store .set_finalized_checkpoint(state.finalized_checkpoint()); - let finalized_slot = - compute_start_slot_at_epoch::(self.fc_store.finalized_checkpoint().epoch); - - // Note: the `if` statement here is not part of the specification, but I claim that it - // is an optimization and equivalent to the specification. See this PR for more - // information: - // - // https://github.com/ethereum/eth2.0-specs/pull/1880 - if *self.fc_store.justified_checkpoint() != state.current_justified_checkpoint() - && (state.current_justified_checkpoint().epoch - > self.fc_store.justified_checkpoint().epoch - || self - .get_ancestor(self.fc_store.justified_checkpoint().root, finalized_slot)? - != Some(self.fc_store.finalized_checkpoint().root)) - { - self.fc_store - .set_justified_checkpoint(state.current_justified_checkpoint()) - .map_err(Error::UnableToSetJustifiedCheckpoint)?; - } + self.fc_store + .set_justified_checkpoint(state.current_justified_checkpoint()) + .map_err(Error::UnableToSetJustifiedCheckpoint)?; } let target_slot = block @@ -629,6 +622,35 @@ where Ok(()) } + /// Validates the `epoch` against the current time according to the fork choice store. + /// + /// ## Specification + /// + /// Equivalent to: + /// + /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/fork-choice.md#validate_target_epoch_against_current_time + fn validate_target_epoch_against_current_time( + &self, + target_epoch: Epoch, + ) -> Result<(), InvalidAttestation> { + let slot_now = self.fc_store.get_current_slot(); + let epoch_now = slot_now.epoch(E::slots_per_epoch()); + + // Attestation must be from the current or previous epoch. + if target_epoch > epoch_now { + return Err(InvalidAttestation::FutureEpoch { + attestation_epoch: target_epoch, + current_epoch: epoch_now, + }); + } else if target_epoch + 1 < epoch_now { + return Err(InvalidAttestation::PastEpoch { + attestation_epoch: target_epoch, + current_epoch: epoch_now, + }); + } + Ok(()) + } + /// Validates the `indexed_attestation` for application to fork choice. /// /// ## Specification @@ -639,6 +661,7 @@ where fn validate_on_attestation( &self, indexed_attestation: &IndexedAttestation, + is_from_block: AttestationFromBlock, ) -> Result<(), InvalidAttestation> { // There is no point in processing an attestation with an empty bitfield. Reject // it immediately. @@ -649,21 +672,10 @@ where return Err(InvalidAttestation::EmptyAggregationBitfield); } - let slot_now = self.fc_store.get_current_slot(); - let epoch_now = slot_now.epoch(E::slots_per_epoch()); let target = indexed_attestation.data.target; - // Attestation must be from the current or previous epoch. - if target.epoch > epoch_now { - return Err(InvalidAttestation::FutureEpoch { - attestation_epoch: target.epoch, - current_epoch: epoch_now, - }); - } else if target.epoch + 1 < epoch_now { - return Err(InvalidAttestation::PastEpoch { - attestation_epoch: target.epoch, - current_epoch: epoch_now, - }); + if matches!(is_from_block, AttestationFromBlock::False) { + self.validate_target_epoch_against_current_time(target.epoch)?; } if target.epoch != indexed_attestation.data.slot.epoch(E::slots_per_epoch()) { @@ -746,6 +758,7 @@ where &mut self, current_slot: Slot, attestation: &IndexedAttestation, + is_from_block: AttestationFromBlock, ) -> Result<(), Error> { // Ensure the store is up-to-date. self.update_time(current_slot)?; @@ -767,7 +780,7 @@ where return Ok(()); } - self.validate_on_attestation(attestation)?; + self.validate_on_attestation(attestation, is_from_block)?; if attestation.data.slot < self.fc_store.get_current_slot() { for validator_index in attestation.attesting_indices.iter() { diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 7dd80b7982c..ba031cdf7fe 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -2,8 +2,8 @@ mod fork_choice; mod fork_choice_store; pub use crate::fork_choice::{ - Error, ForkChoice, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, - PersistedForkChoice, QueuedAttestation, + AttestationFromBlock, Error, ForkChoice, InvalidAttestation, InvalidBlock, + PayloadVerificationStatus, PersistedForkChoice, QueuedAttestation, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::Block as ProtoBlock;