diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 3c8ea30779..f05b972679 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -4855,8 +4855,8 @@ impl BeaconChain { return Err(Box::new(DoNotReOrg::NotProposing.into())); } - // TODO(gloas): reorg weight logic needs updating for GLOAS. For now use - // total weight which is correct for pre-GLOAS and conservative for post-GLOAS. + // TODO(gloas): reorg weight logic needs updating for Gloas. For now use + // total weight which is correct for pre-Gloas and conservative for post-Gloas. let head_weight = info.head_node.weight(); let parent_weight = info.parent_node.weight(); diff --git a/beacon_node/beacon_chain/src/persisted_fork_choice.rs b/beacon_node/beacon_chain/src/persisted_fork_choice.rs index 592ea9ecd7..8edccbbe98 100644 --- a/beacon_node/beacon_chain/src/persisted_fork_choice.rs +++ b/beacon_node/beacon_chain/src/persisted_fork_choice.rs @@ -22,27 +22,38 @@ pub struct PersistedForkChoice { pub fork_choice_store: PersistedForkChoiceStoreV28, } -macro_rules! impl_store_item { - ($type:ty) => { - impl store::StoreItem for $type { - fn db_column() -> DBColumn { - DBColumn::ForkChoice - } +impl PersistedForkChoiceV28 { + pub fn from_bytes(bytes: &[u8], store_config: &StoreConfig) -> Result { + let decompressed_bytes = store_config + .decompress_bytes(bytes) + .map_err(Error::Compression)?; + Self::from_ssz_bytes(&decompressed_bytes).map_err(Into::into) + } - fn as_store_bytes(&self) -> Vec { - self.as_ssz_bytes() - } + pub fn as_bytes(&self, store_config: &StoreConfig) -> Result, Error> { + let encode_timer = metrics::start_timer(&metrics::FORK_CHOICE_ENCODE_TIMES); + let ssz_bytes = self.as_ssz_bytes(); + drop(encode_timer); - fn from_store_bytes(bytes: &[u8]) -> std::result::Result { - Self::from_ssz_bytes(bytes).map_err(Into::into) - } - } - }; + let _compress_timer = metrics::start_timer(&metrics::FORK_CHOICE_COMPRESS_TIMES); + store_config + .compress_bytes(&ssz_bytes) + .map_err(Error::Compression) + } + + pub fn as_kv_store_op( + &self, + key: Hash256, + store_config: &StoreConfig, + ) -> Result { + Ok(KeyValueStoreOp::PutKeyValue( + DBColumn::ForkChoice, + key.as_slice().to_vec(), + self.as_bytes(store_config)?, + )) + } } -impl_store_item!(PersistedForkChoiceV28); -impl_store_item!(PersistedForkChoiceV29); - impl PersistedForkChoiceV29 { pub fn from_bytes(bytes: &[u8], store_config: &StoreConfig) -> Result { let decompressed_bytes = store_config @@ -83,3 +94,12 @@ impl From for PersistedForkChoiceV29 { } } } + +impl From for PersistedForkChoiceV28 { + fn from(v29: PersistedForkChoiceV29) -> Self { + Self { + fork_choice_v28: v29.fork_choice.into(), + fork_choice_store: v29.fork_choice_store, + } + } +} diff --git a/beacon_node/beacon_chain/src/schema_change.rs b/beacon_node/beacon_chain/src/schema_change.rs index fa2ab70d21..841f28e37d 100644 --- a/beacon_node/beacon_chain/src/schema_change.rs +++ b/beacon_node/beacon_chain/src/schema_change.rs @@ -22,13 +22,13 @@ pub fn migrate_schema( (_, _) if from == to && to == CURRENT_SCHEMA_VERSION => Ok(()), // Upgrade from v28 to v29. (SchemaVersion(28), SchemaVersion(29)) => { - upgrade_to_v29::(&db)?; - db.store_schema_version_atomically(to, vec![]) + let ops = upgrade_to_v29::(&db)?; + db.store_schema_version_atomically(to, ops) } // Downgrade from v29 to v28. (SchemaVersion(29), SchemaVersion(28)) => { - downgrade_from_v29::(&db)?; - db.store_schema_version_atomically(to, vec![]) + let ops = downgrade_from_v29::(&db)?; + db.store_schema_version_atomically(to, ops) } // Anything else is an error. (_, _) => Err(HotColdDBError::UnsupportedSchemaVersion { diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v29.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v29.rs index 6c82e8a737..3069200fce 100644 --- a/beacon_node/beacon_chain/src/schema_change/migration_schema_v29.rs +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v29.rs @@ -1,12 +1,8 @@ -use crate::beacon_chain::BeaconChainTypes; +use crate::beacon_chain::{BeaconChainTypes, FORK_CHOICE_DB_KEY}; use crate::persisted_fork_choice::{PersistedForkChoiceV28, PersistedForkChoiceV29}; -use ssz::Decode; use store::hot_cold_store::HotColdDB; use store::{DBColumn, Error as StoreError, KeyValueStore, KeyValueStoreOp}; -use types::{EthSpec, Hash256}; - -/// The key used to store the fork choice in the database. -const FORK_CHOICE_DB_KEY: Hash256 = Hash256::ZERO; +use types::EthSpec; /// Upgrade from schema v28 to v29. /// @@ -14,24 +10,25 @@ const FORK_CHOICE_DB_KEY: Hash256 = Hash256::ZERO; /// virtual tree walk). /// - Fails if the persisted fork choice contains any V17 (pre-Gloas) proto /// nodes at or after the Gloas fork slot. +/// +/// Returns a list of store ops to be applied atomically with the schema version write. pub fn upgrade_to_v29( db: &HotColdDB, -) -> Result<(), StoreError> { +) -> Result, StoreError> { let gloas_fork_slot = db .spec .gloas_fork_epoch .map(|epoch| epoch.start_slot(T::EthSpec::slots_per_epoch())); - // Load the persisted fork choice (v28 format, uncompressed SSZ). + // Load the persisted fork choice (v28 format). let Some(fc_bytes) = db .hot_db .get_bytes(DBColumn::ForkChoice, FORK_CHOICE_DB_KEY.as_slice())? else { - return Ok(()); + return Ok(vec![]); }; - let mut persisted_v28 = - PersistedForkChoiceV28::from_ssz_bytes(&fc_bytes).map_err(StoreError::SszDecodeError)?; + let persisted_v28 = PersistedForkChoiceV28::from_bytes(&fc_bytes, db.get_config())?; // Check for V17 nodes at/after the Gloas fork slot. if let Some(gloas_fork_slot) = gloas_fork_slot { @@ -52,39 +49,30 @@ pub fn upgrade_to_v29( } } - // Clear best_child/best_descendant — replaced by the virtual tree walk. - for node in &mut persisted_v28.fork_choice_v28.proto_array_v28.nodes { - node.best_child = None; - node.best_descendant = None; - } - - // Convert to v29 and write back. + // Convert to v29 and encode. let persisted_v29 = PersistedForkChoiceV29::from(persisted_v28); - let fc_bytes = persisted_v29 - .as_bytes(db.get_config()) - .map_err(|e| StoreError::MigrationError(format!("failed to encode v29: {:?}", e)))?; - db.hot_db.do_atomically(vec![KeyValueStoreOp::PutKeyValue( - DBColumn::ForkChoice, - FORK_CHOICE_DB_KEY.as_slice().to_vec(), - fc_bytes, - )])?; - Ok(()) + Ok(vec![ + persisted_v29.as_kv_store_op(FORK_CHOICE_DB_KEY, db.get_config())?, + ]) } -/// Downgrade from schema v29 to v28 (no-op). +/// Downgrade from schema v29 to v28. /// +/// Converts the persisted fork choice from V29 format back to V28. /// Fails if the persisted fork choice contains any V29 proto nodes, as these contain /// payload-specific fields that cannot be losslessly converted back to V17 format. +/// +/// Returns a list of store ops to be applied atomically with the schema version write. pub fn downgrade_from_v29( db: &HotColdDB, -) -> Result<(), StoreError> { +) -> Result, StoreError> { // Load the persisted fork choice (v29 format, compressed). let Some(fc_bytes) = db .hot_db .get_bytes(DBColumn::ForkChoice, FORK_CHOICE_DB_KEY.as_slice())? else { - return Ok(()); + return Ok(vec![]); }; let persisted_v29 = @@ -111,5 +99,10 @@ pub fn downgrade_from_v29( )); } - Ok(()) + // Convert to v28 and encode. + let persisted_v28 = PersistedForkChoiceV28::from(persisted_v29); + + Ok(vec![ + persisted_v28.as_kv_store_op(FORK_CHOICE_DB_KEY, db.get_config())?, + ]) } 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/src/validator/mod.rs b/beacon_node/http_api/src/validator/mod.rs index 3d96b85870..412851233e 100644 --- a/beacon_node/http_api/src/validator/mod.rs +++ b/beacon_node/http_api/src/validator/mod.rs @@ -671,7 +671,7 @@ pub fn post_validator_prepare_beacon_proposer( .await; // TODO(gloas): verify this is correct. We skip proposer preparation for - // GLOAS because the execution payload is no longer embedded in the beacon + // Gloas because the execution payload is no longer embedded in the beacon // block (it's in the payload envelope), so the head block's // execution_payload() is unavailable. let next_slot = current_slot + 1; 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 3b13cd4429..cedd42cf01 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)] @@ -174,11 +170,11 @@ pub enum InvalidAttestation { /// The attestation is attesting to a state that is later than itself. (Viz., attesting to the /// future). AttestsToFutureBlock { block: Slot, attestation: Slot }, - /// Post-GLOAS: attestation index must be 0 or 1. + /// Post-Gloas: attestation index must be 0 or 1. InvalidAttestationIndex { index: u64 }, - /// A same-slot attestation has a non-zero index, which is invalid post-GLOAS. + /// A same-slot attestation has a non-zero index, which is invalid post-Gloas. InvalidSameSlotAttestationIndex { slot: Slot }, - /// Post-GLOAS: attestation with index == 1 (payload_present) requires the block's + /// Post-Gloas: attestation with index == 1 (payload_present) requires the block's /// payload to have been received (`root in store.payload_states`). PayloadNotReceived { beacon_block_root: Hash256 }, /// A payload attestation votes payload_present for a block in the current slot, which is @@ -260,10 +256,19 @@ pub struct QueuedAttestation { attesting_indices: Vec, block_root: Hash256, target_epoch: Epoch, - /// Per GLOAS spec: `payload_present = attestation.data.index == 1`. + /// Per Gloas spec: `payload_present = attestation.data.index == 1`. payload_present: bool, } +/// Legacy queued attestation without payload_present (pre-Gloas, schema V28). +#[derive(Clone, PartialEq, Encode, Decode)] +pub struct QueuedAttestationV28 { + slot: Slot, + attesting_indices: Vec, + block_root: Hash256, + target_epoch: Epoch, +} + impl<'a, E: EthSpec> From> for QueuedAttestation { fn from(a: IndexedAttestationRef<'a, E>) -> Self { Self { @@ -276,19 +281,6 @@ impl<'a, E: EthSpec> From> for QueuedAttestation { } } -/// Used for queuing payload attestations (PTC votes) from the current slot. -/// Payload attestations have different dequeue timing than regular attestations: -/// gossiped payload attestations need an extra slot of delay (slot + 1 < current_slot). -#[derive(Clone, PartialEq, Encode, Decode)] -pub struct QueuedPayloadAttestation { - slot: Slot, - /// Resolved PTC committee positions (not validator indices). - ptc_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( @@ -310,22 +302,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 +346,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 +360,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 } } @@ -423,22 +395,7 @@ where .map_err(Error::BeaconStateError)?; let (execution_status, execution_payload_parent_hash, execution_payload_block_hash) = - if let Ok(execution_payload) = anchor_block.message().execution_payload() { - // Pre-Gloas forks: hashes come from the execution payload. - if execution_payload.is_default_with_empty_roots() { - (ExecutionStatus::irrelevant(), None, None) - } else { - // Assume that this payload is valid, since the anchor should be a - // trusted block and state. - ( - ExecutionStatus::Valid(execution_payload.block_hash()), - Some(execution_payload.parent_hash()), - Some(execution_payload.block_hash()), - ) - } - } else if let Ok(signed_bid) = - anchor_block.message().body().signed_execution_payload_bid() - { + if let Ok(signed_bid) = anchor_block.message().body().signed_execution_payload_bid() { // Gloas: execution status is irrelevant post-Gloas; payload validation // is decoupled from beacon blocks. ( @@ -446,6 +403,19 @@ where Some(signed_bid.message.parent_block_hash), Some(signed_bid.message.block_hash), ) + } else if let Ok(execution_payload) = anchor_block.message().execution_payload() { + // Pre-Gloas forks: do not set payload hashes, they are only used post-Gloas. + if execution_payload.is_default_with_empty_roots() { + (ExecutionStatus::irrelevant(), None, None) + } else { + // Assume that this payload is valid, since the anchor should be a + // trusted block and state. + ( + ExecutionStatus::Valid(execution_payload.block_hash()), + None, + None, + ) + } } else { // Pre-merge: no execution payload at all. (ExecutionStatus::irrelevant(), None, None) @@ -465,6 +435,7 @@ where execution_status, execution_payload_parent_hash, execution_payload_block_hash, + anchor_block.message().proposer_index(), spec, )?; @@ -472,13 +443,12 @@ 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, justified_hash: None, finalized_hash: None, - // These will be updated during the next call to `Self::get_head`. + // This will be updated during the next call to `Self::get_head`. head_root: Hash256::zero(), }, _phantom: PhantomData, @@ -966,14 +936,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) }; @@ -1163,7 +1125,7 @@ where { let index = indexed_attestation.data().index; - // Post-GLOAS: attestation index must be 0 or 1. + // Post-Gloas: attestation index must be 0 or 1. if index > 1 { return Err(InvalidAttestation::InvalidAttestationIndex { index }); } @@ -1176,6 +1138,7 @@ where } // index == 1 (payload_present) requires the block's payload to have been received. + // TODO(gloas): could optimise by adding `payload_received` to `Block` if index == 1 && !self .proto_array @@ -1334,10 +1297,13 @@ 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(()); } + // TODO(gloas): Should ignore wrong-slot payload attestations at the caller, 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). self.validate_on_payload_attestation(attestation, is_from_block)?; // Resolve validator indices to PTC committee positions. @@ -1346,34 +1312,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 +1353,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 +1439,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 +1594,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() @@ -1758,8 +1677,7 @@ where let mut fork_choice = Self { fc_store, proto_array, - queued_attestations: persisted.queued_attestations, - queued_payload_attestations: persisted.queued_payload_attestations, + queued_attestations: vec![], // Will be updated in the following call to `Self::get_head`. forkchoice_update_parameters: ForkchoiceUpdateParameters { head_hash: None, @@ -1799,8 +1717,6 @@ where pub fn to_persisted(&self) -> PersistedForkChoice { PersistedForkChoice { proto_array: self.proto_array().as_ssz_container(), - queued_attestations: self.queued_attestations().to_vec(), - queued_payload_attestations: self.queued_payload_attestations.clone(), } } @@ -1823,9 +1739,8 @@ pub struct PersistedForkChoice { pub proto_array_v28: proto_array::core::SszContainerV28, #[superstruct(only(V29))] pub proto_array: proto_array::core::SszContainerV29, - pub queued_attestations: Vec, - #[superstruct(only(V29))] - pub queued_payload_attestations: Vec, + #[superstruct(only(V28))] + pub queued_attestations_v28: Vec, } pub type PersistedForkChoice = PersistedForkChoiceV29; @@ -1834,8 +1749,15 @@ impl From for PersistedForkChoiceV29 { fn from(v28: PersistedForkChoiceV28) -> Self { Self { proto_array: v28.proto_array_v28.into(), - queued_attestations: v28.queued_attestations, - queued_payload_attestations: vec![], + } + } +} + +impl From for PersistedForkChoiceV28 { + fn from(v29: PersistedForkChoiceV29) -> Self { + Self { + proto_array_v28: v29.proto_array.into(), + queued_attestations_v28: vec![], } } } diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 70f1dbc215..8f479125b7 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -5,8 +5,7 @@ mod metrics; pub use crate::fork_choice::{ AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, PersistedForkChoice, - PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, QueuedPayloadAttestation, - ResetPayloadStatuses, + PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, 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 839d0f4c5c..241e25d3e2 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -73,9 +73,9 @@ impl ForkChoiceTest { Self { harness } } - /// Creates a new tester with the GLOAS fork active at epoch 1. + /// Creates a new tester with the Gloas fork active at epoch 1. /// Genesis is a standard Fulu block (epoch 0), so block production works normally. - /// Tests that need GLOAS semantics should advance the chain into epoch 1 first. + /// Tests that need Gloas semantics should advance the chain into epoch 1 first. /// Get a value from the `ForkChoice` instantiation. fn get(&self, func: T) -> U where diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index 34d7f2e48e..ff9d70bad5 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -144,6 +144,7 @@ impl ForkChoiceTestDefinition { ExecutionStatus::Optimistic(ExecutionBlockHash::zero()), self.execution_payload_parent_hash, self.execution_payload_block_hash, + 0, &spec, ) .expect("should create fork choice struct"); 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 0fb120328c..18d7a40b82 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 @@ -52,7 +52,7 @@ pub fn get_gloas_chain_following_test_definition() -> ForkChoiceTestDefinition { }); // Mark root_1 as having received its execution payload so that - // its FULL virtual node exists in the GLOAS fork choice tree. + // its FULL virtual node exists in the Gloas fork choice tree. ops.push(Operation::ProcessExecutionPayload { block_root: get_root(1), }); @@ -262,7 +262,7 @@ pub fn get_gloas_find_head_vote_transition_test_definition() -> ForkChoiceTestDe }); // Mark root_1 as having received its execution payload so that - // its FULL virtual node exists in the GLOAS fork choice tree. + // its FULL virtual node exists in the Gloas fork choice tree. ops.push(Operation::ProcessExecutionPayload { block_root: get_root(1), }); @@ -367,7 +367,7 @@ pub fn get_gloas_weight_priority_over_payload_preference_test_definition() }); // Mark root_1 as having received its execution payload so that - // its FULL virtual node exists in the GLOAS fork choice tree. + // its FULL virtual node exists in the Gloas fork choice tree. ops.push(Operation::ProcessExecutionPayload { block_root: get_root(1), }); @@ -537,7 +537,7 @@ pub fn get_gloas_interleaved_attestations_test_definition() -> ForkChoiceTestDef }); // Mark root_1 as having received its execution payload so that - // its FULL virtual node exists in the GLOAS fork choice tree. + // its FULL virtual node exists in the Gloas fork choice tree. ops.push(Operation::ProcessExecutionPayload { block_root: get_root(1), }); @@ -718,6 +718,137 @@ pub fn get_gloas_payload_received_interleaving_test_definition() -> ForkChoiceTe mod tests { use super::*; + fn gloas_fork_boundary_spec() -> ChainSpec { + let mut spec = MainnetEthSpec::default_spec(); + spec.proposer_score_boost = Some(50); + spec.gloas_fork_epoch = Some(Epoch::new(1)); + spec + } + + /// Gloas fork boundary: a chain starting pre-Gloas (V17 nodes) that crosses into + /// Gloas (V29 nodes). The head should advance through the fork boundary. + /// + /// Parameters: + /// - `skip_first_gloas_slot`: if true, there is no block at the first Gloas slot (slot 32); + /// the first V29 block appears at slot 33. + /// - `first_gloas_block_full`: if true, the first V29 block extends the parent V17 node's + /// EL chain (Full parent payload status). If false, it doesn't (Empty). + fn get_gloas_fork_boundary_test_definition( + skip_first_gloas_slot: bool, + first_gloas_block_full: bool, + ) -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Block at slot 31 — last pre-Gloas slot. Created as a V17 node because + // gloas_fork_epoch = 1 → Gloas starts at slot 32. + // + // The test harness sets execution_status = Optimistic(ExecutionBlockHash::from_root(root)), + // so this V17 node's EL block hash = ExecutionBlockHash::from_root(get_root(1)). + ops.push(Operation::ProcessBlock { + slot: Slot::new(31), + root: get_root(1), + parent_root: get_root(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + }); + + // First Gloas block (V29 node). + let gloas_slot = if skip_first_gloas_slot { 33 } else { 32 }; + + // The first Gloas block should always have the pre-Gloas block as its execution parent, + // although this is currently not checked anywhere (the spec doesn't mention this). + ops.push(Operation::ProcessBlock { + slot: Slot::new(gloas_slot), + 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)), + }); + + // Parent payload status of fork boundary block should always be Empty. + let expected_parent_status = PayloadStatus::Empty; + ops.push(Operation::AssertParentPayloadStatus { + block_root: get_root(2), + expected_status: expected_parent_status, + }); + + // Mark root 2's execution payload as received so the Full virtual child exists. + if first_gloas_block_full { + ops.push(Operation::ProcessExecutionPayload { + block_root: get_root(2), + }); + } + + // Extend the chain with another V29 block (Full child of root 2). + ops.push(Operation::ProcessBlock { + slot: Slot::new(gloas_slot + 1), + root: get_root(3), + parent_root: get_root(2), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: if first_gloas_block_full { + Some(get_hash(2)) + } else { + Some(get_hash(1)) + }, + execution_payload_block_hash: Some(get_hash(3)), + }); + + // Head should advance to the tip of the chain through the fork boundary. + ops.push(Operation::FindHead { + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + justified_state_balances: vec![1], + expected_head: get_root(3), + current_slot: Slot::new(gloas_slot + 1), + expected_payload_status: None, + }); + + ops.push(Operation::AssertParentPayloadStatus { + block_root: get_root(3), + expected_status: if first_gloas_block_full { + PayloadStatus::Full + } else { + PayloadStatus::Empty + }, + }); + + ForkChoiceTestDefinition { + finalized_block_slot: Slot::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + operations: ops, + // Genesis is V17 (slot 0 < Gloas fork slot 32), these are unused for V17. + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: Some(gloas_fork_boundary_spec()), + } + } + + #[test] + fn fork_boundary_no_skip_full() { + get_gloas_fork_boundary_test_definition(false, true).run(); + } + + #[test] + fn fork_boundary_no_skip_empty() { + get_gloas_fork_boundary_test_definition(false, false).run(); + } + + #[test] + fn fork_boundary_skip_first_gloas_slot_full() { + get_gloas_fork_boundary_test_definition(true, true).run(); + } + + #[test] + fn fork_boundary_skip_first_gloas_slot_empty() { + get_gloas_fork_boundary_test_definition(true, false).run(); + } + #[test] fn chain_following() { let test = get_gloas_chain_following_test_definition(); diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 933e9eb078..e9d5e02db5 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 @@ -143,6 +143,8 @@ pub struct ProtoNode { pub full_payload_weight: u64, #[superstruct(only(V29), partial_getter(copy))] pub execution_payload_block_hash: ExecutionBlockHash, + #[superstruct(only(V29), partial_getter(copy))] + pub execution_payload_parent_hash: ExecutionBlockHash, /// Equivalent to spec's `block_timeliness[root][ATTESTATION_TIMELINESS_INDEX]`. #[superstruct(only(V29), partial_getter(copy))] pub block_timeliness_attestation_threshold: bool, @@ -181,7 +183,6 @@ pub struct ProtoNode { impl ProtoNode { /// Generic version of spec's `parent_payload_status` that works for pre-Gloas nodes by /// considering their parents Empty. - /// Pre-Gloas nodes have no ePBS, default to Empty. pub fn get_parent_payload_status(&self) -> PayloadStatus { self.parent_payload_status().unwrap_or(PayloadStatus::Empty) } @@ -535,7 +536,7 @@ impl ProtoArray { .parent_root .and_then(|parent| self.indices.get(&parent).copied()); - let node = if !spec.fork_name_at_slot::(current_slot).gloas_enabled() { + let node = if !spec.fork_name_at_slot::(block.slot).gloas_enabled() { ProtoNode::V17(ProtoNodeV17 { slot: block.slot, root: block.root, @@ -570,31 +571,31 @@ impl ProtoArray { block_root: block.root, })?; - let parent_payload_status: PayloadStatus = if let Some(parent_node) = - parent_index.and_then(|idx| self.nodes.get(idx)) - { - // Get the parent's execution block hash, handling both V17 and V29 nodes. - // V17 parents occur during the Gloas fork transition. - // TODO(gloas): the spec's `get_parent_payload_status` assumes all blocks are - // post-Gloas with bids. Revisit once the spec clarifies fork-transition behavior. - let parent_el_block_hash = match parent_node { - ProtoNode::V29(v29) => Some(v29.execution_payload_block_hash), - ProtoNode::V17(v17) => v17.execution_status.block_hash(), - }; - // Per spec's `is_parent_node_full`: if the child's EL parent hash - // matches the parent's EL block hash, the child extends the parent's - // payload chain, meaning the parent was Full. - if parent_el_block_hash.is_some_and(|hash| execution_payload_parent_hash == hash) { - PayloadStatus::Full + let parent_payload_status: PayloadStatus = + if let Some(parent_node) = parent_index.and_then(|idx| self.nodes.get(idx)) { + match parent_node { + ProtoNode::V29(v29) => { + // Both parent and child are Gloas blocks. The parent is full if the + // block hash in the parent node matches the parent block hash in the + // child bid. + if execution_payload_parent_hash == v29.execution_payload_block_hash { + PayloadStatus::Full + } else { + PayloadStatus::Empty + } + } + ProtoNode::V17(_) => { + // Parent is pre-Gloas, pre-Gloas blocks are treated as having Empty + // payload status. This case is reached during the fork transition. + PayloadStatus::Empty + } + } } else { - PayloadStatus::Empty - } - } else { - // Parent is missing (genesis or pruned due to finalization). Default to Full - // since this path should only be hit at Gloas genesis, and extending the payload - // chain is the safe default. - PayloadStatus::Full - }; + // TODO(gloas): re-assess this assumption + // Parent is missing (genesis or pruned due to finalization). Default to Full + // since this path should only be hit at Gloas genesis. + PayloadStatus::Full + }; // Per spec `get_forkchoice_store`: the anchor (genesis) block has // its payload state initialized (`payload_states = {anchor_root: ...}`). @@ -614,14 +615,13 @@ 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, empty_payload_weight: 0, full_payload_weight: 0, execution_payload_block_hash, + execution_payload_parent_hash, // Per spec `get_forkchoice_store`: the anchor block's PTC votes are // initialized to all-True, ensuring `is_payload_timely` and // `is_payload_data_available` return true for the anchor. @@ -642,7 +642,7 @@ impl ProtoArray { block_timeliness_attestation_threshold: is_genesis || (is_current_slot && time_into_slot < spec.get_unaggregated_attestation_due()), - // TODO(gloas): use GLOAS-specific PTC due threshold once + // TODO(gloas): use Gloas-specific PTC due threshold once // `get_payload_attestation_due_ms` is on ChainSpec. block_timeliness_ptc_threshold: is_genesis || (is_current_slot && time_into_slot < spec.get_slot_duration() / 2), @@ -1431,30 +1431,11 @@ impl ProtoArray { .nodes .get(node.proto_node_index) .ok_or(Error::InvalidNodeIndex(node.proto_node_index))?; - - // V17 (pre-GLOAS) nodes don't have payload_received or parent_payload_status. - // Skip the virtual Empty/Full split and return real children directly. - if proto_node.as_v17().is_ok() { - let child_indices = children_index - .get(node.proto_node_index) - .map(|c| c.as_slice()) - .unwrap_or(&[]); - return Ok(child_indices - .iter() - .filter_map(|&child_index| { - let child_node = self.nodes.get(child_index)?; - Some(( - IndexedForkChoiceNode { - root: child_node.root(), - proto_node_index: child_index, - payload_status: PayloadStatus::Pending, - }, - child_node.clone(), - )) - }) - .collect()); + let mut children = vec![(node.with_status(PayloadStatus::Empty), proto_node.clone())]; + // The FULL virtual child only exists if the payload has been received. + if proto_node.payload_received().is_ok_and(|received| received) { + children.push((node.with_status(PayloadStatus::Full), proto_node.clone())); } - // TODO(gloas) this is the actual change we want to keep once PTC is implemented // let mut children = vec![(node.with_status(PayloadStatus::Empty), proto_node.clone())]; // // The FULL virtual child only exists if the payload has been received. @@ -1465,11 +1446,7 @@ impl ProtoArray { // TODO(gloas) remove this and uncomment the code above once we implement PTC // Skip Empty/Full split: go straight to Full when payload received, // giving full payload weight 100% without PTC votes. - let children = if proto_node.payload_received().is_ok_and(|received| received) { - vec![(node.with_status(PayloadStatus::Full), proto_node.clone())] - } else { - vec![(node.with_status(PayloadStatus::Empty), proto_node.clone())] - }; + // TODO(gloas) delete up to here Ok(children) @@ -1482,7 +1459,6 @@ impl ProtoArray { .iter() .filter_map(|&child_index| { let child_node = self.nodes.get(child_index)?; - // Skip parent_payload_status filter for V17 children (they don't have it) if child_node.get_parent_payload_status() != node.payload_status { return None; } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index cb467f2531..72440b83b8 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -33,6 +33,47 @@ pub struct VoteTracker { next_payload_present: bool, } +// Can be deleted once the V28 schema migration is buried. +// Matches the on-disk format from schema v28: current_root, next_root, next_epoch. +#[derive(Default, PartialEq, Clone, Encode, Decode)] +pub struct VoteTrackerV28 { + current_root: Hash256, + next_root: Hash256, + next_epoch: Epoch, +} + +// This impl is only used upon upgrade from pre-Gloas to Gloas with all pre-Gloas nodes. +// The payload status is `false` for pre-Gloas nodes. +impl From for VoteTracker { + fn from(v: VoteTrackerV28) -> Self { + VoteTracker { + current_root: v.current_root, + next_root: v.next_root, + // The v28 format stored next_epoch rather than slots. Default to 0 since the + // vote tracker will be updated on the next attestation. + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, + } + } +} + +// This impl is only used upon downgrade from V29 to V28, with exclusively pre-Gloas nodes. +impl From for VoteTrackerV28 { + fn from(v: VoteTracker) -> Self { + // Drop the payload_present fields. This is safe because this is only called on pre-Gloas + // nodes. + VoteTrackerV28 { + current_root: v.current_root, + next_root: v.next_root, + // The v28 format stored next_epoch. Default to 0 since the vote tracker will be + // updated on the next attestation. + next_epoch: Epoch::new(0), + } + } +} + pub struct LatestMessage { pub slot: Slot, pub root: Hash256, @@ -479,6 +520,7 @@ impl ProtoArrayForkChoice { execution_status: ExecutionStatus, execution_payload_parent_hash: Option, execution_payload_block_hash: Option, + proposer_index: u64, spec: &ChainSpec, ) -> Result { let mut proto_array = ProtoArray { @@ -505,7 +547,7 @@ impl ProtoArrayForkChoice { unrealized_finalized_checkpoint: Some(finalized_checkpoint), execution_payload_parent_hash, execution_payload_block_hash, - proposer_index: Some(0), + proposer_index: Some(proposer_index), }; proto_array @@ -988,7 +1030,7 @@ impl ProtoArrayForkChoice { .unwrap_or_else(|_| ExecutionStatus::irrelevant()), unrealized_justified_checkpoint: block.unrealized_justified_checkpoint(), unrealized_finalized_checkpoint: block.unrealized_finalized_checkpoint(), - execution_payload_parent_hash: None, + execution_payload_parent_hash: block.execution_payload_parent_hash().ok(), execution_payload_block_hash: block.execution_payload_block_hash().ok(), proposer_index: block.proposer_index().ok(), }) @@ -1005,7 +1047,8 @@ impl ProtoArrayForkChoice { } /// Returns whether the execution payload for a block has been received. - /// Returns `false` for pre-GLOAS (V17) nodes or unknown blocks. + /// + /// Returns `false` for pre-Gloas (V17) nodes or unknown blocks. pub fn is_payload_received(&self, block_root: &Hash256) -> bool { self.get_proto_node(block_root) .and_then(|node| node.payload_received().ok()) @@ -1317,6 +1360,7 @@ mod test_compute_deltas { execution_status, None, None, + 0, &spec, ) .unwrap(); @@ -1471,6 +1515,7 @@ mod test_compute_deltas { execution_status, None, None, + 0, &spec, ) .unwrap(); diff --git a/consensus/proto_array/src/ssz_container.rs b/consensus/proto_array/src/ssz_container.rs index 5edc1cd313..80a6702210 100644 --- a/consensus/proto_array/src/ssz_container.rs +++ b/consensus/proto_array/src/ssz_container.rs @@ -2,7 +2,7 @@ use crate::proto_array::ProposerBoost; use crate::{ Error, JustifiedBalances, proto_array::{ProtoArray, ProtoNode, ProtoNodeV17}, - proto_array_fork_choice::{ElasticList, ProtoArrayForkChoice, VoteTracker}, + proto_array_fork_choice::{ElasticList, ProtoArrayForkChoice, VoteTracker, VoteTrackerV28}, }; use ssz::{Encode, four_byte_option_impl}; use ssz_derive::{Decode, Encode}; @@ -22,6 +22,9 @@ pub type SszContainer = SszContainerV29; no_enum )] pub struct SszContainer { + #[superstruct(only(V28))] + pub votes_v28: Vec, + #[superstruct(only(V29))] pub votes: Vec, pub prune_threshold: usize, // Deprecated, remove in a future schema migration @@ -75,9 +78,19 @@ impl TryFrom<(SszContainerV29, JustifiedBalances)> for ProtoArrayForkChoice { impl From for SszContainerV29 { fn from(v28: SszContainerV28) -> Self { Self { - votes: v28.votes, + votes: v28.votes_v28.into_iter().map(Into::into).collect(), prune_threshold: v28.prune_threshold, - nodes: v28.nodes.into_iter().map(ProtoNode::V17).collect(), + nodes: v28 + .nodes + .into_iter() + .map(|mut node| { + // best_child/best_descendant are no longer used (replaced by + // the virtual tree walk). Clear during conversion. + node.best_child = None; + node.best_descendant = None; + ProtoNode::V17(node) + }) + .collect(), indices: v28.indices, previous_proposer_boost: v28.previous_proposer_boost, } @@ -88,7 +101,7 @@ impl From for SszContainerV29 { impl From for SszContainerV28 { fn from(v29: SszContainerV29) -> Self { Self { - votes: v29.votes, + votes_v28: v29.votes.into_iter().map(Into::into).collect(), prune_threshold: v29.prune_threshold, // These checkpoints are not consumed in v28 paths since the upgrade from v17, // we can safely default the values.