From 9f1f68c3ee940bc4bf4830af3dbc8b7f2966fe9e Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 26 Mar 2026 10:39:04 +1100 Subject: [PATCH] Add back AttestationFromBlock --- beacon_node/beacon_chain/src/beacon_chain.rs | 7 ++-- .../beacon_chain/src/block_verification.rs | 11 +++++-- consensus/fork_choice/src/fork_choice.rs | 32 ++++++++++++------- consensus/fork_choice/src/lib.rs | 8 ++--- consensus/fork_choice/tests/tests.rs | 18 ++++++++--- consensus/proto_array/src/proto_array.rs | 2 -- 6 files changed, 51 insertions(+), 27 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index c4cc6925ba..9fa32d5dcc 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -85,8 +85,8 @@ use execution_layer::{ }; use fixed_bytes::FixedBytesExtended; use fork_choice::{ - ExecutionStatus, ForkChoice, ForkchoiceUpdateParameters, InvalidationOperation, - PayloadVerificationStatus, ResetPayloadStatuses, + AttestationFromBlock, ExecutionStatus, ForkChoice, ForkchoiceUpdateParameters, + InvalidationOperation, PayloadVerificationStatus, ResetPayloadStatuses, }; use futures::channel::mpsc::Sender; use itertools::Itertools; @@ -2297,7 +2297,7 @@ impl BeaconChain { .on_attestation( self.slot()?, verified.indexed_attestation().to_ref(), - false, + AttestationFromBlock::False, &self.spec, ) .map_err(Into::into) @@ -4757,6 +4757,7 @@ impl BeaconChain { }) } + // TODO(gloas): wrong for Gloas, needs an update pub fn overridden_forkchoice_update_params_or_failure_reason( &self, canonical_forkchoice_params: &ForkchoiceUpdateParameters, diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index d4c63a0551..bc29486326 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -71,7 +71,7 @@ use bls::{PublicKey, PublicKeyBytes}; use educe::Educe; use eth2::types::{BlockGossip, EventKind}; use execution_layer::PayloadStatus; -pub use fork_choice::PayloadVerificationStatus; +pub use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; use metrics::TryExt; use parking_lot::RwLockReadGuard; use proto_array::Block as ProtoBlock; @@ -1666,7 +1666,12 @@ impl ExecutionPendingBlock { .get_indexed_attestation(&state, attestation) .map_err(|e| BlockError::PerBlockProcessingError(e.into_with_index(i)))?; - match fork_choice.on_attestation(current_slot, indexed_attestation, true, &chain.spec) { + match fork_choice.on_attestation( + current_slot, + indexed_attestation, + AttestationFromBlock::True, + &chain.spec, + ) { 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. @@ -1689,7 +1694,7 @@ impl ExecutionPendingBlock { match fork_choice.on_payload_attestation( current_slot, indexed_payload_attestation, - true, + AttestationFromBlock::True, &ptc.0, ) { Ok(()) => Ok(()), diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 896bb87c2d..d970b437b7 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -328,6 +328,15 @@ fn dequeue_payload_attestations( } /// Denotes whether an attestation we are processing was 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 +#[derive(Clone, Copy)] +pub enum AttestationFromBlock { + True, + False, +} + /// Parameters which are cached between calls to `ForkChoice::get_head`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct ForkchoiceUpdateParameters { @@ -1082,7 +1091,7 @@ where fn validate_on_attestation( &self, indexed_attestation: IndexedAttestationRef, - is_from_block: bool, + is_from_block: AttestationFromBlock, spec: &ChainSpec, ) -> Result<(), InvalidAttestation> { // There is no point in processing an attestation with an empty bitfield. Reject @@ -1096,7 +1105,7 @@ where let target = indexed_attestation.data().target; - if !is_from_block { + if matches!(is_from_block, AttestationFromBlock::False) { self.validate_target_epoch_against_current_time(target.epoch)?; } @@ -1193,7 +1202,7 @@ where fn validate_on_payload_attestation( &self, indexed_payload_attestation: &IndexedPayloadAttestation, - is_from_block: bool, + is_from_block: AttestationFromBlock, ) -> Result<(), InvalidAttestation> { if indexed_payload_attestation.attesting_indices.is_empty() { return Err(InvalidAttestation::EmptyAggregationBitfield); @@ -1215,7 +1224,7 @@ where // Gossip payload attestations must be for the current slot. // https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/fork-choice.md - if !is_from_block + if matches!(is_from_block, AttestationFromBlock::False) && indexed_payload_attestation.data.slot != self.fc_store.get_current_slot() { return Err(InvalidAttestation::PayloadAttestationNotCurrentSlot { @@ -1227,7 +1236,7 @@ where // A payload attestation voting payload_present for a block in the current slot is // invalid: the payload cannot be known yet. This only applies to gossip attestations; // payload attestations from blocks have already been validated by the block producer. - if !is_from_block + if matches!(is_from_block, AttestationFromBlock::False) && self.fc_store.get_current_slot() == block.slot && indexed_payload_attestation.data.payload_present { @@ -1258,7 +1267,7 @@ where &mut self, system_time_current_slot: Slot, attestation: IndexedAttestationRef, - is_from_block: bool, + is_from_block: AttestationFromBlock, spec: &ChainSpec, ) -> Result<(), Error> { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_ATTESTATION_TIMES); @@ -1319,7 +1328,7 @@ where &mut self, system_time_current_slot: Slot, attestation: &IndexedPayloadAttestation, - is_from_block: bool, + is_from_block: AttestationFromBlock, ptc: &[usize], ) -> Result<(), Error> { self.update_time(system_time_current_slot)?; @@ -1339,10 +1348,11 @@ where let processing_slot = self.fc_store.get_current_slot(); // Payload attestations from blocks can be applied in the next slot (S+1 for data.slot=S), // while gossiped payload attestations are delayed one extra slot. - let should_process_now = if is_from_block { - attestation.data.slot < processing_slot - } else { - attestation.data.slot.saturating_add(1_u64) < processing_slot + let should_process_now = match is_from_block { + AttestationFromBlock::True => attestation.data.slot < processing_slot, + AttestationFromBlock::False => { + attestation.data.slot.saturating_add(1_u64) < processing_slot + } }; if should_process_now { diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index de3e709a84..93a8b376f5 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -3,10 +3,10 @@ mod fork_choice_store; mod metrics; pub use crate::fork_choice::{ - Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, InvalidAttestation, - InvalidBlock, PayloadVerificationStatus, PersistedForkChoice, PersistedForkChoiceV17, - PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, QueuedPayloadAttestation, - ResetPayloadStatuses, + AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, + InvalidAttestation, InvalidBlock, PayloadVerificationStatus, PersistedForkChoice, + PersistedForkChoiceV17, PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, + QueuedPayloadAttestation, ResetPayloadStatuses, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::{ diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 44da1af148..0a7000d476 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -10,8 +10,8 @@ use beacon_chain::{ use bls::AggregateSignature; use fixed_bytes::FixedBytesExtended; use fork_choice::{ - ForkChoiceStore, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, - QueuedAttestation, QueuedPayloadAttestation, + AttestationFromBlock, ForkChoiceStore, InvalidAttestation, InvalidBlock, + PayloadVerificationStatus, QueuedAttestation, QueuedPayloadAttestation, }; use state_processing::state_advance::complete_state_advance; use std::fmt; @@ -1033,7 +1033,12 @@ async fn payload_attestation_for_previous_slot_is_accepted_at_next_slot() { let result = chain .canonical_head .fork_choice_write_lock() - .on_payload_attestation(current_slot, &payload_attestation, true, ptc); + .on_payload_attestation( + current_slot, + &payload_attestation, + AttestationFromBlock::True, + ptc, + ); assert!( result.is_ok(), @@ -1082,7 +1087,12 @@ async fn non_block_payload_attestation_for_previous_slot_is_rejected() { let result = chain .canonical_head .fork_choice_write_lock() - .on_payload_attestation(s_plus_1, &payload_attestation, false, ptc); + .on_payload_attestation( + s_plus_1, + &payload_attestation, + AttestationFromBlock::False, + ptc, + ); assert!( matches!( result, diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 1731a79afb..3f8db8d1fb 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1217,8 +1217,6 @@ impl ProtoArray { return false; }; - - // Skip invalid children — they aren't in store.blocks in the spec. let children: Vec = self .nodes