From 715d6bfa0cf815b325f0763f7e20b2c956f9ba5a Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 13 May 2026 09:26:27 +1000 Subject: [PATCH] All tests bar invalid_message passing, changes might be dubious --- .../beacon_chain/src/block_verification.rs | 23 ++- .../gossip_verified_payload_attestation.rs | 70 ++------ consensus/fork_choice/src/fork_choice.rs | 65 +++++--- .../src/fork_choice_test_definition.rs | 11 +- .../src/proto_array_fork_choice.rs | 10 +- testing/ef_tests/src/cases/fork_choice.rs | 154 ++++++++++++++++-- 6 files changed, 230 insertions(+), 103 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 9a43147233..5075869b54 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/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 ExecutionPendingBlock { * 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 ExecutionPendingBlock { } // Register each attestation in the block with fork choice. - for (i, attestation) in block.message().body().attestations().enumerate() { - let indexed_attestation = consensus_context - .get_indexed_attestation(&state, attestation) - .map_err(|e| BlockError::PerBlockProcessingError(e.into_with_index(i)))?; - + for indexed_attestation in indexed_attestations { match fork_choice.on_attestation( current_slot, - indexed_attestation, + indexed_attestation.to_ref(), AttestationFromBlock::True, &chain.spec, ) { diff --git a/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs b/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs index c36c73b344..53746995ea 100644 --- a/beacon_node/beacon_chain/src/payload_attestation_verification/gossip_verified_payload_attestation.rs +++ b/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 VerifiedPayloadAttestationMessage { // 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 VerifiedPayloadAttestationMessage { // [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, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 4fdd8004cf..f5dba28a0a 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/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> for QueuedAttestation { - fn from(a: IndexedAttestationRef<'a, E>) -> Self { +impl QueuedAttestation { + fn from_indexed_attestation( + 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> { 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::(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::(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::(attestation.data().slot) - .gloas_enabled() - && attestation.data().index == 1; + .gloas_enabled(); + 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() } diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index d537f16bb2..58d767ec05 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/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!( diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index f0cba5c49d..64aae65e5d 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/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; diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index 1113cbe72f..15285d6e63 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/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::{ - VerifiedAttestation, obtain_indexed_attestation_and_committees_per_slot, - }, + attestation_verification::VerifiedAttestation, 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 Case for ForkChoiceTest { } Step::AttesterSlashing { attester_slashing, - valid: _, - } => tester.process_attester_slashing(attester_slashing.to_ref()), + valid, + } => { + 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 Tester { 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 Tester { 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 Tester { } pub fn process_attestation(&self, attestation: &Attestation) -> Result<(), Error> { - let (indexed_attestation, _) = obtain_indexed_attestation_and_committees_per_slot( - &self.harness.chain, - attestation.to_ref(), + let target_root = attestation.data().target.root; + let target_block = self + .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> = ManuallyVerifiedAttestation { attestation, @@ -906,10 +969,37 @@ impl Tester { &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 Tester { }) } - pub fn process_attester_slashing(&self, attester_slashing: AttesterSlashingRef) { + pub fn process_attester_slashing( + &self, + attester_slashing: AttesterSlashingRef, + ) -> 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) {