All tests bar invalid_message passing, changes might be dubious

beacon_node/beacon_chain/src/block_verification.rs

@@ -48,6 +48,9 @@
 // returned alongside.
 #![allow(clippy::result_large_err)]
 
+use crate::attestation_verification::{
+    Error as AttestationVerificationError, obtain_indexed_attestation_and_committees_per_slot,
+};
 use crate::beacon_snapshot::PreProcessingSnapshot;
 use crate::blob_verification::GossipBlobError;
 use crate::block_verification_types::{AsBlock, BlockImportData, LookupBlock, RangeSyncBlock};
@@ -1648,6 +1651,18 @@ impl<T: BeaconChainTypes> ExecutionPendingBlock<T> {
          * free real estate.
          */
         let current_slot = chain.slot()?;
+        let mut indexed_attestations = vec![];
+        for attestation in block.message().body().attestations() {
+            match obtain_indexed_attestation_and_committees_per_slot(chain, attestation) {
+                Ok((indexed_attestation, _)) => indexed_attestations.push(indexed_attestation),
+                Err(AttestationVerificationError::BeaconChainError(e)) => {
+                    return Err(BlockError::BeaconChainError(e));
+                }
+                // Ignore invalid attestations whilst importing attestations from a block. The
+                // block might be very old and therefore the attestations useless to fork choice.
+                Err(_) => {}
+            }
+        }
         let mut fork_choice = chain.canonical_head.fork_choice_write_lock();
 
         // Register each attester slashing in the block with fork choice.
@@ -1656,14 +1671,10 @@ impl<T: BeaconChainTypes> ExecutionPendingBlock<T> {
         }
 
         // Register each attestation in the block with fork choice.
-        for (i, attestation) in block.message().body().attestations().enumerate() {
+        for indexed_attestation in indexed_attestations {
-            let indexed_attestation = consensus_context
-                .get_indexed_attestation(&state, attestation)
-                .map_err(|e| BlockError::PerBlockProcessingError(e.into_with_index(i)))?;
-
             match fork_choice.on_attestation(
                 current_slot,
-                indexed_attestation,
+                indexed_attestation.to_ref(),
                 AttestationFromBlock::True,
                 &chain.spec,
             ) {

beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs

@@ -8,12 +8,10 @@ use bls::AggregateSignature;
 use educe::Educe;
 use eth2::types::{EventKind, ForkVersionedResponse};
 use parking_lot::RwLock;
-use safe_arith::SafeArith;
 use slot_clock::SlotClock;
 use state_processing::per_block_processing::signature_sets::indexed_payload_attestation_signature_set;
-use state_processing::state_advance::partial_state_advance;
 use std::borrow::Cow;
-use types::{ChainSpec, EthSpec, IndexedPayloadAttestation, PTC, PayloadAttestationMessage, Slot};
+use types::{ChainSpec, IndexedPayloadAttestation, PTC, PayloadAttestationMessage, Slot};
 
 pub struct GossipVerificationContext<'a, T: BeaconChainTypes> {
     pub slot_clock: &'a T::SlotClock,
@@ -67,62 +65,26 @@ impl<T: BeaconChainTypes> VerifiedPayloadAttestationMessage<T> {
         // 2. Blocks we've seen that are invalid (REJECT).
         // Presently both cases return IGNORE.
         let beacon_block_root = payload_attestation_message.data.beacon_block_root;
-        if ctx
+        let Some(block) = ctx
             .canonical_head
             .fork_choice_read_lock()
             .get_block(&beacon_block_root)
-            .is_none()
+        else {
-        {
             return Err(Error::UnknownHeadBlock { beacon_block_root });
-        }
-
-        // Get head state for PTC computation. If the cached head state is too stale
-        // (e.g. during liveness failures with many skipped slots), fall back to loading
-        // a more recent state from the store and advancing it if necessary.
-        let head = ctx.canonical_head.cached_head();
-        let head_state = &head.snapshot.beacon_state;
-
-        let message_epoch = slot.epoch(T::EthSpec::slots_per_epoch());
-        let state_epoch = head_state.current_epoch();
-
-        // get_ptc can serve epochs in [state_epoch - 1, state_epoch + min_seed_lookahead].
-        // If the message epoch is beyond that range, the head state is stale.
-        let advanced_state = if message_epoch
-            > state_epoch
-                .safe_add(ctx.spec.min_seed_lookahead)
-                .map_err(BeaconChainError::from)?
-        {
-            let head_block_root = head.head_block_root();
-            let target_slot = message_epoch.start_slot(T::EthSpec::slots_per_epoch());
-
-            let (state_root, mut state) = ctx
-                .store
-                .get_advanced_hot_state(
-                    head_block_root,
-                    target_slot,
-                    head.snapshot.beacon_state_root(),
-                )
-                .map_err(BeaconChainError::from)?
-                .ok_or(BeaconChainError::MissingBeaconState(
-                    head.snapshot.beacon_state_root(),
-                ))?;
-
-            if state
-                .current_epoch()
-                .safe_add(ctx.spec.min_seed_lookahead)
-                .map_err(BeaconChainError::from)?
-                < message_epoch
-            {
-                partial_state_advance(&mut state, Some(state_root), target_slot, ctx.spec)
-                    .map_err(BeaconChainError::from)?;
-            }
-
-            Some(state)
-        } else {
-            None
         };
 
-        let state = advanced_state.as_ref().unwrap_or(head_state);
+        // Spec: use `store.block_states[data.beacon_block_root]` to derive the PTC for this
+        // payload attestation. The canonical head can be on a different branch.
+        let state = ctx
+            .store
+            .get_hot_state(&block.state_root, true)
+            .map_err(BeaconChainError::from)?
+            .ok_or_else(|| {
+                BeaconChainError::DBInconsistent(format!(
+                    "Missing state for payload attestation block {:?}",
+                    block.state_root
+                ))
+            })?;
 
         // [REJECT] `validator_index` is within `get_ptc(state, data.slot)`.
         let ptc = state.get_ptc(slot, ctx.spec)?;
@@ -146,7 +108,7 @@ impl<T: BeaconChainTypes> VerifiedPayloadAttestationMessage<T> {
             // [REJECT] The signature is valid with respect to the `validator_index`.
             let pubkey_cache = ctx.validator_pubkey_cache.read();
             let signature_set = indexed_payload_attestation_signature_set(
-                state,
+                &state,
                 |validator_index| pubkey_cache.get(validator_index).map(Cow::Borrowed),
                 &indexed_payload_attestation.signature,
                 &indexed_payload_attestation,

consensus/fork_choice/src/fork_choice.rs

@@ -283,6 +283,8 @@ pub struct QueuedAttestation {
     target_epoch: Epoch,
     /// Per Gloas spec: `payload_present = attestation.data.index == 1`.
     payload_present: bool,
+    /// Pre-Gloas latest messages update by target epoch. Gloas updates by slot.
+    update_by_slot: bool,
 }
 
 /// Legacy queued attestation without payload_present (pre-Gloas, schema V28).
@@ -294,14 +296,18 @@ pub struct QueuedAttestationV28 {
     target_epoch: Epoch,
 }
 
-impl<'a, E: EthSpec> From<IndexedAttestationRef<'a, E>> for QueuedAttestation {
+impl QueuedAttestation {
-    fn from(a: IndexedAttestationRef<'a, E>) -> Self {
+    fn from_indexed_attestation<E: EthSpec>(
+        a: IndexedAttestationRef<'_, E>,
+        update_by_slot: bool,
+    ) -> Self {
         Self {
             slot: a.data().slot,
             attesting_indices: a.attesting_indices_to_vec(),
             block_root: a.data().beacon_block_root,
             target_epoch: a.data().target.epoch,
-            payload_present: a.data().index == 1,
+            payload_present: update_by_slot && a.data().index == 1,
+            update_by_slot,
         }
     }
 }
@@ -770,18 +776,30 @@ where
     ) -> Result<(), Error<T::Error>> {
         let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_BLOCK_TIMES);
 
-        // If this block has already been processed we do not need to reprocess it.
-        // We check this immediately in case re-processing the block mutates some property of the
-        // global fork choice store, e.g. the justified checkpoints or the proposer boost root.
-        if self.proto_array.contains_block(&block_root) {
-            return Ok(());
-        }
-
         // Provide the slot (as per the system clock) to the `fc_store` and then return its view of
         // the current slot. The `fc_store` will ensure that the `current_slot` is never
         // decreasing, a property which we must maintain.
         let current_slot = self.update_time(system_time_current_slot)?;
 
+        // Check block is later than the finalized epoch slot (optimization to reduce calls to
+        // get_ancestor).
+        let finalized_slot =
+            compute_start_slot_at_epoch::<E>(self.fc_store.finalized_checkpoint().epoch);
+        if block.slot() <= finalized_slot {
+            return Err(Error::InvalidBlock(InvalidBlock::FinalizedSlot {
+                finalized_slot,
+                block_slot: block.slot(),
+            }));
+        }
+
+        // If this block has already been processed we do not need to reprocess it.
+        // We check this before parent lookup and state updates in case re-processing the block
+        // mutates some property of the global fork choice store, e.g. the justified checkpoints or
+        // the proposer boost root. The finalized-slot check above still applies to match the spec.
+        if self.proto_array.contains_block(&block_root) {
+            return Ok(());
+        }
+
         // Parent block must be known.
         let parent_block = self
             .proto_array
@@ -799,17 +817,6 @@ where
             }));
         }
 
-        // Check that block is later than the finalized epoch slot (optimization to reduce calls to
-        // get_ancestor).
-        let finalized_slot =
-            compute_start_slot_at_epoch::<E>(self.fc_store.finalized_checkpoint().epoch);
-        if block.slot() <= finalized_slot {
-            return Err(Error::InvalidBlock(InvalidBlock::FinalizedSlot {
-                finalized_slot,
-                block_slot: block.slot(),
-            }));
-        }
-
         // Check block is a descendant of the finalized block at the checkpoint finalized slot.
         //
         // Note: the specification uses `hash_tree_root(block)` instead of `block.parent_root` for
@@ -1317,10 +1324,10 @@ where
         self.validate_on_attestation(attestation, is_from_block, spec)?;
 
         // Per Gloas spec: `payload_present = attestation.data.index == 1`.
-        let payload_present = spec
+        let is_gloas = spec
             .fork_name_at_slot::<E>(attestation.data().slot)
-            .gloas_enabled()
+            .gloas_enabled();
-            && attestation.data().index == 1;
+        let payload_present = is_gloas && attestation.data().index == 1;
 
         if attestation.data().slot < self.fc_store.get_current_slot() {
             for validator_index in attestation.attesting_indices_iter() {
@@ -1329,6 +1336,8 @@ where
                     attestation.data().beacon_block_root,
                     attestation.data().slot,
                     payload_present,
+                    is_gloas,
+                    E::slots_per_epoch(),
                 )?;
             }
         } else {
@@ -1339,7 +1348,10 @@ where
             // Delay consideration in the fork choice until their slot is in the past.
             // ```
             self.queued_attestations
-                .push(QueuedAttestation::from(attestation));
+                .push(QueuedAttestation::from_indexed_attestation(
+                    attestation,
+                    is_gloas,
+                ));
         }
 
         Ok(())
@@ -1506,6 +1518,8 @@ where
                     attestation.block_root,
                     attestation.slot,
                     attestation.payload_present,
+                    attestation.update_by_slot,
+                    E::slots_per_epoch(),
                 )?;
             }
         }
@@ -1901,6 +1915,7 @@ mod tests {
                 block_root: Hash256::zero(),
                 target_epoch: Epoch::new(0),
                 payload_present: false,
+                update_by_slot: false,
             })
             .collect()
     }

