diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index faa396966ff..89196d76c19 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -160,6 +160,12 @@ pub enum Error { /// /// The peer has sent an invalid message. CommitteeIndexNonZero(usize), + /// The committee index is too high (>1) for Gloas + /// + /// ## Peer scoring + /// + /// The peer has sent an invalid message. + CommitteeIndexTooHigh(usize), /// The `attestation.data.beacon_block_root` block is unknown. /// /// ## Peer scoring @@ -550,9 +556,6 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { } .tree_hash_root(); - // [New in Electra:EIP7549] - verify_committee_index(attestation)?; - if chain .observed_attestations .write() @@ -595,6 +598,9 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { // attestation and do not delay consideration for later. let head_block = verify_head_block_is_known(chain, attestation.data(), None)?; + // [EIP-7732] Attestation data index validation for gloas fork and [EIP-7549] for Electra + verify_attestation_data_index(attestation, &head_block, chain)?; + // Check the attestation target root is consistent with the head root. // // This check is not in the specification, however we guard against it since it opens us up @@ -868,10 +874,34 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { &chain.spec, )?; + // Attestations must be for a known block. If the block is unknown, we simply drop the + // attestation and do not delay consideration for later. + // + // Enforce a maximum skip distance for unaggregated attestations. + let head_block = verify_head_block_is_known( + chain, + &attestation.data, + chain.config.import_max_skip_slots, + )?; + let fork_name = chain .spec .fork_name_at_slot::(attestation.data.slot); - if fork_name.electra_enabled() { + if fork_name.gloas_enabled() { + // [New in Gloas:EIP7732] + if attestation.data.index > 1 { + return Err(Error::CommitteeIndexTooHigh( + attestation.data.index as usize, + )); + } + + // [New in Gloas:EIP7732] + if head_block.slot == attestation.data.slot && attestation.data.index != 0 { + return Err(Error::CommitteeIndexNonZero( + attestation.data.index as usize, + )); + } + } else if fork_name.electra_enabled() { // [New in Electra:EIP7549] if attestation.data.index != 0 { return Err(Error::CommitteeIndexNonZero( @@ -880,16 +910,6 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { } } - // Attestations must be for a known block. If the block is unknown, we simply drop the - // attestation and do not delay consideration for later. - // - // Enforce a maximum skip distance for unaggregated attestations. - let head_block = verify_head_block_is_known( - chain, - &attestation.data, - chain.config.import_max_skip_slots, - )?; - // Check the attestation target root is consistent with the head root. verify_attestation_target_root::(&head_block, &attestation.data)?; @@ -1402,18 +1422,43 @@ pub fn verify_signed_aggregate_signatures( Ok(verify_signature_sets(signature_sets.iter())) } -/// Verify that the `attestation` committee index is properly set for the attestation's fork. +/// Verify that the `attestation` data index is properly set for the attestation's fork. /// This function will only apply verification post-Electra. -pub fn verify_committee_index(attestation: AttestationRef) -> Result<(), Error> { +pub fn verify_attestation_data_index( + attestation: AttestationRef, + head_block: &ProtoBlock, + chain: &BeaconChain, +) -> Result<(), Error> { + let fork_name = chain + .spec + .fork_name_at_slot::(attestation.data().slot); + + // Check Electra+ committee_bits validation if let Ok(committee_bits) = attestation.committee_bits() { // Check to ensure that the attestation is for a single committee. - let num_committee_bits = get_committee_indices::(committee_bits); + let num_committee_bits = get_committee_indices::(committee_bits); if num_committee_bits.len() != 1 { return Err(Error::NotExactlyOneCommitteeBitSet( num_committee_bits.len(), )); } + } + + if fork_name.gloas_enabled() { + // [New in Gloas:EIP7732] + if attestation.data().index > 1 { + return Err(Error::CommitteeIndexTooHigh( + attestation.data().index as usize, + )); + } + // [New in Gloas:EIP7732] + if head_block.slot == attestation.data().slot && attestation.data().index != 0 { + return Err(Error::CommitteeIndexNonZero( + attestation.data().index as usize, + )); + } + } else if fork_name.electra_enabled() { // Ensure the attestation index is set to zero post Electra. if attestation.data().index != 0 { return Err(Error::CommitteeIndexNonZero( diff --git a/beacon_node/beacon_chain/src/block_verification.rs b/beacon_node/beacon_chain/src/block_verification.rs index f27f281ac87..3bede671c6a 100644 --- a/beacon_node/beacon_chain/src/block_verification.rs +++ b/beacon_node/beacon_chain/src/block_verification.rs @@ -334,6 +334,16 @@ pub enum BlockError { max_blobs_at_epoch: usize, block: usize, }, + /// The block's execution payload bid has a `parent_block_root` that doesn't match + /// the block's `parent_root`. + /// + /// ## Peer scoring + /// + /// This block is invalid and the peer should be penalised. + BidParentBlockRootMismatch { + block_parent_root: Hash256, + bid_parent_root: Hash256, + }, } /// Which specific signature(s) are invalid in a SignedBeaconBlock @@ -878,6 +888,7 @@ impl GossipVerifiedBlock { // Do not gossip blocks that claim to contain more blobs than the max allowed // at the given block epoch. + // Note that `blob_kzg_commitments` were removed from the body in gloas fork, so this check will no longer apply if let Ok(commitments) = block.message().body().blob_kzg_commitments() { let max_blobs_at_epoch = chain .spec @@ -1030,8 +1041,28 @@ impl GossipVerifiedBlock { } // Validate the block's execution_payload (if any). + // Note that `execution_payload` was removed from the body in gloas fork, so this check will no longer apply validate_execution_payload_for_gossip(&parent_block, block.message(), chain)?; + // TODO(EIP-7732): from the spec: https://ethereum.github.io/consensus-specs/specs/gloas/p2p-interface/#beacon_block + // we will need to satisfy this condition: + // If execution_payload verification of block's execution payload parent by an execution node is complete: + // [REJECT] The block's execution payload parent (defined by bid.parent_block_hash) passes all validation. + // discussed the options on eth r&d: https://discord.com/channels/595666850260713488/874767108809031740/1420935382706425867 + // the idea is that during envelope processing, if we get back an `INVALID` status from the EL, then we add the `block_hash` to a list and if the next block's bid's `parent_block_hash` is in the list, we ignore the block. Something like this should be implemented as part of the envelope processing flow + + // [REJECT] `bid.parent_block_root` == `block.parent_root` + // This check is only applicable post-Gloas when blocks contain execution payload bids + if let Ok(signed_bid) = block.message().body().signed_execution_payload_bid() { + let bid = &signed_bid.message; + if bid.parent_block_root != block.message().parent_root() { + return Err(BlockError::BidParentBlockRootMismatch { + block_parent_root: block.message().parent_root(), + bid_parent_root: bid.parent_block_root, + }); + } + } + // Beacon API block_gossip events if let Some(event_handler) = chain.event_handler.as_ref() && event_handler.has_block_gossip_subscribers() diff --git a/beacon_node/beacon_chain/src/execution_payload.rs b/beacon_node/beacon_chain/src/execution_payload.rs index 7cad2104ebc..1b29f6c69f0 100644 --- a/beacon_node/beacon_chain/src/execution_payload.rs +++ b/beacon_node/beacon_chain/src/execution_payload.rs @@ -304,7 +304,7 @@ pub fn validate_execution_payload_for_gossip( block: BeaconBlockRef<'_, T::EthSpec>, chain: &BeaconChain, ) -> Result<(), BlockError> { - // Only apply this validation if this is a Bellatrix beacon block. + // Only apply this validation if this is a post-Bellatrix or pre-Gloas beacon block. if let Ok(execution_payload) = block.body().execution_payload() { // This logic should match `is_execution_enabled`. We use only the execution block hash of // the parent here in order to avoid loading the parent state during gossip verification. diff --git a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs index eb70147c6ef..e03979bb1a8 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -1350,7 +1350,8 @@ impl NetworkBeaconProcessor { | Err(e @ BlockError::ParentExecutionPayloadInvalid { .. }) | Err(e @ BlockError::KnownInvalidExecutionPayload(_)) | Err(e @ BlockError::GenesisBlock) - | Err(e @ BlockError::InvalidBlobCount { .. }) => { + | Err(e @ BlockError::InvalidBlobCount { .. }) + | Err(e @ BlockError::BidParentBlockRootMismatch { .. }) => { warn!(error = %e, "Could not verify block for gossip. Rejecting the block"); self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); self.gossip_penalize_peer( @@ -2389,7 +2390,9 @@ impl NetworkBeaconProcessor { } AttnError::CommitteeIndexNonZero(index) => { /* - * The validator index is not set to zero after Electra. + * The committee index is not set to zero when required by the fork rules. + * - Electra: always must be zero + * - Gloas: must be zero if attestation.slot == block.slot * * The peer has published an invalid consensus message. */ @@ -2407,6 +2410,26 @@ impl NetworkBeaconProcessor { "attn_comm_index_non_zero", ); } + AttnError::CommitteeIndexTooHigh(index) => { + /* + * The committee index is too high for the gloas fork rules. + * + * The peer has published an invalid consensus message. + */ + debug!( + %peer_id, + block = ?beacon_block_root, + ?attestation_type, + committee_index = index, + "Committee index too high" + ); + self.propagate_validation_result(message_id, peer_id, MessageAcceptance::Reject); + self.gossip_penalize_peer( + peer_id, + PeerAction::LowToleranceError, + "attn_comm_index_too_high", + ); + } AttnError::UnknownHeadBlock { beacon_block_root } => { trace!( %peer_id,