Skip to content

Commit

Permalink
Fix failing attestation tests and misc electra attestation cleanup (s…
Browse files Browse the repository at this point in the history 
…igp#5810)

* - get attestation related beacon chain tests to pass
- observed attestations are now keyed off of data + committee index
- rename op pool attestationref to compactattestationref
- remove unwraps in agg pool and use options instead
- cherry pick some changes from ef-tests-electra

* cargo fmt

* fix failing test

* Revert dockerfile changes

* make committee_index return option

* function args shouldnt be a ref to attestation ref

* fmt

* fix dup imports

---------

Co-authored-by: realbigsean <seananderson33@GMAIL.com>
  • Loading branch information
@eserilev @realbigsean
eserilev and realbigsean committed Jun 25, 2024
1 parent 9ac59ac commit afc769e
Show file tree
Hide file tree
Showing 24 changed files with 612 additions and 276 deletions.
124 changes: 86 additions & 38 deletions beacon_node/beacon_chain/src/attestation_verification.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,10 @@ use proto_array::Block as ProtoBlock;
use slog::debug;
use slot_clock::SlotClock;
use state_processing::{
common::{attesting_indices_base, attesting_indices_electra},
common::{
attesting_indices_base,
attesting_indices_electra::{self, get_committee_indices},
},
per_block_processing::errors::{AttestationValidationError, BlockOperationError},
signature_sets::{
indexed_attestation_signature_set_from_pubkeys,
Expand All @@ -57,10 +60,11 @@ use state_processing::{
use std::borrow::Cow;
use strum::AsRefStr;
use tree_hash::TreeHash;
use tree_hash_derive::TreeHash;
use types::{
Attestation, AttestationRef, BeaconCommittee, BeaconStateError::NoCommitteeFound, ChainSpec,
CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, SelectionProof,
SignedAggregateAndProof, Slot, SubnetId,
Attestation, AttestationData, AttestationRef, BeaconCommittee, BeaconStateError,
BeaconStateError::NoCommitteeFound, ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName,
Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId,
};

pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations};
Expand Down Expand Up @@ -262,9 +266,30 @@ pub enum Error {
BeaconChainError(BeaconChainError),
}

// TODO(electra) the error conversion changes here are to get a test case to pass
// this could easily be cleaned up
impl From<BeaconChainError> for Error {
fn from(e: BeaconChainError) -> Self {
Self::BeaconChainError(e)
match &e {
BeaconChainError::BeaconStateError(beacon_state_error) => {
if let BeaconStateError::AggregatorNotInCommittee { aggregator_index } =
beacon_state_error
{
Self::AggregatorNotInCommittee {
aggregator_index: *aggregator_index,
}
} else if let BeaconStateError::InvalidSelectionProof { aggregator_index } =
beacon_state_error
{
Self::InvalidSelectionProof {
aggregator_index: *aggregator_index,
}
} else {
Error::BeaconChainError(e)
}
}
_ => Error::BeaconChainError(e),
}
}
}

Expand All @@ -286,6 +311,12 @@ struct IndexedAggregatedAttestation<'a, T: BeaconChainTypes> {
observed_attestation_key_root: Hash256,
}

#[derive(TreeHash)]
pub struct ObservedAttestationKey {
pub committee_index: u64,
pub attestation_data: AttestationData,
}

/// Wraps a `Attestation` that has been verified up until the point that an `IndexedAttestation` can
/// be derived.
///
Expand Down Expand Up @@ -486,14 +517,21 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
});
}

// Ensure the valid aggregated attestation has not already been seen locally.
let attestation_data = attestation.data();
let attestation_data_root = attestation_data.tree_hash_root();
let observed_attestation_key_root = ObservedAttestationKey {
committee_index: attestation
.committee_index()
.ok_or(Error::NotExactlyOneCommitteeBitSet(0))?,
attestation_data: attestation.data().clone(),
}
.tree_hash_root();

// [New in Electra:EIP7549]
verify_committee_index(attestation, &chain.spec)?;

if chain
.observed_attestations
.write()
.is_known_subset(attestation, attestation_data_root)
.is_known_subset(attestation, observed_attestation_key_root)
.map_err(|e| Error::BeaconChainError(e.into()))?
{
metrics::inc_counter(&metrics::AGGREGATED_ATTESTATION_SUBSETS);
Expand Down Expand Up @@ -555,10 +593,8 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
chain: &BeaconChain<T>,
) -> Result<Self, AttestationSlashInfo<'a, T, Error>> {
use AttestationSlashInfo::*;

