From bc6cf0f88290cb6c8de35ab8623dc0363e65cb92 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Wed, 1 Apr 2026 11:17:33 +1100 Subject: [PATCH] Remove payload attestation queueing and more cleanups --- beacon_node/http_api/src/lib.rs | 4 + beacon_node/http_api/tests/tests.rs | 4 + consensus/fork_choice/src/fork_choice.rs | 113 +++--------------- consensus/proto_array/src/proto_array.rs | 6 +- .../src/proto_array_fork_choice.rs | 6 +- 5 files changed, 34 insertions(+), 99 deletions(-) diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 5df1078617..0bb04888b7 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2148,10 +2148,14 @@ pub fn serve( execution_status: execution_status_string, best_child: node .best_child() + .ok() + .flatten() .and_then(|index| proto_array.nodes.get(index)) .map(|child| child.root()), best_descendant: node .best_descendant() + .ok() + .flatten() .and_then(|index| proto_array.nodes.get(index)) .map(|descendant| descendant.root()), }, diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 14bfb5ce92..b28816302c 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -3179,10 +3179,14 @@ impl ApiTester { .unwrap_or_else(|| "irrelevant".to_string()), best_child: node .best_child() + .ok() + .flatten() .and_then(|index| expected_proto_array.nodes.get(index)) .map(|child| child.root()), best_descendant: node .best_descendant() + .ok() + .flatten() .and_then(|index| expected_proto_array.nodes.get(index)) .map(|descendant| descendant.root()), }, diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 771104a02f..a80ec99a25 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -138,10 +138,6 @@ pub enum InvalidBlock { finalized_root: Hash256, block_ancestor: Option, }, - MissingExecutionPayloadBid { - block_slot: Slot, - block_root: Hash256, - }, } #[derive(Debug)] @@ -310,22 +306,6 @@ fn dequeue_attestations( std::mem::replace(queued_attestations, remaining) } -/// Returns all values in `queued` that have `slot + 1 < current_slot`. -/// Payload attestations need an extra slot of delay compared to regular attestations. -fn dequeue_payload_attestations( - current_slot: Slot, - queued: &mut Vec, -) -> Vec { - let remaining = queued.split_off( - queued - .iter() - .position(|a| a.slot.saturating_add(1_u64) >= current_slot) - .unwrap_or(queued.len()), - ); - - std::mem::replace(queued, remaining) -} - /// Denotes whether an attestation we are processing was received from a block or from gossip. /// Equivalent to the `is_from_block` `bool` in: /// @@ -370,9 +350,6 @@ pub struct ForkChoice { proto_array: ProtoArrayForkChoice, /// Attestations that arrived at the current slot and must be queued for later processing. queued_attestations: Vec, - /// Payload attestations (PTC votes) that must be queued for later processing. - /// These have different dequeue timing than regular attestations. - queued_payload_attestations: Vec, /// Stores a cache of the values required to be sent to the execution layer. forkchoice_update_parameters: ForkchoiceUpdateParameters, _phantom: PhantomData, @@ -387,7 +364,6 @@ where self.fc_store == other.fc_store && self.proto_array == other.proto_array && self.queued_attestations == other.queued_attestations - && self.queued_payload_attestations == other.queued_payload_attestations } } @@ -472,7 +448,6 @@ where fc_store, proto_array, queued_attestations: vec![], - queued_payload_attestations: vec![], // This will be updated during the next call to `Self::get_head`. forkchoice_update_parameters: ForkchoiceUpdateParameters { head_hash: None, @@ -966,14 +941,6 @@ where Some(signed_bid.message.block_hash), ) } else { - if spec.fork_name_at_slot::(block.slot()).gloas_enabled() { - return Err(Error::InvalidBlock( - InvalidBlock::MissingExecutionPayloadBid { - block_slot: block.slot(), - block_root, - }, - )); - } (None, None) }; @@ -1334,11 +1301,21 @@ where ) -> Result<(), Error> { self.update_time(system_time_current_slot)?; - if attestation.data.beacon_block_root == Hash256::zero() { + if attestation.data.beacon_block_root.is_zero() { return Ok(()); } - self.validate_on_payload_attestation(attestation, is_from_block)?; + match self.validate_on_payload_attestation(attestation, is_from_block) { + Ok(()) => (), + Err(InvalidAttestation::PayloadAttestationNotCurrentSlot { .. }) => { + // Just ignore wrong-slot payload attestations, they could have been processed at + // the correct slot when received on gossip, but then have the wrong-slot by the + // time they make it to here (TOCTOU). + // TODO(gloas): consider moving this to the call site for gossip processing + return Ok(()); + } + Err(e) => return Err(e.into()), + } // Resolve validator indices to PTC committee positions. let ptc_indices: Vec = attestation @@ -1346,34 +1323,13 @@ where .filter_map(|vi| ptc.iter().position(|&p| p == *vi as usize)) .collect(); - 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 = 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 { - for &ptc_index in &ptc_indices { - self.proto_array.process_payload_attestation( - attestation.data.beacon_block_root, - ptc_index, - attestation.data.payload_present, - attestation.data.blob_data_available, - )?; - } - } else { - self.queued_payload_attestations - .push(QueuedPayloadAttestation { - slot: attestation.data.slot, - ptc_indices, - block_root: attestation.data.beacon_block_root, - payload_present: attestation.data.payload_present, - blob_data_available: attestation.data.blob_data_available, - }); + for &ptc_index in &ptc_indices { + self.proto_array.process_payload_attestation( + attestation.data.beacon_block_root, + ptc_index, + attestation.data.payload_present, + attestation.data.blob_data_available, + )?; } Ok(()) @@ -1408,7 +1364,6 @@ where // Process any attestations that might now be eligible. self.process_attestation_queue()?; - self.process_payload_attestation_queue()?; Ok(self.fc_store.get_current_slot()) } @@ -1495,26 +1450,6 @@ where Ok(()) } - /// Processes and removes from the queue any queued payload attestations which may now be - /// eligible for processing. Payload attestations use `slot + 1 < current_slot` timing. - fn process_payload_attestation_queue(&mut self) -> Result<(), Error> { - let current_slot = self.fc_store.get_current_slot(); - for attestation in - dequeue_payload_attestations(current_slot, &mut self.queued_payload_attestations) - { - for &ptc_index in &attestation.ptc_indices { - self.proto_array.process_payload_attestation( - attestation.block_root, - ptc_index, - attestation.payload_present, - attestation.blob_data_available, - )?; - } - } - - Ok(()) - } - /// Returns `true` if the block is known **and** a descendant of the finalized root. pub fn contains_block(&self, block_root: &Hash256) -> bool { self.proto_array.contains_block(block_root) @@ -1670,11 +1605,6 @@ where &self.queued_attestations } - /// Returns a reference to the currently queued payload attestations. - pub fn queued_payload_attestations(&self) -> &[QueuedPayloadAttestation] { - &self.queued_payload_attestations - } - /// Returns the store's `proposer_boost_root`. pub fn proposer_boost_root(&self) -> Hash256 { self.fc_store.proposer_boost_root() @@ -1759,7 +1689,6 @@ where fc_store, proto_array, queued_attestations: persisted.queued_attestations, - queued_payload_attestations: persisted.queued_payload_attestations, // Will be updated in the following call to `Self::get_head`. forkchoice_update_parameters: ForkchoiceUpdateParameters { head_hash: None, @@ -1800,7 +1729,6 @@ where PersistedForkChoice { proto_array: self.proto_array().as_ssz_container(), queued_attestations: self.queued_attestations().to_vec(), - queued_payload_attestations: self.queued_payload_attestations.clone(), } } @@ -1824,8 +1752,6 @@ pub struct PersistedForkChoice { #[superstruct(only(V29))] pub proto_array: proto_array::core::SszContainerV29, pub queued_attestations: Vec, - #[superstruct(only(V29))] - pub queued_payload_attestations: Vec, } pub type PersistedForkChoice = PersistedForkChoiceV29; @@ -1835,7 +1761,6 @@ impl From for PersistedForkChoiceV29 { Self { proto_array: v28.proto_array_v28.into(), queued_attestations: v28.queued_attestations, - queued_payload_attestations: vec![], } } } diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index f68d3eb71b..452679d7a3 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -117,10 +117,10 @@ pub struct ProtoNode { pub finalized_checkpoint: Checkpoint, #[superstruct(getter(copy))] pub weight: u64, - #[superstruct(getter(copy))] + #[superstruct(only(V17), partial_getter(copy))] #[ssz(with = "four_byte_option_usize")] pub best_child: Option, - #[superstruct(getter(copy))] + #[superstruct(only(V17), partial_getter(copy))] #[ssz(with = "four_byte_option_usize")] pub best_descendant: Option, /// Indicates if an execution node has marked this block as valid. Also contains the execution @@ -614,8 +614,6 @@ impl ProtoArray { justified_checkpoint: block.justified_checkpoint, finalized_checkpoint: block.finalized_checkpoint, weight: 0, - best_child: None, - best_descendant: None, unrealized_justified_checkpoint: block.unrealized_justified_checkpoint, unrealized_finalized_checkpoint: block.unrealized_finalized_checkpoint, parent_payload_status, diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 6c90af1302..cb467f2531 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -997,7 +997,11 @@ impl ProtoArrayForkChoice { /// Returns the `block.execution_status` field, if the block is present. pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { let block = self.get_proto_node(block_root)?; - block.execution_status().ok() + Some( + block + .execution_status() + .unwrap_or_else(|_| ExecutionStatus::irrelevant()), + ) } /// Returns whether the execution payload for a block has been received.