consensus/proto_array/src/fork_choice_test_definition.rs

@@ -338,7 +338,14 @@ impl ForkChoiceTestDefinition {
                     attestation_slot,
                 } => {
                     fork_choice
-                        .process_attestation(validator_index, block_root, attestation_slot, false)
+                        .process_attestation(
+                            validator_index,
+                            block_root,
+                            attestation_slot,
+                            false,
+                            false,
+                            MainnetEthSpec::slots_per_epoch(),
+                        )
                         .unwrap_or_else(|_| {
                             panic!(
                                 "process_attestation op at index {} returned error",
@@ -359,6 +366,8 @@ impl ForkChoiceTestDefinition {
                             block_root,
                             attestation_slot,
                             payload_present,
+                            true,
+                            MainnetEthSpec::slots_per_epoch(),
                         )
                         .unwrap_or_else(|_| {
                             panic!(

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -594,10 +594,18 @@ impl ProtoArrayForkChoice {
         block_root: Hash256,
         attestation_slot: Slot,
         payload_present: bool,
+        update_by_slot: bool,
+        slots_per_epoch: u64,
     ) -> Result<(), String> {
         let vote = self.votes.get_mut(validator_index);
 
-        if attestation_slot > vote.next_slot || *vote == VoteTracker::default() {
+        let is_newer_vote = if update_by_slot {
+            attestation_slot > vote.next_slot
+        } else {
+            attestation_slot.epoch(slots_per_epoch) > vote.next_slot.epoch(slots_per_epoch)
+        };
+
+        if is_newer_vote || *vote == VoteTracker::default() {
             vote.next_root = block_root;
             vote.next_slot = attestation_slot;
             vote.next_payload_present = payload_present;

testing/ef_tests/src/cases/fork_choice.rs

@@ -15,9 +15,7 @@ use beacon_chain::data_column_verification::GossipVerifiedDataColumn;
 use beacon_chain::slot_clock::SlotClock;
 use beacon_chain::{
     AvailabilityProcessingStatus, BeaconChainTypes, CachedHead, ChainConfig, NotifyExecutionLayer,
-    attestation_verification::{
+    attestation_verification::VerifiedAttestation,
-        VerifiedAttestation, obtain_indexed_attestation_and_committees_per_slot,
-    },
     blob_verification::GossipVerifiedBlob,
     custody_context::NodeCustodyType,
     test_utils::{BeaconChainHarness, EphemeralHarnessType},
@@ -29,7 +27,9 @@ use execution_layer::{
 use serde::Deserialize;
 use ssz_derive::Decode;
 use state_processing::VerifySignatures;
+use state_processing::common::{attesting_indices_base, attesting_indices_electra};
 use state_processing::envelope_processing::verify_execution_payload_envelope;
+use state_processing::per_block_processing::verify_attester_slashing;
 use state_processing::state_advance::complete_state_advance;
 use std::future::Future;
 use std::sync::Arc;
@@ -413,8 +413,21 @@ impl<E: EthSpec> Case for ForkChoiceTest<E> {
                 }
                 Step::AttesterSlashing {
                     attester_slashing,
-                    valid: _,
+                    valid,
-                } => tester.process_attester_slashing(attester_slashing.to_ref()),
+                } => {
+                    let result = tester.process_attester_slashing(attester_slashing.to_ref());
+                    match valid {
+                        Some(false) => {
+                            if result.is_ok() {
+                                return Err(Error::DidntFail(
+                                    "attester slashing marked valid=false should have been rejected"
+                                        .into(),
+                                ));
+                            }
+                        }
+                        _ => result?,
+                    }
+                }
                 Step::PowBlock { pow_block } => tester.process_pow_block(pow_block),
                 Step::OnPayloadInfo {
                     block_hash,
@@ -703,7 +716,7 @@ impl<E: EthSpec> Tester<E> {
                 let _ = self.process_attestation(&att);
             }
             for attester_slashing in block.message().body().attester_slashings() {
-                self.process_attester_slashing(attester_slashing);
+                self.process_attester_slashing(attester_slashing)?;
             }
         }
 
@@ -807,7 +820,7 @@ impl<E: EthSpec> Tester<E> {
                 let _ = self.process_attestation(&att);
             }
             for attester_slashing in block.message().body().attester_slashings() {
-                self.process_attester_slashing(attester_slashing);
+                self.process_attester_slashing(attester_slashing)?;
             }
         }
 
@@ -885,11 +898,61 @@ impl<E: EthSpec> Tester<E> {
     }
 
     pub fn process_attestation(&self, attestation: &Attestation<E>) -> Result<(), Error> {
-        let (indexed_attestation, _) = obtain_indexed_attestation_and_committees_per_slot(
+        let target_root = attestation.data().target.root;
-            &self.harness.chain,
+        let target_block = self
-            attestation.to_ref(),
+            .harness
+            .chain
+            .canonical_head
+            .fork_choice_read_lock()
+            .get_block(&target_root)
+            .ok_or_else(|| {
+                Error::InternalError(format!("attestation target block {target_root:?} unknown"))
+            })?;
+        let mut target_state = self
+            .harness
+            .chain
+            .store
+            .get_hot_state(&target_block.state_root, CACHE_STATE_IN_TESTS)
+            .map_err(|e| Error::InternalError(format!("failed to load target state: {e:?}")))?
+            .ok_or_else(|| {
+                Error::InternalError(format!(
+                    "attestation target state {:?} unknown",
+                    target_block.state_root
+                ))
+            })?;
+        let target_epoch_start_slot = attestation
+            .data()
+            .target
+            .epoch
+            .start_slot(E::slots_per_epoch());
+        complete_state_advance(
+            &mut target_state,
+            Some(target_block.state_root),
+            target_epoch_start_slot,
+            &self.harness.chain.spec,
         )
-        .map_err(|e| Error::InternalError(format!("attestation indexing failed with {:?}", e)))?;
+        .map_err(|e| {
+            Error::InternalError(format!("failed to advance attestation target state: {e:?}"))
+        })?;
+
+        let indexed_attestation = match attestation.to_ref() {
+            AttestationRef::Base(att) => {
+                let committee = target_state
+                    .get_beacon_committee(att.data.slot, att.data.index)
+                    .map_err(|e| {
+                        Error::InternalError(format!("attestation committee lookup failed: {e:?}"))
+                    })?;
+                attesting_indices_base::get_indexed_attestation(committee.committee, att).map_err(
+                    |e| Error::InternalError(format!("attestation indexing failed: {e:?}")),
+                )?
+            }
+            AttestationRef::Electra(att) => {
+                attesting_indices_electra::get_indexed_attestation_from_state(&target_state, att)
+                    .map_err(|e| {
+                        Error::InternalError(format!("attestation indexing failed: {e:?}"))
+                    })?
+            }
+        };
         let verified_attestation: ManuallyVerifiedAttestation<EphemeralHarnessType<E>> =
             ManuallyVerifiedAttestation {
                 attestation,
@@ -906,10 +969,37 @@ impl<E: EthSpec> Tester<E> {
         &self,
         message: &PayloadAttestationMessage,
     ) -> Result<(), Error> {
-        let head = self.harness.chain.canonical_head.cached_head();
-        let head_state = &head.snapshot.beacon_state;
         let slot = message.data.slot;
-        let ptc = head_state
+
+        let block = {
+            let fork_choice = self.harness.chain.canonical_head.fork_choice_read_lock();
+            fork_choice
+                .get_block(&message.data.beacon_block_root)
+                .ok_or_else(|| {
+                    Error::InternalError(format!(
+                        "payload attestation block {:?} not found",
+                        message.data.beacon_block_root
+                    ))
+                })?
+        };
+        let state = self
+            .harness
+            .chain
+            .store
+            .get_hot_state(&block.state_root, CACHE_STATE_IN_TESTS)
+            .map_err(|e| {
+                Error::InternalError(format!(
+                    "failed to load payload attestation block state: {e:?}"
+                ))
+            })?
+            .ok_or_else(|| {
+                Error::InternalError(format!(
+                    "payload attestation block state {:?} not found",
+                    block.state_root
+                ))
+            })?;
+
+        let ptc = state
             .get_ptc(slot, &self.spec)
             .map_err(|e| Error::InternalError(format!("get_ptc failed with {:?}", e)))?;
 
@@ -935,12 +1025,44 @@ impl<E: EthSpec> Tester<E> {
             })
     }
 
-    pub fn process_attester_slashing(&self, attester_slashing: AttesterSlashingRef<E>) {
+    pub fn process_attester_slashing(
+        &self,
+        attester_slashing: AttesterSlashingRef<E>,
+    ) -> Result<(), Error> {
+        let justified_block = {
+            let fork_choice = self.harness.chain.canonical_head.fork_choice_read_lock();
+            fork_choice
+                .get_block(&fork_choice.justified_checkpoint().root)
+                .ok_or_else(|| Error::InternalError("justified block not found".into()))?
+        };
+        let justified_state = self
+            .harness
+            .chain
+            .store
+            .get_hot_state(&justified_block.state_root, CACHE_STATE_IN_TESTS)
+            .map_err(|e| Error::InternalError(format!("failed to load justified state: {e:?}")))?
+            .ok_or_else(|| {
+                Error::InternalError(format!(
+                    "justified state {:?} not found",
+                    justified_block.state_root
+                ))
+            })?;
+
+        verify_attester_slashing(
+            &justified_state,
+            attester_slashing,
+            VerifySignatures::True,
+            &self.harness.chain.spec,
+        )
+        .map_err(|e| Error::InternalError(format!("invalid attester slashing: {e:?}")))?;
+
         self.harness
             .chain
             .canonical_head
             .fork_choice_write_lock()
-            .on_attester_slashing(attester_slashing)
+            .on_attester_slashing(attester_slashing);
+
+        Ok(())
     }
 
     pub fn process_pow_block(&self, pow_block: &PowBlock) {