let attestation = signed_aggregate.message().aggregate();
let aggregator_index = signed_aggregate.message().aggregator_index();
let attestation_data_root = match Self::verify_early_checks(signed_aggregate, chain) {
let observed_attestation_key_root = match Self::verify_early_checks(signed_aggregate, chain)
{
Ok(root) => root,
Err(e) => {
return Err(SignatureNotChecked(
Expand All @@ -567,12 +603,12 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
))
}
};

// Committees must be sorted by ascending index order 0..committees_per_slot
let get_indexed_attestation_with_committee =
|(committees, _): (Vec<BeaconCommittee>, CommitteesPerSlot)| {
match attestation {
AttestationRef::Base(att) => {
match signed_aggregate {
SignedAggregateAndProof::Base(signed_aggregate) => {
let att = &signed_aggregate.message.aggregate;
let aggregator_index = signed_aggregate.message.aggregator_index;
let committee = committees
.iter()
.filter(|&committee| committee.index == att.data.index)
Expand All @@ -582,13 +618,13 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
index: att.data.index,
})?;

// TODO(electra):
// Note: this clones the signature which is known to be a relatively slow operation.
//
// Future optimizations should remove this clone.
if let Some(committee) = committee {
// TODO(electra):
// Note: this clones the signature which is known to be a relatively slow operation.
//
// Future optimizations should remove this clone.
let selection_proof = SelectionProof::from(
signed_aggregate.message().selection_proof().clone(),
signed_aggregate.message.selection_proof.clone(),
);

if !selection_proof
Expand All @@ -602,6 +638,7 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
if !committee.committee.contains(&(aggregator_index as usize)) {
return Err(Error::AggregatorNotInCommittee { aggregator_index });
}

attesting_indices_base::get_indexed_attestation(
committee.committee,
att,
Expand All @@ -614,13 +651,18 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> {
})
}
}
AttestationRef::Electra(att) => {
attesting_indices_electra::get_indexed_attestation(&committees, att)
.map_err(|e| BeaconChainError::from(e).into())
SignedAggregateAndProof::Electra(signed_aggregate) => {
attesting_indices_electra::get_indexed_attestation_from_signed_aggregate(
&committees,
signed_aggregate,
&chain.spec,
)
.map_err(|e| BeaconChainError::from(e).into())
}
}
};

let attestation = signed_aggregate.message().aggregate();
let indexed_attestation = match map_attestation_committees(
chain,
attestation,
Expand Down Expand Up @@ -659,7 +701,7 @@ impl<'a, T: BeaconChainTypes> VerifiedAggregatedAttestation<'a, T> {
if let ObserveOutcome::Subset = chain
.observed_attestations
.write()
.observe_item(attestation, Some(attestation_data_root))
.observe_item(attestation, Some(observed_attestation_key_root))
.map_err(|e| Error::BeaconChainError(e.into()))?
{
metrics::inc_counter(&metrics::AGGREGATED_ATTESTATION_SUBSETS);
Expand Down Expand Up @@ -802,7 +844,7 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> {
}

// [New in Electra:EIP7549]
verify_committee_index(attestation)?;
verify_committee_index(attestation, &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.
Expand All @@ -826,7 +868,7 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> {
chain: &BeaconChain<T>,
) -> Result<(u64, SubnetId), Error> {
let expected_subnet_id = SubnetId::compute_subnet_for_attestation::<T::EthSpec>(
&attestation,
attestation,
committees_per_slot,
&chain.spec,
)
Expand Down Expand Up @@ -1130,7 +1172,7 @@ pub fn verify_propagation_slot_range<S: SlotClock, E: EthSpec>(

let current_fork =
spec.fork_name_at_slot::<E>(slot_clock.now().ok_or(BeaconChainError::UnableToReadSlot)?);
let earliest_permissible_slot = if !current_fork.deneb_enabled() {
let earliest_permissible_slot = if current_fork < ForkName::Deneb {
one_epoch_prior
// EIP-7045
} else {
Expand Down Expand Up @@ -1302,10 +1344,17 @@ pub fn verify_signed_aggregate_signatures<T: BeaconChainTypes>(

/// Verify that the `attestation` committee index is properly set for the attestation's fork.
/// This function will only apply verification post-Electra.
pub fn verify_committee_index<E: EthSpec>(attestation: AttestationRef<E>) -> Result<(), Error> {
if let Ok(committee_bits) = attestation.committee_bits() {
pub fn verify_committee_index<E: EthSpec>(
attestation: AttestationRef<E>,
spec: &ChainSpec,
) -> Result<(), Error> {
if spec.fork_name_at_slot::<E>(attestation.data().slot) >= ForkName::Electra {
// Check to ensure that the attestation is for a single committee.
let num_committee_bits = get_committee_indices::<E>(committee_bits);
let num_committee_bits = get_committee_indices::<E>(
attestation
.committee_bits()
.map_err(|e| Error::BeaconChainError(e.into()))?,
);
if num_committee_bits.len() != 1 {
return Err(Error::NotExactlyOneCommitteeBitSet(
num_committee_bits.len(),
Expand Down Expand Up @@ -1358,7 +1407,8 @@ pub fn obtain_indexed_attestation_and_committees_per_slot<T: BeaconChainTypes>(
attesting_indices_electra::get_indexed_attestation(&committees, att)
.map(|attestation| (attestation, committees_per_slot))
.map_err(|e| {
if e == BlockOperationError::BeaconStateError(NoCommitteeFound) {
let index = att.committee_index().unwrap_or(0);
if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) {
Error::NoCommitteeForSlotAndIndex {
slot: att.data.slot,
index: att.committee_index(),
Expand All @@ -1372,16 +1422,14 @@ pub fn obtain_indexed_attestation_and_committees_per_slot<T: BeaconChainTypes>(
})
}

// TODO(electra) update comments below to reflect logic changes
// i.e. this now runs the map_fn on a list of committees for the slot of the provided attestation
/// Runs the `map_fn` with the committee and committee count per slot for the given `attestation`.
///
/// This function exists in this odd "map" pattern because efficiently obtaining the committees for
/// an attestation's slot can be complex. It might involve reading straight from the
/// an attestations slot can be complex. It might involve reading straight from the
/// `beacon_chain.shuffling_cache` or it might involve reading it from a state from the DB. Due to
/// the complexities of `RwLock`s on the shuffling cache, a simple `Cow` isn't suitable here.
///
/// If the committees for an `attestation`'s slot aren't found in the `shuffling_cache`, we will read a state
/// If the committees for an `attestation`'s slot isn't found in the `shuffling_cache`, we will read a state
/// from disk and then update the `shuffling_cache`.
fn map_attestation_committees<T, F, R>(
chain: &BeaconChain<T>,
Expand Down Expand Up @@ -1421,7 +1469,7 @@ where
.unwrap_or_else(|_| {
Err(Error::NoCommitteeForSlotAndIndex {
slot: attestation.data().slot,
index: attestation.committee_index(),
index: attestation.committee_index().unwrap_or(0),
})
}))
})
Expand Down
22 changes: 20 additions & 2 deletions beacon_node/beacon_chain/src/beacon_chain.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1661,6 +1661,24 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
}
}

/// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`.
///
/// The attestation will be obtained from `self.naive_aggregation_pool`.
pub fn get_aggregated_attestation_base(
&self,
attestation: AttestationRef<T::EthSpec>,
) -> Result<Option<Attestation<T::EthSpec>>, Error> {
match attestation {
AttestationRef::Base(att) => self.get_aggregated_attestation_base(&att.data),
AttestationRef::Electra(att) => self.get_aggregated_attestation_electra(
att.data.slot,
&att.data.tree_hash_root(),
att.committee_index()
.ok_or(Error::AttestationCommitteeIndexNotSet)?,
),
}
}

/// Returns an aggregated `Attestation`, if any, that has a matching `attestation.data`.
///
/// The attestation will be obtained from `self.naive_aggregation_pool`.
Expand Down Expand Up @@ -2212,7 +2230,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
self.log,
"Stored unaggregated attestation";
"outcome" => ?outcome,
"index" => attestation.data().index,
"index" => attestation.committee_index(),
"slot" => attestation.data().slot.as_u64(),
),
Err(NaiveAggregationError::SlotTooLow {
Expand All @@ -2231,7 +2249,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
self.log,
"Failed to store unaggregated attestation";
"error" => ?e,
"index" => attestation.data().index,
"index" => attestation.committee_index(),
"slot" => attestation.data().slot.as_u64(),
);
return Err(Error::from(e).into());
Expand Down
1 change: 0 additions & 1 deletion beacon_node/beacon_chain/src/early_attester_cache.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ impl<E: EthSpec> EarlyAttesterCache<E> {
item.committee_lengths
.get_committee_length::<E>(request_slot, request_index, spec)?;

// TODO(electra) make fork-agnostic
let attestation = if spec.fork_name_at_slot::<E>(request_slot) >= ForkName::Electra {
let mut committee_bits = BitVector::default();
if committee_len > 0 {
Expand Down
20 changes: 7 additions & 13 deletions beacon_node/beacon_chain/src/naive_aggregation_pool.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,18 +151,10 @@ impl<E: EthSpec> AggregateMap for AggregatedAttestationMap<E> {
.collect::<Vec<_>>(),
};

let committee_index = set_bits
.first()
.copied()
.ok_or(Error::NoAggregationBitsSet)?;

if set_bits.len() > 1 {
return Err(Error::MoreThanOneAggregationBitSet(set_bits.len()));
}
let attestation_key = AttestationKey::from_attestation_ref(a)?;
let attestation_key_root = attestation_key.tree_hash_root();

let attestation_data_root = a.data().tree_hash_root();

if let Some(existing_attestation) = self.map.get_mut(&attestation_data_root) {
if let Some(existing_attestation) = self.map.get_mut(&attestation_key_root) {
if existing_attestation
.get_aggregation_bit(committee_index)
.map_err(|_| Error::InconsistentBitfieldLengths)?
Expand All @@ -180,8 +172,10 @@ impl<E: EthSpec> AggregateMap for AggregatedAttestationMap<E> {
}

self.map
.insert(attestation_data_root, a.clone_as_attestation());
Ok(InsertOutcome::NewItemInserted { committee_index })
.insert(attestation_key_root, a.clone_as_attestation());
Ok(InsertOutcome::NewItemInserted {
committee_index: aggregation_bit,
})
}
}

Expand Down
Loading

0 comments on commit afc769e

Please sign in to comment.