diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index fd85fce5fd..a5beb4d2b8 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -85,7 +85,7 @@ use execution_layer::{ }; use fixed_bytes::FixedBytesExtended; use fork_choice::{ - AttestationFromBlock, ExecutionStatus, ForkChoice, ForkchoiceUpdateParameters, + ExecutionStatus, ForkChoice, ForkchoiceUpdateParameters, InvalidationOperation, PayloadVerificationStatus, ResetPayloadStatuses, }; use futures::channel::mpsc::Sender; @@ -2262,7 +2262,7 @@ impl BeaconChain { .on_attestation( self.slot()?, verified.indexed_attestation().to_ref(), - AttestationFromBlock::False, + false, &self.spec, ) .map_err(Into::into) diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index ee0bb9e6ff..41daa2c460 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -71,7 +71,7 @@ use bls::{PublicKey, PublicKeyBytes}; use educe::Educe; use eth2::types::{BlockGossip, EventKind}; use execution_layer::PayloadStatus; -pub use fork_choice::{AttestationFromBlock, PayloadVerificationStatus}; +pub use fork_choice::PayloadVerificationStatus; use metrics::TryExt; use parking_lot::RwLockReadGuard; use proto_array::Block as ProtoBlock; @@ -1664,7 +1664,7 @@ impl ExecutionPendingBlock { match fork_choice.on_attestation( current_slot, indexed_attestation, - AttestationFromBlock::True, + true, &chain.spec, ) { Ok(()) => Ok(()), @@ -1685,7 +1685,7 @@ impl ExecutionPendingBlock { match fork_choice.on_payload_attestation( current_slot, indexed_payload_attestation, - AttestationFromBlock::True, + true, ) { Ok(()) => Ok(()), // Ignore invalid payload attestations whilst importing from a block. diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index 5305621d28..26361c7941 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -312,14 +312,6 @@ fn dequeue_payload_attestations( } /// Denotes whether an attestation we are processing was received from a block or from gossip. -/// Equivalent to the `is_from_block` `bool` in: -/// -/// https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#validate_on_attestation -#[derive(Clone, Copy)] -pub enum AttestationFromBlock { - True, - False, -} /// Parameters which are cached between calls to `ForkChoice::get_head`. #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -1056,7 +1048,7 @@ where fn validate_on_attestation( &self, indexed_attestation: IndexedAttestationRef, - is_from_block: AttestationFromBlock, + is_from_block: bool, spec: &ChainSpec, ) -> Result<(), InvalidAttestation> { // There is no point in processing an attestation with an empty bitfield. Reject @@ -1070,7 +1062,7 @@ where let target = indexed_attestation.data().target; - if matches!(is_from_block, AttestationFromBlock::False) { + if !is_from_block { self.validate_target_epoch_against_current_time(target.epoch)?; } @@ -1148,7 +1140,7 @@ where fn validate_on_payload_attestation( &self, indexed_payload_attestation: &IndexedPayloadAttestation, - _is_from_block: AttestationFromBlock, + _is_from_block: bool, ) -> Result<(), InvalidAttestation> { if indexed_payload_attestation.attesting_indices.is_empty() { return Err(InvalidAttestation::EmptyAggregationBitfield); @@ -1198,7 +1190,7 @@ where &mut self, system_time_current_slot: Slot, attestation: IndexedAttestationRef, - is_from_block: AttestationFromBlock, + is_from_block: bool, spec: &ChainSpec, ) -> Result<(), Error> { let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_ATTESTATION_TIMES); @@ -1251,7 +1243,7 @@ where &mut self, system_time_current_slot: Slot, attestation: &IndexedPayloadAttestation, - is_from_block: AttestationFromBlock, + is_from_block: bool, ) -> Result<(), Error> { self.update_time(system_time_current_slot)?; @@ -1264,9 +1256,10 @@ where let processing_slot = self.fc_store.get_current_slot(); // Payload attestations from blocks can be applied in the next slot (S+1 for data.slot=S), // while non-block payload attestations are delayed one extra slot. - let should_process_now = match is_from_block { - AttestationFromBlock::True => attestation.data.slot < processing_slot, - AttestationFromBlock::False => attestation.data.slot + 1_u64 < processing_slot, + let should_process_now = if is_from_block { + attestation.data.slot < processing_slot + } else { + attestation.data.slot.saturating_add(1_u64) < processing_slot }; if should_process_now { diff --git a/consensus/fork_choice/src/lib.rs b/consensus/fork_choice/src/lib.rs index 6091de6fdd..d3a9d24622 100644 --- a/consensus/fork_choice/src/lib.rs +++ b/consensus/fork_choice/src/lib.rs @@ -3,7 +3,7 @@ mod fork_choice_store; mod metrics; pub use crate::fork_choice::{ - AttestationFromBlock, Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, + Error, ForkChoice, ForkChoiceView, ForkchoiceUpdateParameters, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, PersistedForkChoice, PersistedForkChoiceV17, PersistedForkChoiceV28, PersistedForkChoiceV29, QueuedAttestation, QueuedPayloadAttestation, ResetPayloadStatuses, diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 029e861289..eea94b2e77 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -10,7 +10,7 @@ use beacon_chain::{ use bls::AggregateSignature; use fixed_bytes::FixedBytesExtended; use fork_choice::{ - AttestationFromBlock, ForkChoiceStore, InvalidAttestation, InvalidBlock, + ForkChoiceStore, InvalidAttestation, InvalidBlock, PayloadVerificationStatus, QueuedAttestation, QueuedPayloadAttestation, }; use state_processing::state_advance::complete_state_advance; @@ -1033,7 +1033,7 @@ async fn payload_attestation_for_previous_slot_is_accepted_at_next_slot() { .on_payload_attestation( current_slot, &payload_attestation, - AttestationFromBlock::True, + true, ); assert!( @@ -1082,7 +1082,7 @@ async fn non_block_payload_attestation_at_next_slot_is_delayed() { let result = chain .canonical_head .fork_choice_write_lock() - .on_payload_attestation(s_plus_1, &payload_attestation, AttestationFromBlock::False); + .on_payload_attestation(s_plus_1, &payload_attestation, false); assert!( result.is_ok(), "payload attestation should be accepted for queueing" 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 49afae2d4a..ad45d073c2 100644 --- a/consensus/proto_array/src/fork_choice_test_definition/votes.rs +++ b/consensus/proto_array/src/fork_choice_test_definition/votes.rs @@ -339,7 +339,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { execution_payload_block_hash: None, }); - // Ensure that 5 becomes the head. + // Ensure that 5 is filtered out and the head stays at 4. // // 0 // / \ @@ -347,9 +347,9 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 3 // | - // 4 + // 4 <- head // / - // head-> 5 + // 5 ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(1), @@ -360,7 +360,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { root: get_root(0), }, justified_state_balances: balances.clone(), - expected_head: get_root(5), + expected_head: get_root(4), }); // Add block 6, which has a justified epoch of 0. @@ -476,8 +476,8 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { execution_payload_block_hash: None, }); - // Ensure that 9 is the head. The branch rooted at 5 remains viable and its best descendant - // is selected. + // 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. // // 0 // / \ @@ -487,13 +487,13 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { // | // 4 // / \ - // 5 6 + // 5 6 <- head // | // 7 // | // 8 // / - // head-> 9 + // 9 ops.push(Operation::FindHead { justified_checkpoint: Checkpoint { epoch: Epoch::new(1), @@ -504,7 +504,7 @@ pub fn get_votes_test_definition() -> ForkChoiceTestDefinition { root: get_root(0), }, justified_state_balances: balances.clone(), - expected_head: get_root(9), + expected_head: get_root(6), }); // Change fork-choice justified epoch to 1, and the start block to 5 and ensure that 9 is diff --git a/consensus/proto_array/src/proto_array.rs b/consensus/proto_array/src/proto_array.rs index 7403937d39..f062fef418 100644 --- a/consensus/proto_array/src/proto_array.rs +++ b/consensus/proto_array/src/proto_array.rs @@ -130,7 +130,6 @@ pub struct ProtoNode { #[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, } @@ -1152,7 +1151,7 @@ impl ProtoArray { return false; } - let genesis_epoch = Epoch::new(1); + let genesis_epoch = Epoch::new(0); 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(); diff --git a/consensus/proto_array/src/proto_array_fork_choice.rs b/consensus/proto_array/src/proto_array_fork_choice.rs index 9400aafed7..37054d9552 100644 --- a/consensus/proto_array/src/proto_array_fork_choice.rs +++ b/consensus/proto_array/src/proto_array_fork_choice.rs @@ -530,6 +530,8 @@ impl ProtoArrayForkChoice { if attestation_slot > vote.next_slot || *vote == VoteTracker::default() { vote.next_root = block_root; vote.next_slot = attestation_slot; + vote.next_payload_present = false; + vote.next_blob_data_available = false; } Ok(())