diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 57c0bab341f..11cf0abe7e8 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -60,7 +60,7 @@ use strum::AsRefStr; use tree_hash::TreeHash; use tree_hash_derive::TreeHash; use types::{ - Attestation, AttestationData, AttestationRef, BeaconCommittee, + Attestation, AttestationData, AttestationRef, BeaconCommittee, BeaconStateError, BeaconStateError::NoCommitteeFound, ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, SelectionProof, SignedAggregateAndProof, Slot, SubnetId, }; @@ -264,9 +264,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 for Error { fn from(e: BeaconChainError) -> Self { - Error::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), + } } } @@ -404,7 +425,6 @@ fn process_slash_info( use AttestationSlashInfo::*; if let Some(slasher) = chain.slasher.as_ref() { - let (indexed_attestation, check_signature, err) = match slash_info { SignatureNotChecked(attestation, err) => { match obtain_indexed_attestation_and_committees_per_slot(chain, attestation) { @@ -436,7 +456,6 @@ fn process_slash_info( } } - // Supply to slasher. slasher.accept_attestation(indexed_attestation); @@ -565,9 +584,6 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { chain: &BeaconChain, ) -> Result> { use AttestationSlashInfo::*; - - let attestation = signed_aggregate.message().aggregate(); - let aggregator_index = signed_aggregate.message().aggregator_index(); let observed_attestation_key_root = match Self::verify_early_checks(signed_aggregate, chain) { Ok(root) => root, @@ -580,8 +596,10 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }; let get_indexed_attestation_with_committee = |(committees, _): (Vec, 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) @@ -591,13 +609,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 @@ -611,7 +629,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, @@ -624,27 +642,18 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }) } } - AttestationRef::Electra(att) => { - for committee in committees.iter() { - let selection_proof = SelectionProof::from( - signed_aggregate.message().selection_proof().clone(), - ); - - if !selection_proof - .is_aggregator(committee.committee.len(), &chain.spec) - .map_err(|e| Error::BeaconChainError(e.into()))? - { - return Err(Error::InvalidSelectionProof { aggregator_index }); - } - } - - 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, diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 22f0ad072b8..678c3c1286a 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2063,7 +2063,6 @@ impl BeaconChain { // This method is called for API and gossip attestations, so this covers all aggregated attestation events if let Some(event_handler) = self.event_handler.as_ref() { if event_handler.has_attestation_subscribers() { - event_handler.register(EventKind::Attestation(Box::new( v.attestation().clone_as_attestation(), ))); diff --git a/beacon_node/beacon_chain/tests/attestation_verification.rs b/beacon_node/beacon_chain/tests/attestation_verification.rs index 33df5e2ed27..e632aa1e301 100644 --- a/beacon_node/beacon_chain/tests/attestation_verification.rs +++ b/beacon_node/beacon_chain/tests/attestation_verification.rs @@ -423,6 +423,7 @@ impl GossipTester { /* * Batch verification */ + println!("desc {}", desc); let mut results = self .harness .chain @@ -771,6 +772,7 @@ async fn aggregated_gossip_verification() { } }, |_, err| { + println!("{:?}", err); assert!(matches!( err, // Naively we should think this condition would trigger this error: @@ -780,12 +782,7 @@ async fn aggregated_gossip_verification() { // However, the following error is triggered first: AttnError::AggregatorNotInCommittee { aggregator_index - } | - // unless were working with electra attestations - // in which case this error is triggered instead: - AttnError::AggregatorPubkeyUnknown( - aggregator_index - ) + } if aggregator_index == VALIDATOR_COUNT as u64 )) }, diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 134ab2d4371..8a9e60e2fab 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -51,6 +51,100 @@ pub mod attesting_indices_electra { use safe_arith::SafeArith; use types::*; + // TODO(electra) remove duplicate code + // get_indexed_attestation is almost an exact duplicate + // the only differences are the invalid selection proof + // and aggregator not in committee checks + pub fn get_indexed_attestation_from_signed_aggregate( + committees: &[BeaconCommittee], + signed_aggregate: &SignedAggregateAndProofElectra, + spec: &ChainSpec, + ) -> Result, BeaconStateError> { + let mut output: HashSet = HashSet::new(); + + let committee_bits = &signed_aggregate.message.aggregate.committee_bits; + let aggregation_bits = &signed_aggregate.message.aggregate.aggregation_bits; + let aggregator_index = signed_aggregate.message.aggregator_index; + let attestation = &signed_aggregate.message.aggregate; + + let committee_indices = get_committee_indices::(committee_bits); + + let mut committee_offset = 0; + + let committees_map: HashMap = committees + .iter() + .map(|committee| (committee.index, committee)) + .collect(); + + let committee_count_per_slot = committees.len() as u64; + let mut participant_count = 0; + + // 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()); + + for index in committee_indices { + if let Some(&beacon_committee) = committees_map.get(&index) { + if !selection_proof + .is_aggregator(beacon_committee.committee.len(), spec) + .map_err(BeaconStateError::ArithError)? + { + return Err(BeaconStateError::InvalidSelectionProof { aggregator_index }); + } + + if !beacon_committee + .committee + .contains(&(aggregator_index as usize)) + { + return Err(BeaconStateError::AggregatorNotInCommittee { aggregator_index }); + } + + // This check is new to the spec's `process_attestation` in Electra. + if index >= committee_count_per_slot { + return Err(BeaconStateError::InvalidCommitteeIndex(index)); + } + + participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; + let committee_attesters = beacon_committee + .committee + .iter() + .enumerate() + .filter_map(|(i, &index)| { + if let Ok(aggregation_bit_index) = committee_offset.safe_add(i) { + if aggregation_bits.get(aggregation_bit_index).unwrap_or(false) { + return Some(index as u64); + } + } + None + }) + .collect::>(); + + output.extend(committee_attesters); + + committee_offset.safe_add_assign(beacon_committee.committee.len())?; + } else { + return Err(Error::NoCommitteeFound(index)); + } + } + + // This check is new to the spec's `process_attestation` in Electra. + if participant_count as usize != aggregation_bits.len() { + return Err(Error::InvalidBitfield); + } + + let mut indices = output.into_iter().collect_vec(); + indices.sort_unstable(); + + Ok(IndexedAttestation::Electra(IndexedAttestationElectra { + attesting_indices: VariableList::new(indices)?, + data: attestation.data.clone(), + signature: attestation.signature.clone(), + })) + } + pub fn get_indexed_attestation( committees: &[BeaconCommittee], attestation: &AttestationElectra, diff --git a/consensus/state_processing/src/per_block_processing/signature_sets.rs b/consensus/state_processing/src/per_block_processing/signature_sets.rs index 94480625c23..be10398a331 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -437,7 +437,7 @@ where let message = slot.signing_root(domain); let signature = signed_aggregate_and_proof.message().selection_proof(); let validator_index = signed_aggregate_and_proof.message().aggregator_index(); - + println!("validator index, {}", validator_index); Ok(SignatureSet::single_pubkey( signature, get_pubkey(validator_index as usize).ok_or(Error::ValidatorUnknown(validator_index))?, diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index f2fdbc27802..b0da5b7eed3 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -163,6 +163,12 @@ pub enum Error { NonExecutionAddresWithdrawalCredential, NoCommitteeFound(CommitteeIndex), InvalidCommitteeIndex(CommitteeIndex), + InvalidSelectionProof { + aggregator_index: u64, + }, + AggregatorNotInCommittee { + aggregator_index: u64, + }, } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index 204c15ebd0d..ddf1dedb040 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -74,7 +74,6 @@ impl SignedAggregateAndProof { genesis_validators_root, spec, ); - let target_epoch = message.aggregate().data().slot.epoch(E::slots_per_epoch()); let domain = spec.get_domain( target_epoch,