From d5c5077a31aa5c28ddff3944305966157b1e8d37 Mon Sep 17 00:00:00 2001 From: hopinheimer Date: Tue, 24 Feb 2026 17:40:11 -0500 Subject: [PATCH] implement scoring mechanisms and plumbing --- beacon_node/beacon_chain/src/beacon_chain.rs | 26 +- .../beacon_chain/src/block_production/mod.rs | 4 +- .../beacon_chain/src/block_verification.rs | 20 + .../beacon_chain/src/persisted_fork_choice.rs | 21 +- .../src/schema_change/migration_schema_v23.rs | 21 +- .../src/schema_change/migration_schema_v28.rs | 37 +- .../tests/payload_invalidation.rs | 4 +- beacon_node/beacon_chain/tests/tests.rs | 14 +- beacon_node/http_api/src/lib.rs | 58 ++- beacon_node/http_api/tests/tests.rs | 59 ++- consensus/fork_choice/src/fork_choice.rs | 151 +++++- consensus/fork_choice/src/lib.rs | 3 +- consensus/fork_choice/tests/tests.rs | 30 ++ consensus/proto_array/src/bin.rs | 3 - consensus/proto_array/src/error.rs | 4 +- .../src/fork_choice_test_definition.rs | 204 +++++++- .../execution_status.rs | 57 +- .../ffg_updates.rs | 38 +- .../gloas_payload.rs | 222 ++++++++ .../fork_choice_test_definition/no_votes.rs | 15 + .../src/fork_choice_test_definition/votes.rs | 73 ++- consensus/proto_array/src/lib.rs | 4 +- consensus/proto_array/src/proto_array.rs | 492 ++++++++++++------ .../src/proto_array_fork_choice.rs | 348 ++++++++++--- consensus/proto_array/src/ssz_container.rs | 76 ++- testing/ef_tests/src/cases/fork_choice.rs | 2 +- 26 files changed, 1573 insertions(+), 413 deletions(-) create mode 100644 consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 9f62bf11f5..f95a2fb503 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1441,7 +1441,7 @@ impl BeaconChain { .proto_array() .heads_descended_from_finalization::(fork_choice.finalized_checkpoint()) .iter() - .map(|node| (node.root, node.slot)) + .map(|node| (node.root(), node.slot())) .collect() } @@ -4776,7 +4776,7 @@ impl BeaconChain { // The slot of our potential re-org block is always 1 greater than the head block because we // only attempt single-slot re-orgs. - let head_slot = info.head_node.slot; + let head_slot = info.head_node.slot(); let re_org_block_slot = head_slot + 1; let fork_choice_slot = info.current_slot; @@ -4811,9 +4811,9 @@ impl BeaconChain { .fork_name_at_slot::(re_org_block_slot) .fulu_enabled() { - info.head_node.current_epoch_shuffling_id + info.head_node.current_epoch_shuffling_id() } else { - info.head_node.next_epoch_shuffling_id + info.head_node.next_epoch_shuffling_id() } .shuffling_decision_block; let proposer_index = self @@ -4844,8 +4844,8 @@ impl BeaconChain { // and the actual weight of the parent against the parent re-org threshold. let (head_weak, parent_strong) = if fork_choice_slot == re_org_block_slot { ( - info.head_node.weight < info.re_org_head_weight_threshold, - info.parent_node.weight > info.re_org_parent_weight_threshold, + info.head_node.weight() < info.re_org_head_weight_threshold, + info.parent_node.weight() > info.re_org_parent_weight_threshold, ) } else { (true, true) @@ -4853,7 +4853,7 @@ impl BeaconChain { if !head_weak { return Err(Box::new( DoNotReOrg::HeadNotWeak { - head_weight: info.head_node.weight, + head_weight: info.head_node.weight(), re_org_head_weight_threshold: info.re_org_head_weight_threshold, } .into(), @@ -4862,7 +4862,7 @@ impl BeaconChain { if !parent_strong { return Err(Box::new( DoNotReOrg::ParentNotStrong { - parent_weight: info.parent_node.weight, + parent_weight: info.parent_node.weight(), re_org_parent_weight_threshold: info.re_org_parent_weight_threshold, } .into(), @@ -4880,9 +4880,13 @@ impl BeaconChain { return Err(Box::new(DoNotReOrg::HeadNotLate.into())); } - let parent_head_hash = info.parent_node.execution_status.block_hash(); + let parent_head_hash = info + .parent_node + .execution_status() + .ok() + .and_then(|execution_status| execution_status.block_hash()); let forkchoice_update_params = ForkchoiceUpdateParameters { - head_root: info.parent_node.root, + head_root: info.parent_node.root(), head_hash: parent_head_hash, justified_hash: canonical_forkchoice_params.justified_hash, finalized_hash: canonical_forkchoice_params.finalized_hash, @@ -4890,7 +4894,7 @@ impl BeaconChain { debug!( canonical_head = ?head_block_root, - ?info.parent_node.root, + parent_root = ?info.parent_node.root(), slot = %fork_choice_slot, "Fork choice update overridden" ); diff --git a/beacon_node/beacon_chain/src/block_production/mod.rs b/beacon_node/beacon_chain/src/block_production/mod.rs index 76c8b77e93..f924461012 100644 --- a/beacon_node/beacon_chain/src/block_production/mod.rs +++ b/beacon_node/beacon_chain/src/block_production/mod.rs @@ -200,7 +200,7 @@ impl BeaconChain { }) .ok()?; drop(proposer_head_timer); - let re_org_parent_block = proposer_head.parent_node.root; + let re_org_parent_block = proposer_head.parent_node.root(); let (state_root, state) = self .store @@ -213,7 +213,7 @@ impl BeaconChain { info!( weak_head = ?canonical_head, parent = ?re_org_parent_block, - head_weight = proposer_head.head_node.weight, + head_weight = proposer_head.head_node.weight(), threshold_weight = proposer_head.re_org_head_weight_threshold, "Attempting re-org due to weak head" ); diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index e0943d5d93..be1974b812 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -1683,6 +1683,26 @@ impl ExecutionPendingBlock { Err(e) => Err(BlockError::BeaconChainError(Box::new(e.into()))), }?; } + + // Register each payload attestation in the block with fork choice. + if let Ok(payload_attestations) = block.message().body().payload_attestations() { + for (i, payload_attestation) in payload_attestations.iter().enumerate() { + let indexed_payload_attestation = consensus_context + .get_indexed_payload_attestation(&state, payload_attestation, &chain.spec) + .map_err(|e| BlockError::PerBlockProcessingError(e.into_with_index(i)))?; + + match fork_choice.on_payload_attestation( + current_slot, + indexed_payload_attestation, + AttestationFromBlock::True, + ) { + Ok(()) => Ok(()), + // Ignore invalid payload attestations whilst importing from a block. + Err(ForkChoiceError::InvalidAttestation(_)) => Ok(()), + Err(e) => Err(BlockError::BeaconChainError(Box::new(e.into()))), + }?; + } + } drop(fork_choice); Ok(Self { diff --git a/beacon_node/beacon_chain/src/persisted_fork_choice.rs b/beacon_node/beacon_chain/src/persisted_fork_choice.rs index d8fcc0901b..5551e1d7c9 100644 --- a/beacon_node/beacon_chain/src/persisted_fork_choice.rs +++ b/beacon_node/beacon_chain/src/persisted_fork_choice.rs @@ -9,10 +9,10 @@ use superstruct::superstruct; use types::Hash256; // If adding a new version you should update this type alias and fix the breakages. -pub type PersistedForkChoice = PersistedForkChoiceV28; +pub type PersistedForkChoice = PersistedForkChoiceV29; #[superstruct( - variants(V17, V28), + variants(V17, V28, V29), variant_attributes(derive(Encode, Decode)), no_enum )] @@ -20,10 +20,12 @@ pub struct PersistedForkChoice { #[superstruct(only(V17))] pub fork_choice_v17: fork_choice::PersistedForkChoiceV17, #[superstruct(only(V28))] - pub fork_choice: fork_choice::PersistedForkChoiceV28, + pub fork_choice_v28: fork_choice::PersistedForkChoiceV28, + #[superstruct(only(V29))] + pub fork_choice: fork_choice::PersistedForkChoiceV29, #[superstruct(only(V17))] pub fork_choice_store_v17: PersistedForkChoiceStoreV17, - #[superstruct(only(V28))] + #[superstruct(only(V28, V29))] pub fork_choice_store: PersistedForkChoiceStoreV28, } @@ -47,7 +49,7 @@ macro_rules! impl_store_item { impl_store_item!(PersistedForkChoiceV17); -impl PersistedForkChoiceV28 { +impl PersistedForkChoiceV29 { pub fn from_bytes(bytes: &[u8], store_config: &StoreConfig) -> Result { let decompressed_bytes = store_config .decompress_bytes(bytes) @@ -78,3 +80,12 @@ impl PersistedForkChoiceV28 { )) } } + +impl From for PersistedForkChoiceV29 { + fn from(v28: PersistedForkChoiceV28) -> Self { + Self { + fork_choice: v28.fork_choice_v28.into(), + fork_choice_store: v28.fork_choice_store, + } + } +} diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v23.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v23.rs index e238e1efb6..a6671c55be 100644 --- a/beacon_node/beacon_chain/src/schema_change/migration_schema_v23.rs +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v23.rs @@ -110,22 +110,21 @@ pub fn downgrade_from_v23( // Doesn't matter what policy we use for invalid payloads, as our head calculation just // considers descent from finalization. let reset_payload_statuses = ResetPayloadStatuses::OnlyWithInvalidPayload; - let fork_choice = ForkChoice::from_persisted( - persisted_fork_choice.fork_choice_v17.try_into()?, - reset_payload_statuses, - fc_store, - &db.spec, - ) - .map_err(|e| { - Error::MigrationError(format!("Error loading fork choice from persisted: {e:?}")) - })?; + let persisted_fc_v28: fork_choice::PersistedForkChoiceV28 = + persisted_fork_choice.fork_choice_v17.try_into()?; + let persisted_fc_v29: fork_choice::PersistedForkChoiceV29 = persisted_fc_v28.into(); + let fork_choice = + ForkChoice::from_persisted(persisted_fc_v29, reset_payload_statuses, fc_store, &db.spec) + .map_err(|e| { + Error::MigrationError(format!("Error loading fork choice from persisted: {e:?}")) + })?; let heads = fork_choice .proto_array() .heads_descended_from_finalization::(fork_choice.finalized_checkpoint()); - let head_roots = heads.iter().map(|node| node.root).collect(); - let head_slots = heads.iter().map(|node| node.slot).collect(); + let head_roots = heads.iter().map(|node| node.root()).collect(); + let head_slots = heads.iter().map(|node| node.slot()).collect(); let persisted_beacon_chain_v22 = PersistedBeaconChainV22 { _canonical_head_block_root: DUMMY_CANONICAL_HEAD_BLOCK_ROOT, diff --git a/beacon_node/beacon_chain/src/schema_change/migration_schema_v28.rs b/beacon_node/beacon_chain/src/schema_change/migration_schema_v28.rs index 5885eaabc0..86b96080d4 100644 --- a/beacon_node/beacon_chain/src/schema_change/migration_schema_v28.rs +++ b/beacon_node/beacon_chain/src/schema_change/migration_schema_v28.rs @@ -1,7 +1,7 @@ use crate::{ BeaconChain, BeaconChainTypes, BeaconForkChoiceStore, PersistedForkChoiceStoreV17, beacon_chain::FORK_CHOICE_DB_KEY, - persisted_fork_choice::{PersistedForkChoiceV17, PersistedForkChoiceV28}, + persisted_fork_choice::PersistedForkChoiceV17, summaries_dag::{DAGStateSummary, StateSummariesDAG}, }; use fork_choice::{ForkChoice, ForkChoiceStore, ResetPayloadStatuses}; @@ -88,8 +88,11 @@ pub fn upgrade_to_v28( // Construct top-level ForkChoice struct using the patched fork choice store, and the converted // proto array. let reset_payload_statuses = ResetPayloadStatuses::OnlyWithInvalidPayload; + let persisted_fc_v28: fork_choice::PersistedForkChoiceV28 = + persisted_fork_choice_v17.fork_choice_v17.try_into()?; + let persisted_fc_v29: fork_choice::PersistedForkChoiceV29 = persisted_fc_v28.into(); let fork_choice = ForkChoice::from_persisted( - persisted_fork_choice_v17.fork_choice_v17.try_into()?, + persisted_fc_v29, reset_payload_statuses, fc_store, db.get_chain_spec(), @@ -118,26 +121,22 @@ pub fn downgrade_from_v28( return Ok(vec![]); }; - // Recreate V28 persisted fork choice, then convert each field back to its V17 version. - let persisted_fork_choice = PersistedForkChoiceV28 { - fork_choice: fork_choice.to_persisted(), - fork_choice_store: fork_choice.fc_store().to_persisted(), - }; - + let persisted_v29 = fork_choice.to_persisted(); + let fc_store_v28 = fork_choice.fc_store().to_persisted(); let justified_balances = fork_choice.fc_store().justified_balances(); - // 1. Create `proto_array::PersistedForkChoiceV17`. - let fork_choice_v17: fork_choice::PersistedForkChoiceV17 = ( - persisted_fork_choice.fork_choice, - justified_balances.clone(), - ) - .into(); + // Convert V29 proto_array back to legacy V28 for downgrade. + let persisted_fork_choice_v28 = fork_choice::PersistedForkChoiceV28 { + proto_array_v28: persisted_v29.proto_array.into(), + queued_attestations: persisted_v29.queued_attestations, + }; - let fork_choice_store_v17: PersistedForkChoiceStoreV17 = ( - persisted_fork_choice.fork_choice_store, - justified_balances.clone(), - ) - .into(); + // 1. Create `proto_array::PersistedForkChoiceV17`. + let fork_choice_v17: fork_choice::PersistedForkChoiceV17 = + (persisted_fork_choice_v28, justified_balances.clone()).into(); + + let fork_choice_store_v17: PersistedForkChoiceStoreV17 = + (fc_store_v28, justified_balances.clone()).into(); let persisted_fork_choice_v17 = PersistedForkChoiceV17 { fork_choice_v17, diff --git a/beacon_node/beacon_chain/tests/payload_invalidation.rs b/beacon_node/beacon_chain/tests/payload_invalidation.rs index eb8e57a5d5..b1e2fd2ccc 100644 --- a/beacon_node/beacon_chain/tests/payload_invalidation.rs +++ b/beacon_node/beacon_chain/tests/payload_invalidation.rs @@ -1498,7 +1498,7 @@ async fn weights_after_resetting_optimistic_status() { .fork_choice_read_lock() .proto_array() .iter_nodes(&head.head_block_root()) - .map(|node| (node.root, node.weight)) + .map(|node| (node.root(), node.weight())) .collect::>(); rig.invalidate_manually(roots[1]).await; @@ -1518,7 +1518,7 @@ async fn weights_after_resetting_optimistic_status() { .fork_choice_read_lock() .proto_array() .iter_nodes(&head.head_block_root()) - .map(|node| (node.root, node.weight)) + .map(|node| (node.root(), node.weight())) .collect::>(); assert_eq!(original_weights, new_weights); diff --git a/beacon_node/beacon_chain/tests/tests.rs b/beacon_node/beacon_chain/tests/tests.rs index b052ba66f1..10c0b429a9 100644 --- a/beacon_node/beacon_chain/tests/tests.rs +++ b/beacon_node/beacon_chain/tests/tests.rs @@ -590,7 +590,10 @@ async fn unaggregated_attestations_added_to_fork_choice_some_none() { if slot <= num_blocks_produced && slot != 0 { assert_eq!( - latest_message.unwrap().1, + latest_message + .expect("latest message should be present") + .slot + .epoch(MinimalEthSpec::slots_per_epoch()), slot.epoch(MinimalEthSpec::slots_per_epoch()), "Latest message epoch for {} should be equal to epoch {}.", validator, @@ -700,10 +703,12 @@ async fn unaggregated_attestations_added_to_fork_choice_all_updated() { let validator_slots: Vec<(&usize, Slot)> = validators.iter().zip(slots).collect(); for (validator, slot) in validator_slots { - let latest_message = fork_choice.latest_message(*validator); + let latest_message = fork_choice + .latest_message(*validator) + .expect("latest message should be present"); assert_eq!( - latest_message.unwrap().1, + latest_message.slot.epoch(MinimalEthSpec::slots_per_epoch()), slot.epoch(MinimalEthSpec::slots_per_epoch()), "Latest message slot should be equal to attester duty." ); @@ -714,8 +719,7 @@ async fn unaggregated_attestations_added_to_fork_choice_all_updated() { .expect("Should get block root at slot"); assert_eq!( - latest_message.unwrap().0, - *block_root, + latest_message.root, *block_root, "Latest message block root should be equal to block at slot." ); } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 92a1ad934d..3077439b6f 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -2078,52 +2078,64 @@ pub fn serve( .nodes .iter() .map(|node| { - let execution_status = if node.execution_status.is_execution_enabled() { - Some(node.execution_status.to_string()) + let execution_status = if node + .execution_status() + .is_ok_and(|status| status.is_execution_enabled()) + { + node.execution_status() + .ok() + .map(|status| status.to_string()) } else { None }; + let execution_status_string = node + .execution_status() + .ok() + .map(|status| status.to_string()) + .unwrap_or_else(|| "n/a".to_string()); + ForkChoiceNode { - slot: node.slot, - block_root: node.root, + slot: node.slot(), + block_root: node.root(), parent_root: node - .parent + .parent() .and_then(|index| proto_array.nodes.get(index)) - .map(|parent| parent.root), - justified_epoch: node.justified_checkpoint.epoch, - finalized_epoch: node.finalized_checkpoint.epoch, - weight: node.weight, + .map(|parent| parent.root()), + justified_epoch: node.justified_checkpoint().epoch, + finalized_epoch: node.finalized_checkpoint().epoch, + weight: node.weight(), validity: execution_status, execution_block_hash: node - .execution_status - .block_hash() + .execution_status() + .ok() + .and_then(|status| status.block_hash()) .map(|block_hash| block_hash.into_root()), extra_data: ForkChoiceExtraData { - target_root: node.target_root, - justified_root: node.justified_checkpoint.root, - finalized_root: node.finalized_checkpoint.root, + target_root: node.target_root(), + justified_root: node.justified_checkpoint().root, + finalized_root: node.finalized_checkpoint().root, unrealized_justified_root: node - .unrealized_justified_checkpoint + .unrealized_justified_checkpoint() .map(|checkpoint| checkpoint.root), unrealized_finalized_root: node - .unrealized_finalized_checkpoint + .unrealized_finalized_checkpoint() .map(|checkpoint| checkpoint.root), unrealized_justified_epoch: node - .unrealized_justified_checkpoint + .unrealized_justified_checkpoint() .map(|checkpoint| checkpoint.epoch), unrealized_finalized_epoch: node - .unrealized_finalized_checkpoint + .unrealized_finalized_checkpoint() .map(|checkpoint| checkpoint.epoch), - execution_status: node.execution_status.to_string(), + execution_status: execution_status_string, best_child: node - .best_child + .best_child() .and_then(|index| proto_array.nodes.get(index)) - .map(|child| child.root), + .map(|child| child.root()), best_descendant: node - .best_descendant + .best_descendant() .and_then(|index| proto_array.nodes.get(index)) - .map(|descendant| descendant.root), + .map(|descendant| descendant.root()), }, } }) diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 7e3eb8b980..a43c8216da 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -33,7 +33,7 @@ use lighthouse_network::{Enr, PeerId, types::SyncState}; use network::NetworkReceivers; use network_utils::enr_ext::EnrExt; use operation_pool::attestation_storage::CheckpointKey; -use proto_array::ExecutionStatus; +use proto_array::{ExecutionStatus, core::ProtoNode}; use reqwest::{RequestBuilder, Response, StatusCode}; use sensitive_url::SensitiveUrl; use slot_clock::SlotClock; @@ -3072,51 +3072,61 @@ impl ApiTester { .nodes .iter() .map(|node| { - let execution_status = if node.execution_status.is_execution_enabled() { - Some(node.execution_status.to_string()) + let execution_status = if node + .execution_status() + .is_ok_and(|status| status.is_execution_enabled()) + { + node.execution_status() + .ok() + .map(|status| status.to_string()) } else { None }; ForkChoiceNode { - slot: node.slot, - block_root: node.root, + slot: node.slot(), + block_root: node.root(), parent_root: node - .parent + .parent() .and_then(|index| expected_proto_array.nodes.get(index)) - .map(|parent| parent.root), - justified_epoch: node.justified_checkpoint.epoch, - finalized_epoch: node.finalized_checkpoint.epoch, - weight: node.weight, + .map(|parent| parent.root()), + justified_epoch: node.justified_checkpoint().epoch, + finalized_epoch: node.finalized_checkpoint().epoch, + weight: node.weight(), validity: execution_status, execution_block_hash: node - .execution_status - .block_hash() + .execution_status() + .ok() + .and_then(|status| status.block_hash()) .map(|block_hash| block_hash.into_root()), extra_data: ForkChoiceExtraData { - target_root: node.target_root, - justified_root: node.justified_checkpoint.root, - finalized_root: node.finalized_checkpoint.root, + target_root: node.target_root(), + justified_root: node.justified_checkpoint().root, + finalized_root: node.finalized_checkpoint().root, unrealized_justified_root: node - .unrealized_justified_checkpoint + .unrealized_justified_checkpoint() .map(|checkpoint| checkpoint.root), unrealized_finalized_root: node - .unrealized_finalized_checkpoint + .unrealized_finalized_checkpoint() .map(|checkpoint| checkpoint.root), unrealized_justified_epoch: node - .unrealized_justified_checkpoint + .unrealized_justified_checkpoint() .map(|checkpoint| checkpoint.epoch), unrealized_finalized_epoch: node - .unrealized_finalized_checkpoint + .unrealized_finalized_checkpoint() .map(|checkpoint| checkpoint.epoch), - execution_status: node.execution_status.to_string(), + execution_status: node + .execution_status() + .ok() + .map(|status| status.to_string()) + .unwrap_or_else(|| "n/a".to_string()), best_child: node - .best_child + .best_child() .and_then(|index| expected_proto_array.nodes.get(index)) - .map(|child| child.root), + .map(|child| child.root()), best_descendant: node - .best_descendant + .best_descendant() .and_then(|index| expected_proto_array.nodes.get(index)) - .map(|descendant| descendant.root), + .map(|descendant| descendant.root()), }, } }) @@ -7048,6 +7058,7 @@ impl ApiTester { .core_proto_array_mut() .nodes .last_mut() + && let ProtoNode::V17(head_node) = head_node { head_node.execution_status = ExecutionStatus::Optimistic(ExecutionBlockHash::zero()) } diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 3e1c2dc361..77442a62f5 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -20,7 +20,8 @@ use tracing::{debug, instrument, warn}; use types::{ AbstractExecPayload, AttestationShufflingId, AttesterSlashingRef, BeaconBlockRef, BeaconState, BeaconStateError, ChainSpec, Checkpoint, Epoch, EthSpec, ExecPayload, ExecutionBlockHash, - Hash256, IndexedAttestationRef, RelativeEpoch, SignedBeaconBlock, Slot, + Hash256, IndexedAttestationRef, IndexedPayloadAttestation, RelativeEpoch, SignedBeaconBlock, + Slot, }; #[derive(Debug)] @@ -138,10 +139,10 @@ pub enum InvalidBlock { finalized_root: Hash256, block_ancestor: Option, }, - MissingExecutionPayloadBid{ + MissingExecutionPayloadBid { block_slot: Slot, block_root: Hash256, - } + }, } #[derive(Debug)] @@ -174,6 +175,9 @@ 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 }, + /// A same-slot attestation has a non-zero index, indicating a payload attestation during the + /// same slot as the block. Payload attestations must only arrive in subsequent slots. + PayloadAttestationDuringSameSlot { slot: Slot }, } impl From for Error { @@ -401,6 +405,9 @@ where current_epoch_shuffling_id, next_epoch_shuffling_id, execution_status, + None, + None, + spec, )?; let mut fork_choice = Self { @@ -889,23 +896,22 @@ where }; let (execution_payload_parent_hash, execution_payload_block_hash) = - if let Ok(signed_bid) = block.body().signed_execution_payload_bid() { - ( - Some(signed_bid.message.parent_block_hash), - 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) - }; + if let Ok(signed_bid) = block.body().signed_execution_payload_bid() { + ( + Some(signed_bid.message.parent_block_hash), + 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) + }; // This does not apply a vote to the block, it just makes fork choice aware of the block so // it can still be identified as the head even if it doesn't have any votes. @@ -935,7 +941,6 @@ where unrealized_finalized_checkpoint: Some(unrealized_finalized_checkpoint), execution_payload_parent_hash, execution_payload_block_hash, - }, current_slot, self.justified_checkpoint(), @@ -1081,6 +1086,46 @@ where }); } + // Same-slot attestations must have index == 0 (i.e., indicate pending payload status). + // Payload-present attestations (index == 1) for the same slot as the block are invalid + // because PTC votes should only arrive in subsequent slots. + if indexed_attestation.data().slot == block.slot && indexed_attestation.data().index != 0 { + return Err(InvalidAttestation::PayloadAttestationDuringSameSlot { slot: block.slot }); + } + + Ok(()) + } + + /// Validates a payload attestation for application to fork choice. + fn validate_on_payload_attestation( + &self, + indexed_payload_attestation: &IndexedPayloadAttestation, + _is_from_block: AttestationFromBlock, + ) -> Result<(), InvalidAttestation> { + if indexed_payload_attestation.attesting_indices.is_empty() { + return Err(InvalidAttestation::EmptyAggregationBitfield); + } + + let block = self + .proto_array + .get_block(&indexed_payload_attestation.data.beacon_block_root) + .ok_or(InvalidAttestation::UnknownHeadBlock { + beacon_block_root: indexed_payload_attestation.data.beacon_block_root, + })?; + + if block.slot > indexed_payload_attestation.data.slot { + return Err(InvalidAttestation::AttestsToFutureBlock { + block: block.slot, + attestation: indexed_payload_attestation.data.slot, + }); + } + + if indexed_payload_attestation.data.slot == block.slot + && indexed_payload_attestation.data.payload_present + { + return Err(InvalidAttestation::PayloadAttestationDuringSameSlot { slot: block.slot }); + } + Ok(()) } @@ -1154,6 +1199,43 @@ where Ok(()) } + /// Register a payload attestation with the fork choice DAG. + pub fn on_payload_attestation( + &mut self, + system_time_current_slot: Slot, + attestation: &IndexedPayloadAttestation, + is_from_block: AttestationFromBlock, + ) -> Result<(), Error> { + self.update_time(system_time_current_slot)?; + + if attestation.data.beacon_block_root == Hash256::zero() { + return Ok(()); + } + + self.validate_on_payload_attestation(attestation, is_from_block)?; + + if attestation.data.slot < self.fc_store.get_current_slot() { + for validator_index in attestation.attesting_indices_iter() { + self.proto_array.process_attestation( + *validator_index as usize, + attestation.data.beacon_block_root, + attestation.data.slot, + attestation.data.payload_present, + )?; + } + } 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, + }); + } + + Ok(()) + } + /// Apply an attester slashing to fork choice. /// /// We assume that the attester slashing provided to this function has already been verified. @@ -1564,7 +1646,7 @@ where /// /// This is used when persisting the state of the fork choice to disk. #[superstruct( - variants(V17, V28), + variants(V17, V28, V29), variant_attributes(derive(Encode, Decode, Clone)), no_enum )] @@ -1572,30 +1654,42 @@ pub struct PersistedForkChoice { #[superstruct(only(V17))] pub proto_array_bytes: Vec, #[superstruct(only(V28))] - pub proto_array: proto_array::core::SszContainerV28, + pub proto_array_v28: proto_array::core::SszContainerLegacyV28, + #[superstruct(only(V29))] + pub proto_array: proto_array::core::SszContainerV29, pub queued_attestations: Vec, } -pub type PersistedForkChoice = PersistedForkChoiceV28; +pub type PersistedForkChoice = PersistedForkChoiceV29; impl TryFrom for PersistedForkChoiceV28 { type Error = ssz::DecodeError; fn try_from(v17: PersistedForkChoiceV17) -> Result { let container_v17 = - proto_array::core::SszContainerV17::from_ssz_bytes(&v17.proto_array_bytes)?; - let container_v28 = container_v17.into(); + proto_array::core::SszContainerLegacyV17::from_ssz_bytes(&v17.proto_array_bytes)?; + let container_v28: proto_array::core::SszContainerLegacyV28 = container_v17.into(); Ok(Self { - proto_array: container_v28, + proto_array_v28: container_v28, queued_attestations: v17.queued_attestations, }) } } +impl From for PersistedForkChoiceV29 { + fn from(v28: PersistedForkChoiceV28) -> Self { + Self { + proto_array: v28.proto_array_v28.into(), + queued_attestations: v28.queued_attestations, + } + } +} + impl From<(PersistedForkChoiceV28, JustifiedBalances)> for PersistedForkChoiceV17 { fn from((v28, balances): (PersistedForkChoiceV28, JustifiedBalances)) -> Self { - let container_v17 = proto_array::core::SszContainerV17::from((v28.proto_array, balances)); + let container_v17 = + proto_array::core::SszContainerLegacyV17::from((v28.proto_array_v28, balances)); let proto_array_bytes = container_v17.as_ssz_bytes(); Self { @@ -1640,6 +1734,7 @@ 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 afe06dee1b..87438f2f85 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -5,7 +5,8 @@ mod metrics; pub use crate::fork_choice::{ AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, PersistedForkChoice, - PersistedForkChoiceV17, PersistedForkChoiceV28, QueuedAttestation, ResetPayloadStatuses, + PersistedForkChoiceV17, 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 d3a84ee85b..86ef0e2f90 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -923,6 +923,36 @@ async fn invalid_attestation_future_block() { .await; } +/// Payload attestations (index == 1) are invalid when they refer to a block in the same slot. +#[tokio::test] +async fn invalid_attestation_payload_during_same_slot() { + ForkChoiceTest::new() + .apply_blocks_without_new_attestations(1) + .await + .apply_attestation_to_chain( + MutationDelay::NoDelay, + |attestation, chain| { + let block_slot = chain + .get_blinded_block(&attestation.data().beacon_block_root) + .expect("should read attested block") + .expect("attested block should exist") + .slot(); + + attestation.data_mut().slot = block_slot; + attestation.data_mut().target.epoch = block_slot.epoch(E::slots_per_epoch()); + attestation.data_mut().index = 1; + }, + |result| { + assert_invalid_attestation!( + result, + InvalidAttestation::PayloadAttestationDuringSameSlot { slot } + if slot == Slot::new(1) + ) + }, + ) + .await; +} + /// 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 94a10fb127..e1d307affb 100644 --- a/consensus/proto_array/src/bin.rs +++ b/consensus/proto_array/src/bin.rs @@ -1,4 +1,3 @@ -/* FIXME(sproul) use proto_array::fork_choice_test_definition::*; use std::fs::File; @@ -25,5 +24,3 @@ fn write_test_def_to_yaml(filename: &str, def: ForkChoiceTestDefinition) { let file = File::create(filename).expect("Should be able to open file"); serde_yaml::to_writer(file, &def).expect("Should be able to write YAML to file"); } -*/ -fn main() {} diff --git a/consensus/proto_array/src/error.rs b/consensus/proto_array/src/error.rs index c3e60277a3..d6bd7f2cbf 100644 --- a/consensus/proto_array/src/error.rs +++ b/consensus/proto_array/src/error.rs @@ -55,10 +55,10 @@ pub enum Error { InvalidEpochOffset(u64), Arith(ArithError), GloasNotImplemented, - InvalidNodeVariant{ + InvalidNodeVariant { block_root: Hash256, }, - BrokenBlock{ + BrokenBlock { block_root: Hash256, }, } diff --git a/consensus/proto_array/src/fork_choice_test_definition.rs b/consensus/proto_array/src/fork_choice_test_definition.rs index ac765b51d8..ec4227584a 100644 --- a/consensus/proto_array/src/fork_choice_test_definition.rs +++ b/consensus/proto_array/src/fork_choice_test_definition.rs @@ -1,21 +1,23 @@ -/* FIXME(sproul) fix these tests later mod execution_status; mod ffg_updates; +mod gloas_payload; mod no_votes; mod votes; -use crate::proto_array_fork_choice::{Block, ExecutionStatus, ProtoArrayForkChoice}; +use crate::proto_array::PayloadTiebreak; +use crate::proto_array_fork_choice::{Block, ExecutionStatus, PayloadStatus, ProtoArrayForkChoice}; use crate::{InvalidationOperation, JustifiedBalances}; use fixed_bytes::FixedBytesExtended; use serde::{Deserialize, Serialize}; use std::collections::BTreeSet; use types::{ - AttestationShufflingId, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, Hash256, + AttestationShufflingId, ChainSpec, Checkpoint, Epoch, EthSpec, ExecutionBlockHash, Hash256, MainnetEthSpec, Slot, }; pub use execution_status::*; pub use ffg_updates::*; +pub use gloas_payload::*; pub use no_votes::*; pub use votes::*; @@ -45,11 +47,17 @@ pub enum Operation { parent_root: Hash256, justified_checkpoint: Checkpoint, finalized_checkpoint: Checkpoint, + #[serde(default)] + execution_payload_parent_hash: Option, + #[serde(default)] + execution_payload_block_hash: Option, }, ProcessAttestation { validator_index: usize, block_root: Hash256, - target_epoch: Epoch, + attestation_slot: Slot, + #[serde(default)] + payload_present: bool, }, Prune { finalized_root: Hash256, @@ -64,6 +72,24 @@ pub enum Operation { block_root: Hash256, weight: u64, }, + AssertPayloadWeights { + block_root: Hash256, + expected_full_weight: u64, + expected_empty_weight: u64, + }, + AssertParentPayloadStatus { + block_root: Hash256, + expected_status: PayloadStatus, + }, + AssertHeadPayloadStatus { + head_root: Hash256, + expected_status: PayloadStatus, + }, + SetPayloadTiebreak { + block_root: Hash256, + is_timely: bool, + is_data_available: bool, + }, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -72,12 +98,23 @@ pub struct ForkChoiceTestDefinition { pub justified_checkpoint: Checkpoint, pub finalized_checkpoint: Checkpoint, pub operations: Vec, + #[serde(default)] + pub execution_payload_parent_hash: Option, + #[serde(default)] + pub execution_payload_block_hash: Option, + #[serde(skip)] + pub spec: Option, } impl ForkChoiceTestDefinition { pub fn run(self) { - let mut spec = MainnetEthSpec::default_spec(); - spec.proposer_score_boost = Some(50); + let spec = self.spec.unwrap_or_else(|| { + let mut spec = MainnetEthSpec::default_spec(); + spec.proposer_score_boost = Some(50); + // Legacy test definitions target pre-Gloas behaviour unless explicitly overridden. + spec.gloas_fork_epoch = None; + spec + }); let junk_shuffling_id = AttestationShufflingId::from_components(Epoch::new(0), Hash256::zero()); @@ -90,6 +127,9 @@ impl ForkChoiceTestDefinition { junk_shuffling_id.clone(), junk_shuffling_id, ExecutionStatus::Optimistic(ExecutionBlockHash::zero()), + self.execution_payload_parent_hash, + self.execution_payload_block_hash, + &spec, ) .expect("should create fork choice struct"); let equivocating_indices = BTreeSet::new(); @@ -189,6 +229,8 @@ impl ForkChoiceTestDefinition { parent_root, justified_checkpoint, finalized_checkpoint, + execution_payload_parent_hash, + execution_payload_block_hash, } => { let block = Block { slot, @@ -212,6 +254,8 @@ impl ForkChoiceTestDefinition { ), unrealized_justified_checkpoint: None, unrealized_finalized_checkpoint: None, + execution_payload_parent_hash, + execution_payload_block_hash, }; fork_choice .process_block::( @@ -219,6 +263,7 @@ impl ForkChoiceTestDefinition { slot, self.justified_checkpoint, self.finalized_checkpoint, + &spec, ) .unwrap_or_else(|e| { panic!( @@ -228,14 +273,19 @@ impl ForkChoiceTestDefinition { }); check_bytes_round_trip(&fork_choice); } - // FIXME(sproul): update with payload_present Operation::ProcessAttestation { validator_index, block_root, - target_epoch, + attestation_slot, + payload_present, } => { fork_choice - .process_attestation(validator_index, block_root, target_epoch, false) + .process_attestation( + validator_index, + block_root, + attestation_slot, + payload_present, + ) .unwrap_or_else(|_| { panic!( "process_attestation op at index {} returned error", @@ -289,8 +339,141 @@ impl ForkChoiceTestDefinition { Operation::AssertWeight { block_root, weight } => assert_eq!( fork_choice.get_weight(&block_root).unwrap(), weight, - "block weight" + "block weight at op index {}", + op_index ), + Operation::AssertPayloadWeights { + block_root, + expected_full_weight, + expected_empty_weight, + } => { + let block_index = fork_choice + .proto_array + .indices + .get(&block_root) + .unwrap_or_else(|| { + panic!( + "AssertPayloadWeights: block root not found at op index {}", + op_index + ) + }); + let node = fork_choice + .proto_array + .nodes + .get(*block_index) + .unwrap_or_else(|| { + panic!( + "AssertPayloadWeights: node not found at op index {}", + op_index + ) + }); + let v29 = node.as_v29().unwrap_or_else(|_| { + panic!( + "AssertPayloadWeights: node is not V29 at op index {}", + op_index + ) + }); + assert_eq!( + v29.full_payload_weight, expected_full_weight, + "full_payload_weight mismatch at op index {}", + op_index + ); + assert_eq!( + v29.empty_payload_weight, expected_empty_weight, + "empty_payload_weight mismatch at op index {}", + op_index + ); + } + Operation::AssertParentPayloadStatus { + block_root, + expected_status, + } => { + let block_index = fork_choice + .proto_array + .indices + .get(&block_root) + .unwrap_or_else(|| { + panic!( + "AssertParentPayloadStatus: block root not found at op index {}", + op_index + ) + }); + let node = fork_choice + .proto_array + .nodes + .get(*block_index) + .unwrap_or_else(|| { + panic!( + "AssertParentPayloadStatus: node not found at op index {}", + op_index + ) + }); + let v29 = node.as_v29().unwrap_or_else(|_| { + panic!( + "AssertParentPayloadStatus: node is not V29 at op index {}", + op_index + ) + }); + assert_eq!( + v29.parent_payload_status, expected_status, + "parent_payload_status mismatch at op index {}", + op_index + ); + } + Operation::AssertHeadPayloadStatus { + head_root, + expected_status, + } => { + let actual = fork_choice + .head_payload_status(&head_root) + .unwrap_or_else(|| { + panic!( + "AssertHeadPayloadStatus: head root not found at op index {}", + op_index + ) + }); + assert_eq!( + actual, expected_status, + "head_payload_status mismatch at op index {}", + op_index + ); + } + Operation::SetPayloadTiebreak { + block_root, + is_timely, + is_data_available, + } => { + let block_index = fork_choice + .proto_array + .indices + .get(&block_root) + .unwrap_or_else(|| { + panic!( + "SetPayloadTiebreak: block root not found at op index {}", + op_index + ) + }); + let node = fork_choice + .proto_array + .nodes + .get_mut(*block_index) + .unwrap_or_else(|| { + panic!( + "SetPayloadTiebreak: node not found at op index {}", + op_index + ) + }); + let node_v29 = node.as_v29_mut().unwrap_or_else(|_| { + panic!( + "SetPayloadTiebreak: node is not V29 at op index {}", + op_index + ) + }); + node_v29.payload_tiebreak = PayloadTiebreak { + is_timely, + is_data_available, + }; + } } } } @@ -325,4 +508,3 @@ fn check_bytes_round_trip(original: &ProtoArrayForkChoice) { "fork choice should encode and decode without change" ); } -*/ 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 aa26a84306..93c97d09db 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 @@ -35,6 +35,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is 2 @@ -73,6 +75,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -101,7 +105,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(1), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -143,7 +148,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(2), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is 2 since 1 and 2 both have a vote @@ -196,6 +202,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -245,7 +253,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(3), - target_epoch: Epoch::new(3), + attestation_slot: Slot::new(3), + payload_present: false, }); // Ensure that the head is still 2 @@ -347,7 +356,8 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(1), - target_epoch: Epoch::new(3), + attestation_slot: Slot::new(3), + payload_present: false, }); // Ensure that the head has switched back to 1 @@ -399,6 +409,9 @@ pub fn get_execution_status_test_definition_01() -> ForkChoiceTestDefinition { root: get_root(0), }, operations: ops, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } @@ -437,6 +450,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is 2 @@ -475,6 +490,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -503,7 +520,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(1), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -545,7 +563,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(2), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is 2 since 1 and 2 both have a vote @@ -598,6 +617,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -647,7 +668,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(3), - target_epoch: Epoch::new(3), + attestation_slot: Slot::new(3), + payload_present: false, }); // Move validator #1 vote from 2 to 3 @@ -660,7 +682,8 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(3), - target_epoch: Epoch::new(3), + attestation_slot: Slot::new(3), + payload_present: false, }); // Ensure that the head is now 3. @@ -763,6 +786,9 @@ pub fn get_execution_status_test_definition_02() -> ForkChoiceTestDefinition { root: get_root(0), }, operations: ops, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } @@ -801,6 +827,8 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is 2 @@ -839,6 +867,8 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -867,7 +897,8 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(1), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -909,7 +940,8 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(1), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is 1. @@ -962,6 +994,8 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is now 3, applying a proposer boost to 3 as well. @@ -1065,6 +1099,9 @@ pub fn get_execution_status_test_definition_03() -> ForkChoiceTestDefinition { root: get_root(0), }, operations: ops, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } 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 3b31616145..ee55ea649f 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 @@ -27,6 +27,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { 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, }); ops.push(Operation::ProcessBlock { slot: Slot::new(2), @@ -34,6 +36,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { parent_root: get_root(1), justified_checkpoint: get_checkpoint(1), finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(3), @@ -41,6 +45,8 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { parent_root: get_root(2), justified_checkpoint: get_checkpoint(2), finalized_checkpoint: get_checkpoint(1), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that with justified epoch 0 we find 3 @@ -101,6 +107,9 @@ pub fn get_ffg_case_01_test_definition() -> ForkChoiceTestDefinition { justified_checkpoint: get_checkpoint(0), finalized_checkpoint: get_checkpoint(0), operations: ops, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } @@ -137,6 +146,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { 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, }); ops.push(Operation::ProcessBlock { slot: Slot::new(2), @@ -147,6 +158,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { root: get_root(1), }, finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(3), @@ -157,6 +170,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { root: get_root(1), }, finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(4), @@ -167,6 +182,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { root: get_root(1), }, finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(5), @@ -177,6 +194,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { root: get_root(3), }, finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Right branch @@ -186,6 +205,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { 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, }); ops.push(Operation::ProcessBlock { slot: Slot::new(2), @@ -193,6 +214,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { parent_root: get_root(2), justified_checkpoint: get_checkpoint(0), finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(3), @@ -200,6 +223,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { parent_root: get_root(4), justified_checkpoint: get_checkpoint(0), finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(4), @@ -210,6 +235,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { root: get_root(2), }, finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(5), @@ -220,6 +247,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { root: get_root(4), }, finalized_checkpoint: get_checkpoint(0), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that if we start at 0 we find 10 (just: 0, fin: 0). @@ -282,7 +311,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(1), - target_epoch: Epoch::new(0), + attestation_slot: Slot::new(0), + payload_present: false, }); // Ensure that if we start at 0 we find 9 (just: 0, fin: 0). @@ -345,7 +375,8 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(2), - target_epoch: Epoch::new(0), + attestation_slot: Slot::new(0), + payload_present: false, }); // Ensure that if we start at 0 we find 10 (just: 0, fin: 0). @@ -489,6 +520,9 @@ pub fn get_ffg_case_02_test_definition() -> ForkChoiceTestDefinition { justified_checkpoint: get_checkpoint(0), finalized_checkpoint: get_checkpoint(0), operations: ops, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } 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 new file mode 100644 index 0000000000..b6568106e3 --- /dev/null +++ b/consensus/proto_array/src/fork_choice_test_definition/gloas_payload.rs @@ -0,0 +1,222 @@ +use super::*; + +fn gloas_spec() -> ChainSpec { + let mut spec = MainnetEthSpec::default_spec(); + spec.proposer_score_boost = Some(50); + spec.gloas_fork_epoch = Some(Epoch::new(0)); + spec +} + +pub fn get_gloas_chain_following_test_definition() -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + // Build two branches off genesis where one child extends parent's payload chain (Full) + // and the other does not (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)), + }); + + // Extend both branches to verify that head selection follows the selected chain. + 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)), + }); + + ops.push(Operation::AssertParentPayloadStatus { + block_root: get_root(1), + expected_status: PayloadStatus::Full, + }); + ops.push(Operation::AssertParentPayloadStatus { + block_root: get_root(2), + expected_status: PayloadStatus::Empty, + }); + + // With equal full/empty parent weights, tiebreak decides which chain to follow. + 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), + }); + + ops.push(Operation::SetPayloadTiebreak { + block_root: get_root(0), + is_timely: false, + is_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), + }); + + 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_payload_probe_test_definition() -> ForkChoiceTestDefinition { + let mut ops = vec![]; + + 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)), + }); + + // One Full and one Empty vote for the same head block: tie should probe as Full. + ops.push(Operation::ProcessAttestation { + validator_index: 0, + block_root: get_root(1), + attestation_slot: Slot::new(2), + payload_present: true, + }); + ops.push(Operation::ProcessAttestation { + validator_index: 1, + block_root: get_root(1), + attestation_slot: Slot::new(2), + payload_present: 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(1), + }); + ops.push(Operation::AssertPayloadWeights { + block_root: get_root(1), + expected_full_weight: 1, + expected_empty_weight: 1, + }); + ops.push(Operation::AssertHeadPayloadStatus { + head_root: get_root(1), + expected_status: PayloadStatus::Full, + }); + + // Flip validator 0 to Empty; probe should now report Empty. + ops.push(Operation::ProcessAttestation { + validator_index: 0, + block_root: get_root(1), + attestation_slot: Slot::new(3), + payload_present: 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(1), + }); + ops.push(Operation::AssertPayloadWeights { + block_root: get_root(1), + expected_full_weight: 0, + expected_empty_weight: 2, + }); + ops.push(Operation::AssertHeadPayloadStatus { + head_root: get_root(1), + expected_status: PayloadStatus::Empty, + }); + + // Same-slot attestation to a new head candidate should be Pending (no payload bucket change). + ops.push(Operation::ProcessBlock { + slot: Slot::new(3), + root: get_root(5), + 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(5)), + }); + ops.push(Operation::ProcessAttestation { + validator_index: 2, + block_root: get_root(5), + attestation_slot: Slot::new(3), + payload_present: true, + }); + 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(5), + }); + ops.push(Operation::AssertPayloadWeights { + block_root: get_root(5), + expected_full_weight: 0, + expected_empty_weight: 0, + }); + ops.push(Operation::AssertHeadPayloadStatus { + head_root: get_root(5), + expected_status: PayloadStatus::Full, + }); + + ForkChoiceTestDefinition { + finalized_block_slot: Slot::new(0), + justified_checkpoint: get_checkpoint(0), + finalized_checkpoint: get_checkpoint(0), + operations: ops, + execution_payload_parent_hash: Some(get_hash(42)), + execution_payload_block_hash: Some(get_hash(0)), + spec: Some(gloas_spec()), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn chain_following() { + let test = get_gloas_chain_following_test_definition(); + test.run(); + } + + #[test] + fn payload_probe() { + let test = get_gloas_payload_probe_test_definition(); + test.run(); + } +} diff --git a/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs b/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs index d20eaacb99..61e4c1270c 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/no_votes.rs @@ -36,6 +36,8 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: Hash256::zero(), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, // Ensure the head is 2 // @@ -71,6 +73,8 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: Hash256::zero(), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, // Ensure the head is still 2 // @@ -108,6 +112,8 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: Hash256::zero(), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, // Ensure 2 is still the head // @@ -147,6 +153,8 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: Hash256::zero(), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, // Ensure the head is 4. // @@ -185,6 +193,8 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: Hash256::zero(), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, // Ensure the head is now 5 whilst the justified epoch is 0. // @@ -271,6 +281,8 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: Hash256::zero(), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, // Ensure 6 is the head // @@ -305,6 +317,9 @@ pub fn get_no_votes_test_definition() -> ForkChoiceTestDefinition { root: Hash256::zero(), }, operations, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } 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 01994fff9b..d170e0974f 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/votes.rs @@ -35,6 +35,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is 2 @@ -73,6 +75,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -101,7 +105,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(1), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is now 1, because 1 has a vote. @@ -130,7 +135,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(2), - target_epoch: Epoch::new(2), + attestation_slot: Slot::new(2), + payload_present: false, }); // Ensure that the head is 2 since 1 and 2 both have a vote @@ -170,6 +176,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is still 2 @@ -202,7 +210,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(3), - target_epoch: Epoch::new(3), + attestation_slot: Slot::new(3), + payload_present: false, }); // Ensure that the head is still 2 @@ -236,7 +245,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(1), - target_epoch: Epoch::new(3), + attestation_slot: Slot::new(3), + payload_present: false, }); // Ensure that the head is now 3 @@ -280,6 +290,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure that the head is now 4 @@ -327,9 +339,11 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(2), root: get_root(1), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); - // Ensure that 5 is filtered out and the head stays at 4. + // Ensure that 5 becomes the head. // // 0 // / \ @@ -337,9 +351,9 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 // | - // 4 <- head + // 4 // / - // 5 + // head-> 5 ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(1), @@ -350,7 +364,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { root: get_root(0), }, justified_state_balances: balances.clone(), - expected_head: get_root(4), + expected_head: get_root(5), }); // Add block 6, which has a justified epoch of 0. @@ -376,6 +390,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(1), root: get_root(0), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Move both votes to 5. @@ -392,12 +408,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(5), - target_epoch: Epoch::new(4), + attestation_slot: Slot::new(4), + payload_present: false, }); ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(5), - target_epoch: Epoch::new(4), + attestation_slot: Slot::new(4), + payload_present: false, }); // Add blocks 7, 8 and 9. Adding these blocks helps test the `best_descendant` @@ -430,6 +448,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(2), root: get_root(5), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(0), @@ -443,6 +463,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(2), root: get_root(5), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); ops.push(Operation::ProcessBlock { slot: Slot::new(0), @@ -456,10 +478,12 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(2), root: get_root(5), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); - // Ensure that 6 is the head, even though 5 has all the votes. This is testing to ensure - // that 5 is filtered out due to a differing justified epoch. + // Ensure that 9 is the head. The branch rooted at 5 remains viable and its best descendant + // is selected. // // 0 // / \ @@ -469,13 +493,13 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 4 // / \ - // 5 6 <- head + // 5 6 // | // 7 // | // 8 // / - // 9 + // head-> 9 ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(1), @@ -486,7 +510,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { root: get_root(0), }, justified_state_balances: balances.clone(), - expected_head: get_root(6), + expected_head: get_root(9), }); // Change fork-choice justified epoch to 1, and the start block to 5 and ensure that 9 is @@ -545,12 +569,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 0, block_root: get_root(9), - target_epoch: Epoch::new(5), + attestation_slot: Slot::new(5), + payload_present: false, }); ops.push(Operation::ProcessAttestation { validator_index: 1, block_root: get_root(9), - target_epoch: Epoch::new(5), + attestation_slot: Slot::new(5), + payload_present: false, }); // Add block 10 @@ -582,6 +608,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(2), root: get_root(5), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Double-check the head is still 9 (no diagram this time) @@ -621,12 +649,14 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { ops.push(Operation::ProcessAttestation { validator_index: 2, block_root: get_root(10), - target_epoch: Epoch::new(5), + attestation_slot: Slot::new(5), + payload_present: false, }); ops.push(Operation::ProcessAttestation { validator_index: 3, block_root: get_root(10), - target_epoch: Epoch::new(5), + attestation_slot: Slot::new(5), + payload_present: false, }); // Check the head is now 10. @@ -817,6 +847,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { epoch: Epoch::new(2), root: get_root(5), }, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }); // Ensure the head is now 11 @@ -854,6 +886,9 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { root: get_root(0), }, operations: ops, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, + spec: None, } } diff --git a/consensus/proto_array/src/lib.rs b/consensus/proto_array/src/lib.rs index 1f126246b3..b131fb403e 100644 --- a/consensus/proto_array/src/lib.rs +++ b/consensus/proto_array/src/lib.rs @@ -16,5 +16,7 @@ pub use error::Error; pub mod core { pub use super::proto_array::{ProposerBoost, ProtoArray, ProtoNode}; pub use super::proto_array_fork_choice::VoteTracker; - pub use super::ssz_container::{SszContainer, SszContainerV17, SszContainerV28}; + pub use super::ssz_container::{ + SszContainer, SszContainerLegacyV17, SszContainerLegacyV28, SszContainerV29, + }; } diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 1eb7cc9d88..926767093f 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -1,5 +1,5 @@ use crate::error::InvalidBestNodeInfo; -use crate::{Block, ExecutionStatus, JustifiedBalances, error::Error, PayloadStatus}; +use crate::{Block, ExecutionStatus, JustifiedBalances, PayloadStatus, error::Error}; use fixed_bytes::FixedBytesExtended; use serde::{Deserialize, Serialize}; use ssz::Encode; @@ -68,13 +68,12 @@ impl InvalidationOperation { } } - #[superstruct( variants(V17, V29), - variant_attributes(derive(Clone, PartialEq, Debug, Encode, Decode, Serialize, Deserialize)), + variant_attributes(derive(Clone, PartialEq, Debug, Encode, Decode, Serialize, Deserialize)) )] #[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Clone)] -#[ssz(enum_behaviour = "transparent")] +#[ssz(enum_behaviour = "union")] pub struct ProtoNode { /// The `slot` is not necessary for `ProtoArray`, it just exists so external components can /// easily query the block slot. This is useful for upstream fork choice logic. @@ -130,6 +129,10 @@ 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. + /// Per spec: prefer Full if block was timely and data is available; otherwise prefer Empty. + #[superstruct(only(V29), partial_getter(copy))] + pub payload_tiebreak: PayloadTiebreak, } #[derive(PartialEq, Debug, Encode, Decode, Serialize, Deserialize, Copy, Clone)] @@ -147,6 +150,83 @@ impl Default for ProposerBoost { } } +#[derive(Clone, PartialEq, Debug, Copy)] +pub struct NodeDelta { + pub delta: i64, + pub empty_delta: i64, + pub full_delta: i64, + pub payload_tiebreaker: Option, +} + +impl NodeDelta { + /// Determine the payload bucket for a vote based on whether the vote's slot matches the + /// block's slot (Pending), or the vote's `payload_present` flag (Full/Empty). + pub fn payload_status( + vote_slot: Slot, + payload_present: bool, + block_slot: Slot, + ) -> PayloadStatus { + if vote_slot == block_slot { + PayloadStatus::Pending + } else if payload_present { + PayloadStatus::Full + } else { + PayloadStatus::Empty + } + } + + /// Add a balance to the appropriate payload status. + pub fn add_payload_delta( + &mut self, + status: PayloadStatus, + balance: u64, + index: usize, + ) -> Result<(), Error> { + let field = match status { + PayloadStatus::Full => &mut self.full_delta, + PayloadStatus::Empty => &mut self.empty_delta, + PayloadStatus::Pending => return Ok(()), + }; + *field = field + .checked_add(balance as i64) + .ok_or(Error::DeltaOverflow(index))?; + Ok(()) + } + + /// Subtract a balance from the appropriate payload status. + pub fn sub_payload_delta( + &mut self, + status: PayloadStatus, + balance: u64, + index: usize, + ) -> Result<(), Error> { + let field = match status { + PayloadStatus::Full => &mut self.full_delta, + PayloadStatus::Empty => &mut self.empty_delta, + PayloadStatus::Pending => return Ok(()), + }; + *field = field + .checked_sub(balance as i64) + .ok_or(Error::DeltaOverflow(index))?; + Ok(()) + } +} + +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 @@ -174,7 +254,7 @@ impl ProtoArray { #[allow(clippy::too_many_arguments)] pub fn apply_score_changes( &mut self, - mut deltas: Vec, + mut deltas: Vec, best_justified_checkpoint: Checkpoint, best_finalized_checkpoint: Checkpoint, new_justified_balances: &JustifiedBalances, @@ -206,16 +286,32 @@ impl ProtoArray { continue; } - let mut node_delta = if let Ok(proto_node) = node.as_v17() && proto_node.execution_status.is_invalid() { + let execution_status_is_invalid = if let Ok(proto_node) = node.as_v17() + && proto_node.execution_status.is_invalid() + { + true + } else { + false + }; + + let node_deltas = deltas + .get(node_index) + .copied() + .ok_or(Error::InvalidNodeDelta(node_index))?; + + let mut node_delta = if execution_status_is_invalid { // If the node has an invalid execution payload, reduce its weight to zero. 0_i64 .checked_sub(node.weight() as i64) .ok_or(Error::InvalidExecutionDeltaOverflow(node_index))? } else { - deltas - .get(node_index) - .copied() - .ok_or(Error::InvalidNodeDelta(node_index))? + node_deltas.delta + }; + + let (node_empty_delta, node_full_delta) = if node.as_v29().is_ok() { + (node_deltas.empty_delta, node_deltas.full_delta) + } else { + (0, 0) }; // If we find the node for which the proposer boost was previously applied, decrease @@ -250,27 +346,17 @@ impl ProtoArray { // Apply the delta to the node. if execution_status_is_invalid { - // Invalid nodes always have a weight of 0. - node.weight() = 0 - } else if node_delta < 0 { - // Note: I am conflicted about whether to use `saturating_sub` or `checked_sub` - // here. - // - // I can't think of any valid reason why `node_delta.abs()` should be greater than - // `node.weight`, so I have chosen `checked_sub` to try and fail-fast if there is - // some error. - // - // However, I am not fully convinced that some valid case for `saturating_sub` does - // not exist. - node.weight() = node - .weight() - .checked_sub(node_delta.unsigned_abs()) - .ok_or(Error::DeltaOverflow(node_index))?; + *node.weight_mut() = 0; } else { - node.weight = node - .weight() - .checked_add(node_delta as u64) - .ok_or(Error::DeltaOverflow(node_index))?; + *node.weight_mut() = apply_delta(node.weight(), node_delta, node_index)?; + } + + // Apply post-Gloas score deltas. + if let Ok(node) = node.as_v29_mut() { + node.empty_payload_weight = + 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)?; } // Update the parent delta (if any). @@ -279,8 +365,32 @@ impl ProtoArray { .get_mut(parent_index) .ok_or(Error::InvalidParentDelta(parent_index))?; - // Back-propagate the nodes delta to its parent. - *parent_delta += node_delta; + // Back-propagate the node's delta to its parent. + parent_delta.delta = parent_delta + .delta + .checked_add(node_delta) + .ok_or(Error::DeltaOverflow(parent_index))?; + + // Per spec's `is_supporting_vote`: a vote for descendant B supports + // ancestor A's payload status based on B's `parent_payload_status`. + // Route the child's *total* weight delta to the parent's appropriate + // payload bucket. + match node.parent_payload_status() { + Ok(PayloadStatus::Full) => { + parent_delta.full_delta = parent_delta + .full_delta + .checked_add(node_delta) + .ok_or(Error::DeltaOverflow(parent_index))?; + } + Ok(PayloadStatus::Empty) => { + parent_delta.empty_delta = parent_delta + .empty_delta + .checked_add(node_delta) + .ok_or(Error::DeltaOverflow(parent_index))?; + } + // Pending or V17 nodes: no payload propagation. + _ => {} + } } } @@ -357,26 +467,40 @@ impl ProtoArray { unrealized_finalized_checkpoint: block.unrealized_finalized_checkpoint, }) } else { - let execution_payload_block_hash = block - .execution_payload_block_hash - .ok_or(Error::BrokenBlock{block_root: block.root})?; + let execution_payload_block_hash = + block + .execution_payload_block_hash + .ok_or(Error::BrokenBlock { + block_root: block.root, + })?; - let parent_payload_status: PayloadStatus = - if let Some(parent_node) = - parent_index.and_then(|idx| self.nodes.get(idx)) - { - let v29 = parent_node - .as_v29() - .map_err(|_| Error::InvalidNodeVariant{block_root: block.root})?; - if execution_payload_block_hash == v29.execution_payload_block_hash - { - PayloadStatus::Empty - } else { - PayloadStatus::Full - } - } else { - PayloadStatus::Full + let execution_payload_parent_hash = + block + .execution_payload_parent_hash + .ok_or(Error::BrokenBlock { + 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. + 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 + } else { + PayloadStatus::Empty + } + } else { + PayloadStatus::Full + }; ProtoNode::V29(ProtoNodeV29 { slot: block.slot, @@ -397,6 +521,7 @@ impl ProtoArray { empty_payload_weight: 0, full_payload_weight: 0, execution_payload_block_hash, + payload_tiebreak: PayloadTiebreak::default(), }) }; @@ -408,7 +533,9 @@ impl ProtoArray { .get(parent_index) .ok_or(Error::InvalidNodeIndex(parent_index))?; - if let Ok(status) = parent.execution_status() && status.is_invalid() { + if let Ok(status) = parent.execution_status() + && status.is_invalid() + { return Err(Error::ParentExecutionStatusIsInvalid { block_root: block.root, parent_root: parent.root(), @@ -469,33 +596,43 @@ impl ProtoArray { .nodes .get_mut(index) .ok_or(Error::InvalidNodeIndex(index))?; - let parent_index = match node.execution_status { - // We have reached a node that we already know is valid. No need to iterate further - // since we assume an ancestors have already been set to valid. - ExecutionStatus::Valid(_) => return Ok(()), - // We have reached an irrelevant node, this node is prior to a terminal execution - // block. There's no need to iterate further, it's impossible for this block to have - // any relevant ancestors. - ExecutionStatus::Irrelevant(_) => return Ok(()), - // The block has an unknown status, set it to valid since any ancestor of a valid - // payload can be considered valid. - ExecutionStatus::Optimistic(payload_block_hash) => { - node.execution_status = ExecutionStatus::Valid(payload_block_hash); + let parent_index = match node { + ProtoNode::V17(node) => match node.execution_status { + // We have reached a node that we already know is valid. No need to iterate further + // since we assume an ancestors have already been set to valid. + ExecutionStatus::Valid(_) => return Ok(()), + // We have reached an irrelevant node, this node is prior to a terminal execution + // block. There's no need to iterate further, it's impossible for this block to have + // any relevant ancestors. + ExecutionStatus::Irrelevant(_) => return Ok(()), + // The block has an unknown status, set it to valid since any ancestor of a valid + // payload can be considered valid. + ExecutionStatus::Optimistic(payload_block_hash) => { + node.execution_status = ExecutionStatus::Valid(payload_block_hash); + if let Some(parent_index) = node.parent { + parent_index + } else { + // We have reached the root block, iteration complete. + return Ok(()); + } + } + // An ancestor of the valid payload was invalid. This is a serious error which + // indicates a consensus failure in the execution node. This is unrecoverable. + ExecutionStatus::Invalid(ancestor_payload_block_hash) => { + return Err(Error::InvalidAncestorOfValidPayload { + ancestor_block_root: node.root, + ancestor_payload_block_hash, + }); + } + }, + // Gloas nodes don't carry `ExecutionStatus`. + ProtoNode::V29(node) => { if let Some(parent_index) = node.parent { parent_index } else { - // We have reached the root block, iteration complete. return Ok(()); } } - // An ancestor of the valid payload was invalid. This is a serious error which - // indicates a consensus failure in the execution node. This is unrecoverable. - ExecutionStatus::Invalid(ancestor_payload_block_hash) => { - return Err(Error::InvalidAncestorOfValidPayload { - ancestor_block_root: node.root, - ancestor_payload_block_hash, - }); - } }; index = parent_index; @@ -551,10 +688,11 @@ impl ProtoArray { .get_mut(index) .ok_or(Error::InvalidNodeIndex(index))?; - match node.execution_status { - ExecutionStatus::Valid(hash) - | ExecutionStatus::Invalid(hash) - | ExecutionStatus::Optimistic(hash) => { + let node_execution_status = node.execution_status(); + match node_execution_status { + Ok(ExecutionStatus::Valid(hash)) + | Ok(ExecutionStatus::Invalid(hash)) + | Ok(ExecutionStatus::Optimistic(hash)) => { // If we're no longer processing the `head_block_root` and the last valid // ancestor is unknown, exit this loop and proceed to invalidate and // descendants of `head_block_root`/`latest_valid_ancestor_root`. @@ -563,7 +701,7 @@ impl ProtoArray { // supplied, don't validate any ancestors. The alternative is to invalidate // *all* ancestors, which would likely involve shutting down the client due to // an invalid justified checkpoint. - if !latest_valid_ancestor_is_descendant && node.root != head_block_root { + if !latest_valid_ancestor_is_descendant && node.root() != head_block_root { break; } else if op.latest_valid_ancestor() == Some(hash) { // If the `best_child` or `best_descendant` of the latest valid hash was @@ -574,63 +712,67 @@ impl ProtoArray { // defend against errors which might result in an invalid block being set as // head. if node - .best_child + .best_child() .is_some_and(|i| invalidated_indices.contains(&i)) { - node.best_child = None + *node.best_child_mut() = None } if node - .best_descendant + .best_descendant() .is_some_and(|i| invalidated_indices.contains(&i)) { - node.best_descendant = None + *node.best_descendant_mut() = None } break; } } - ExecutionStatus::Irrelevant(_) => break, + Ok(ExecutionStatus::Irrelevant(_)) => break, + Err(_) => break, } // Only invalidate the head block if either: // // - The head block was specifically indicated to be invalidated. // - The latest valid hash is a known ancestor. - if node.root != head_block_root + if node.root() != head_block_root || op.invalidate_block_root() || latest_valid_ancestor_is_descendant { - match &node.execution_status { + match node.execution_status() { // It's illegal for an execution client to declare that some previously-valid block // is now invalid. This is a consensus failure on their behalf. - ExecutionStatus::Valid(hash) => { + Ok(ExecutionStatus::Valid(hash)) => { return Err(Error::ValidExecutionStatusBecameInvalid { - block_root: node.root, - payload_block_hash: *hash, + block_root: node.root(), + payload_block_hash: hash, }); } - ExecutionStatus::Optimistic(hash) => { + Ok(ExecutionStatus::Optimistic(hash)) => { invalidated_indices.insert(index); - node.execution_status = ExecutionStatus::Invalid(*hash); + if let ProtoNode::V17(node) = node { + node.execution_status = ExecutionStatus::Invalid(hash); + } // It's impossible for an invalid block to lead to a "best" block, so set these // fields to `None`. // // Failing to set these values will result in `Self::node_leads_to_viable_head` // returning `false` for *valid* ancestors of invalid blocks. - node.best_child = None; - node.best_descendant = None; + *node.best_child_mut() = None; + *node.best_descendant_mut() = None; } // The block is already invalid, but keep going backwards to ensure all ancestors // are updated. - ExecutionStatus::Invalid(_) => (), + Ok(ExecutionStatus::Invalid(_)) => (), // This block is pre-merge, therefore it has no execution status. Nor do its // ancestors. - ExecutionStatus::Irrelevant(_) => break, + Ok(ExecutionStatus::Irrelevant(_)) => break, + Err(_) => (), } } - if let Some(parent_index) = node.parent { + if let Some(parent_index) = node.parent() { index = parent_index } else { // The root of the block tree has been reached (aka the finalized block), without @@ -664,24 +806,27 @@ impl ProtoArray { .get_mut(index) .ok_or(Error::InvalidNodeIndex(index))?; - if let Some(parent_index) = node.parent + if let Some(parent_index) = node.parent() && invalidated_indices.contains(&parent_index) { - match &node.execution_status { - ExecutionStatus::Valid(hash) => { + match node.execution_status() { + Ok(ExecutionStatus::Valid(hash)) => { return Err(Error::ValidExecutionStatusBecameInvalid { - block_root: node.root, - payload_block_hash: *hash, + block_root: node.root(), + payload_block_hash: hash, }); } - ExecutionStatus::Optimistic(hash) | ExecutionStatus::Invalid(hash) => { - node.execution_status = ExecutionStatus::Invalid(*hash) + Ok(ExecutionStatus::Optimistic(hash)) | Ok(ExecutionStatus::Invalid(hash)) => { + if let ProtoNode::V17(node) = node { + node.execution_status = ExecutionStatus::Invalid(hash) + } } - ExecutionStatus::Irrelevant(_) => { + Ok(ExecutionStatus::Irrelevant(_)) => { return Err(Error::IrrelevantDescendant { - block_root: node.root, + block_root: node.root(), }); } + Err(_) => (), } invalidated_indices.insert(index); @@ -724,13 +869,15 @@ impl ProtoArray { // practically possible to set a new justified root if we are unable to find a new head. // // This scenario is *unsupported*. It represents a serious consensus failure. - if justified_node.execution_status.is_invalid() { + if let Ok(execution_status) = justified_node.execution_status() + && execution_status.is_invalid() + { return Err(Error::InvalidJustifiedCheckpointExecutionStatus { justified_root: *justified_root, }); } - let best_descendant_index = justified_node.best_descendant.unwrap_or(justified_index); + let best_descendant_index = justified_node.best_descendant().unwrap_or(justified_index); let best_node = self .nodes @@ -749,13 +896,13 @@ impl ProtoArray { start_root: *justified_root, justified_checkpoint: best_justified_checkpoint, finalized_checkpoint: best_finalized_checkpoint, - head_root: best_node.root, - head_justified_checkpoint: best_node.justified_checkpoint, - head_finalized_checkpoint: best_node.finalized_checkpoint, + head_root: best_node.root(), + head_justified_checkpoint: *best_node.justified_checkpoint(), + head_finalized_checkpoint: *best_node.finalized_checkpoint(), }))); } - Ok(best_node.root) + Ok(best_node.root()) } /// Update the tree with new finalization information. The tree is only actually pruned if both @@ -788,7 +935,7 @@ impl ProtoArray { .nodes .get(node_index) .ok_or(Error::InvalidNodeIndex(node_index))? - .root; + .root(); self.indices.remove(root); } @@ -805,19 +952,19 @@ impl ProtoArray { // Iterate through all the existing nodes and adjust their indices to match the new layout // of `self.nodes`. for node in self.nodes.iter_mut() { - if let Some(parent) = node.parent { + if let Some(parent) = node.parent() { // If `node.parent` is less than `finalized_index`, set it to `None`. - node.parent = parent.checked_sub(finalized_index); + *node.parent_mut() = parent.checked_sub(finalized_index); } - if let Some(best_child) = node.best_child { - node.best_child = Some( + if let Some(best_child) = node.best_child() { + *node.best_child_mut() = Some( best_child .checked_sub(finalized_index) .ok_or(Error::IndexOverflow("best_child"))?, ); } - if let Some(best_descendant) = node.best_descendant { - node.best_descendant = Some( + if let Some(best_descendant) = node.best_descendant() { + *node.best_descendant_mut() = Some( best_descendant .checked_sub(finalized_index) .ok_or(Error::IndexOverflow("best_descendant"))?, @@ -905,19 +1052,32 @@ 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() { - // Tie-breaker of equal weights by root. - if *child.root() >= *best_child.root() { - change_to_child - } else { - no_change - } } else { - // Choose the winner by weight. - if child.weight() > best_child.weight() { + // 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). + let child_matches = child_matches_parent_payload_preference(parent, child); + let best_child_matches = + child_matches_parent_payload_preference(parent, best_child); + + if child_matches && !best_child_matches { change_to_child - } else { + } 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 { + // Choose the winner by weight. + if child.weight() > best_child.weight() { + change_to_child + } else { + no_change + } } } } @@ -988,11 +1148,13 @@ impl ProtoArray { best_justified_checkpoint: Checkpoint, best_finalized_checkpoint: Checkpoint, ) -> bool { - if let Ok(proto_node) = node.as_v17() && proto_node.execution_status.is_invalid() { + if let Ok(proto_node) = node.as_v17() + && proto_node.execution_status.is_invalid() + { return false; } - let genesis_epoch = Epoch::new(0); + let genesis_epoch = Epoch::new(1); let current_epoch = current_slot.epoch(E::slots_per_epoch()); let node_epoch = node.slot().epoch(E::slots_per_epoch()); let node_justified_checkpoint = node.justified_checkpoint(); @@ -1006,7 +1168,7 @@ impl ProtoArray { } else { // The block is not from a prior epoch, therefore the voting source // is not pulled up. - node_justified_checkpoint + *node_justified_checkpoint }; let correct_justified = best_justified_checkpoint.epoch == genesis_epoch @@ -1015,7 +1177,7 @@ impl ProtoArray { let correct_finalized = best_finalized_checkpoint.epoch == genesis_epoch || self - .is_finalized_checkpoint_or_descendant::(node.root, best_finalized_checkpoint); + .is_finalized_checkpoint_or_descendant::(node.root(), best_finalized_checkpoint); correct_justified && correct_finalized } @@ -1037,7 +1199,7 @@ impl ProtoArray { block_root: &Hash256, ) -> impl Iterator + 'a { self.iter_nodes(block_root) - .map(|node| (node.root, node.slot)) + .map(|node| (node.root(), node.slot())) } /// Returns `true` if the `descendant_root` has an ancestor with `ancestor_root`. Always @@ -1058,8 +1220,8 @@ impl ProtoArray { .and_then(|ancestor_index| self.nodes.get(*ancestor_index)) .and_then(|ancestor| { self.iter_block_roots(&descendant_root) - .take_while(|(_root, slot)| *slot >= ancestor.slot) - .find(|(_root, slot)| *slot == ancestor.slot) + .take_while(|(_root, slot)| *slot >= ancestor.slot()) + .find(|(_root, slot)| *slot == ancestor.slot()) .map(|(root, _slot)| root == ancestor_root) }) .unwrap_or(false) @@ -1098,15 +1260,15 @@ impl ProtoArray { // Run this check once, outside of the loop rather than inside the loop. // If the conditions don't match for this node then they're unlikely to // start matching for its ancestors. - for checkpoint in &[node.finalized_checkpoint, node.justified_checkpoint] { - if checkpoint == &best_finalized_checkpoint { + for checkpoint in &[node.finalized_checkpoint(), node.justified_checkpoint()] { + if **checkpoint == best_finalized_checkpoint { return true; } } for checkpoint in &[ - node.unrealized_finalized_checkpoint, - node.unrealized_justified_checkpoint, + node.unrealized_finalized_checkpoint(), + node.unrealized_justified_checkpoint(), ] { if checkpoint.is_some_and(|cp| cp == best_finalized_checkpoint) { return true; @@ -1116,13 +1278,13 @@ impl ProtoArray { loop { // If `node` is less than or equal to the finalized slot then `node` // must be the finalized block. - if node.slot <= finalized_slot { - return node.root == finalized_root; + if node.slot() <= finalized_slot { + return node.root() == finalized_root; } // Since `node` is from a higher slot that the finalized checkpoint, // replace `node` with the parent of `node`. - if let Some(parent) = node.parent.and_then(|index| self.nodes.get(index)) { + if let Some(parent) = node.parent().and_then(|index| self.nodes.get(index)) { node = parent } else { // If `node` is not the finalized block and its parent does not @@ -1144,11 +1306,12 @@ impl ProtoArray { .iter() .rev() .find(|node| { - node.execution_status - .block_hash() + node.execution_status() + .ok() + .and_then(|execution_status| execution_status.block_hash()) .is_some_and(|node_block_hash| node_block_hash == *block_hash) }) - .map(|node| node.root) + .map(|node| node.root()) } /// Returns all nodes that have zero children and are descended from the finalized checkpoint. @@ -1163,9 +1326,9 @@ impl ProtoArray { self.nodes .iter() .filter(|node| { - node.best_child.is_none() + node.best_child().is_none() && self.is_finalized_checkpoint_or_descendant::( - node.root, + node.root(), best_finalized_checkpoint, ) }) @@ -1173,6 +1336,30 @@ impl ProtoArray { } } +/// For V29 parents, returns `true` if the child's `parent_payload_status` matches the parent's +/// preferred payload status. When full and empty weights are unequal, the higher weight wins. +/// When equal, the tiebreaker uses the parent's `payload_tiebreak`: prefer Full if the block +/// was timely and data is available; otherwise prefer Empty. +/// For V17 parents (or mixed), always returns `true` (no payload preference). +fn child_matches_parent_payload_preference(parent: &ProtoNode, child: &ProtoNode) -> bool { + let (Ok(parent_v29), Ok(child_v29)) = (parent.as_v29(), child.as_v29()) else { + return true; + }; + let prefers_full = if parent_v29.full_payload_weight > parent_v29.empty_payload_weight { + true + } else if parent_v29.empty_payload_weight > parent_v29.full_payload_weight { + false + } else { + // Equal weights: tiebreaker per spec + parent_v29.payload_tiebreak.is_timely && parent_v29.payload_tiebreak.is_data_available + }; + if prefers_full { + child_v29.parent_payload_status == PayloadStatus::Full + } else { + child_v29.parent_payload_status == PayloadStatus::Empty + } +} + /// 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 @@ -1188,6 +1375,19 @@ pub fn calculate_committee_fraction( .checked_div(100) } +/// Apply a signed delta to an unsigned weight, returning an error on overflow. +fn apply_delta(weight: u64, delta: i64, index: usize) -> Result { + if delta < 0 { + weight + .checked_sub(delta.unsigned_abs()) + .ok_or(Error::DeltaOverflow(index)) + } else { + weight + .checked_add(delta as u64) + .ok_or(Error::DeltaOverflow(index)) + } +} + /// Reverse iterator over one path through a `ProtoArray`. pub struct Iter<'a> { next_node_index: Option, @@ -1200,7 +1400,7 @@ impl<'a> Iterator for Iter<'a> { fn next(&mut self) -> Option { let next_node_index = self.next_node_index?; let node = self.proto_array.nodes.get(next_node_index)?; - self.next_node_index = node.parent; + self.next_node_index = node.parent(); Some(node) } } diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 928e8ce860..e1c893db9a 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -2,7 +2,7 @@ use crate::{ JustifiedBalances, error::Error, proto_array::{ - InvalidationOperation, Iter, ProposerBoost, ProtoArray, ProtoNode, + InvalidationOperation, Iter, NodeDelta, ProposerBoost, ProtoArray, ProtoNode, calculate_committee_fraction, }, ssz_container::SszContainer, @@ -28,15 +28,17 @@ pub const DEFAULT_PRUNE_THRESHOLD: usize = 256; pub struct VoteTracker { current_root: Hash256, next_root: Hash256, + current_slot: Slot, next_slot: Slot, + current_payload_present: bool, next_payload_present: bool, } // FIXME(sproul): version this type pub struct LatestMessage { - slot: Slot, - root: Hash256, - payload_present: bool, + pub slot: Slot, + pub root: Hash256, + pub payload_present: bool, } /// Represents the verification status of an execution payload pre-Gloas. @@ -448,7 +450,7 @@ impl ProtoArrayForkChoice { execution_status: ExecutionStatus, execution_payload_parent_hash: Option, execution_payload_block_hash: Option, - + spec: &ChainSpec, ) -> Result { let mut proto_array = ProtoArray { prune_threshold: DEFAULT_PRUNE_THRESHOLD, @@ -474,7 +476,6 @@ impl ProtoArrayForkChoice { unrealized_finalized_checkpoint: Some(finalized_checkpoint), execution_payload_parent_hash, execution_payload_block_hash, - }; proto_array @@ -569,9 +570,16 @@ impl ProtoArrayForkChoice { ) -> Result { let old_balances = &mut self.balances; let new_balances = justified_state_balances; + let node_slots = self + .proto_array + .nodes + .iter() + .map(|node| node.slot()) + .collect::>(); let deltas = compute_deltas( &self.proto_array.indices, + &node_slots, &mut self.votes, &old_balances.effective_balances, &new_balances.effective_balances, @@ -628,13 +636,13 @@ impl ProtoArrayForkChoice { )?; // Only re-org a single slot. This prevents cascading failures during asynchrony. - let head_slot_ok = info.head_node.slot + 1 == current_slot; + let head_slot_ok = info.head_node.slot() + 1 == current_slot; if !head_slot_ok { return Err(DoNotReOrg::HeadDistance.into()); } // Only re-org if the head's weight is less than the heads configured committee fraction. - let head_weight = info.head_node.weight; + let head_weight = info.head_node.weight(); let re_org_head_weight_threshold = info.re_org_head_weight_threshold; let weak_head = head_weight < re_org_head_weight_threshold; if !weak_head { @@ -646,7 +654,7 @@ impl ProtoArrayForkChoice { } // Only re-org if the parent's weight is greater than the parents configured committee fraction. - let parent_weight = info.parent_node.weight; + let parent_weight = info.parent_node.weight(); let re_org_parent_weight_threshold = info.re_org_parent_weight_threshold; let parent_strong = parent_weight > re_org_parent_weight_threshold; if !parent_strong { @@ -685,14 +693,14 @@ impl ProtoArrayForkChoice { let parent_node = nodes.pop().ok_or(DoNotReOrg::MissingHeadOrParentNode)?; let head_node = nodes.pop().ok_or(DoNotReOrg::MissingHeadOrParentNode)?; - let parent_slot = parent_node.slot; - let head_slot = head_node.slot; + let parent_slot = parent_node.slot(); + let head_slot = head_node.slot(); let re_org_block_slot = head_slot + 1; // Check finalization distance. let proposal_epoch = re_org_block_slot.epoch(E::slots_per_epoch()); let finalized_epoch = head_node - .unrealized_finalized_checkpoint + .unrealized_finalized_checkpoint() .ok_or(DoNotReOrg::MissingHeadFinalizedCheckpoint)? .epoch; let epochs_since_finalization = proposal_epoch.saturating_sub(finalized_epoch).as_u64(); @@ -724,10 +732,10 @@ impl ProtoArrayForkChoice { } // Check FFG. - let ffg_competitive = parent_node.unrealized_justified_checkpoint - == head_node.unrealized_justified_checkpoint - && parent_node.unrealized_finalized_checkpoint - == head_node.unrealized_finalized_checkpoint; + let ffg_competitive = parent_node.unrealized_justified_checkpoint() + == head_node.unrealized_justified_checkpoint() + && parent_node.unrealized_finalized_checkpoint() + == head_node.unrealized_finalized_checkpoint(); if !ffg_competitive { return Err(DoNotReOrg::JustificationAndFinalizationNotCompetitive.into()); } @@ -755,10 +763,10 @@ impl ProtoArrayForkChoice { /// This will operate on *all* blocks, even those that do not descend from the finalized /// ancestor. pub fn contains_invalid_payloads(&mut self) -> bool { - self.proto_array - .nodes - .iter() - .any(|node| node.execution_status.is_invalid()) + self.proto_array.nodes.iter().any(|node| { + node.execution_status() + .is_ok_and(|status| status.is_invalid()) + }) } /// For all nodes, regardless of their relationship to the finalized block, set their execution @@ -783,9 +791,11 @@ impl ProtoArrayForkChoice { .get_mut(node_index) .ok_or("unreachable index out of bounds in proto_array nodes")?; - match node.execution_status { - ExecutionStatus::Invalid(block_hash) => { - node.execution_status = ExecutionStatus::Optimistic(block_hash); + match node.execution_status() { + Ok(ExecutionStatus::Invalid(block_hash)) => { + if let ProtoNode::V17(node) = node { + node.execution_status = ExecutionStatus::Optimistic(block_hash); + } // Restore the weight of the node, it would have been set to `0` in // `apply_score_changes` when it was invalidated. @@ -795,7 +805,7 @@ impl ProtoArrayForkChoice { .iter() .enumerate() .filter_map(|(validator_index, vote)| { - if vote.current_root == node.root { + if vote.current_root == node.root() { // Any voting validator that does not have a balance should be // ignored. This is consistent with `compute_deltas`. self.balances.effective_balances.get(validator_index) @@ -808,7 +818,7 @@ impl ProtoArrayForkChoice { // If the invalid root was boosted, apply the weight to it and // ancestors. if let Some(proposer_score_boost) = spec.proposer_score_boost - && self.proto_array.previous_proposer_boost.root == node.root + && self.proto_array.previous_proposer_boost.root == node.root() { // Compute the score based upon the current balances. We can't rely on // the `previous_proposr_boost.score` since it is set to zero with an @@ -829,12 +839,12 @@ impl ProtoArrayForkChoice { if restored_weight > 0 { let mut node_or_ancestor = node; loop { - node_or_ancestor.weight = node_or_ancestor - .weight + *node_or_ancestor.weight_mut() = node_or_ancestor + .weight() .checked_add(restored_weight) .ok_or("Overflow when adding weight to ancestor")?; - if let Some(parent_index) = node_or_ancestor.parent { + if let Some(parent_index) = node_or_ancestor.parent() { node_or_ancestor = self .proto_array .nodes @@ -850,11 +860,14 @@ impl ProtoArrayForkChoice { } // There are no balance changes required if the node was either valid or // optimistic. - ExecutionStatus::Valid(block_hash) | ExecutionStatus::Optimistic(block_hash) => { - node.execution_status = ExecutionStatus::Optimistic(block_hash) + Ok(ExecutionStatus::Valid(block_hash)) + | Ok(ExecutionStatus::Optimistic(block_hash)) => { + if let ProtoNode::V17(node) = node { + node.execution_status = ExecutionStatus::Optimistic(block_hash) + } } // An irrelevant node cannot become optimistic, this is a no-op. - ExecutionStatus::Irrelevant(_) => (), + Ok(ExecutionStatus::Irrelevant(_)) | Err(_) => (), } } @@ -891,30 +904,34 @@ impl ProtoArrayForkChoice { pub fn get_block(&self, block_root: &Hash256) -> Option { let block = self.get_proto_node(block_root)?; let parent_root = block - .parent + .parent() .and_then(|i| self.proto_array.nodes.get(i)) - .map(|parent| parent.root); + .map(|parent| parent.root()); Some(Block { - slot: block.slot, - root: block.root, + slot: block.slot(), + root: block.root(), parent_root, - state_root: block.state_root, - target_root: block.target_root, - current_epoch_shuffling_id: block.current_epoch_shuffling_id.clone(), - next_epoch_shuffling_id: block.next_epoch_shuffling_id.clone(), - justified_checkpoint: block.justified_checkpoint, - finalized_checkpoint: block.finalized_checkpoint, - execution_status: block.execution_status, - unrealized_justified_checkpoint: block.unrealized_justified_checkpoint, - unrealized_finalized_checkpoint: block.unrealized_finalized_checkpoint, + state_root: block.state_root(), + target_root: block.target_root(), + current_epoch_shuffling_id: block.current_epoch_shuffling_id().clone(), + next_epoch_shuffling_id: block.next_epoch_shuffling_id().clone(), + justified_checkpoint: *block.justified_checkpoint(), + finalized_checkpoint: *block.finalized_checkpoint(), + execution_status: block + .execution_status() + .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_block_hash: block.execution_payload_block_hash().ok(), }) } /// Returns the `block.execution_status` field, if the block is present. pub fn get_block_execution_status(&self, block_root: &Hash256) -> Option { let block = self.get_proto_node(block_root)?; - Some(block.execution_status) + block.execution_status().ok() } /// Returns the weight of a given block. @@ -923,7 +940,22 @@ impl ProtoArrayForkChoice { self.proto_array .nodes .get(*block_index) - .map(|node| node.weight) + .map(|node| node.weight()) + } + + /// 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 `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 { + Some(PayloadStatus::Full) + } else { + Some(PayloadStatus::Empty) + } } /// See `ProtoArray` documentation. @@ -1039,15 +1071,30 @@ impl ProtoArrayForkChoice { /// - If a value in `indices` is greater to or equal to `indices.len()`. /// - If some `Hash256` in `votes` is not a key in `indices` (except for `Hash256::zero()`, this is /// always valid). -// FIXME(sproul): implement get-weight changes here fn compute_deltas( indices: &HashMap, + node_slots: &[Slot], votes: &mut ElasticList, old_balances: &[u64], new_balances: &[u64], equivocating_indices: &BTreeSet, -) -> Result, Error> { - let mut deltas = vec![0_i64; indices.len()]; +) -> Result, Error> { + let block_slot = |index: usize| -> Result { + node_slots + .get(index) + .copied() + .ok_or(Error::InvalidNodeDelta(index)) + }; + + let mut deltas = vec![ + NodeDelta { + delta: 0, + empty_delta: 0, + full_delta: 0, + payload_tiebreaker: None, + }; + indices.len() + ]; for (val_index, vote) in votes.iter_mut().enumerate() { // There is no need to create a score change if the validator has never voted or both their @@ -1072,17 +1119,25 @@ fn compute_deltas( let old_balance = old_balances.get(val_index).copied().unwrap_or(0); if let Some(current_delta_index) = indices.get(&vote.current_root).copied() { - let delta = deltas - .get(current_delta_index) - .ok_or(Error::InvalidNodeDelta(current_delta_index))? + let node_delta = deltas + .get_mut(current_delta_index) + .ok_or(Error::InvalidNodeDelta(current_delta_index))?; + node_delta.delta = node_delta + .delta .checked_sub(old_balance as i64) .ok_or(Error::DeltaOverflow(current_delta_index))?; - // Array access safe due to check on previous line. - deltas[current_delta_index] = delta; + let status = NodeDelta::payload_status( + vote.current_slot, + vote.current_payload_present, + block_slot(current_delta_index)?, + ); + node_delta.sub_payload_delta(status, old_balance, current_delta_index)?; } vote.current_root = Hash256::zero(); + vote.current_slot = Slot::new(0); + vote.current_payload_present = false; } // We've handled this slashed validator, continue without applying an ordinary delta. continue; @@ -1099,34 +1154,52 @@ fn compute_deltas( // on-boarded less validators than the prior fork. let new_balance = new_balances.get(val_index).copied().unwrap_or(0); - if vote.current_root != vote.next_root || old_balance != new_balance { + if vote.current_root != vote.next_root + || old_balance != new_balance + || vote.current_payload_present != vote.next_payload_present + || vote.current_slot != vote.next_slot + { // We ignore the vote if it is not known in `indices`. We assume that it is outside // of our tree (i.e., pre-finalization) and therefore not interesting. if let Some(current_delta_index) = indices.get(&vote.current_root).copied() { - let delta = deltas - .get(current_delta_index) - .ok_or(Error::InvalidNodeDelta(current_delta_index))? + let node_delta = deltas + .get_mut(current_delta_index) + .ok_or(Error::InvalidNodeDelta(current_delta_index))?; + node_delta.delta = node_delta + .delta .checked_sub(old_balance as i64) .ok_or(Error::DeltaOverflow(current_delta_index))?; - // Array access safe due to check on previous line. - deltas[current_delta_index] = delta; + let status = NodeDelta::payload_status( + vote.current_slot, + vote.current_payload_present, + block_slot(current_delta_index)?, + ); + node_delta.sub_payload_delta(status, old_balance, current_delta_index)?; } // We ignore the vote if it is not known in `indices`. We assume that it is outside // of our tree (i.e., pre-finalization) and therefore not interesting. if let Some(next_delta_index) = indices.get(&vote.next_root).copied() { - let delta = deltas - .get(next_delta_index) - .ok_or(Error::InvalidNodeDelta(next_delta_index))? + let node_delta = deltas + .get_mut(next_delta_index) + .ok_or(Error::InvalidNodeDelta(next_delta_index))?; + node_delta.delta = node_delta + .delta .checked_add(new_balance as i64) .ok_or(Error::DeltaOverflow(next_delta_index))?; - // Array access safe due to check on previous line. - deltas[next_delta_index] = delta; + let status = NodeDelta::payload_status( + vote.next_slot, + vote.next_payload_present, + block_slot(next_delta_index)?, + ); + node_delta.add_payload_delta(status, new_balance, next_delta_index)?; } vote.current_root = vote.next_root; + vote.current_slot = vote.next_slot; + vote.current_payload_present = vote.next_payload_present; } } @@ -1144,8 +1217,13 @@ mod test_compute_deltas { Hash256::from_low_u64_be(i as u64 + 1) } + fn test_node_slots(count: usize) -> Vec { + vec![Slot::new(0); count] + } + #[test] fn finalized_descendant() { + let spec = MainnetEthSpec::default_spec(); let genesis_slot = Slot::new(0); let genesis_epoch = Epoch::new(0); @@ -1176,6 +1254,9 @@ mod test_compute_deltas { junk_shuffling_id.clone(), junk_shuffling_id.clone(), execution_status, + None, + None, + &spec, ) .unwrap(); @@ -1195,10 +1276,13 @@ mod test_compute_deltas { execution_status, unrealized_justified_checkpoint: Some(genesis_checkpoint), unrealized_finalized_checkpoint: Some(genesis_checkpoint), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, genesis_slot + 1, genesis_checkpoint, genesis_checkpoint, + &spec, ) .unwrap(); @@ -1220,10 +1304,13 @@ mod test_compute_deltas { execution_status, unrealized_justified_checkpoint: None, unrealized_finalized_checkpoint: None, + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, genesis_slot + 1, genesis_checkpoint, genesis_checkpoint, + &spec, ) .unwrap(); @@ -1299,6 +1386,7 @@ mod test_compute_deltas { /// *checkpoint*, not just the finalized *block*. #[test] fn finalized_descendant_edge_case() { + let spec = MainnetEthSpec::default_spec(); let get_block_root = Hash256::from_low_u64_be; let genesis_slot = Slot::new(0); let junk_state_root = Hash256::zero(); @@ -1320,6 +1408,9 @@ mod test_compute_deltas { junk_shuffling_id.clone(), junk_shuffling_id.clone(), execution_status, + None, + None, + &spec, ) .unwrap(); @@ -1348,10 +1439,13 @@ mod test_compute_deltas { execution_status, unrealized_justified_checkpoint: Some(genesis_checkpoint), unrealized_finalized_checkpoint: Some(genesis_checkpoint), + execution_payload_parent_hash: None, + execution_payload_block_hash: None, }, Slot::from(block.slot), genesis_checkpoint, genesis_checkpoint, + &spec, ) .unwrap(); }; @@ -1454,7 +1548,10 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: Hash256::zero(), next_root: Hash256::zero(), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); old_balances.push(0); new_balances.push(0); @@ -1462,6 +1559,7 @@ mod test_compute_deltas { let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1505,7 +1603,10 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: Hash256::zero(), next_root: hash_from_index(0), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); old_balances.push(BALANCE); new_balances.push(BALANCE); @@ -1513,6 +1614,7 @@ mod test_compute_deltas { let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1563,7 +1665,10 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: Hash256::zero(), next_root: hash_from_index(i), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); old_balances.push(BALANCE); new_balances.push(BALANCE); @@ -1571,6 +1676,7 @@ mod test_compute_deltas { let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1616,7 +1722,10 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: hash_from_index(0), next_root: hash_from_index(1), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); old_balances.push(BALANCE); new_balances.push(BALANCE); @@ -1624,6 +1733,7 @@ mod test_compute_deltas { let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1680,18 +1790,25 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: hash_from_index(1), next_root: Hash256::zero(), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); // One validator moves their vote from the block to something outside the tree. votes.0.push(VoteTracker { current_root: hash_from_index(1), next_root: Hash256::from_low_u64_be(1337), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1733,7 +1850,10 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: hash_from_index(0), next_root: hash_from_index(1), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); old_balances.push(OLD_BALANCE); new_balances.push(NEW_BALANCE); @@ -1741,6 +1861,7 @@ mod test_compute_deltas { let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1802,12 +1923,16 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: hash_from_index(1), next_root: hash_from_index(2), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); } let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1858,12 +1983,16 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: hash_from_index(1), next_root: hash_from_index(2), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); } let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1912,7 +2041,10 @@ mod test_compute_deltas { votes.0.push(VoteTracker { current_root: hash_from_index(1), next_root: hash_from_index(2), - next_epoch: Epoch::new(0), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: false, }); } @@ -1921,6 +2053,7 @@ mod test_compute_deltas { let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &old_balances, &new_balances, @@ -1950,6 +2083,7 @@ mod test_compute_deltas { // Re-computing the deltas should be a no-op (no repeat deduction for the slashed validator). let deltas = compute_deltas( &indices, + &test_node_slots(indices.len()), &mut votes, &new_balances, &new_balances, @@ -1958,4 +2092,68 @@ mod test_compute_deltas { .expect("should compute deltas"); assert_eq!(deltas, vec![0, 0]); } + + #[test] + fn payload_bucket_changes_on_non_pending_vote() { + const BALANCE: u64 = 42; + + let mut indices = HashMap::new(); + indices.insert(hash_from_index(1), 0); + + let node_slots = vec![Slot::new(0)]; + let mut votes = ElasticList(vec![VoteTracker { + current_root: hash_from_index(1), + next_root: hash_from_index(1), + current_slot: Slot::new(1), + next_slot: Slot::new(1), + current_payload_present: false, + next_payload_present: true, + }]); + + let deltas = compute_deltas( + &indices, + &node_slots, + &mut votes, + &[BALANCE], + &[BALANCE], + &BTreeSet::new(), + ) + .expect("should compute deltas"); + + assert_eq!(deltas[0].delta, 0); + assert_eq!(deltas[0].empty_delta, -(BALANCE as i64)); + assert_eq!(deltas[0].full_delta, BALANCE as i64); + } + + #[test] + fn pending_vote_only_updates_regular_weight() { + const BALANCE: u64 = 42; + + let mut indices = HashMap::new(); + indices.insert(hash_from_index(1), 0); + + let node_slots = vec![Slot::new(0)]; + let mut votes = ElasticList(vec![VoteTracker { + current_root: hash_from_index(1), + next_root: hash_from_index(1), + current_slot: Slot::new(0), + next_slot: Slot::new(0), + current_payload_present: false, + next_payload_present: true, + }]); + + let deltas = compute_deltas( + &indices, + &node_slots, + &mut votes, + &[BALANCE], + &[BALANCE], + &BTreeSet::new(), + ) + .expect("should compute deltas"); + + assert_eq!(deltas[0].delta, 0); + assert_eq!(deltas[0].empty_delta, 0); + assert_eq!(deltas[0].full_delta, 0); + } } diff --git a/consensus/proto_array/src/ssz_container.rs b/consensus/proto_array/src/ssz_container.rs index 1e01b74c8c..07baaa4786 100644 --- a/consensus/proto_array/src/ssz_container.rs +++ b/consensus/proto_array/src/ssz_container.rs @@ -1,7 +1,7 @@ use crate::proto_array::ProposerBoost; use crate::{ Error, JustifiedBalances, - proto_array::{ProtoArray, ProtoNodeV17}, + proto_array::{ProtoArray, ProtoNode, ProtoNodeV17}, proto_array_fork_choice::{ElasticList, ProtoArrayForkChoice, VoteTracker}, }; use ssz::{Encode, four_byte_option_impl}; @@ -14,14 +14,15 @@ use types::{Checkpoint, Hash256}; // selector. four_byte_option_impl!(four_byte_option_checkpoint, Checkpoint); -pub type SszContainer = SszContainerV28; +pub type SszContainer = SszContainerV29; +// Legacy containers (V17/V28) for backward compatibility with older schema versions. #[superstruct( variants(V17, V28), variant_attributes(derive(Encode, Decode, Clone)), no_enum )] -pub struct SszContainer { +pub struct SszContainerLegacy { pub votes: Vec, #[superstruct(only(V17))] pub balances: Vec, @@ -35,7 +36,21 @@ pub struct SszContainer { pub previous_proposer_boost: ProposerBoost, } -impl SszContainer { +/// Current container version. Uses union-encoded `ProtoNode` to support mixed V17/V29 nodes. +#[derive(Encode, Decode, Clone)] +pub struct SszContainerV29 { + pub votes: Vec, + pub prune_threshold: usize, + // Deprecated, remove in a future schema migration + justified_checkpoint: Checkpoint, + // Deprecated, remove in a future schema migration + finalized_checkpoint: Checkpoint, + pub nodes: Vec, + pub indices: Vec<(Hash256, usize)>, + pub previous_proposer_boost: ProposerBoost, +} + +impl SszContainerV29 { pub fn from_proto_array( from: &ProtoArrayForkChoice, justified_checkpoint: Checkpoint, @@ -55,10 +70,10 @@ impl SszContainer { } } -impl TryFrom<(SszContainer, JustifiedBalances)> for ProtoArrayForkChoice { +impl TryFrom<(SszContainerV29, JustifiedBalances)> for ProtoArrayForkChoice { type Error = Error; - fn try_from((from, balances): (SszContainer, JustifiedBalances)) -> Result { + fn try_from((from, balances): (SszContainerV29, JustifiedBalances)) -> Result { let proto_array = ProtoArray { prune_threshold: from.prune_threshold, nodes: from.nodes, @@ -74,9 +89,9 @@ impl TryFrom<(SszContainer, JustifiedBalances)> for ProtoArrayForkChoice { } } -// Convert V17 to V28 by dropping balances. -impl From for SszContainerV28 { - fn from(v17: SszContainerV17) -> Self { +// Convert legacy V17 to V28 by dropping balances. +impl From for SszContainerLegacyV28 { + fn from(v17: SszContainerLegacyV17) -> Self { Self { votes: v17.votes, prune_threshold: v17.prune_threshold, @@ -89,9 +104,9 @@ impl From for SszContainerV28 { } } -// Convert V28 to V17 by re-adding balances. -impl From<(SszContainerV28, JustifiedBalances)> for SszContainerV17 { - fn from((v28, balances): (SszContainerV28, JustifiedBalances)) -> Self { +// Convert legacy V28 to V17 by re-adding balances. +impl From<(SszContainerLegacyV28, JustifiedBalances)> for SszContainerLegacyV17 { + fn from((v28, balances): (SszContainerLegacyV28, JustifiedBalances)) -> Self { Self { votes: v28.votes, balances: balances.effective_balances.clone(), @@ -104,3 +119,40 @@ impl From<(SszContainerV28, JustifiedBalances)> for SszContainerV17 { } } } + +// Convert legacy V28 to current V29. +impl From for SszContainerV29 { + fn from(v28: SszContainerLegacyV28) -> Self { + Self { + votes: v28.votes, + prune_threshold: v28.prune_threshold, + justified_checkpoint: v28.justified_checkpoint, + finalized_checkpoint: v28.finalized_checkpoint, + nodes: v28.nodes.into_iter().map(ProtoNode::V17).collect(), + indices: v28.indices, + previous_proposer_boost: v28.previous_proposer_boost, + } + } +} + +// Downgrade current V29 to legacy V28 (lossy: V29 nodes lose payload-specific fields). +impl From for SszContainerLegacyV28 { + fn from(v29: SszContainerV29) -> Self { + Self { + votes: v29.votes, + prune_threshold: v29.prune_threshold, + justified_checkpoint: v29.justified_checkpoint, + finalized_checkpoint: v29.finalized_checkpoint, + nodes: v29 + .nodes + .into_iter() + .filter_map(|node| match node { + ProtoNode::V17(v17) => Some(v17), + ProtoNode::V29(_) => None, + }) + .collect(), + indices: v29.indices, + previous_proposer_boost: v29.previous_proposer_boost, + } + } +} diff --git a/testing/ef_tests/src/cases/fork_choice.rs b/testing/ef_tests/src/cases/fork_choice.rs index ca77dc8d79..ca28f3a2ca 100644 --- a/testing/ef_tests/src/cases/fork_choice.rs +++ b/testing/ef_tests/src/cases/fork_choice.rs @@ -939,7 +939,7 @@ impl Tester { DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION, ); let proposer_head = match proposer_head_result { - Ok(head) => head.parent_node.root, + Ok(head) => head.parent_node.root(), Err(ProposerHeadError::DoNotReOrg(_)) => canonical_head, _ => panic!("Unexpected error in get proposer head"), };