From 91f483415a5d3d6ed6a72262c0fa1ae1100892e1 Mon Sep 17 00:00:00 2001 From: shane-moore Date: Sun, 17 Aug 2025 11:38:18 -0700 Subject: [PATCH] Squash of "Gloas modify process_attestations #8283" --- .../common/get_attestation_participation.rs | 41 ++++- .../src/per_block_processing/errors.rs | 8 +- .../process_operations.rs | 155 +++++++++++++++++- .../verify_attestation.rs | 11 +- consensus/types/src/state/beacon_state.rs | 26 ++- testing/ef_tests/src/cases/operations.rs | 19 ++- 6 files changed, 244 insertions(+), 16 deletions(-) diff --git a/consensus/state_processing/src/common/get_attestation_participation.rs b/consensus/state_processing/src/common/get_attestation_participation.rs index 71bf6329f1..aabd5003f4 100644 --- a/consensus/state_processing/src/common/get_attestation_participation.rs +++ b/consensus/state_processing/src/common/get_attestation_participation.rs @@ -1,4 +1,5 @@ use integer_sqrt::IntegerSquareRoot; +use safe_arith::SafeArith; use smallvec::SmallVec; use types::{AttestationData, BeaconState, ChainSpec, EthSpec}; use types::{ @@ -22,18 +23,46 @@ pub fn get_attestation_participation_flag_indices( inclusion_delay: u64, spec: &ChainSpec, ) -> Result, Error> { + // Matching source let justified_checkpoint = if data.target.epoch == state.current_epoch() { state.current_justified_checkpoint() } else { state.previous_justified_checkpoint() }; - - // Matching roots. let is_matching_source = data.source == justified_checkpoint; - let is_matching_target = is_matching_source - && data.target.root == *state.get_block_root_at_epoch(data.target.epoch)?; - let is_matching_head = - is_matching_target && data.beacon_block_root == *state.get_block_root(data.slot)?; + + // Matching target + let target_root = *state.get_block_root_at_epoch(data.target.epoch)?; + let target_root_matches = data.target.root == target_root; + let is_matching_target = is_matching_source && target_root_matches; + + // Matching head + let head_root = *state.get_block_root(data.slot)?; + let head_root_matches = data.beacon_block_root == head_root; + + let is_matching_head = if state.fork_name_unchecked().gloas_enabled() { + let payload_matches = if state.is_attestation_same_slot(data)? { + // For same-slot attestations, data.index must be 0 + if data.index != 0 { + return Err(Error::BadOverloadedDataIndex(data.index)); + } + true + } else { + // For non same-slot attestations, check execution payload availability + // TODO(EIP7732) Discuss if we want to return new error BeaconStateError::InvalidExecutionPayloadAvailabilityIndex here for bit out of bounds or use something like BeaconStateError::InvalidBitfield + let slot_index = data + .slot + .as_usize() + .safe_rem(E::slots_per_historical_root())?; + state + .execution_payload_availability()? + .get(slot_index) + .map_err(|_| Error::InvalidExecutionPayloadAvailabilityIndex(slot_index))? + }; + is_matching_target && head_root_matches && payload_matches + } else { + is_matching_target && head_root_matches + }; if !is_matching_source { return Err(Error::IncorrectAttestationSource); diff --git a/consensus/state_processing/src/per_block_processing/errors.rs b/consensus/state_processing/src/per_block_processing/errors.rs index f3f5ef96af..9c04b56e39 100644 --- a/consensus/state_processing/src/per_block_processing/errors.rs +++ b/consensus/state_processing/src/per_block_processing/errors.rs @@ -98,6 +98,8 @@ pub enum BlockProcessingError { ExecutionPayloadBidInvalid { reason: ExecutionPayloadBidInvalid, }, + /// Builder payment index out of bounds (Gloas) + BuilderPaymentIndexOutOfBounds(usize), } impl From for BlockProcessingError { @@ -345,7 +347,10 @@ pub enum AttestationInvalid { attestation: Slot, }, /// Attestation slot is too far in the past to be included in a block. - IncludedTooLate { state: Slot, attestation: Slot }, + IncludedTooLate { + state: Slot, + attestation: Slot, + }, /// Attestation target epoch does not match attestation slot. TargetEpochSlotMismatch { target_epoch: Epoch, @@ -378,6 +383,7 @@ pub enum AttestationInvalid { BadSignature, /// The indexed attestation created from this attestation was found to be invalid. BadIndexedAttestation(IndexedAttestationInvalid), + BadOverloadedDataIndex, } impl From> diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index 1ee5fd349c..7f709b291e 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -1,11 +1,11 @@ use super::*; -use crate::VerifySignatures; use crate::common::{ get_attestation_participation_flag_indices, increase_balance, initiate_validator_exit, slash_validator, }; use crate::per_block_processing::errors::{BlockProcessingError, IntoWithIndex}; use crate::per_block_processing::verify_payload_attestation::verify_payload_attestation; +use safe_arith::SafeArith; use ssz_types::FixedVector; use typenum::U33; use types::consts::altair::{PARTICIPATION_FLAG_WEIGHTS, PROPOSER_WEIGHT, WEIGHT_DENOMINATOR}; @@ -221,6 +221,149 @@ pub mod altair_deneb { } } +// TODO(EIP-7732): add test cases to `consensus/state_processing/src/per_block_processing/tests.rs` to handle gloas. +// The tests will require being able to build gloas blocks, which currently fails due to errors as mentioned here. +// https://github.com/sigp/lighthouse/pull/8273 +pub mod gloas { + use super::*; + use crate::common::update_progressive_balances_cache::update_progressive_balances_on_attestation; + + pub fn process_attestations<'a, E: EthSpec, I>( + state: &mut BeaconState, + attestations: I, + verify_signatures: VerifySignatures, + ctxt: &mut ConsensusContext, + spec: &ChainSpec, + ) -> Result<(), BlockProcessingError> + where + I: Iterator>, + { + attestations.enumerate().try_for_each(|(i, attestation)| { + process_attestation(state, attestation, i, ctxt, verify_signatures, spec) + }) + } + + pub fn process_attestation( + state: &mut BeaconState, + attestation: AttestationRef, + att_index: usize, + ctxt: &mut ConsensusContext, + verify_signatures: VerifySignatures, + spec: &ChainSpec, + ) -> Result<(), BlockProcessingError> { + let proposer_index = ctxt.get_proposer_index(state, spec)?; + let previous_epoch = ctxt.previous_epoch; + let current_epoch = ctxt.current_epoch; + + let indexed_att = verify_attestation_for_block_inclusion( + state, + attestation, + ctxt, + verify_signatures, + spec, + ) + .map_err(|e| e.into_with_index(att_index))?; + + // Matching roots, participation flag indices + let data = attestation.data(); + let inclusion_delay = state.slot().safe_sub(data.slot)?.as_u64(); + let participation_flag_indices = + get_attestation_participation_flag_indices(state, data, inclusion_delay, spec)?; + + // [New in EIP-7732] + let current_epoch_target = data.target.epoch == state.current_epoch(); + let slot_mod = data + .slot + .as_usize() + .safe_rem(E::slots_per_epoch() as usize)?; + let payment_index = if current_epoch_target { + (E::slots_per_epoch() as usize).safe_add(slot_mod)? + } else { + slot_mod + }; + // Accumulate weight for same-slot attestations + let mut accumulated_weight = 0; + + // Update epoch participation flags. + let mut proposer_reward_numerator = 0; + for index in indexed_att.attesting_indices_iter() { + let index = *index as usize; + + let validator_effective_balance = state.epoch_cache().get_effective_balance(index)?; + let validator_slashed = state.slashings_cache().is_slashed(index); + + // [New in EIP7732] + // For same-slot attestations, check if we're setting any new flags + // If we are, this validator hasn't contributed to this slot's quorum yet + let mut will_set_new_flag = false; + + for (flag_index, &weight) in PARTICIPATION_FLAG_WEIGHTS.iter().enumerate() { + let epoch_participation = state.get_epoch_participation_mut( + data.target.epoch, + previous_epoch, + current_epoch, + )?; + + if participation_flag_indices.contains(&flag_index) { + let validator_participation = epoch_participation + .get_mut(index) + .ok_or(BeaconStateError::ParticipationOutOfBounds(index))?; + + if !validator_participation.has_flag(flag_index)? { + validator_participation.add_flag(flag_index)?; + proposer_reward_numerator + .safe_add_assign(state.get_base_reward(index)?.safe_mul(weight)?)?; + will_set_new_flag = true; + + update_progressive_balances_on_attestation( + state, + data.target.epoch, + flag_index, + validator_effective_balance, + validator_slashed, + )?; + } + } + } + + // Check that payment_index is valid and get payment amount + let builder_payments = state.builder_pending_payments_mut()?; + let payment_amount = builder_payments + .get(payment_index) + .ok_or(BlockProcessingError::BuilderPaymentIndexOutOfBounds( + payment_index, + ))? + .withdrawal + .amount; + + // Collect validators for Gloas builder payment processing + // We will only add weight for same-slot attestations when any new flag is set + // This ensures each validator contributes exactly once per slot + if will_set_new_flag && state.is_attestation_same_slot(data)? && payment_amount > 0 { + accumulated_weight.safe_add_assign(validator_effective_balance)?; + } + } + + let proposer_reward_denominator = WEIGHT_DENOMINATOR + .safe_sub(PROPOSER_WEIGHT)? + .safe_mul(WEIGHT_DENOMINATOR)? + .safe_div(PROPOSER_WEIGHT)?; + let proposer_reward = proposer_reward_numerator.safe_div(proposer_reward_denominator)?; + increase_balance(state, proposer_index as usize, proposer_reward)?; + + // Update builder payment weight + if accumulated_weight > 0 { + let builder_payments = state.builder_pending_payments_mut()?; + let payment = builder_payments.get_mut(payment_index).ok_or( + BlockProcessingError::BuilderPaymentIndexOutOfBounds(payment_index), + )?; + payment.weight.safe_add_assign(accumulated_weight)?; + } + + Ok(()) + } +} + /// Validates each `ProposerSlashing` and updates the state, short-circuiting on an invalid object. /// /// Returns `Ok(())` if the validation and state updates completed successfully, otherwise returns @@ -294,7 +437,15 @@ pub fn process_attestations>( ctxt: &mut ConsensusContext, spec: &ChainSpec, ) -> Result<(), BlockProcessingError> { - if state.fork_name_unchecked().altair_enabled() { + if state.fork_name_unchecked().gloas_enabled() { + gloas::process_attestations( + state, + block_body.attestations(), + verify_signatures, + ctxt, + spec, + )?; + } else if state.fork_name_unchecked().altair_enabled() { altair_deneb::process_attestations( state, block_body.attestations(), diff --git a/consensus/state_processing/src/per_block_processing/verify_attestation.rs b/consensus/state_processing/src/per_block_processing/verify_attestation.rs index 0d1fd17768..68672773cf 100644 --- a/consensus/state_processing/src/per_block_processing/verify_attestation.rs +++ b/consensus/state_processing/src/per_block_processing/verify_attestation.rs @@ -66,6 +66,10 @@ pub fn verify_attestation_for_state<'ctxt, E: EthSpec>( // NOTE: choosing a validation based on the attestation's fork // rather than the state's fork makes this simple, but technically the spec // defines this verification based on the state's fork. + // Verify data.index based on attestation variant. + // The attestation variant is determined by the block body variant, which matches the fork. + + // TODO(EIP-7732): discuss if it makes more sense to match on `ForkName` instead of attestation type. A reason against is an edge case like at the Gloas fork boundary, the first gloas block will contain attestations for a Fulu block, so I would think we would want this validation to still be with respect to fulu rules. But perhaps I'm wrong? match attestation { AttestationRef::Base(_) => { verify!( @@ -74,7 +78,12 @@ pub fn verify_attestation_for_state<'ctxt, E: EthSpec>( ); } AttestationRef::Electra(_) => { - verify!(data.index == 0, Invalid::BadCommitteeIndex); + let fork_at_attestation_slot = spec.fork_name_at_slot::(data.slot); + if fork_at_attestation_slot.gloas_enabled() { + verify!(data.index < 2, Invalid::BadOverloadedDataIndex); + } else { + verify!(data.index == 0, Invalid::BadCommitteeIndex); + } } } diff --git a/consensus/types/src/state/beacon_state.rs b/consensus/types/src/state/beacon_state.rs index 3ca559c03b..7a872170d0 100644 --- a/consensus/types/src/state/beacon_state.rs +++ b/consensus/types/src/state/beacon_state.rs @@ -25,8 +25,8 @@ use typenum::Unsigned; use crate::{ BuilderPendingPayment, BuilderPendingWithdrawal, ExecutionBlockHash, ExecutionPayloadBid, attestation::{ - AttestationDuty, BeaconCommittee, Checkpoint, CommitteeIndex, PTC, ParticipationFlags, - PendingAttestation, + AttestationData, AttestationDuty, BeaconCommittee, Checkpoint, CommitteeIndex, PTC, + ParticipationFlags, PendingAttestation, }, block::{BeaconBlock, BeaconBlockHeader, SignedBeaconBlockHash}, consolidation::PendingConsolidation, @@ -175,6 +175,8 @@ pub enum BeaconStateError { NonExecutionAddressWithdrawalCredential, NoCommitteeFound(CommitteeIndex), InvalidCommitteeIndex(CommitteeIndex), + /// `Attestation.data.index` field is invalid in overloaded data index scenario. + BadOverloadedDataIndex(u64), InvalidSelectionProof { aggregator_index: u64, }, @@ -198,6 +200,7 @@ pub enum BeaconStateError { }, InvalidIndicesCount, PleaseNotifyTheDevs(String), + InvalidExecutionPayloadAvailabilityIndex(usize), } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. @@ -2055,6 +2058,25 @@ impl BeaconState { Ok(cache.get_attestation_duties(validator_index)) } + /// Check if the attestation is for the block proposed at the attestation slot. + /// + /// Returns `true` if the attestation's block root matches the block root at the + /// attestation's slot, and the block root differs from the previous slot's root. + pub fn is_attestation_same_slot( + &self, + data: &AttestationData, + ) -> Result { + if data.slot == 0 { + return Ok(true); + } + + let blockroot = data.beacon_block_root; + let slot_blockroot = *self.get_block_root(data.slot)?; + let prev_blockroot = *self.get_block_root(data.slot.safe_sub(1)?)?; + + Ok(blockroot == slot_blockroot && blockroot != prev_blockroot) + } + /// Compute the total active balance cache from scratch. /// /// This method should rarely be invoked because single-pass epoch processing keeps the total diff --git a/testing/ef_tests/src/cases/operations.rs b/testing/ef_tests/src/cases/operations.rs index 63b46945c2..86e63ca363 100644 --- a/testing/ef_tests/src/cases/operations.rs +++ b/testing/ef_tests/src/cases/operations.rs @@ -7,7 +7,8 @@ use ssz::Decode; use state_processing::common::update_progressive_balances_cache::initialize_progressive_balances_cache; use state_processing::epoch_cache::initialize_epoch_cache; use state_processing::per_block_processing::process_operations::{ - process_consolidation_requests, process_deposit_requests, process_withdrawal_requests, + altair_deneb, base, gloas, process_consolidation_requests, process_deposit_requests, + process_withdrawal_requests, }; use state_processing::{ ConsensusContext, @@ -16,8 +17,8 @@ use state_processing::{ errors::BlockProcessingError, process_block_header, process_execution_payload, process_operations::{ - altair_deneb, base, process_attester_slashings, process_bls_to_execution_changes, - process_deposits, process_exits, process_proposer_slashings, + process_attester_slashings, process_bls_to_execution_changes, process_deposits, + process_exits, process_proposer_slashings, }, process_sync_aggregate, process_withdrawals, }, @@ -99,7 +100,17 @@ impl Operation for Attestation { ) -> Result<(), BlockProcessingError> { initialize_epoch_cache(state, spec)?; let mut ctxt = ConsensusContext::new(state.slot()); - if state.fork_name_unchecked().altair_enabled() { + if state.fork_name_unchecked().gloas_enabled() { + initialize_progressive_balances_cache(state, spec)?; + gloas::process_attestation( + state, + self.to_ref(), + 0, + &mut ctxt, + VerifySignatures::True, + spec, + ) + } else if state.fork_name_unchecked().altair_enabled() { initialize_progressive_balances_cache(state, spec)?; altair_deneb::process_attestation( state,