diff --git a/Cargo.lock b/Cargo.lock index 5a8e76a8a8..555eb019d9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3583,6 +3583,7 @@ name = "fork_choice" version = "0.1.0" dependencies = [ "beacon_chain", + "bls", "ethereum_ssz", "ethereum_ssz_derive", "fixed_bytes", diff --git a/consensus/fork_choice/Cargo.toml b/consensus/fork_choice/Cargo.toml index a07aa38aa5..df47a5c9d1 100644 --- a/consensus/fork_choice/Cargo.toml +++ b/consensus/fork_choice/Cargo.toml @@ -19,5 +19,6 @@ types = { workspace = true } [dev-dependencies] beacon_chain = { workspace = true } +bls = { workspace = true } store = { workspace = true } tokio = { workspace = true } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 77442a62f5..ae08b3675f 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -249,7 +249,6 @@ pub struct QueuedAttestation { attesting_indices: Vec, block_root: Hash256, target_epoch: Epoch, - payload_present: bool, } impl<'a, E: EthSpec> From> for QueuedAttestation { @@ -259,11 +258,22 @@ impl<'a, E: EthSpec> From> for QueuedAttestation { attesting_indices: a.attesting_indices_to_vec(), block_root: a.data().beacon_block_root, target_epoch: a.data().target.epoch, - payload_present: a.data().index == 1, } } } +/// Used for queuing payload attestations (PTC votes) from the current slot. +/// Payload attestations have different dequeue timing than regular attestations: +/// non-block payload attestations need an extra slot of delay (slot + 1 < current_slot). +#[derive(Clone, PartialEq, Encode, Decode)] +pub struct QueuedPayloadAttestation { + slot: Slot, + attesting_indices: Vec, + block_root: Hash256, + payload_present: bool, + blob_data_available: bool, +} + /// Returns all values in `self.queued_attestations` that have a slot that is earlier than the /// current slot. Also removes those values from `self.queued_attestations`. fn dequeue_attestations( @@ -285,6 +295,22 @@ 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: /// @@ -329,6 +355,9 @@ 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, @@ -343,6 +372,7 @@ 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 } } @@ -414,6 +444,7 @@ 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, @@ -1120,7 +1151,7 @@ where }); } - if indexed_payload_attestation.data.slot == block.slot + if self.fc_store.get_current_slot() == block.slot && indexed_payload_attestation.data.payload_present { return Err(InvalidAttestation::PayloadAttestationDuringSameSlot { slot: block.slot }); @@ -1177,12 +1208,10 @@ where if attestation.data().slot < self.fc_store.get_current_slot() { for validator_index in attestation.attesting_indices_iter() { - let payload_present = attestation.data().index == 1; self.proto_array.process_attestation( *validator_index as usize, attestation.data().beacon_block_root, attestation.data().slot, - payload_present, )?; } } else { @@ -1214,23 +1243,33 @@ where self.validate_on_payload_attestation(attestation, is_from_block)?; - if attestation.data.slot < self.fc_store.get_current_slot() { + 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 non-block 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 + 1_u64 < processing_slot, + }; + + if should_process_now { for validator_index in attestation.attesting_indices_iter() { - self.proto_array.process_attestation( + self.proto_array.process_payload_attestation( *validator_index as usize, attestation.data.beacon_block_root, - attestation.data.slot, + processing_slot, attestation.data.payload_present, + attestation.data.blob_data_available, )?; } } else { - self.queued_attestations.push(QueuedAttestation { - slot: attestation.data.slot, - attesting_indices: attestation.attesting_indices.iter().copied().collect(), - block_root: attestation.data.beacon_block_root, - target_epoch: attestation.data.slot.epoch(E::slots_per_epoch()), - payload_present: attestation.data.payload_present, - }); + self.queued_payload_attestations + .push(QueuedPayloadAttestation { + slot: attestation.data.slot, + attesting_indices: attestation.attesting_indices.iter().copied().collect(), + block_root: attestation.data.beacon_block_root, + payload_present: attestation.data.payload_present, + blob_data_available: attestation.data.blob_data_available, + }); } Ok(()) @@ -1265,6 +1304,7 @@ 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()) } @@ -1339,12 +1379,31 @@ where &mut self.queued_attestations, ) { for validator_index in attestation.attesting_indices.iter() { - // FIXME(sproul): backwards compat/fork abstraction self.proto_array.process_attestation( *validator_index as usize, attestation.block_root, attestation.slot, + )?; + } + } + + 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 validator_index in attestation.attesting_indices.iter() { + self.proto_array.process_payload_attestation( + *validator_index as usize, + attestation.block_root, + current_slot, attestation.payload_present, + attestation.blob_data_available, )?; } } @@ -1507,6 +1566,11 @@ 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() @@ -1591,6 +1655,7 @@ 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, @@ -1633,6 +1698,7 @@ where .proto_array() .as_ssz_container(self.justified_checkpoint(), self.finalized_checkpoint()), queued_attestations: self.queued_attestations().to_vec(), + queued_payload_attestations: self.queued_payload_attestations.clone(), } } @@ -1658,6 +1724,8 @@ 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; @@ -1682,6 +1750,7 @@ impl From for PersistedForkChoiceV29 { Self { proto_array: v28.proto_array_v28.into(), queued_attestations: v28.queued_attestations, + queued_payload_attestations: vec![], } } } @@ -1734,7 +1803,6 @@ mod tests { attesting_indices: vec![], block_root: Hash256::zero(), target_epoch: Epoch::new(0), - payload_present: false, }) .collect() } diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 87438f2f85..6091de6fdd 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -6,7 +6,7 @@ pub use crate::fork_choice::{ AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, PersistedForkChoice, PersistedForkChoiceV17, PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, - ResetPayloadStatuses, + QueuedPayloadAttestation, ResetPayloadStatuses, }; pub use fork_choice_store::ForkChoiceStore; pub use proto_array::{ diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 86ef0e2f90..9887e2eb92 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -7,9 +7,11 @@ use beacon_chain::{ BeaconChain, BeaconChainError, BeaconForkChoiceStore, ChainConfig, ForkChoiceError, StateSkipConfig, WhenSlotSkipped, }; +use bls::AggregateSignature; use fixed_bytes::FixedBytesExtended; use fork_choice::{ - ForkChoiceStore, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, QueuedAttestation, + AttestationFromBlock, ForkChoiceStore, InvalidAttestation, InvalidBlock, + PayloadVerificationStatus, QueuedAttestation, QueuedPayloadAttestation, }; use state_processing::state_advance::complete_state_advance; use std::fmt; @@ -19,8 +21,8 @@ use store::MemoryStore; use types::SingleAttestation; use types::{ BeaconBlockRef, BeaconState, ChainSpec, Checkpoint, Epoch, EthSpec, ForkName, Hash256, - IndexedAttestation, MainnetEthSpec, RelativeEpoch, SignedBeaconBlock, Slot, SubnetId, - test_utils::generate_deterministic_keypair, + IndexedAttestation, IndexedPayloadAttestation, MainnetEthSpec, PayloadAttestationData, + RelativeEpoch, SignedBeaconBlock, Slot, SubnetId, test_utils::generate_deterministic_keypair, }; pub type E = MainnetEthSpec; @@ -154,6 +156,28 @@ impl ForkChoiceTest { self } + /// Inspect the queued payload attestations in fork choice. + #[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(); @@ -953,6 +977,119 @@ async fn invalid_attestation_payload_during_same_slot() { .await; } +/// A payload attestation for block A at slot S should be accepted when processed at slot S+1. +#[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(), + }; + + let result = chain + .canonical_head + .fork_choice_write_lock() + .on_payload_attestation( + current_slot, + &payload_attestation, + AttestationFromBlock::True, + ); + + assert!( + result.is_ok(), + "payload attestation at slot S should be accepted at S+1, got: {:?}", + result + ); + + let latest_message = chain + .canonical_head + .fork_choice_read_lock() + .latest_message(0) + .expect("latest message should exist"); + assert_eq!(latest_message.slot, current_slot); + assert!(latest_message.payload_present); +} + +/// Non-block payload attestations at slot S+1 for data.slot S are delayed; they are not applied +/// until a later slot. +#[tokio::test] +async fn non_block_payload_attestation_at_next_slot_is_delayed() { + 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 s_plus_1 = block_a.slot().saturating_add(1_u64); + let s_plus_2 = block_a.slot().saturating_add(2_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(), + }; + + let result = chain + .canonical_head + .fork_choice_write_lock() + .on_payload_attestation(s_plus_1, &payload_attestation, AttestationFromBlock::False); + assert!( + result.is_ok(), + "payload attestation should be accepted for queueing" + ); + + // Vote should not be applied yet; message remains unset. + let latest_before = chain + .canonical_head + .fork_choice_read_lock() + .latest_message(0); + assert!( + latest_before.is_none(), + "non-block payload attestation at S+1 should not apply immediately" + ); + + // Advance fork choice time to S+2, queue should now be processed. + chain + .canonical_head + .fork_choice_write_lock() + .update_time(s_plus_2) + .expect("update_time should succeed"); + + let latest_after = chain + .canonical_head + .fork_choice_read_lock() + .latest_message(0) + .expect("latest message should exist after delay"); + assert_eq!(latest_after.slot, s_plus_2); + assert!(latest_after.payload_present); +} + /// Specification v0.12.1: /// /// assert target.root == get_ancestor(store, attestation.data.beacon_block_root, target_slot) diff --git a/consensus/proto_array/src/bin.rs b/consensus/proto_array/src/bin.rs index e1d307affb..c5df3f17e4 100644 --- a/consensus/proto_array/src/bin.rs +++ b/consensus/proto_array/src/bin.rs @@ -18,6 +18,14 @@ 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) { diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index ec4227584a..f88cf06349 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -56,8 +56,14 @@ pub enum Operation { validator_index: usize, block_root: Hash256, attestation_slot: Slot, - #[serde(default)] + }, + ProcessPayloadAttestation { + validator_index: usize, + block_root: Hash256, + attestation_slot: Slot, payload_present: bool, + #[serde(default)] + blob_data_available: bool, }, Prune { finalized_root: Hash256, @@ -277,18 +283,35 @@ impl ForkChoiceTestDefinition { validator_index, block_root, attestation_slot, - payload_present, } => { fork_choice - .process_attestation( + .process_attestation(validator_index, block_root, attestation_slot) + .unwrap_or_else(|_| { + panic!( + "process_attestation op at index {} returned error", + op_index + ) + }); + check_bytes_round_trip(&fork_choice); + } + Operation::ProcessPayloadAttestation { + validator_index, + block_root, + attestation_slot, + payload_present, + blob_data_available, + } => { + fork_choice + .process_payload_attestation( validator_index, block_root, attestation_slot, payload_present, + blob_data_available, ) .unwrap_or_else(|_| { panic!( - "process_attestation op at index {} returned error", + "process_payload_attestation op at index {} returned error", op_index ) }); diff --git a/consensus/proto_array/src/fork_choice_test_definition/execution_status.rs b/consensus/proto_array/src/fork_choice_test_definition/execution_status.rs index 93c97d09db..318407f598 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/execution_status.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/execution_status.rs @@ -106,7 +106,6 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -149,7 +148,6 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(2), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is 2 since 1 and 2 both have a vote @@ -254,7 +252,6 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(3), attestation_slot: Slot::new(3), - payload_present: false, }); // Ensure that the head is still 2 @@ -357,7 +354,6 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(1), attestation_slot: Slot::new(3), - payload_present: false, }); // Ensure that the head has switched back to 1 @@ -521,7 +517,6 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -564,7 +559,6 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(2), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is 2 since 1 and 2 both have a vote @@ -669,7 +663,6 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(3), attestation_slot: Slot::new(3), - payload_present: false, }); // Move validator #1 vote from 2 to 3 @@ -683,7 +676,6 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(3), attestation_slot: Slot::new(3), - payload_present: false, }); // Ensure that the head is now 3. @@ -898,7 +890,6 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -941,7 +932,6 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(1), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is 1. diff --git a/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs b/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs index ee55ea649f..88665a22ad 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/ffg_updates.rs @@ -312,7 +312,6 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(0), - payload_present: false, }); // Ensure that if we start at 0 we find 9 (just: 0, fin: 0). @@ -376,7 +375,6 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(2), attestation_slot: Slot::new(0), - payload_present: false, }); // Ensure that if we start at 0 we find 10 (just: 0, fin: 0). 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 b6568106e3..7579b01636 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 @@ -109,18 +109,21 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { execution_payload_block_hash: Some(get_hash(1)), }); - // One Full and one Empty vote for the same head block: tie should probe as Full. - ops.push(Operation::ProcessAttestation { + // One Full and one Empty vote for the same head block: tie probes via runtime tiebreak, + // which defaults to Empty unless timely+data-available evidence is set. + ops.push(Operation::ProcessPayloadAttestation { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(2), payload_present: true, + blob_data_available: false, }); - ops.push(Operation::ProcessAttestation { + ops.push(Operation::ProcessPayloadAttestation { validator_index: 1, block_root: get_root(1), attestation_slot: Slot::new(2), payload_present: false, + blob_data_available: false, }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), @@ -135,15 +138,16 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { }); ops.push(Operation::AssertHeadPayloadStatus { head_root: get_root(1), - expected_status: PayloadStatus::Full, + expected_status: PayloadStatus::Empty, }); // Flip validator 0 to Empty; probe should now report Empty. - ops.push(Operation::ProcessAttestation { + ops.push(Operation::ProcessPayloadAttestation { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(3), payload_present: false, + blob_data_available: false, }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), @@ -171,11 +175,12 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { execution_payload_parent_hash: Some(get_hash(1)), execution_payload_block_hash: Some(get_hash(5)), }); - ops.push(Operation::ProcessAttestation { + ops.push(Operation::ProcessPayloadAttestation { validator_index: 2, block_root: get_root(5), attestation_slot: Slot::new(3), payload_present: true, + blob_data_available: false, }); ops.push(Operation::FindHead { justified_checkpoint: get_checkpoint(0), @@ -190,7 +195,250 @@ pub fn get_gloas_payload_probe_test_definition() -> ForkChoiceTestDefinition { }); ops.push(Operation::AssertHeadPayloadStatus { head_root: get_root(5), - expected_status: PayloadStatus::Full, + expected_status: PayloadStatus::Empty, + }); + + 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()), + } +} + +pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Competing branches with distinct payload ancestry (Full vs Empty from 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)), + }); + ops.push(Operation::ProcessBlock { + slot: Slot::new(1), + root: get_root(2), + parent_root: get_root(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(99)), + execution_payload_block_hash: Some(get_hash(2)), + }); + ops.push(Operation::ProcessBlock { + slot: Slot::new(2), + root: get_root(3), + parent_root: get_root(1), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(1)), + execution_payload_block_hash: Some(get_hash(3)), + }); + ops.push(Operation::ProcessBlock { + slot: Slot::new(2), + root: get_root(4), + parent_root: get_root(2), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(100)), + execution_payload_block_hash: Some(get_hash(4)), + }); + + // Equal branch weights: tiebreak FULL picks branch rooted at 3. + 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], + expected_head: get_root(3), + }); + + // Validator 0 votes Empty branch -> head flips to 4. + ops.push(Operation::ProcessPayloadAttestation { + 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), + finalized_checkpoint: get_checkpoint(0), + justified_state_balances: vec![1], + expected_head: get_root(4), + }); + + // Latest-message update back to Full branch -> head returns to 3. + ops.push(Operation::ProcessPayloadAttestation { + 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), + finalized_checkpoint: get_checkpoint(0), + justified_state_balances: vec![1], + expected_head: get_root(3), + }); + ops.push(Operation::AssertPayloadWeights { + block_root: get_root(3), + expected_full_weight: 1, + expected_empty_weight: 0, + }); + + 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()), + } +} + +pub fn get_gloas_weight_priority_over_payload_preference_test_definition() +-> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Build two branches where one child extends payload (Full) and the other doesn't (Empty). + 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)), + }); + ops.push(Operation::ProcessBlock { + slot: Slot::new(1), + root: get_root(2), + parent_root: get_root(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(99)), + execution_payload_block_hash: Some(get_hash(2)), + }); + ops.push(Operation::ProcessBlock { + slot: Slot::new(2), + root: get_root(3), + parent_root: get_root(1), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(1)), + execution_payload_block_hash: Some(get_hash(3)), + }); + ops.push(Operation::ProcessBlock { + slot: Slot::new(2), + root: get_root(4), + parent_root: get_root(2), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(100)), + execution_payload_block_hash: Some(get_hash(4)), + }); + + // Parent prefers Full on equal branch weights. + 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], + expected_head: get_root(3), + }); + + // Add two Empty votes to make the Empty branch strictly heavier. + ops.push(Operation::ProcessPayloadAttestation { + validator_index: 0, + block_root: get_root(4), + attestation_slot: Slot::new(3), + payload_present: false, + blob_data_available: false, + }); + ops.push(Operation::ProcessPayloadAttestation { + 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), + finalized_checkpoint: get_checkpoint(0), + justified_state_balances: vec![1, 1], + expected_head: get_root(4), + }); + + 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()), + } +} + +pub fn get_gloas_parent_empty_when_child_points_to_grandparent_test_definition() +-> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Build a three-block chain A -> B -> C (CL parent links). + // A: EL parent = genesis hash(0), EL hash = hash(1). + 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)), + }); + + // B: EL parent = hash(1), EL hash = hash(2). + ops.push(Operation::ProcessBlock { + slot: Slot::new(2), + root: get_root(2), + parent_root: get_root(1), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(1)), + execution_payload_block_hash: Some(get_hash(2)), + }); + + // C: CL parent is B, but EL parent points to A (hash 1), not B (hash 2). + // This models B's payload not arriving in time, so C records parent status as Empty. + ops.push(Operation::ProcessBlock { + slot: Slot::new(3), + root: get_root(3), + parent_root: get_root(2), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: Some(get_hash(1)), + execution_payload_block_hash: Some(get_hash(3)), + }); + + ops.push(Operation::AssertParentPayloadStatus { + block_root: get_root(3), + expected_status: PayloadStatus::Empty, }); ForkChoiceTestDefinition { @@ -219,4 +467,22 @@ mod tests { let test = get_gloas_payload_probe_test_definition(); test.run(); } + + #[test] + fn find_head_vote_transition() { + let test = get_gloas_find_head_vote_transition_test_definition(); + test.run(); + } + + #[test] + fn weight_priority_over_payload_preference() { + let test = get_gloas_weight_priority_over_payload_preference_test_definition(); + test.run(); + } + + #[test] + fn parent_empty_when_child_points_to_grandparent() { + let test = get_gloas_parent_empty_when_child_points_to_grandparent_test_definition(); + test.run(); + } } diff --git a/consensus/proto_array/src/fork_choice_test_definition/votes.rs b/consensus/proto_array/src/fork_choice_test_definition/votes.rs index d170e0974f..49afae2d4a 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/votes.rs @@ -106,7 +106,6 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(1), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -136,7 +135,6 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(2), attestation_slot: Slot::new(2), - payload_present: false, }); // Ensure that the head is 2 since 1 and 2 both have a vote @@ -211,7 +209,6 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(3), attestation_slot: Slot::new(3), - payload_present: false, }); // Ensure that the head is still 2 @@ -246,7 +243,6 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 1, block_root: get_root(1), attestation_slot: Slot::new(3), - payload_present: false, }); // Ensure that the head is now 3 @@ -409,13 +405,11 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(5), attestation_slot: Slot::new(4), - payload_present: false, }); ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(5), attestation_slot: Slot::new(4), - payload_present: false, }); // Add blocks 7, 8 and 9. Adding these blocks helps test the `best_descendant` @@ -570,13 +564,11 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 0, block_root: get_root(9), attestation_slot: Slot::new(5), - payload_present: false, }); ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(9), attestation_slot: Slot::new(5), - payload_present: false, }); // Add block 10 @@ -650,13 +642,11 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { validator_index: 2, block_root: get_root(10), attestation_slot: Slot::new(5), - payload_present: false, }); ops.push(Operation::ProcessAttestation { validator_index: 3, block_root: get_root(10), attestation_slot: Slot::new(5), - payload_present: false, }); // Check the head is now 10. diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 926767093f..7403937d39 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -357,6 +357,9 @@ 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_deltas.payload_tiebreaker { + node.payload_tiebreak = payload_tiebreaker; + } } // Update the parent delta (if any). @@ -1052,10 +1055,14 @@ impl ProtoArray { } 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 { - // Both viable or both non-viable. For V29 parents, prefer the child - // whose parent_payload_status matches the parent's payload preference - // (Full if full_payload_weight >= empty_payload_weight, else Empty). + // Equal weights: for V29 parents, prefer the child whose + // parent_payload_status matches the parent's payload preference. let child_matches = child_matches_parent_payload_preference(parent, child); let best_child_matches = child_matches_parent_payload_preference(parent, best_child); @@ -1064,20 +1071,11 @@ impl ProtoArray { change_to_child } else if !child_matches && best_child_matches { no_change - } else if child.weight() == best_child.weight() { - // Tie-breaker of equal weights by root. - if *child.root() >= *best_child.root() { - change_to_child - } else { - no_change - } + } else if *child.root() >= *best_child.root() { + // Final tie-breaker of equal weights by root. + change_to_child } else { - // Choose the winner by weight. - if child.weight() > best_child.weight() { - change_to_child - } else { - no_change - } + no_change } } } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index e1c893db9a..9400aafed7 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -32,6 +32,8 @@ 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 @@ -39,6 +41,7 @@ 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. @@ -521,7 +524,24 @@ impl ProtoArrayForkChoice { validator_index: usize, block_root: Hash256, attestation_slot: Slot, + ) -> Result<(), String> { + let vote = self.votes.get_mut(validator_index); + + if attestation_slot > vote.next_slot || *vote == VoteTracker::default() { + vote.next_root = block_root; + vote.next_slot = attestation_slot; + } + + Ok(()) + } + + pub fn process_payload_attestation( + &mut self, + validator_index: usize, + block_root: Hash256, + attestation_slot: Slot, payload_present: bool, + blob_data_available: bool, ) -> Result<(), String> { let vote = self.votes.get_mut(validator_index); @@ -529,6 +549,7 @@ impl ProtoArrayForkChoice { vote.next_root = block_root; vote.next_slot = attestation_slot; vote.next_payload_present = payload_present; + vote.next_blob_data_available = blob_data_available; } Ok(()) @@ -945,13 +966,19 @@ impl ProtoArrayForkChoice { /// Returns the payload status of the head node based on accumulated weights. /// - /// Returns `Full` if `full_payload_weight >= empty_payload_weight` (Full wins ties per spec's - /// `get_payload_status_tiebreaker` natural ordering FULL=2 > EMPTY=1). + /// Returns `Full` if `full_payload_weight > empty_payload_weight`. + /// Returns `Empty` if `empty_payload_weight > full_payload_weight`. + /// 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 { let node = self.get_proto_node(head_root)?; let v29 = node.as_v29().ok()?; - if v29.full_payload_weight >= v29.empty_payload_weight { + 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 { Some(PayloadStatus::Full) } else { Some(PayloadStatus::Empty) @@ -985,6 +1012,7 @@ 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 { @@ -1079,6 +1107,17 @@ 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) @@ -1138,6 +1177,7 @@ 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; @@ -1195,11 +1235,21 @@ 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; } } @@ -1552,6 +1602,8 @@ 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); @@ -1607,6 +1659,8 @@ 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); @@ -1669,6 +1723,8 @@ 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); @@ -1726,6 +1782,8 @@ 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); @@ -1794,6 +1852,8 @@ 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. @@ -1804,6 +1864,8 @@ 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( @@ -1854,6 +1916,8 @@ 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); @@ -1927,6 +1991,8 @@ 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, }); } @@ -1987,6 +2053,8 @@ 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, }); } @@ -2045,6 +2113,8 @@ 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, }); } @@ -2108,6 +2178,8 @@ 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( @@ -2140,6 +2212,8 @@ 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(