From dda45d3a06d134550fae78bfc9384dd4c482e619 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 1 Jun 2026 11:07:54 +0300 Subject: [PATCH] Update should_build_on_full to match the updated spec plus new test case --- .../src/block_production/gloas.rs | 2 +- consensus/fork_choice/src/fork_choice.rs | 3 +- .../src/fork_choice_test_definition.rs | 33 +++++++ .../gloas_payload.rs | 91 +++++++++++++++++++ consensus/proto_array/src/proto_array.rs | 8 ++ .../src/proto_array_fork_choice.rs | 3 +- 6 files changed, 137 insertions(+), 3 deletions(-) diff --git a/beacon_node/beacon_chain/src/block_production/gloas.rs b/beacon_node/beacon_chain/src/block_production/gloas.rs index 82dad6f6ad..90fc60524a 100644 --- a/beacon_node/beacon_chain/src/block_production/gloas.rs +++ b/beacon_node/beacon_chain/src/block_production/gloas.rs @@ -163,7 +163,7 @@ impl BeaconChain { let should_build_on_full = self .canonical_head .fork_choice_read_lock() - .should_build_on_full(&parent_root, parent_payload_status) + .should_build_on_full(&parent_root, parent_payload_status, produce_at_slot) .map_err(|e| { BlockProductionError::BeaconChain(Box::new(BeaconChainError::ForkChoiceError(e))) })?; diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 2de8ce7d81..53f2980059 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1542,9 +1542,10 @@ where &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + proposal_slot: Slot, ) -> Result> { self.proto_array - .should_build_on_full::(block_root, parent_payload_status) + .should_build_on_full::(block_root, parent_payload_status, proposal_slot) .map_err(Error::ProtoArrayStringError) } diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 43b76ec7cb..1948d73c63 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -124,6 +124,15 @@ pub enum Operation { #[serde(default)] proposer_boost_root: Option, }, + /// Assert the result of `should_build_on_full` for the parent `block_root`, where + /// `parent_payload_status` is the status the proposer would build on and `proposal_slot` + /// is the slot being proposed. + AssertShouldBuildOnFull { + block_root: Hash256, + parent_payload_status: PayloadStatus, + proposal_slot: Slot, + expected: bool, + }, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -606,6 +615,30 @@ impl ForkChoiceTestDefinition { op_index ); } + Operation::AssertShouldBuildOnFull { + block_root, + parent_payload_status, + proposal_slot, + expected, + } => { + let actual = fork_choice + .should_build_on_full::( + &block_root, + parent_payload_status, + proposal_slot, + ) + .unwrap_or_else(|e| { + panic!( + "should_build_on_full op at index {} returned error: {}", + op_index, e + ) + }); + assert_eq!( + actual, expected, + "should_build_on_full mismatch at op index {}", + op_index + ); + } } } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs index ac4f8992c4..8c48284bf1 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs @@ -971,6 +971,91 @@ pub fn get_gloas_proposer_boost_flips_ancestor_test_definition() -> ForkChoiceTe } } +/// Exercises the slot check in `should_build_on_full`: for a parent from an earlier slot the +/// function returns `true` without consulting PTC data-availability votes; only for a parent +/// from the immediately preceding slot are those votes consulted. +pub fn get_gloas_should_build_on_full_test_definition() -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Block 1 at slot 1, child of genesis. + ops.push(Operation::ProcessBlock { + slot: Slot::new(1), + root: get_root(1), + parent_root: get_root(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(0)), + execution_payload_block_hash: Some(get_hash(1)), + }); + + // PTC has voted the payload data as *unavailable*: `is_timely` sets `payload_received` so the + // votes are consulted, while clearing the data-availability bits gives the "false" votes an + // absolute majority. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(1), + is_timely: true, + is_data_available: false, + }); + + // An `Empty` parent is never built on full, regardless of slot. The `Empty` check + // precedes the slot check, so neither the previous-slot case (block slot 1, proposal + // slot 2) nor an earlier-slot case (proposal slot 3) builds on full. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Empty, + proposal_slot: Slot::new(2), + expected: false, + }); + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Empty, + proposal_slot: Slot::new(3), + expected: false, + }); + + // `Full` parent from the immediately preceding slot (block slot 1, proposal slot 2): the PTC + // votes are consulted, and since data is unavailable the proposer does not build on full. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(2), + expected: false, + }); + + // `Full` parent from an *earlier* slot (block slot 1, proposal slot 3): the slot check + // short-circuits to `true` without consulting the (unavailable) PTC votes. + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(3), + expected: true, + }); + + // Flip the PTC view to *available* and re-check the previous-slot case: the votes now permit + // building on full, confirming that case actually consults them. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(1), + is_timely: true, + is_data_available: true, + }); + ops.push(Operation::AssertShouldBuildOnFull { + block_root: get_root(1), + parent_payload_status: PayloadStatus::Full, + proposal_slot: Slot::new(2), + expected: true, + }); + + ForkChoiceTestDefinition { + finalized_block_slot: Slot::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + operations: ops, + execution_payload_parent_hash: Some(get_hash(42)), + execution_payload_block_hash: Some(get_hash(0)), + spec: Some(gloas_spec()), + } +} + #[cfg(test)] mod tests { use super::*; @@ -1160,6 +1245,12 @@ mod tests { test.run(); } + #[test] + fn should_build_on_full_slot_check() { + let test = get_gloas_should_build_on_full_test_definition(); + test.run(); + } + /// Test that execution payload invalidation propagates across the V17→V29 fork /// boundary: after invalidating a V17 parent, head must not select any descendant. /// diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 48efa480b0..d95e01aceb 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1512,10 +1512,13 @@ impl ProtoArray { /// Called by the proposer to decide whether to build on the full or empty /// parent pending node. Returns false if the PTC has voted the data as unavailable. + /// For a parent from an earlier slot the `Empty` or `Full` node has already been resolved + /// by attestation weight in `get_head`. pub fn should_build_on_full( &self, fc_node: &IndexedForkChoiceNode, proto_node: &ProtoNode, + proposal_slot: Slot, ) -> Result { if fc_node.payload_status == PayloadStatus::Pending { return Err(Error::InvalidPayloadStatus { @@ -1527,6 +1530,11 @@ impl ProtoArray { if fc_node.payload_status == PayloadStatus::Empty { return Ok(false); } + + if proto_node.slot().saturating_add(1u64) != proposal_slot { + return Ok(true); + } + // Check that false votes have not achieved an absolute majority. This allows the payload to be // considered available when either a majority have voted true or not enough votes have // been cast either way. diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 96d2302266..a798c3b160 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -1015,6 +1015,7 @@ impl ProtoArrayForkChoice { &self, block_root: &Hash256, parent_payload_status: PayloadStatus, + proposal_slot: Slot, ) -> Result { let block_index = self .proto_array @@ -1032,7 +1033,7 @@ impl ProtoArrayForkChoice { payload_status: parent_payload_status, }; self.proto_array - .should_build_on_full::(&fc_node, proto_node) + .should_build_on_full::(&fc_node, proto_node, proposal_slot) .map_err(|e| format!("{e:?}")) }