From c7670ede0289bf395baeb6961e27d209f2db9dea Mon Sep 17 00:00:00 2001 From: dapplion <35266934+dapplion@users.noreply.github.com> Date: Wed, 25 Mar 2026 23:07:45 -0500 Subject: [PATCH] Cleanup and spec parity fixes - Add missing payload attestation slot check: spec returns early when data.slot != block.slot (PTC votes only for assigned block) - Remove dead ignored tests (need mock EL Gloas support to run) - Remove unused new_with_gloas and inspect_queued_payload_attestations - Remove gloas entries from bin.rs (not part of this PR) - Collapse nested if in payload attestation error handling (clippy) - Rename env -> envelope in load_parent - Add TODO(gloas) for parent_head_hash in re-org path - Remove head_payload_status from ForkchoiceUpdateParameters (lives on CachedHead, sourced from get_head return) --- .../beacon_chain/src/block_verification.rs | 7 +- consensus/fork_choice/src/fork_choice.rs | 6 + consensus/fork_choice/tests/tests.rs | 128 +----------------- consensus/proto_array/src/bin.rs | 8 -- 4 files changed, 10 insertions(+), 139 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index cb561dff24..53acc70b6e 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1698,10 +1698,9 @@ impl ExecutionPendingBlock { indexed_payload_attestation, AttestationFromBlock::True, &ptc.0, - ) { - if !matches!(e, ForkChoiceError::InvalidAttestation(_)) { - return Err(BlockError::BeaconChainError(Box::new(e.into()))); - } + ) && !matches!(e, ForkChoiceError::InvalidAttestation(_)) + { + return Err(BlockError::BeaconChainError(Box::new(e.into()))); } } } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 38fcfb3400..1f25afee8e 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1216,6 +1216,12 @@ where }); } + // Spec: `if data.slot != state.slot: return` — PTC votes can only + // change the vote for their assigned beacon block. + if block.slot != indexed_payload_attestation.data.slot { + return Ok(()); + } + // Gossip payload attestations must be for the current slot. // https://github.com/ethereum/consensus-specs/blob/master/specs/gloas/fork-choice.md if matches!(is_from_block, AttestationFromBlock::False) diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index b2a6cd5668..839d0f4c5c 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -11,7 +11,7 @@ use bls::AggregateSignature; use fixed_bytes::FixedBytesExtended; use fork_choice::{ AttestationFromBlock, ForkChoiceStore, InvalidAttestation, InvalidBlock, - PayloadVerificationStatus, QueuedAttestation, QueuedPayloadAttestation, + PayloadVerificationStatus, QueuedAttestation, }; use state_processing::state_advance::complete_state_advance; use std::fmt; @@ -76,19 +76,6 @@ impl ForkChoiceTest { /// Creates a new tester with the GLOAS fork active at epoch 1. /// Genesis is a standard Fulu block (epoch 0), so block production works normally. /// Tests that need GLOAS semantics should advance the chain into epoch 1 first. - pub fn new_with_gloas() -> Self { - let mut spec = ForkName::latest_stable().make_genesis_spec(ChainSpec::default()); - spec.gloas_fork_epoch = Some(Epoch::new(1)); - let harness = BeaconChainHarness::builder(MainnetEthSpec) - .spec(spec.into()) - .deterministic_keypairs(VALIDATOR_COUNT) - .fresh_ephemeral_store() - .mock_execution_layer() - .build(); - - Self { harness } - } - /// Get a value from the `ForkChoice` instantiation. fn get(&self, func: T) -> U where @@ -172,29 +159,6 @@ impl ForkChoiceTest { self } - // TODO(gloas): add inspect_queued_payload_attestations when payload - // attestation queueing tests are implemented. - #[allow(dead_code)] - pub fn inspect_queued_payload_attestations(self, mut func: F) -> Self - where - F: FnMut(&[QueuedPayloadAttestation]), - { - self.harness - .chain - .canonical_head - .fork_choice_write_lock() - .update_time(self.harness.chain.slot().unwrap()) - .unwrap(); - func( - self.harness - .chain - .canonical_head - .fork_choice_read_lock() - .queued_payload_attestations(), - ); - self - } - /// Skip a slot, without producing a block. pub fn skip_slot(self) -> Self { self.harness.advance_slot(); @@ -964,96 +928,6 @@ async fn invalid_attestation_future_block() { .await; } -/// Payload attestations (index == 1) are invalid when they refer to a block in the same slot. -/// This check only applies when GLOAS is active. -/// -/// TODO(gloas): un-ignore once the test harness supports Gloas block production. -/// The validation logic is gated on `spec.fork_name_at_slot().gloas_enabled()` in -/// `validate_on_attestation`, which requires a block to exist at a GLOAS-enabled slot. -/// Currently the mock execution layer cannot produce Gloas blocks (no -/// `signed_execution_payload_bid` support). -/// TODO(gloas): un-ignore once mock EL supports Gloas blocks. -/// https://github.com/sigp/lighthouse/issues/9025 -#[ignore] -#[tokio::test] -async fn invalid_attestation_payload_during_same_slot() { - ForkChoiceTest::new_with_gloas() - .apply_blocks_without_new_attestations(1) - .await - .apply_attestation_to_chain( - MutationDelay::NoDelay, - |attestation, chain| { - let block_slot = chain - .get_blinded_block(&attestation.data().beacon_block_root) - .expect("should read attested block") - .expect("attested block should exist") - .slot(); - - attestation.data_mut().slot = block_slot; - attestation.data_mut().target.epoch = block_slot.epoch(E::slots_per_epoch()); - attestation.data_mut().index = 1; - }, - |result| { - assert_invalid_attestation!( - result, - InvalidAttestation::InvalidSameSlotAttestationIndex { slot } - if slot == Slot::new(1) - ) - }, - ) - .await; -} - -/// A payload attestation for block A at slot S should be accepted when processed at slot S+1. -/// TODO(gloas): un-ignore once mock EL supports Gloas blocks. Payload -/// attestations require V29 nodes which need Gloas block production. -/// https://github.com/sigp/lighthouse/issues/9025 -#[ignore] -#[tokio::test] -async fn payload_attestation_for_previous_slot_is_accepted_at_next_slot() { - let test = ForkChoiceTest::new() - .apply_blocks_without_new_attestations(1) - .await; - - let chain = &test.harness.chain; - let block_a = chain - .block_at_slot(Slot::new(1), WhenSlotSkipped::Prev) - .expect("lookup should succeed") - .expect("block A should exist"); - let block_a_root = block_a.canonical_root(); - let current_slot = block_a.slot().saturating_add(1_u64); - - let payload_attestation = IndexedPayloadAttestation:: { - attesting_indices: vec![0_u64].try_into().expect("valid attesting indices"), - data: PayloadAttestationData { - beacon_block_root: block_a_root, - slot: Slot::new(1), - payload_present: true, - blob_data_available: true, - }, - signature: AggregateSignature::empty(), - }; - - // PTC mapping: validator 0 is at ptc position 0. - let ptc = &[0_usize]; - - let result = chain - .canonical_head - .fork_choice_write_lock() - .on_payload_attestation( - current_slot, - &payload_attestation, - AttestationFromBlock::True, - ptc, - ); - - assert!( - result.is_ok(), - "payload attestation at slot S should be accepted at S+1, got: {:?}", - result - ); -} - /// Gossip payload attestations must be for the current slot. A payload attestation for slot S /// received at slot S+1 should be rejected per the spec. #[tokio::test] diff --git a/consensus/proto_array/src/bin.rs b/consensus/proto_array/src/bin.rs index c5df3f17e4..e1d307affb 100644 --- a/consensus/proto_array/src/bin.rs +++ b/consensus/proto_array/src/bin.rs @@ -18,14 +18,6 @@ fn main() { "execution_status_03.yaml", get_execution_status_test_definition_03(), ); - write_test_def_to_yaml( - "gloas_chain_following.yaml", - get_gloas_chain_following_test_definition(), - ); - write_test_def_to_yaml( - "gloas_payload_probe.yaml", - get_gloas_payload_probe_test_definition(), - ); } fn write_test_def_to_yaml(filename: &str, def: ForkChoiceTestDefinition) {