diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index 9b2515f975..c140c431bc 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1665,10 +1665,15 @@ impl ExecutionPendingBlock { .get_indexed_payload_attestation(&state, payload_attestation, &chain.spec) .map_err(|e| BlockError::PerBlockProcessingError(e.into_with_index(i)))?; + let ptc = state + .get_ptc(indexed_payload_attestation.data.slot, &chain.spec) + .map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?; + match fork_choice.on_payload_attestation( current_slot, indexed_payload_attestation, true, + &ptc.0, ) { Ok(()) => Ok(()), // Ignore invalid payload attestations whilst importing from a block. diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index ea17e20f02..63220f0bc6 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -275,7 +275,8 @@ impl<'a, E: EthSpec> From> for QueuedAttestation { #[derive(Clone, PartialEq, Encode, Decode)] pub struct QueuedPayloadAttestation { slot: Slot, - attesting_indices: Vec, + /// Resolved PTC committee positions (not validator indices). + ptc_indices: Vec, block_root: Hash256, payload_present: bool, blob_data_available: bool, @@ -1267,11 +1268,16 @@ where } /// Register a payload attestation with the fork choice DAG. + /// + /// `ptc` is the PTC committee for the attestation's slot: a list of validator indices + /// ordered by committee position. Each attesting validator index is resolved to its + /// position within `ptc` (its `ptc_index`) before being applied to the proto-array. pub fn on_payload_attestation( &mut self, system_time_current_slot: Slot, attestation: &IndexedPayloadAttestation, is_from_block: bool, + ptc: &[usize], ) -> Result<(), Error> { self.update_time(system_time_current_slot)?; @@ -1281,6 +1287,12 @@ where self.validate_on_payload_attestation(attestation, is_from_block)?; + // Resolve validator indices to PTC committee positions. + let ptc_indices: Vec = attestation + .attesting_indices_iter() + .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. @@ -1291,11 +1303,10 @@ where }; if should_process_now { - for validator_index in attestation.attesting_indices_iter() { + for &ptc_index in &ptc_indices { self.proto_array.process_payload_attestation( - *validator_index as usize, attestation.data.beacon_block_root, - processing_slot, + ptc_index, attestation.data.payload_present, attestation.data.blob_data_available, )?; @@ -1304,7 +1315,7 @@ where self.queued_payload_attestations .push(QueuedPayloadAttestation { slot: attestation.data.slot, - attesting_indices: attestation.attesting_indices.iter().copied().collect(), + ptc_indices, block_root: attestation.data.beacon_block_root, payload_present: attestation.data.payload_present, blob_data_available: attestation.data.blob_data_available, @@ -1436,11 +1447,10 @@ where for attestation in dequeue_payload_attestations(current_slot, &mut self.queued_payload_attestations) { - for validator_index in attestation.attesting_indices.iter() { + for &ptc_index in &attestation.ptc_indices { self.proto_array.process_payload_attestation( - *validator_index as usize, attestation.block_root, - current_slot, + ptc_index, attestation.payload_present, attestation.blob_data_available, )?; diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 6ec1c8aeba..44da1af148 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -1027,10 +1027,13 @@ async fn payload_attestation_for_previous_slot_is_accepted_at_next_slot() { 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, true); + .on_payload_attestation(current_slot, &payload_attestation, true, ptc); assert!( result.is_ok(), @@ -1074,10 +1077,12 @@ async fn non_block_payload_attestation_for_previous_slot_is_rejected() { signature: AggregateSignature::empty(), }; + let ptc = &[0_usize]; + let result = chain .canonical_head .fork_choice_write_lock() - .on_payload_attestation(s_plus_1, &payload_attestation, false); + .on_payload_attestation(s_plus_1, &payload_attestation, false, ptc); assert!( matches!( result, diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 8451e2dc80..45aed23b29 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -4,7 +4,6 @@ mod gloas_payload; mod no_votes; mod votes; -use crate::proto_array::PayloadTiebreak; use crate::proto_array_fork_choice::{Block, ExecutionStatus, PayloadStatus, ProtoArrayForkChoice}; use crate::{InvalidationOperation, JustifiedBalances}; use fixed_bytes::FixedBytesExtended; @@ -299,15 +298,14 @@ impl ForkChoiceTestDefinition { Operation::ProcessPayloadAttestation { validator_index, block_root, - attestation_slot, + attestation_slot: _, payload_present, blob_data_available, } => { fork_choice .process_payload_attestation( - validator_index, block_root, - attestation_slot, + validator_index, payload_present, blob_data_available, ) @@ -450,7 +448,7 @@ impl ForkChoiceTestDefinition { expected_status, } => { let actual = fork_choice - .head_payload_status(&head_root) + .head_payload_status::(&head_root) .unwrap_or_else(|| { panic!( "AssertHeadPayloadStatus: head root not found at op index {}", @@ -494,10 +492,11 @@ impl ForkChoiceTestDefinition { op_index ) }); - node_v29.payload_tiebreak = PayloadTiebreak { - is_timely, - is_data_available, - }; + // Set all bits (exceeds any threshold) or clear all bits. + let fill = if is_timely { 0xFF } else { 0x00 }; + node_v29.payload_timeliness_votes.fill(fill); + let fill = if is_data_available { 0xFF } else { 0x00 }; + node_v29.payload_data_availability_votes.fill(fill); } } } 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 01f804c9aa..9a0043a467 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 @@ -134,17 +134,20 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { expected_head: get_root(1), current_slot: Slot::new(0), }); + // PTC votes write to bitfields only, not to full/empty weight. + // Weight is 0 because no CL attestations target this block. ops.push(Operation::AssertPayloadWeights { block_root: get_root(1), - expected_full_weight: 1, - expected_empty_weight: 1, + expected_full_weight: 0, + expected_empty_weight: 0, }); + // With MainnetEthSpec PTC_SIZE=512, 1 bit set out of 256 threshold → not timely → Empty. ops.push(Operation::AssertHeadPayloadStatus { head_root: get_root(1), expected_status: PayloadStatus::Empty, }); - // Flip validator 0 to Empty; probe should now report Empty. + // Flip validator 0 to Empty; both bits now clear. ops.push(Operation::ProcessPayloadAttestation { validator_index: 0, block_root: get_root(1), @@ -162,7 +165,7 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::AssertPayloadWeights { block_root: get_root(1), expected_full_weight: 0, - expected_empty_weight: 2, + expected_empty_weight: 0, }); ops.push(Operation::AssertHeadPayloadStatus { head_root: get_root(1), @@ -214,6 +217,8 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { } } +/// Test that CL attestation weight can flip the head between Full/Empty branches, +/// overriding the tiebreaker. pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDefinition { let mut ops = vec![]; @@ -269,13 +274,11 @@ pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDe current_slot: Slot::new(0), }); - // Validator 0 votes Empty branch -> head flips to 4. - ops.push(Operation::ProcessPayloadAttestation { + // CL attestation to Empty branch (root 4) from validator 0 → head flips to 4. + ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(4), attestation_slot: Slot::new(3), - payload_present: false, - blob_data_available: false, }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), @@ -285,13 +288,11 @@ pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDe current_slot: Slot::new(0), }); - // Latest-message update back to Full branch -> head returns to 3. - ops.push(Operation::ProcessPayloadAttestation { + // CL attestation back to Full branch (root 3) → head returns to 3. + ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(3), attestation_slot: Slot::new(4), - payload_present: true, - blob_data_available: false, }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), @@ -300,11 +301,6 @@ pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDe expected_head: get_root(3), current_slot: Slot::new(0), }); - ops.push(Operation::AssertPayloadWeights { - block_root: get_root(3), - expected_full_weight: 1, - expected_empty_weight: 0, - }); ForkChoiceTestDefinition { finalized_block_slot: Slot::new(0), @@ -317,6 +313,7 @@ pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDe } } +/// CL attestation weight overrides payload preference tiebreaker. pub fn get_gloas_weight_priority_over_payload_preference_test_definition() -> ForkChoiceTestDefinition { let mut ops = vec![]; @@ -359,7 +356,7 @@ pub fn get_gloas_weight_priority_over_payload_preference_test_definition() execution_payload_block_hash: Some(get_hash(4)), }); - // Parent prefers Full on equal branch weights. + // Parent prefers Full on equal branch weights (tiebreaker). ops.push(Operation::SetPayloadTiebreak { block_root: get_root(0), is_timely: true, @@ -373,20 +370,17 @@ pub fn get_gloas_weight_priority_over_payload_preference_test_definition() current_slot: Slot::new(0), }); - // Add two Empty votes to make the Empty branch strictly heavier. - ops.push(Operation::ProcessPayloadAttestation { + // Two CL attestations to the Empty branch make it strictly heavier, + // overriding the Full tiebreaker. + ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(4), attestation_slot: Slot::new(3), - payload_present: false, - blob_data_available: false, }); - ops.push(Operation::ProcessPayloadAttestation { + ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(4), attestation_slot: Slot::new(3), - payload_present: false, - blob_data_available: false, }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), @@ -462,21 +456,13 @@ pub fn get_gloas_parent_empty_when_child_points_to_grandparent_test_definition() } } -/// Test interleaving of blocks, regular attestations, and late-arriving PTC votes. -/// -/// Exercises the spec's `get_weight` rule: FULL/EMPTY virtual nodes at `current_slot - 1` -/// have weight 0, so payload preference is determined solely by the tiebreaker. +/// Test interleaving of blocks, regular attestations, and tiebreaker. /// /// genesis → block 1 (Full) → block 3 /// → block 2 (Empty) → block 4 /// -/// Timeline: -/// 1. Blocks 1 (Full) and 2 (Empty) arrive at slot 1 -/// 2. Regular attestations arrive (equal weight per branch) -/// 3. Child blocks 3 and 4 arrive at slot 2 -/// 4. PTC votes arrive for genesis (2 Full), making genesis prefer Full by weight -/// 5. At current_slot=1 (genesis is current-1), PTC weights are ignored → tiebreaker decides -/// 6. At current_slot=100 (genesis is old), PTC weights apply → Full branch wins +/// With equal CL weight, tiebreaker determines which branch wins. +/// An extra CL attestation can override the tiebreaker. pub fn get_gloas_interleaved_attestations_test_definition() -> ForkChoiceTestDefinition { let mut ops = vec![]; @@ -532,60 +518,46 @@ pub fn get_gloas_interleaved_attestations_test_definition() -> ForkChoiceTestDef execution_payload_block_hash: Some(get_hash(4)), }); - // Step 4: PTC votes arrive for genesis, 2 Full votes from fresh validators. - // Vals 0 and 1 can't be reused because they already have votes at slot 1. - // Vals 2 and 3 target genesis; CL weight on genesis doesn't affect branch comparison. - ops.push(Operation::ProcessPayloadAttestation { - validator_index: 2, - block_root: get_root(0), - attestation_slot: Slot::new(1), - payload_present: true, - blob_data_available: false, - }); - ops.push(Operation::ProcessPayloadAttestation { - validator_index: 3, - block_root: get_root(0), - attestation_slot: Slot::new(1), - payload_present: true, - blob_data_available: false, - }); - - // Set tiebreaker to Empty on genesis. + // Step 4: Set tiebreaker to Empty on genesis → Empty branch wins. ops.push(Operation::SetPayloadTiebreak { block_root: get_root(0), is_timely: false, is_data_available: false, }); - - // Step 5: At current_slot=1, genesis (slot 0) is at current_slot-1. - // Per spec, FULL/EMPTY weights are zeroed → tiebreaker decides. - // Tiebreaker is Empty → Empty branch (block 4) wins. ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), finalized_checkpoint: get_checkpoint(0), - justified_state_balances: vec![1, 1, 1, 1], + justified_state_balances: vec![1, 1], expected_head: get_root(4), current_slot: Slot::new(1), }); - // Step 6: At current_slot=100, genesis (slot 0) is no longer at current_slot-1. - // FULL/EMPTY weights now apply. Genesis has Full > Empty → prefers Full. - // Full branch (block 3) wins despite Empty tiebreaker. + // Step 5: Flip tiebreaker to Full → Full branch wins. + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(0), + is_timely: true, + is_data_available: true, + }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), finalized_checkpoint: get_checkpoint(0), - justified_state_balances: vec![1, 1, 1, 1], + justified_state_balances: vec![1, 1], expected_head: get_root(3), current_slot: Slot::new(100), }); - // Verify the PTC weights are recorded on genesis. - // full = 2 (PTC votes) + 1 (back-propagated from Full child block 1) = 3 - // empty = 0 (PTC votes) + 1 (back-propagated from Empty child block 2) = 1 - ops.push(Operation::AssertPayloadWeights { - block_root: get_root(0), - expected_full_weight: 3, - expected_empty_weight: 1, + // Step 6: Add extra CL weight to Empty branch → overrides Full tiebreaker. + ops.push(Operation::ProcessAttestation { + validator_index: 2, + block_root: get_root(4), + attestation_slot: Slot::new(3), + }); + ops.push(Operation::FindHead { + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + justified_state_balances: vec![1, 1, 1], + expected_head: get_root(4), + current_slot: Slot::new(100), }); ForkChoiceTestDefinition { diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index d0806b9e31..4f89e6084f 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -129,9 +129,16 @@ pub struct ProtoNode { pub full_payload_weight: u64, #[superstruct(only(V29), partial_getter(copy))] pub execution_payload_block_hash: ExecutionBlockHash, - /// Tiebreaker for payload preference when full_payload_weight == empty_payload_weight. - #[superstruct(only(V29), partial_getter(copy))] - pub payload_tiebreak: PayloadTiebreak, + /// PTC timeliness vote bitfield, indexed by PTC committee position. + /// Bit i set means PTC member i voted `payload_present = true`. + /// Tiebreak derived as: `count_ones() > ptc_size / 2`. + #[superstruct(only(V29))] + pub payload_timeliness_votes: Vec, + /// PTC data availability vote bitfield, indexed by PTC committee position. + /// Bit i set means PTC member i voted `blob_data_available = true`. + /// Tiebreak derived as: `count_ones() > ptc_size / 2`. + #[superstruct(only(V29))] + pub payload_data_availability_votes: Vec, } #[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Copy, Clone)] @@ -154,7 +161,6 @@ pub struct NodeDelta { pub delta: i64, pub empty_delta: i64, pub full_delta: i64, - pub payload_tiebreaker: Option, } impl NodeDelta { @@ -192,6 +198,15 @@ impl NodeDelta { Ok(()) } + /// Create a delta that only affects the aggregate `delta` field. + pub fn from_delta(delta: i64) -> Self { + Self { + delta, + empty_delta: 0, + full_delta: 0, + } + } + /// Subtract a balance from the appropriate payload status. pub fn sub_payload_delta( &mut self, @@ -211,21 +226,14 @@ impl NodeDelta { } } +/// Compare NodeDelta with i64 by comparing the aggregate `delta` field. +/// This is used by tests that only care about the total weight delta. impl PartialEq for NodeDelta { fn eq(&self, other: &i64) -> bool { self.delta == *other - && self.empty_delta == 0 - && self.full_delta == 0 - && self.payload_tiebreaker.is_none() } } -#[derive(Clone, Copy, PartialEq, Eq, Debug, Default, Encode, Decode, Serialize, Deserialize)] -pub struct PayloadTiebreak { - pub is_timely: bool, - pub is_data_available: bool, -} - #[derive(PartialEq, Debug, Serialize, Deserialize, Clone)] pub struct ProtoArray { /// Do not attempt to prune the tree unless it has at least this many nodes. Small prunes @@ -363,9 +371,6 @@ impl ProtoArray { apply_delta(node.empty_payload_weight, node_empty_delta, node_index)?; node.full_payload_weight = apply_delta(node.full_payload_weight, node_full_delta, node_index)?; - if let Some(payload_tiebreaker) = node_delta.payload_tiebreaker { - node.payload_tiebreak = payload_tiebreaker; - } } // Update the parent delta (if any). @@ -535,7 +540,8 @@ impl ProtoArray { empty_payload_weight: 0, full_payload_weight: 0, execution_payload_block_hash, - payload_tiebreak: PayloadTiebreak::default(), + payload_timeliness_votes: empty_ptc_bitfield(E::ptc_size()), + payload_data_availability_votes: empty_ptc_bitfield(E::ptc_size()), }) }; @@ -593,10 +599,10 @@ impl ProtoArray { let v29 = node .as_v29_mut() .map_err(|_| Error::InvalidNodeVariant { block_root })?; - v29.payload_tiebreak = PayloadTiebreak { - is_timely: true, - is_data_available: true, - }; + // A valid execution payload means the payload is timely and data is available. + // Set all bits to ensure the threshold is met regardless of PTC size. + v29.payload_timeliness_votes.fill(0xFF); + v29.payload_data_availability_votes.fill(0xFF); Ok(()) } @@ -1062,72 +1068,79 @@ impl ProtoArray { ); let no_change = (parent.best_child(), parent.best_descendant()); - let (new_best_child, new_best_descendant) = if let Some(best_child_index) = - parent.best_child() - { - if best_child_index == child_index && !child_leads_to_viable_head { - // If the child is already the best-child of the parent but it's not viable for - // the head, remove it. - change_to_none - } else if best_child_index == child_index { - // If the child is the best-child already, set it again to ensure that the - // best-descendant of the parent is updated. - change_to_child - } else { - let best_child = self - .nodes - .get(best_child_index) - .ok_or(Error::InvalidBestDescendant(best_child_index))?; - - let best_child_leads_to_viable_head = self.node_leads_to_viable_head::( - best_child, - current_slot, - best_justified_checkpoint, - best_finalized_checkpoint, - )?; - - if child_leads_to_viable_head && !best_child_leads_to_viable_head { - // The child leads to a viable head, but the current best-child doesn't. + let (new_best_child, new_best_descendant) = + if let Some(best_child_index) = parent.best_child() { + if best_child_index == child_index && !child_leads_to_viable_head { + // If the child is already the best-child of the parent but it's not viable for + // the head, remove it. + change_to_none + } else if best_child_index == child_index { + // If the child is the best-child already, set it again to ensure that the + // best-descendant of the parent is updated. change_to_child - } else if !child_leads_to_viable_head && best_child_leads_to_viable_head { - // The best child leads to a viable head, but the child doesn't. - no_change - } else if child.weight() > best_child.weight() { - // Weight is the primary ordering criterion. - change_to_child - } else if child.weight() < best_child.weight() { - no_change } else { - // Equal weights: for V29 parents, prefer the child whose - // parent_payload_status matches the parent's payload preference - // (full vs empty). This corresponds to the spec's - // `get_payload_status_tiebreaker` ordering in `get_head`. - let child_matches = - child_matches_parent_payload_preference(parent, child, current_slot); - let best_child_matches = - child_matches_parent_payload_preference(parent, best_child, current_slot); + let best_child = self + .nodes + .get(best_child_index) + .ok_or(Error::InvalidBestDescendant(best_child_index))?; - if child_matches && !best_child_matches { - // Child extends the preferred payload chain, best_child doesn't. + let best_child_leads_to_viable_head = self.node_leads_to_viable_head::( + best_child, + current_slot, + best_justified_checkpoint, + best_finalized_checkpoint, + )?; + + if child_leads_to_viable_head && !best_child_leads_to_viable_head { + // The child leads to a viable head, but the current best-child doesn't. change_to_child - } else if !child_matches && best_child_matches { - // Best child extends the preferred payload chain, child doesn't. + } else if !child_leads_to_viable_head && best_child_leads_to_viable_head { + // The best child leads to a viable head, but the child doesn't. no_change - } else if *child.root() >= *best_child.root() { - // Final tie-breaker: both match or both don't, break by root. + } else if child.weight() > best_child.weight() { + // Weight is the primary ordering criterion. change_to_child + } else if child.weight() < best_child.weight() { + no_change } else { - no_change + // Equal weights: for V29 parents, prefer the child whose + // parent_payload_status matches the parent's payload preference + // (full vs empty). This corresponds to the spec's + // `get_payload_status_tiebreaker` ordering in `get_head`. + let child_matches = child_matches_parent_payload_preference( + parent, + child, + current_slot, + E::ptc_size(), + ); + let best_child_matches = child_matches_parent_payload_preference( + parent, + best_child, + current_slot, + E::ptc_size(), + ); + + if child_matches && !best_child_matches { + // Child extends the preferred payload chain, best_child doesn't. + change_to_child + } else if !child_matches && best_child_matches { + // Best child extends the preferred payload chain, child doesn't. + no_change + } else if *child.root() >= *best_child.root() { + // Final tie-breaker: both match or both don't, break by root. + change_to_child + } else { + no_change + } } } - } - } else if child_leads_to_viable_head { - // There is no current best-child and the child is viable. - change_to_child - } else { - // There is no current best-child but the child is not viable. - no_change - }; + } else if child_leads_to_viable_head { + // There is no current best-child and the child is viable. + change_to_child + } else { + // There is no current best-child but the child is not viable. + no_change + }; let parent = self .nodes @@ -1393,6 +1406,7 @@ fn child_matches_parent_payload_preference( parent: &ProtoNode, child: &ProtoNode, current_slot: Slot, + ptc_size: usize, ) -> bool { let (Ok(parent_v29), Ok(child_v29)) = (parent.as_v29(), child.as_v29()) else { return true; @@ -1410,7 +1424,8 @@ fn child_matches_parent_payload_preference( false } else { // Equal weights (or current-slot parent): tiebreaker per spec. - parent_v29.payload_tiebreak.is_timely && parent_v29.payload_tiebreak.is_data_available + is_payload_timely(&parent_v29.payload_timeliness_votes, ptc_size) + && is_payload_data_available(&parent_v29.payload_data_availability_votes, ptc_size) }; if prefers_full { child_v29.parent_payload_status == PayloadStatus::Full @@ -1419,6 +1434,26 @@ fn child_matches_parent_payload_preference( } } +/// Count the number of set bits in a byte-slice bitfield. +pub fn count_set_bits(bitfield: &[u8]) -> usize { + bitfield.iter().map(|b| b.count_ones() as usize).sum() +} + +/// Create a zero-initialized bitfield for the given PTC size. +pub fn empty_ptc_bitfield(ptc_size: usize) -> Vec { + vec![0u8; ptc_size.div_ceil(8)] +} + +/// Derive `is_payload_timely` from the timeliness vote bitfield. +pub fn is_payload_timely(timeliness_votes: &[u8], ptc_size: usize) -> bool { + count_set_bits(timeliness_votes) > ptc_size / 2 +} + +/// Derive `is_payload_data_available` from the data availability vote bitfield. +pub fn is_payload_data_available(availability_votes: &[u8], ptc_size: usize) -> bool { + count_set_bits(availability_votes) > ptc_size / 2 +} + /// A helper method to calculate the proposer boost based on the given `justified_balances`. /// /// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#get_latest_attesting_balance diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 66f3627483..021d62e63f 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -3,7 +3,7 @@ use crate::{ error::Error, proto_array::{ InvalidationOperation, Iter, NodeDelta, ProposerBoost, ProtoArray, ProtoNode, - calculate_committee_fraction, + calculate_committee_fraction, is_payload_data_available, is_payload_timely, }, ssz_container::SszContainer, }; @@ -23,8 +23,6 @@ use types::{ pub const DEFAULT_PRUNE_THRESHOLD: usize = 256; #[derive(Default, PartialEq, Clone, Encode, Decode)] -// FIXME(sproul): the "next" naming here is a bit odd -// FIXME(sproul): version this type? pub struct VoteTracker { current_root: Hash256, next_root: Hash256, @@ -32,16 +30,12 @@ pub struct VoteTracker { next_slot: Slot, current_payload_present: bool, next_payload_present: bool, - current_blob_data_available: bool, - next_blob_data_available: bool, } -// FIXME(sproul): version this type pub struct LatestMessage { pub slot: Slot, pub root: Hash256, pub payload_present: bool, - pub blob_data_available: bool, } /// Represents the verification status of an execution payload pre-Gloas. @@ -535,28 +529,53 @@ impl ProtoArrayForkChoice { if attestation_slot > vote.next_slot || *vote == VoteTracker::default() { vote.next_root = block_root; vote.next_slot = attestation_slot; - vote.next_payload_present = false; - vote.next_blob_data_available = false; } Ok(()) } + /// Process a PTC vote by setting the appropriate bits on the target block's V29 node. + /// + /// `ptc_index` is the voter's position in the PTC committee (resolved by the caller). + /// This writes directly to the node's bitfields, bypassing the delta pipeline. pub fn process_payload_attestation( &mut self, - validator_index: usize, block_root: Hash256, - attestation_slot: Slot, + ptc_index: usize, payload_present: bool, blob_data_available: bool, ) -> Result<(), String> { - let vote = self.votes.get_mut(validator_index); + let node_index = self + .proto_array + .indices + .get(&block_root) + .copied() + .ok_or_else(|| { + format!("process_payload_attestation: unknown block root {block_root:?}") + })?; + let node = self.proto_array.nodes.get_mut(node_index).ok_or_else(|| { + format!("process_payload_attestation: invalid node index {node_index}") + })?; + let v29 = node + .as_v29_mut() + .map_err(|_| format!("process_payload_attestation: node {block_root:?} is not V29"))?; - if attestation_slot > vote.next_slot || *vote == VoteTracker::default() { - vote.next_root = block_root; - vote.next_slot = attestation_slot; - vote.next_payload_present = payload_present; - vote.next_blob_data_available = blob_data_available; + let byte_index = ptc_index / 8; + let bit_mask = 1u8 << (ptc_index % 8); + + if let Some(byte) = v29.payload_timeliness_votes.get_mut(byte_index) { + if payload_present { + *byte |= bit_mask; + } else { + *byte &= !bit_mask; + } + } + if let Some(byte) = v29.payload_data_availability_votes.get_mut(byte_index) { + if blob_data_available { + *byte |= bit_mask; + } else { + *byte &= !bit_mask; + } } Ok(()) @@ -978,14 +997,16 @@ impl ProtoArrayForkChoice { /// On ties, consult the node's runtime `payload_tiebreak`: prefer `Full` only when timely and /// data is available, otherwise `Empty`. /// Returns `Empty` otherwise. Returns `None` for V17 nodes. - pub fn head_payload_status(&self, head_root: &Hash256) -> Option { + pub fn head_payload_status(&self, head_root: &Hash256) -> Option { let node = self.get_proto_node(head_root)?; let v29 = node.as_v29().ok()?; if v29.full_payload_weight > v29.empty_payload_weight { Some(PayloadStatus::Full) } else if v29.empty_payload_weight > v29.full_payload_weight { Some(PayloadStatus::Empty) - } else if v29.payload_tiebreak.is_timely && v29.payload_tiebreak.is_data_available { + } else if is_payload_timely(&v29.payload_timeliness_votes, E::ptc_size()) + && is_payload_data_available(&v29.payload_data_availability_votes, E::ptc_size()) + { Some(PayloadStatus::Full) } else { Some(PayloadStatus::Empty) @@ -1019,7 +1040,6 @@ impl ProtoArrayForkChoice { root: vote.next_root, slot: vote.next_slot, payload_present: vote.next_payload_present, - blob_data_available: vote.next_blob_data_available, }) } } else { @@ -1105,17 +1125,6 @@ fn compute_deltas( new_balances: &[u64], equivocating_indices: &BTreeSet, ) -> Result, Error> { - let merge_payload_tiebreaker = - |delta: &mut NodeDelta, incoming: crate::proto_array::PayloadTiebreak| { - delta.payload_tiebreaker = Some(match delta.payload_tiebreaker { - Some(existing) => crate::proto_array::PayloadTiebreak { - is_timely: existing.is_timely || incoming.is_timely, - is_data_available: existing.is_data_available || incoming.is_data_available, - }, - None => incoming, - }); - }; - let block_slot = |index: usize| -> Result { node_slots .get(index) @@ -1128,7 +1137,6 @@ fn compute_deltas( delta: 0, empty_delta: 0, full_delta: 0, - payload_tiebreaker: None, }; indices.len() ]; @@ -1175,7 +1183,6 @@ fn compute_deltas( vote.current_root = Hash256::zero(); vote.current_slot = Slot::new(0); vote.current_payload_present = false; - vote.current_blob_data_available = false; } // We've handled this slashed validator, continue without applying an ordinary delta. continue; @@ -1233,21 +1240,11 @@ fn compute_deltas( block_slot(next_delta_index)?, ); node_delta.add_payload_delta(status, new_balance, next_delta_index)?; - if status != PayloadStatus::Pending { - merge_payload_tiebreaker( - node_delta, - crate::proto_array::PayloadTiebreak { - is_timely: vote.next_payload_present, - is_data_available: vote.next_blob_data_available, - }, - ); - } } vote.current_root = vote.next_root; vote.current_slot = vote.next_slot; vote.current_payload_present = vote.next_payload_present; - vote.current_blob_data_available = vote.next_blob_data_available; } } @@ -1600,8 +1597,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); old_balances.push(0); new_balances.push(0); @@ -1657,8 +1652,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); old_balances.push(BALANCE); new_balances.push(BALANCE); @@ -1721,8 +1714,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); old_balances.push(BALANCE); new_balances.push(BALANCE); @@ -1780,8 +1771,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); old_balances.push(BALANCE); new_balances.push(BALANCE); @@ -1850,8 +1839,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); // One validator moves their vote from the block to something outside the tree. @@ -1862,8 +1849,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); let deltas = compute_deltas( @@ -1914,8 +1899,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); old_balances.push(OLD_BALANCE); new_balances.push(NEW_BALANCE); @@ -1989,8 +1972,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); } @@ -2051,8 +2032,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); } @@ -2111,8 +2090,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: false, - current_blob_data_available: false, - next_blob_data_available: false, }); } @@ -2176,8 +2153,6 @@ mod test_compute_deltas { next_slot: Slot::new(1), current_payload_present: false, next_payload_present: true, - current_blob_data_available: false, - next_blob_data_available: false, }]); let deltas = compute_deltas( @@ -2210,8 +2185,6 @@ mod test_compute_deltas { next_slot: Slot::new(0), current_payload_present: false, next_payload_present: true, - current_blob_data_available: false, - next_blob_data_available: false, }]); let deltas = compute_deltas(