From 4708d8009694b9bbd11fe8b8a80681b816dea915 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 30 Apr 2024 19:49:08 +0300 Subject: [PATCH] Attestation superstruct changes for EIP 7549 (#5644) * update * experiment * superstruct changes * revert * superstruct changes * fix tests * indexed attestation * indexed attestation superstruct * updated TODOs --- .../src/attestation_verification.rs | 27 +- beacon_node/beacon_chain/src/beacon_chain.rs | 26 +- beacon_node/beacon_chain/src/block_reward.rs | 1 + .../beacon_chain/src/early_attester_cache.rs | 24 +- .../src/naive_aggregation_pool.rs | 71 ++-- .../beacon_chain/src/observed_aggregates.rs | 39 +-- .../beacon_chain/src/observed_operations.rs | 4 +- beacon_node/beacon_chain/src/test_utils.rs | 231 +++++-------- .../beacon_chain/src/validator_monitor.rs | 8 +- .../http_api/src/block_packing_efficiency.rs | 29 +- beacon_node/http_api/src/lib.rs | 10 +- .../http_api/src/publish_attestations.rs | 2 +- beacon_node/http_api/tests/tests.rs | 20 +- .../lighthouse_network/src/types/pubsub.rs | 12 +- .../gossip_methods.rs | 4 +- .../src/network_beacon_processor/mod.rs | 2 +- beacon_node/operation_pool/src/attestation.rs | 6 +- .../operation_pool/src/attestation_storage.rs | 63 ++-- beacon_node/operation_pool/src/lib.rs | 9 +- beacon_node/operation_pool/src/persistence.rs | 2 +- beacon_node/store/src/consensus_context.rs | 8 +- consensus/fork_choice/src/fork_choice.rs | 4 +- consensus/fork_choice/tests/tests.rs | 16 +- .../src/common/get_attesting_indices.rs | 14 +- .../src/common/get_indexed_attestation.rs | 25 ++ .../state_processing/src/consensus_context.rs | 64 ++-- .../process_operations.rs | 49 +-- .../per_block_processing/signature_sets.rs | 14 +- .../src/per_block_processing/tests.rs | 87 ++--- .../verify_attestation.rs | 4 +- .../state_processing/src/verify_operation.rs | 4 +- consensus/types/src/aggregate_and_proof.rs | 26 +- consensus/types/src/attestation.rs | 317 ++++++++---------- consensus/types/src/beacon_block.rs | 25 +- consensus/types/src/eth_spec.rs | 8 +- consensus/types/src/indexed_attestation.rs | 95 ++---- .../types/src/signed_aggregate_and_proof.rs | 3 +- lcli/src/indexed_attestations.rs | 11 +- slasher/src/attester_record.rs | 2 +- slasher/src/slasher.rs | 2 +- slasher/src/test_utils.rs | 37 +- validator_client/src/attestation_service.rs | 39 +-- 42 files changed, 603 insertions(+), 841 deletions(-) create mode 100644 consensus/state_processing/src/common/get_indexed_attestation.rs diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 06fba937d84..987d7651917 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -348,7 +348,7 @@ pub trait VerifiedAttestation: Sized { // Inefficient default implementation. This is overridden for gossip verified attestations. fn into_attestation_and_indices(self) -> (Attestation, Vec) { - let attestation = self.attestation().clone_as_attestation(); + let attestation = self.attestation().clone(); let attesting_indices = self.indexed_attestation().attesting_indices_to_vec(); (attestation, attesting_indices) } @@ -489,16 +489,9 @@ impl<'a, T: BeaconChainTypes> IndexedAggregatedAttestation<'a, T> { }); } - 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)?; + // 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(); if chain .observed_attestations @@ -842,8 +835,8 @@ impl<'a, T: BeaconChainTypes> IndexedUnaggregatedAttestation<'a, T> { subnet_id: Option, chain: &BeaconChain, ) -> Result<(u64, SubnetId), Error> { - let expected_subnet_id = SubnetId::compute_subnet_for_attestation::( - attestation, + let expected_subnet_id = SubnetId::compute_subnet_for_attestation_data::( + indexed_attestation.data(), committees_per_slot, &chain.spec, ) @@ -1433,12 +1426,12 @@ where let committees_per_slot = committee_cache.committees_per_slot(); Ok(committee_cache - .get_beacon_committees_at_slot(attestation.data().slot) - .map(|committees| map_fn((committees, committees_per_slot))) - .unwrap_or_else(|_| { + .get_beacon_committee(attestation.data().slot, attestation.data().index) + .map(|committee| map_fn((committee, committees_per_slot))) + .unwrap_or_else(|| { Err(Error::NoCommitteeForSlotAndIndex { slot: attestation.data().slot, - index: attestation.committee_index().unwrap_or(0), + index: attestation.data().index, }) })) }) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index f1d9ce791e0..f585c8c9aa2 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -122,6 +122,7 @@ use store::{ use task_executor::{ShutdownReason, TaskExecutor}; use tokio_stream::Stream; use tree_hash::TreeHash; +use types::attestation::AttestationBase; use types::blob_sidecar::FixedBlobSidecarList; use types::payload::BlockProductionVersion; use types::*; @@ -1993,15 +1994,18 @@ impl BeaconChain { }; drop(cache_timer); - Ok(Attestation::::empty_for_signing( - request_index, - committee_len, - request_slot, - beacon_block_root, - justified_checkpoint, - target, - &self.spec, - )?) + // TODO(electra) implement electra variant + Ok(Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot: request_slot, + index: request_index, + beacon_block_root, + source: justified_checkpoint, + target, + }, + signature: AggregateSignature::empty(), + })) } /// Performs the same validation as `Self::verify_unaggregated_attestation_for_gossip`, but for @@ -2215,7 +2219,7 @@ impl BeaconChain { self.log, "Stored unaggregated attestation"; "outcome" => ?outcome, - "index" => attestation.committee_index(), + "index" => attestation.data().index, "slot" => attestation.data().slot.as_u64(), ), Err(NaiveAggregationError::SlotTooLow { @@ -2234,7 +2238,7 @@ impl BeaconChain { self.log, "Failed to store unaggregated attestation"; "error" => ?e, - "index" => attestation.committee_index(), + "index" => attestation.data().index, "slot" => attestation.data().slot.as_u64(), ); return Err(Error::from(e).into()); diff --git a/beacon_node/beacon_chain/src/block_reward.rs b/beacon_node/beacon_chain/src/block_reward.rs index 69eecc89b8a..4c6804b698b 100644 --- a/beacon_node/beacon_chain/src/block_reward.rs +++ b/beacon_node/beacon_chain/src/block_reward.rs @@ -88,6 +88,7 @@ impl BeaconChain { block .body() .attestations() + .iter() .map(|a| a.data().clone()) .collect() } else { diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index dda699cc6c1..595f941235c 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -6,6 +6,7 @@ use crate::{ use parking_lot::RwLock; use proto_array::Block as ProtoBlock; use std::sync::Arc; +use types::attestation::AttestationBase; use types::*; pub struct CacheItem { @@ -122,16 +123,19 @@ impl EarlyAttesterCache { item.committee_lengths .get_committee_length::(request_slot, request_index, spec)?; - let attestation = Attestation::empty_for_signing( - request_index, - committee_len, - request_slot, - item.beacon_block_root, - item.source, - item.target, - spec, - ) - .map_err(Error::AttestationError)?; + // TODO(electra) make fork-agnostic + let attestation = Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_len) + .map_err(BeaconStateError::from)?, + data: AttestationData { + slot: request_slot, + index: request_index, + beacon_block_root: item.beacon_block_root, + source: item.source, + target: item.target, + }, + signature: AggregateSignature::empty(), + }); metrics::inc_counter(&metrics::BEACON_EARLY_ATTESTER_CACHE_HITS); diff --git a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs index 211aecfe63d..7b729a1f56a 100644 --- a/beacon_node/beacon_chain/src/naive_aggregation_pool.rs +++ b/beacon_node/beacon_chain/src/naive_aggregation_pool.rs @@ -241,19 +241,36 @@ impl AggregateMap for AggregatedAttestationMap { fn insert(&mut self, a: AttestationRef) -> Result { let _timer = metrics::start_timer(&metrics::ATTESTATION_PROCESSING_AGG_POOL_CORE_INSERT); - let aggregation_bit = *a - .set_aggregation_bits() - .iter() - .at_most_one() - .map_err(|iter| Error::MoreThanOneAggregationBitSet(iter.count()))? + let set_bits = match a { + Attestation::Base(att) => att + .aggregation_bits + .iter() + .enumerate() + .filter(|(_i, bit)| *bit) + .map(|(i, _bit)| i) + .collect::>(), + Attestation::Electra(att) => att + .aggregation_bits + .iter() + .enumerate() + .filter(|(_i, bit)| *bit) + .map(|(i, _bit)| i) + .collect::>(), + }; + + let committee_index = set_bits + .first() + .copied() .ok_or(Error::NoAggregationBitsSet)?; let attestation_key = AttestationKey::from_attestation_ref(a)?; let attestation_key_root = attestation_key.tree_hash_root(); - if let Some(existing_attestation) = self.map.get_mut(&attestation_key_root) { + let attestation_data_root = a.data().tree_hash_root(); + + if let Some(existing_attestation) = self.map.get_mut(&attestation_data_root) { if existing_attestation - .get_aggregation_bit(aggregation_bit) + .get_aggregation_bit(committee_index) .map_err(|_| Error::InconsistentBitfieldLengths)? { Ok(InsertOutcome::SignatureAlreadyKnown { @@ -587,22 +604,12 @@ mod tests { type E = types::MainnetEthSpec; - fn get_attestation_base(slot: Slot) -> Attestation { - let mut a: AttestationBase = test_random_instance(); - a.data.slot = slot; - a.aggregation_bits = BitList::with_capacity(4).expect("should create bitlist"); - Attestation::Base(a) - } - - fn get_attestation_electra(slot: Slot) -> Attestation { - let mut a: AttestationElectra = test_random_instance(); - a.data.slot = slot; - a.aggregation_bits = BitList::with_capacity(4).expect("should create bitlist"); - a.committee_bits = BitVector::new(); - a.committee_bits - .set(0, true) - .expect("should set committee bit"); - Attestation::Electra(a) + fn get_attestation(slot: Slot) -> Attestation { + let mut a: Attestation = test_random_instance(); + a.data_mut().slot = slot; + *a.aggregation_bits_base_mut().unwrap() = + BitList::with_capacity(4).expect("should create bitlist"); + a } fn get_sync_contribution(slot: Slot) -> SyncCommitteeContribution { @@ -645,16 +652,10 @@ mod tests { } fn unset_attestation_bit(a: &mut Attestation, i: usize) { - match a { - Attestation::Base(ref mut att) => att - .aggregation_bits - .set(i, false) - .expect("should unset aggregation bit"), - Attestation::Electra(ref mut att) => att - .aggregation_bits - .set(i, false) - .expect("should unset aggregation bit"), - } + a.aggregation_bits_base_mut() + .unwrap() + .set(i, false) + .expect("should unset aggregation bit") } fn unset_sync_contribution_bit(a: &mut SyncCommitteeContribution, i: usize) { @@ -675,8 +676,8 @@ mod tests { a.data().beacon_block_root == block_root } - fn key_from_attestation(a: &Attestation) -> AttestationKey { - AttestationKey::from_attestation_ref(a.to_ref()).expect("should create attestation key") + fn key_from_attestation(a: &Attestation) -> AttestationData { + a.data().clone() } fn mutate_sync_contribution_block_root( diff --git a/beacon_node/beacon_chain/src/observed_aggregates.rs b/beacon_node/beacon_chain/src/observed_aggregates.rs index 00476bfe7af..1829bf1d361 100644 --- a/beacon_node/beacon_chain/src/observed_aggregates.rs +++ b/beacon_node/beacon_chain/src/observed_aggregates.rs @@ -117,45 +117,32 @@ impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { type Item = BitList; fn is_subset(&self, other: &Self::Item) -> bool { match self { - Self::Base(att) => { - if let Ok(extended_aggregation_bits) = att.extend_aggregation_bits() { - return extended_aggregation_bits.is_subset(other); - } - false - } - Self::Electra(att) => att.aggregation_bits.is_subset(other), + Attestation::Base(att) => att.aggregation_bits.is_subset(other), + // TODO(electra) implement electra variant + Attestation::Electra(_) => todo!(), } } fn is_superset(&self, other: &Self::Item) -> bool { match self { - Self::Base(att) => { - if let Ok(extended_aggregation_bits) = att.extend_aggregation_bits() { - return other.is_subset(&extended_aggregation_bits); - } - false - } - Self::Electra(att) => other.is_subset(&att.aggregation_bits), + Attestation::Base(att) => other.is_subset(&att.aggregation_bits), + // TODO(electra) implement electra variant + Attestation::Electra(_) => todo!(), } } /// Returns the sync contribution aggregation bits. - fn get_item(&self) -> Result { + fn get_item(&self) -> Self::Item { match self { - Self::Base(att) => att - .extend_aggregation_bits() - .map_err(|_| Error::GetItemError), - Self::Electra(att) => Ok(att.aggregation_bits.clone()), + Attestation::Base(att) => att.aggregation_bits.clone(), + // TODO(electra) implement electra variant + Attestation::Electra(_) => todo!(), } } - /// Returns the hash tree root of the attestation data augmented with the committee index. - fn root(&self) -> Result { - Ok(ObservedAttestationKey { - committee_index: self.committee_index().ok_or(Error::RootError)?, - attestation_data: self.data().clone(), - } - .tree_hash_root()) + /// Returns the hash tree root of the attestation data. + fn root(&self) -> Hash256 { + self.data().tree_hash_root() } } diff --git a/beacon_node/beacon_chain/src/observed_operations.rs b/beacon_node/beacon_chain/src/observed_operations.rs index 969d03a11b6..b07a90596d6 100644 --- a/beacon_node/beacon_chain/src/observed_operations.rs +++ b/beacon_node/beacon_chain/src/observed_operations.rs @@ -61,12 +61,12 @@ impl ObservableOperation for ProposerSlashing { impl ObservableOperation for AttesterSlashing { fn observed_validators(&self) -> SmallVec<[u64; SMALL_VEC_SIZE]> { let attestation_1_indices = self - .attestation_1() + .attestation_1 .attesting_indices_iter() .copied() .collect::>(); let attestation_2_indices = self - .attestation_2() + .attestation_2 .attesting_indices_iter() .copied() .collect::>(); diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index bd98f19af6f..aadc65e1c8f 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -58,6 +58,7 @@ use store::{config::StoreConfig, HotColdDB, ItemStore, LevelDB, MemoryStore}; use task_executor::TaskExecutor; use task_executor::{test_utils::TestRuntime, ShutdownReason}; use tree_hash::TreeHash; +use types::attestation::AttestationBase; use types::indexed_attestation::IndexedAttestationBase; use types::payload::BlockProductionVersion; pub use types::test_utils::generate_deterministic_keypairs; @@ -1032,18 +1033,21 @@ where *state.get_block_root(target_slot)? }; - Ok(Attestation::empty_for_signing( - index, - committee_len, - slot, - beacon_block_root, - state.current_justified_checkpoint(), - Checkpoint { - epoch, - root: target_root, + // TODO(electra) make test fork-agnostic + Ok(Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot, + index, + beacon_block_root, + source: state.current_justified_checkpoint(), + target: Checkpoint { + epoch, + root: target_root, + }, }, - &self.spec, - )?) + signature: AggregateSignature::empty(), + })) } /// A list of attestations for each committee for the given slot. @@ -1118,14 +1122,11 @@ where ) .unwrap(); - match attestation { - Attestation::Base(ref mut att) => { - att.aggregation_bits.set(i, true).unwrap() - } - Attestation::Electra(ref mut att) => { - att.aggregation_bits.set(i, true).unwrap() - } - } + attestation + .aggregation_bits_base_mut() + .unwrap() + .set(i, true) + .unwrap(); *attestation.signature_mut() = { let domain = self.spec.get_domain( @@ -1146,8 +1147,8 @@ where agg_sig }; - let subnet_id = SubnetId::compute_subnet_for_attestation::( - attestation.to_ref(), + let subnet_id = SubnetId::compute_subnet_for_attestation_data::( + attestation.data(), committee_count, &self.chain.spec, ) @@ -1321,10 +1322,7 @@ where // If there are any attestations in this committee, create an aggregate. if let Some((attestation, _)) = committee_attestations.first() { let bc = state - .get_beacon_committee( - attestation.data().slot, - attestation.committee_index().unwrap(), - ) + .get_beacon_committee(attestation.data().slot, attestation.data().index) .unwrap(); // Find an aggregator if one exists. Return `None` if there are no @@ -1376,6 +1374,19 @@ where // If the chain is able to produce an aggregate, use that. Otherwise, build an // aggregate locally. + let aggregate = self + .chain + .get_aggregated_attestation(attestation.data()) + .unwrap() + .unwrap_or_else(|| { + committee_attestations.iter().skip(1).fold( + attestation.clone(), + |mut agg, (att, _)| { + agg.aggregate(att); + agg + }, + ) + }); let signed_aggregate = SignedAggregateAndProof::from_aggregate( aggregator_index as u64, @@ -1505,34 +1516,23 @@ where ) -> AttesterSlashing { let fork = self.chain.canonical_head.cached_head().head_fork(); - let fork_name = self.spec.fork_name_at_slot::(Slot::new(0)); - - let data = AttestationData { - slot: Slot::new(0), - index: 0, - beacon_block_root: Hash256::zero(), - target: Checkpoint { - root: Hash256::zero(), - epoch: target1.unwrap_or(fork.epoch), - }, - source: Checkpoint { - root: Hash256::zero(), - epoch: source1.unwrap_or(Epoch::new(0)), + let mut attestation_1 = IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(validator_indices).unwrap(), + data: AttestationData { + slot: Slot::new(0), + index: 0, + beacon_block_root: Hash256::zero(), + target: Checkpoint { + root: Hash256::zero(), + epoch: target1.unwrap_or(fork.epoch), + }, + source: Checkpoint { + root: Hash256::zero(), + epoch: source1.unwrap_or(Epoch::new(0)), + }, }, - }; - let mut attestation_1 = if fork_name.electra_enabled() { - IndexedAttestation::Electra(IndexedAttestationElectra { - attesting_indices: VariableList::new(validator_indices).unwrap(), - data, - signature: AggregateSignature::infinity(), - }) - } else { - IndexedAttestation::Base(IndexedAttestationBase { - attesting_indices: VariableList::new(validator_indices).unwrap(), - data, - signature: AggregateSignature::infinity(), - }) - }; + signature: AggregateSignature::infinity(), + }); let mut attestation_2 = attestation_1.clone(); attestation_2.data_mut().index += 1; @@ -1540,41 +1540,21 @@ where attestation_2.data_mut().target.epoch = target2.unwrap_or(fork.epoch); for attestation in &mut [&mut attestation_1, &mut attestation_2] { - match attestation { - IndexedAttestation::Base(attestation) => { - for i in attestation.attesting_indices.iter() { - let sk = &self.validator_keypairs[*i as usize].sk; - - let genesis_validators_root = self.chain.genesis_validators_root; - - let domain = self.chain.spec.get_domain( - attestation.data.target.epoch, - Domain::BeaconAttester, - &fork, - genesis_validators_root, - ); - let message = attestation.data.signing_root(domain); - - attestation.signature.add_assign(&sk.sign(message)); - } - } - IndexedAttestation::Electra(attestation) => { - for i in attestation.attesting_indices.iter() { - let sk = &self.validator_keypairs[*i as usize].sk; + // TODO(electra) we could explore iter mut here + for i in attestation.attesting_indices_to_vec() { + let sk = &self.validator_keypairs[i as usize].sk; let genesis_validators_root = self.chain.genesis_validators_root; - let domain = self.chain.spec.get_domain( - attestation.data.target.epoch, - Domain::BeaconAttester, - &fork, - genesis_validators_root, - ); - let message = attestation.data.signing_root(domain); + let domain = self.chain.spec.get_domain( + attestation.data().target.epoch, + Domain::BeaconAttester, + &fork, + genesis_validators_root, + ); + let message = attestation.data().signing_root(domain); - attestation.signature.add_assign(&sk.sign(message)); - } - } + attestation.signature_mut().add_assign(&sk.sign(message)); } } @@ -1612,81 +1592,36 @@ where }, }; - let (mut attestation_1, mut attestation_2) = if fork_name.electra_enabled() { - let attestation_1 = IndexedAttestationElectra { - attesting_indices: VariableList::new(validator_indices_1).unwrap(), - data: data.clone(), - signature: AggregateSignature::infinity(), - }; - - let attestation_2 = IndexedAttestationElectra { - attesting_indices: VariableList::new(validator_indices_2).unwrap(), - data, - signature: AggregateSignature::infinity(), - }; - - ( - IndexedAttestation::Electra(attestation_1), - IndexedAttestation::Electra(attestation_2), - ) - } else { - let attestation_1 = IndexedAttestationBase { - attesting_indices: VariableList::new(validator_indices_1).unwrap(), - data: data.clone(), - signature: AggregateSignature::infinity(), - }; - - let attestation_2 = IndexedAttestationBase { - attesting_indices: VariableList::new(validator_indices_2).unwrap(), - data, - signature: AggregateSignature::infinity(), - }; + let mut attestation_1 = IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(validator_indices_1).unwrap(), + data: data.clone(), + signature: AggregateSignature::infinity(), + }); - ( - IndexedAttestation::Base(attestation_1), - IndexedAttestation::Base(attestation_2), - ) - }; + let mut attestation_2 = IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(validator_indices_2).unwrap(), + data, + signature: AggregateSignature::infinity(), + }); attestation_2.data_mut().index += 1; let fork = self.chain.canonical_head.cached_head().head_fork(); for attestation in &mut [&mut attestation_1, &mut attestation_2] { - match attestation { - IndexedAttestation::Base(attestation) => { - for i in attestation.attesting_indices.iter() { - let sk = &self.validator_keypairs[*i as usize].sk; - - let genesis_validators_root = self.chain.genesis_validators_root; - - let domain = self.chain.spec.get_domain( - attestation.data.target.epoch, - Domain::BeaconAttester, - &fork, - genesis_validators_root, - ); - let message = attestation.data.signing_root(domain); - - attestation.signature.add_assign(&sk.sign(message)); - } - } - IndexedAttestation::Electra(attestation) => { - for i in attestation.attesting_indices.iter() { - let sk = &self.validator_keypairs[*i as usize].sk; + for i in attestation.attesting_indices_to_vec() { + let sk = &self.validator_keypairs[i as usize].sk; let genesis_validators_root = self.chain.genesis_validators_root; - let domain = self.chain.spec.get_domain( - attestation.data.target.epoch, - Domain::BeaconAttester, - &fork, - genesis_validators_root, - ); - let message = attestation.data.signing_root(domain); + let domain = self.chain.spec.get_domain( + attestation.data().target.epoch, + Domain::BeaconAttester, + &fork, + genesis_validators_root, + ); + let message = attestation.data().signing_root(domain); - attestation.signature.add_assign(&sk.sign(message)); - } - } + attestation.signature_mut().add_assign(&sk.sign(message)); } } diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index 4ce53838191..a81e4f541cf 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -1798,16 +1798,16 @@ impl ValidatorMonitor { self.register_attester_slashing("block", slashing) } - fn register_attester_slashing(&self, src: &str, slashing: AttesterSlashingRef<'_, E>) { - let data = slashing.attestation_1().data(); + fn register_attester_slashing(&self, src: &str, slashing: &AttesterSlashing) { + let data = slashing.attestation_1.data(); let attestation_1_indices: HashSet = slashing - .attestation_1() + .attestation_1 .attesting_indices_iter() .copied() .collect(); slashing - .attestation_2() + .attestation_2 .attesting_indices_iter() .filter(|index| attestation_1_indices.contains(index)) .filter_map(|index| self.get_validator(*index)) diff --git a/beacon_node/http_api/src/block_packing_efficiency.rs b/beacon_node/http_api/src/block_packing_efficiency.rs index 66c71872786..3e511c25dff 100644 --- a/beacon_node/http_api/src/block_packing_efficiency.rs +++ b/beacon_node/http_api/src/block_packing_efficiency.rs @@ -10,8 +10,8 @@ use std::collections::{HashMap, HashSet}; use std::marker::PhantomData; use std::sync::Arc; use types::{ - AttestationRef, BeaconCommittee, BeaconState, BeaconStateError, BlindedPayload, ChainSpec, - Epoch, EthSpec, Hash256, OwnedBeaconCommittee, RelativeEpoch, SignedBeaconBlock, Slot, + Attestation, BeaconCommittee, BeaconState, BeaconStateError, BlindedPayload, ChainSpec, Epoch, + EthSpec, Hash256, OwnedBeaconCommittee, RelativeEpoch, SignedBeaconBlock, Slot, }; use warp_utils::reject::{beacon_chain_error, custom_bad_request, custom_server_error}; @@ -111,9 +111,9 @@ impl PackingEfficiencyHandler { let attestations = block_body.attestations(); let mut attestations_in_block = HashMap::new(); - for attestation in attestations { + for attestation in attestations.iter() { match attestation { - AttestationRef::Base(attn) => { + Attestation::Base(attn) => { for (position, voted) in attn.aggregation_bits.iter().enumerate() { if voted { let unique_attestation = UniqueAttestation { @@ -132,24 +132,9 @@ impl PackingEfficiencyHandler { } } } - AttestationRef::Electra(attn) => { - for (position, voted) in attn.aggregation_bits.iter().enumerate() { - if voted { - let unique_attestation = UniqueAttestation { - slot: attn.data.slot, - committee_index: attn.data.index, - committee_position: position, - }; - let inclusion_distance: u64 = block - .slot() - .as_u64() - .checked_sub(attn.data.slot.as_u64()) - .ok_or(PackingEfficiencyError::InvalidAttestationError)?; - - self.available_attestations.remove(&unique_attestation); - attestations_in_block.insert(unique_attestation, inclusion_distance); - } - } + // TODO(electra) implement electra variant + Attestation::Electra(_) => { + todo!() } } } diff --git a/beacon_node/http_api/src/lib.rs b/beacon_node/http_api/src/lib.rs index 7c8f64a722c..890331b94ae 100644 --- a/beacon_node/http_api/src/lib.rs +++ b/beacon_node/http_api/src/lib.rs @@ -3368,9 +3368,9 @@ pub fn serve( "Failure verifying aggregate and proofs"; "error" => format!("{:?}", e), "request_index" => index, - "aggregator_index" => aggregate.message().aggregator_index(), - "attestation_index" => aggregate.message().aggregate().committee_index(), - "attestation_slot" => aggregate.message().aggregate().data().slot, + "aggregator_index" => aggregate.message.aggregator_index, + "attestation_index" => aggregate.message.aggregate.data().index, + "attestation_slot" => aggregate.message.aggregate.data().slot, ); failures.push(api_types::Failure::new(index, format!("Verification: {:?}", e))); } @@ -3389,8 +3389,8 @@ pub fn serve( "Failure applying verified aggregate attestation to fork choice"; "error" => format!("{:?}", e), "request_index" => index, - "aggregator_index" => verified_aggregate.aggregate().message().aggregator_index(), - "attestation_index" => verified_aggregate.attestation().committee_index(), + "aggregator_index" => verified_aggregate.aggregate().message.aggregator_index, + "attestation_index" => verified_aggregate.attestation().data().index, "attestation_slot" => verified_aggregate.attestation().data().slot, ); failures.push(api_types::Failure::new(index, format!("Fork choice: {:?}", e))); diff --git a/beacon_node/http_api/src/publish_attestations.rs b/beacon_node/http_api/src/publish_attestations.rs index 00654765325..541ba8b7871 100644 --- a/beacon_node/http_api/src/publish_attestations.rs +++ b/beacon_node/http_api/src/publish_attestations.rs @@ -141,7 +141,7 @@ pub async fn publish_attestations( // move the `attestations` vec into the blocking task, so this small overhead is unavoidable. let attestation_metadata = attestations .iter() - .map(|att| (att.data().slot, att.committee_index())) + .map(|att| (att.data().slot, att.data().index)) .collect::>(); // Gossip validate and publish attestations that can be immediately processed. diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 86f2096224b..0a83b73d6ec 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -1806,14 +1806,7 @@ impl ApiTester { pub async fn test_post_beacon_pool_attester_slashings_invalid(mut self) -> Self { let mut slashing = self.attester_slashing.clone(); - match &mut slashing { - AttesterSlashing::Base(ref mut slashing) => { - slashing.attestation_1.data.slot += 1; - } - AttesterSlashing::Electra(ref mut slashing) => { - slashing.attestation_1.data.slot += 1; - } - } + slashing.attestation_1.data_mut().slot += 1; self.client .post_beacon_pool_attester_slashings(&slashing) @@ -3292,7 +3285,6 @@ impl ApiTester { .unwrap() .data; - // TODO(electra) make fork-agnostic let mut attestation = Attestation::Base(AttestationBase { aggregation_bits: BitList::with_capacity(duty.committee_length as usize).unwrap(), data: attestation_data, @@ -3335,14 +3327,8 @@ impl ApiTester { pub async fn test_get_validator_aggregate_and_proofs_invalid(mut self) -> Self { let mut aggregate = self.get_aggregate().await; - match &mut aggregate { - SignedAggregateAndProof::Base(ref mut aggregate) => { - aggregate.message.aggregate.data.slot += 1; - } - SignedAggregateAndProof::Electra(ref mut aggregate) => { - aggregate.message.aggregate.data.slot += 1; - } - } + + aggregate.message.aggregate.data_mut().slot += 1; self.client .post_validator_aggregate_and_proof::(&[aggregate]) diff --git a/beacon_node/lighthouse_network/src/types/pubsub.rs b/beacon_node/lighthouse_network/src/types/pubsub.rs index b443ecd1b9b..242044987b9 100644 --- a/beacon_node/lighthouse_network/src/types/pubsub.rs +++ b/beacon_node/lighthouse_network/src/types/pubsub.rs @@ -404,17 +404,17 @@ impl std::fmt::Display for PubsubMessage { ), PubsubMessage::AggregateAndProofAttestation(att) => write!( f, - "Aggregate and Proof: slot: {}, index: {:?}, aggregator_index: {}", - att.message().aggregate().data().slot, - att.message().aggregate().committee_index(), - att.message().aggregator_index(), + "Aggregate and Proof: slot: {}, index: {}, aggregator_index: {}", + att.message.aggregate.data().slot, + att.message.aggregate.data().index, + att.message.aggregator_index, ), PubsubMessage::Attestation(data) => write!( f, - "Attestation: subnet_id: {}, attestation_slot: {}, attestation_index: {:?}", + "Attestation: subnet_id: {}, attestation_slot: {}, attestation_index: {}", *data.0, data.1.data().slot, - data.1.committee_index(), + data.1.data().index, ), PubsubMessage::VoluntaryExit(_data) => write!(f, "Voluntary Exit"), PubsubMessage::ProposerSlashing(_data) => write!(f, "Proposer Slashing"), 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 8baa0f773db..c00b5565a36 100644 --- a/beacon_node/network/src/network_beacon_processor/gossip_methods.rs +++ b/beacon_node/network/src/network_beacon_processor/gossip_methods.rs @@ -105,7 +105,7 @@ impl VerifiedAttestation for VerifiedAggregate { /// Efficient clone-free implementation that moves out of the `Box`. fn into_attestation_and_indices(self) -> (Attestation, Vec) { - let attestation = self.signed_aggregate.into_attestation(); + let attestation = self.signed_aggregate.message.aggregate; let attesting_indices = self.indexed_attestation.attesting_indices_to_vec(); (attestation, attesting_indices) } @@ -412,7 +412,7 @@ impl NetworkBeaconProcessor { reprocess_tx: Option>, seen_timestamp: Duration, ) { - let beacon_block_root = aggregate.message().aggregate().data().beacon_block_root; + let beacon_block_root = aggregate.message.aggregate.data().beacon_block_root; let result = match self .chain diff --git a/beacon_node/network/src/network_beacon_processor/mod.rs b/beacon_node/network/src/network_beacon_processor/mod.rs index ccdbb10720c..a37b8b99bfb 100644 --- a/beacon_node/network/src/network_beacon_processor/mod.rs +++ b/beacon_node/network/src/network_beacon_processor/mod.rs @@ -142,7 +142,7 @@ impl NetworkBeaconProcessor { processor.process_gossip_aggregate_batch(aggregates, Some(reprocess_tx)) }; - let beacon_block_root = aggregate.message().aggregate().data().beacon_block_root; + let beacon_block_root = aggregate.message.aggregate.data().beacon_block_root; self.try_send(BeaconWorkEvent { drop_during_sync: true, work: Work::GossipAggregate { diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index 97d0583e345..579d37264aa 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -1,4 +1,4 @@ -use crate::attestation_storage::{CompactAttestationRef, CompactIndexedAttestation}; +use crate::attestation_storage::{AttestationRef, CompactIndexedAttestation}; use crate::max_cover::MaxCover; use crate::reward_cache::RewardCache; use state_processing::common::{ @@ -184,8 +184,8 @@ pub fn earliest_attestation_validators( // Bitfield of validators whose attestations are new/fresh. let mut new_validators = match attestation.indexed { CompactIndexedAttestation::Base(indexed_att) => indexed_att.aggregation_bits.clone(), - // This code path is obsolete post altair fork, so we just return an empty bitlist here. - CompactIndexedAttestation::Electra(_) => return BitList::with_capacity(0).unwrap(), + // TODO(electra) per the comments above, this code path is obsolete post altair fork, so maybe we should just return an empty bitlist here? + CompactIndexedAttestation::Electra(_) => todo!(), }; let state_attestations = if attestation.checkpoint.target_epoch == state.current_epoch() { diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 4de9d351f3c..762154b7866 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -2,9 +2,8 @@ use crate::AttestationStats; use itertools::Itertools; use std::collections::{BTreeMap, HashMap}; use types::{ - attestation::{AttestationBase, AttestationElectra}, - superstruct, AggregateSignature, Attestation, AttestationData, BeaconState, BitList, BitVector, - Checkpoint, Epoch, EthSpec, Hash256, Slot, Unsigned, + attestation::{AttestationBase, AttestationElectra}, superstruct, AggregateSignature, Attestation, AttestationData, + BeaconState, BitList, BitVector, Checkpoint, Epoch, EthSpec, Hash256, Slot, }; #[derive(Debug, PartialEq, Eq, Hash, Clone, Copy)] @@ -28,8 +27,9 @@ pub struct CompactIndexedAttestation { #[superstruct(only(Base), partial_getter(rename = "aggregation_bits_base"))] pub aggregation_bits: BitList, #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] - pub aggregation_bits: BitList, + pub aggregation_bits: BitList, pub signature: AggregateSignature, + pub index: u64, #[superstruct(only(Electra))] pub committee_bits: BitVector, } @@ -77,16 +77,11 @@ impl SplitAttestation { attesting_indices, aggregation_bits: attn.aggregation_bits, signature: attestation.signature().clone(), + index: data.index, }) } - Attestation::Electra(attn) => { - CompactIndexedAttestation::Electra(CompactIndexedAttestationElectra { - attesting_indices, - aggregation_bits: attn.aggregation_bits, - signature: attestation.signature().clone(), - committee_bits: attn.committee_bits, - }) - } + // TODO(electra) implement electra variant + Attestation::Electra(_) => todo!(), }; Self { @@ -126,14 +121,12 @@ impl<'a, E: EthSpec> CompactAttestationRef<'a, E> { data: self.attestation_data(), signature: indexed_att.signature.clone(), }), - CompactIndexedAttestation::Electra(indexed_att) => { - Attestation::Electra(AttestationElectra { - aggregation_bits: indexed_att.aggregation_bits.clone(), - data: self.attestation_data(), - signature: indexed_att.signature.clone(), - committee_bits: indexed_att.committee_bits.clone(), - }) - } + CompactIndexedAttestation::Electra(indexed_att) => Attestation::Electra(AttestationElectra { + aggregation_bits: indexed_att.aggregation_bits.clone(), + data: self.attestation_data(), + signature: indexed_att.signature.clone(), + committee_bits: indexed_att.committee_bits.clone(), + }), } } } @@ -156,37 +149,35 @@ impl CheckpointKey { } impl CompactIndexedAttestation { - pub fn should_aggregate(&self, other: &Self) -> bool { + pub fn signers_disjoint_from(&self, other: &Self) -> bool { match (self, other) { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { - this.should_aggregate(other) + this.signers_disjoint_from(other) } - ( - CompactIndexedAttestation::Electra(this), - CompactIndexedAttestation::Electra(other), - ) => this.should_aggregate(other), + (CompactIndexedAttestation::Electra(_), CompactIndexedAttestation::Electra(_)) => { + todo!() + } + // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => false, } } - /// Returns `true` if aggregated, otherwise `false`. - pub fn aggregate(&mut self, other: &Self) -> bool { + pub fn aggregate(&mut self, other: &Self) { match (self, other) { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { - this.aggregate(other); - true + this.aggregate(other) } - ( - CompactIndexedAttestation::Electra(this), - CompactIndexedAttestation::Electra(other), - ) => this.aggregate_same_committee(other), - _ => false, + (CompactIndexedAttestation::Electra(_), CompactIndexedAttestation::Electra(_)) => { + todo!() + } + // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? + _ => (), } } } impl CompactIndexedAttestationBase { - pub fn should_aggregate(&self, other: &Self) -> bool { + pub fn signers_disjoint_from(&self, other: &Self) -> bool { self.aggregation_bits .intersection(&other.aggregation_bits) .is_zero() diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index a1c9ada03a0..f2946da4d42 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -1279,14 +1279,7 @@ mod release_tests { // All the best attestations should be signed by at least `big_step_size` (4) validators. for att in &best_attestations { - match fork_name { - ForkName::Electra => { - assert!(att.num_set_aggregation_bits() >= small_step_size); - } - _ => { - assert!(att.num_set_aggregation_bits() >= big_step_size); - } - }; + assert!(att.num_set_aggregation_bits() >= big_step_size); } } diff --git a/beacon_node/operation_pool/src/persistence.rs b/beacon_node/operation_pool/src/persistence.rs index 79509e5f6cc..766136059d0 100644 --- a/beacon_node/operation_pool/src/persistence.rs +++ b/beacon_node/operation_pool/src/persistence.rs @@ -63,7 +63,7 @@ impl PersistedOperationPool { .iter() .map(|att| { ( - AttestationOnDisk::from(att.clone_as_attestation()), + att.clone_as_attestation(), att.indexed.attesting_indices().clone(), ) }) diff --git a/beacon_node/store/src/consensus_context.rs b/beacon_node/store/src/consensus_context.rs index 281106d9aaa..86679fee8c4 100644 --- a/beacon_node/store/src/consensus_context.rs +++ b/beacon_node/store/src/consensus_context.rs @@ -21,7 +21,13 @@ pub struct OnDiskConsensusContext { /// /// They are not part of the on-disk format. #[ssz(skip_serializing, skip_deserializing)] - indexed_attestations: HashMap>, + indexed_attestations: HashMap< + ( + AttestationData, + BitList, + ), + IndexedAttestation, + >, } impl OnDiskConsensusContext { diff --git a/consensus/fork_choice/src/fork_choice.rs b/consensus/fork_choice/src/fork_choice.rs index c55219a6761..49a08784aff 100644 --- a/consensus/fork_choice/src/fork_choice.rs +++ b/consensus/fork_choice/src/fork_choice.rs @@ -1086,8 +1086,8 @@ where /// Apply an attester slashing to fork choice. /// /// We assume that the attester slashing provided to this function has already been verified. - pub fn on_attester_slashing(&mut self, slashing: AttesterSlashingRef<'_, E>) { - let attesting_indices_set = |att: IndexedAttestationRef<'_, E>| { + pub fn on_attester_slashing(&mut self, slashing: &AttesterSlashing) { + let attesting_indices_set = |att: &IndexedAttestation| { att.attesting_indices_iter() .copied() .collect::>() diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index d2935dbca45..63bf3b51bd0 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -435,12 +435,7 @@ impl ForkChoiceTest { let validator_committee_index = 0; let validator_index = *head .beacon_state - .get_beacon_committee( - current_slot, - attestation - .committee_index() - .expect("should get committee index"), - ) + .get_beacon_committee(current_slot, attestation.data().index) .expect("should get committees") .committee .get(validator_committee_index) @@ -835,13 +830,8 @@ async fn invalid_attestation_empty_bitfield() { .await .apply_attestation_to_chain( MutationDelay::NoDelay, - |attestation, _| match attestation { - IndexedAttestation::Base(ref mut att) => { - att.attesting_indices = vec![].into(); - } - IndexedAttestation::Electra(ref mut att) => { - att.attesting_indices = vec![].into(); - } + |attestation, _| { + *attestation.attesting_indices_base_mut().unwrap() = vec![].into(); }, |result| { assert_invalid_attestation!(result, InvalidAttestation::EmptyAggregationBitfield) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index b131f7679a3..d008d3390e4 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -157,16 +157,12 @@ pub fn get_attesting_indices_from_state( state: &BeaconState, att: AttestationRef, ) -> Result, BeaconStateError> { + let committee = state.get_beacon_committee(att.data().slot, att.data().index)?; match att { - AttestationRef::Base(att) => { - let committee = state.get_beacon_committee(att.data.slot, att.data.index)?; - attesting_indices_base::get_attesting_indices::( - committee.committee, - &att.aggregation_bits, - ) - } - AttestationRef::Electra(att) => { - attesting_indices_electra::get_attesting_indices_from_state::(state, att) + Attestation::Base(att) => { + get_attesting_indices::(committee.committee, &att.aggregation_bits) } + // TODO(electra) implement get_attesting_indices for electra + Attestation::Electra(_) => todo!(), } } diff --git a/consensus/state_processing/src/common/get_indexed_attestation.rs b/consensus/state_processing/src/common/get_indexed_attestation.rs new file mode 100644 index 00000000000..d6c7512961c --- /dev/null +++ b/consensus/state_processing/src/common/get_indexed_attestation.rs @@ -0,0 +1,25 @@ +use super::get_attesting_indices; +use crate::per_block_processing::errors::{AttestationInvalid as Invalid, BlockOperationError}; +use types::{indexed_attestation::IndexedAttestationBase, *}; + +type Result = std::result::Result>; + +/// Convert `attestation` to (almost) indexed-verifiable form. +/// +/// Spec v0.12.1 +pub fn get_indexed_attestation( + committee: &[usize], + attestation: &Attestation, +) -> Result> { + let attesting_indices = match attestation { + Attestation::Base(att) => get_attesting_indices::(committee, &att.aggregation_bits)?, + // TODO(electra) implement get_attesting_indices for electra + Attestation::Electra(_) => todo!(), + }; + + Ok(IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(attesting_indices)?, + data: attestation.data().clone(), + signature: attestation.signature().clone(), + })) +} diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index b0eaf3422d3..96a2d190909 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -21,7 +21,13 @@ pub struct ConsensusContext { /// Block root of the block at `slot`. pub current_block_root: Option, /// Cache of indexed attestations constructed during block processing. - pub indexed_attestations: HashMap>, + pub indexed_attestations: HashMap< + ( + AttestationData, + BitList, + ), + IndexedAttestation, + >, } #[derive(Debug, PartialEq, Clone)] @@ -150,27 +156,35 @@ impl ConsensusContext { pub fn get_indexed_attestation<'a>( &'a mut self, state: &BeaconState, - attestation: AttestationRef<'a, E>, - ) -> Result, BlockOperationError> { - let key = attestation.tree_hash_root(); - match attestation { - AttestationRef::Base(attn) => match self.indexed_attestations.entry(key) { - Entry::Occupied(occupied) => Ok(occupied.into_mut()), - Entry::Vacant(vacant) => { - let committee = state.get_beacon_committee(attn.data.slot, attn.data.index)?; - let indexed_attestation = - attesting_indices_base::get_indexed_attestation(committee.committee, attn)?; - Ok(vacant.insert(indexed_attestation)) + attestation: &Attestation, + ) -> Result<&IndexedAttestation, BlockOperationError> { + let aggregation_bits = match attestation { + Attestation::Base(attn) => { + let mut extended_aggregation_bits: BitList = + BitList::with_capacity(attn.aggregation_bits.len()) + .map_err(BeaconStateError::from)?; + + for (i, bit) in attn.aggregation_bits.iter().enumerate() { + extended_aggregation_bits + .set(i, bit) + .map_err(BeaconStateError::from)?; } - }, - AttestationRef::Electra(attn) => match self.indexed_attestations.entry(key) { - Entry::Occupied(occupied) => Ok(occupied.into_mut()), - Entry::Vacant(vacant) => { - let indexed_attestation = - attesting_indices_electra::get_indexed_attestation_from_state(state, attn)?; - Ok(vacant.insert(indexed_attestation)) - } - }, + extended_aggregation_bits + } + Attestation::Electra(attn) => attn.aggregation_bits.clone(), + }; + + let key = (attestation.data().clone(), aggregation_bits); + + match self.indexed_attestations.entry(key) { + Entry::Occupied(occupied) => Ok(occupied.into_mut()), + Entry::Vacant(vacant) => { + let committee = state + .get_beacon_committee(attestation.data().slot, attestation.data().index)?; + let indexed_attestation = + get_indexed_attestation(committee.committee, attestation)?; + Ok(vacant.insert(indexed_attestation)) + } } .map(|indexed_attestation| (*indexed_attestation).to_ref()) } @@ -182,7 +196,13 @@ impl ConsensusContext { #[must_use] pub fn set_indexed_attestations( mut self, - attestations: HashMap>, + attestations: HashMap< + ( + AttestationData, + BitList, + ), + IndexedAttestation, + >, ) -> Self { self.indexed_attestations = attestations; self diff --git a/consensus/state_processing/src/per_block_processing/process_operations.rs b/consensus/state_processing/src/per_block_processing/process_operations.rs index bd354901a8b..25fc256ceba 100644 --- a/consensus/state_processing/src/per_block_processing/process_operations.rs +++ b/consensus/state_processing/src/per_block_processing/process_operations.rs @@ -76,30 +76,33 @@ pub mod base { ) .map_err(|e| e.into_with_index(i))?; - let AttestationRef::Base(attestation) = attestation else { - // Pending attestations have been deprecated in a altair, this branch should - // never happen - return Err(BlockProcessingError::PendingAttestationInElectra); - }; - - let pending_attestation = PendingAttestation { - aggregation_bits: attestation.aggregation_bits.clone(), - data: attestation.data.clone(), - inclusion_delay: state.slot().safe_sub(attestation.data.slot)?.as_u64(), - proposer_index, + match attestation { + Attestation::Base(att) => { + let pending_attestation = PendingAttestation { + aggregation_bits: att.aggregation_bits.clone(), + data: att.data.clone(), + inclusion_delay: state.slot().safe_sub(att.data.slot)?.as_u64(), + proposer_index, + }; + + if attestation.data().target.epoch == state.current_epoch() { + state + .as_base_mut()? + .current_epoch_attestations + .push(pending_attestation)?; + } else { + state + .as_base_mut()? + .previous_epoch_attestations + .push(pending_attestation)?; + } + } + Attestation::Electra(_) => { + // TODO(electra) pending attestations are only phase 0 + // so we should just raise a relevant error here + todo!() + } }; - - if attestation.data.target.epoch == state.current_epoch() { - state - .as_base_mut()? - .current_epoch_attestations - .push(pending_attestation)?; - } else { - state - .as_base_mut()? - .previous_epoch_attestations - .push(pending_attestation)?; - } } Ok(()) 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 2e00ee03418..443e4099a1f 100644 --- a/consensus/state_processing/src/per_block_processing/signature_sets.rs +++ b/consensus/state_processing/src/per_block_processing/signature_sets.rs @@ -346,15 +346,15 @@ where indexed_attestation_signature_set( state, get_pubkey.clone(), - attester_slashing.attestation_1().signature(), - attester_slashing.attestation_1(), + attester_slashing.attestation_1.signature(), + &attester_slashing.attestation_1, spec, )?, indexed_attestation_signature_set( state, get_pubkey, - attester_slashing.attestation_2().signature(), - attester_slashing.attestation_2(), + attester_slashing.attestation_2.signature(), + &attester_slashing.attestation_2, spec, )?, )) @@ -425,7 +425,7 @@ where E: EthSpec, F: Fn(usize) -> Option>, { - let slot = signed_aggregate_and_proof.message().aggregate().data().slot; + let slot = signed_aggregate_and_proof.message.aggregate.data().slot; let domain = spec.get_domain( slot.epoch(E::slots_per_epoch()), @@ -455,8 +455,8 @@ where F: Fn(usize) -> Option>, { let target_epoch = signed_aggregate_and_proof - .message() - .aggregate() + .message + .aggregate .data() .target .epoch; diff --git a/consensus/state_processing/src/per_block_processing/tests.rs b/consensus/state_processing/src/per_block_processing/tests.rs index 2774dd3d87f..aa19328f92d 100644 --- a/consensus/state_processing/src/per_block_processing/tests.rs +++ b/consensus/state_processing/src/per_block_processing/tests.rs @@ -388,12 +388,7 @@ async fn invalid_attestation_no_committee_for_index() { .clone() .deconstruct() .0; - head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() + head_block.to_mut().body_mut().attestations_mut()[0] .data_mut() .index += 1; let mut ctxt = ConsensusContext::new(state.slot()); @@ -428,21 +423,10 @@ async fn invalid_attestation_wrong_justified_checkpoint() { .clone() .deconstruct() .0; - let old_justified_checkpoint = head_block - .body() - .attestations() - .next() - .unwrap() - .data() - .source; + let old_justified_checkpoint = head_block.body().attestations()[0].data().source; let mut new_justified_checkpoint = old_justified_checkpoint; new_justified_checkpoint.epoch += Epoch::new(1); - head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() + head_block.to_mut().body_mut().attestations_mut()[0] .data_mut() .source = new_justified_checkpoint; @@ -483,12 +467,7 @@ async fn invalid_attestation_bad_aggregation_bitfield_len() { .clone() .deconstruct() .0; - *head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() + *head_block.to_mut().body_mut().attestations_mut()[0] .aggregation_bits_base_mut() .unwrap() = Bitfield::with_capacity(spec.target_committee_size).unwrap(); @@ -523,13 +502,8 @@ async fn invalid_attestation_bad_signature() { .clone() .deconstruct() .0; - *head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() - .signature_mut() = AggregateSignature::empty(); + *head_block.to_mut().body_mut().attestations_mut()[0].signature_mut() = + AggregateSignature::empty(); let mut ctxt = ConsensusContext::new(state.slot()); let result = process_operations::process_attestations( @@ -564,14 +538,9 @@ async fn invalid_attestation_included_too_early() { .clone() .deconstruct() .0; - let new_attesation_slot = head_block.body().attestations().next().unwrap().data().slot + let new_attesation_slot = head_block.body().attestations()[0].data().slot + Slot::new(MainnetEthSpec::slots_per_epoch()); - head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() + head_block.to_mut().body_mut().attestations_mut()[0] .data_mut() .slot = new_attesation_slot; @@ -612,14 +581,9 @@ async fn invalid_attestation_included_too_late() { .clone() .deconstruct() .0; - let new_attesation_slot = head_block.body().attestations().next().unwrap().data().slot + let new_attesation_slot = head_block.body().attestations()[0].data().slot - Slot::new(MainnetEthSpec::slots_per_epoch()); - head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() + head_block.to_mut().body_mut().attestations_mut()[0] .data_mut() .slot = new_attesation_slot; @@ -657,12 +621,7 @@ async fn invalid_attestation_target_epoch_slot_mismatch() { .clone() .deconstruct() .0; - head_block - .to_mut() - .body_mut() - .attestations_mut() - .next() - .unwrap() + head_block.to_mut().body_mut().attestations_mut()[0] .data_mut() .target .epoch += Epoch::new(1); @@ -749,14 +708,10 @@ async fn invalid_attester_slashing_1_invalid() { let harness = get_harness::(EPOCH_OFFSET, VALIDATOR_COUNT).await; let mut attester_slashing = harness.make_attester_slashing(vec![1, 2]); - match &mut attester_slashing { - AttesterSlashing::Base(ref mut attester_slashing) => { - attester_slashing.attestation_1.attesting_indices = VariableList::from(vec![2, 1]); - } - AttesterSlashing::Electra(ref mut attester_slashing) => { - attester_slashing.attestation_1.attesting_indices = VariableList::from(vec![2, 1]); - } - } + *attester_slashing + .attestation_1 + .attesting_indices_base_mut() + .unwrap() = VariableList::from(vec![2, 1]); let mut state = harness.get_current_state(); let mut ctxt = ConsensusContext::new(state.slot()); @@ -787,14 +742,10 @@ async fn invalid_attester_slashing_2_invalid() { let harness = get_harness::(EPOCH_OFFSET, VALIDATOR_COUNT).await; let mut attester_slashing = harness.make_attester_slashing(vec![1, 2]); - match &mut attester_slashing { - AttesterSlashing::Base(ref mut attester_slashing) => { - attester_slashing.attestation_2.attesting_indices = VariableList::from(vec![2, 1]); - } - AttesterSlashing::Electra(ref mut attester_slashing) => { - attester_slashing.attestation_2.attesting_indices = VariableList::from(vec![2, 1]); - } - } + *attester_slashing + .attestation_2 + .attesting_indices_base_mut() + .unwrap() = VariableList::from(vec![2, 1]); let mut state = harness.get_current_state(); let mut ctxt = ConsensusContext::new(state.slot()); diff --git a/consensus/state_processing/src/per_block_processing/verify_attestation.rs b/consensus/state_processing/src/per_block_processing/verify_attestation.rs index 6bfb5d7cfe7..50e2383f35d 100644 --- a/consensus/state_processing/src/per_block_processing/verify_attestation.rs +++ b/consensus/state_processing/src/per_block_processing/verify_attestation.rs @@ -21,7 +21,7 @@ pub fn verify_attestation_for_block_inclusion<'ctxt, E: EthSpec>( ctxt: &'ctxt mut ConsensusContext, verify_signatures: VerifySignatures, spec: &ChainSpec, -) -> Result> { +) -> Result<&'ctxt IndexedAttestation> { let data = attestation.data(); verify!( @@ -65,7 +65,7 @@ pub fn verify_attestation_for_state<'ctxt, E: EthSpec>( ctxt: &'ctxt mut ConsensusContext, verify_signatures: VerifySignatures, spec: &ChainSpec, -) -> Result> { +) -> Result<&'ctxt IndexedAttestation> { let data = attestation.data(); verify!( diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index c4b7c6a026f..06c3ca42c93 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -262,8 +262,8 @@ impl VerifyOperation for AttesterSlashing { #[allow(clippy::arithmetic_side_effects)] fn verification_epochs(&self) -> SmallVec<[Epoch; MAX_FORKS_VERIFIED_AGAINST]> { smallvec![ - self.attestation_1().data().target.epoch, - self.attestation_2().data().target.epoch + self.attestation_1.data().target.epoch, + self.attestation_2.data().target.epoch ] } } diff --git a/consensus/types/src/aggregate_and_proof.rs b/consensus/types/src/aggregate_and_proof.rs index 223b12e7684..54a4a1aed7f 100644 --- a/consensus/types/src/aggregate_and_proof.rs +++ b/consensus/types/src/aggregate_and_proof.rs @@ -88,15 +88,17 @@ impl AggregateAndProof { genesis_validators_root: Hash256, spec: &ChainSpec, ) -> Self { - let selection_proof = selection_proof.unwrap_or_else(|| { - SelectionProof::new::( - aggregate.data().slot, - secret_key, - fork, - genesis_validators_root, - spec, - ) - }); + let selection_proof = selection_proof + .unwrap_or_else(|| { + SelectionProof::new::( + aggregate.data().slot, + secret_key, + fork, + genesis_validators_root, + spec, + ) + }) + .into(); Self::from_attestation( aggregator_index, @@ -133,15 +135,15 @@ impl AggregateAndProof { genesis_validators_root: Hash256, spec: &ChainSpec, ) -> bool { - let target_epoch = self.aggregate().data().slot.epoch(E::slots_per_epoch()); + let target_epoch = self.aggregate.data().slot.epoch(E::slots_per_epoch()); let domain = spec.get_domain( target_epoch, Domain::SelectionProof, fork, genesis_validators_root, ); - let message = self.aggregate().data().slot.signing_root(domain); - self.selection_proof().verify(validator_pubkey, message) + let message = self.aggregate.data().slot.signing_root(domain); + self.selection_proof.verify(validator_pubkey, message) } } diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 88993267a94..2db167b96ae 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -1,9 +1,10 @@ use crate::slot_data::SlotData; -use crate::Checkpoint; use crate::{test_utils::TestRandom, Hash256, Slot}; use derivative::Derivative; +use rand::RngCore; use safe_arith::ArithError; use serde::{Deserialize, Serialize}; +use ssz::Decode; use ssz_derive::{Decode, Encode}; use ssz_types::BitVector; use std::hash::{Hash, Hasher}; @@ -44,10 +45,7 @@ pub enum Error { derivative(PartialEq, Hash(bound = "E: EthSpec")), serde(bound = "E: EthSpec", deny_unknown_fields), arbitrary(bound = "E: EthSpec"), - ), - ref_attributes(derive(TreeHash), tree_hash(enum_behaviour = "transparent")), - cast_error(ty = "Error", expr = "Error::IncorrectStateVariant"), - partial_getter_error(ty = "Error", expr = "Error::IncorrectStateVariant") + ) )] #[derive( Debug, @@ -69,11 +67,49 @@ pub struct Attestation { #[superstruct(only(Base), partial_getter(rename = "aggregation_bits_base"))] pub aggregation_bits: BitList, #[superstruct(only(Electra), partial_getter(rename = "aggregation_bits_electra"))] - pub aggregation_bits: BitList, + pub aggregation_bits: BitList, pub data: AttestationData, #[superstruct(only(Electra))] pub committee_bits: BitVector, pub signature: AggregateSignature, + #[superstruct(only(Electra))] + pub committee_bits: BitVector, +} + +impl Decode for Attestation { + fn is_ssz_fixed_len() -> bool { + false + } + + fn from_ssz_bytes(bytes: &[u8]) -> Result { + if let Ok(result) = AttestationBase::from_ssz_bytes(bytes) { + return Ok(Attestation::Base(result)); + } + + if let Ok(result) = AttestationElectra::from_ssz_bytes(bytes) { + return Ok(Attestation::Electra(result)); + } + + Err(ssz::DecodeError::BytesInvalid(String::from( + "bytes not valid for any fork variant", + ))) + } +} + +impl TestRandom for Attestation { + fn random_for_test(rng: &mut impl RngCore) -> Self { + let aggregation_bits: BitList = BitList::random_for_test(rng); + // let committee_bits: BitList = BitList::random_for_test(rng); + let data = AttestationData::random_for_test(rng); + let signature = AggregateSignature::random_for_test(rng); + + Self::Base(AttestationBase { + aggregation_bits, + // committee_bits, + data, + signature, + }) + } } impl Hash for Attestation { @@ -88,72 +124,38 @@ impl Hash for Attestation { } } -impl Attestation { - /// Produces an attestation with empty signature. - pub fn empty_for_signing( - committee_index: u64, - committee_length: usize, - slot: Slot, - beacon_block_root: Hash256, - source: Checkpoint, - target: Checkpoint, - spec: &ChainSpec, - ) -> Result { - if spec.fork_name_at_slot::(slot).electra_enabled() { - let mut committee_bits: BitVector = BitVector::default(); - committee_bits - .set(committee_index as usize, true) - .map_err(|_| Error::InvalidCommitteeIndex)?; - Ok(Attestation::Electra(AttestationElectra { - aggregation_bits: BitList::with_capacity(committee_length) - .map_err(|_| Error::InvalidCommitteeLength)?, - data: AttestationData { - slot, - index: 0u64, - beacon_block_root, - source, - target, - }, - committee_bits, - signature: AggregateSignature::infinity(), - })) - } else { - Ok(Attestation::Base(AttestationBase { - aggregation_bits: BitList::with_capacity(committee_length) - .map_err(|_| Error::InvalidCommitteeLength)?, - data: AttestationData { - slot, - index: committee_index, - beacon_block_root, - source, - target, - }, - signature: AggregateSignature::infinity(), - })) +impl Hash for Attestation { + fn hash(&self, state: &mut H) + where + H: Hasher, + { + match self { + Attestation::Base(att) => att.hash(state), + Attestation::Electra(att) => att.hash(state), } } +} +impl Attestation { /// Aggregate another Attestation into this one. /// /// The aggregation bitfields must be disjoint, and the data must be the same. - pub fn aggregate(&mut self, other: AttestationRef) { + pub fn aggregate(&mut self, other: &Self) { match self { - Attestation::Base(att) => match other { - AttestationRef::Base(oth) => { - att.aggregate(oth); - } - AttestationRef::Electra(_) => { - debug_assert!(false, "Cannot aggregate base and electra attestations"); - } - }, - Attestation::Electra(att) => match other { - AttestationRef::Base(_) => { - debug_assert!(false, "Cannot aggregate base and electra attestations"); + Attestation::Base(att) => { + debug_assert!(other.as_base().is_ok()); + + if let Ok(other) = other.as_base() { + att.aggregate(other) } - AttestationRef::Electra(oth) => { - att.aggregate(oth); + } + Attestation::Electra(att) => { + debug_assert!(other.as_electra().is_ok()); + + if let Ok(other) = other.as_electra() { + att.aggregate(other) } - }, + } } } @@ -198,9 +200,9 @@ impl Attestation { } } - pub fn committee_index(&self) -> Option { + pub fn committee_index(&self) -> u64 { match self { - Attestation::Base(att) => Some(att.data.index), + Attestation::Base(att) => att.data.index, Attestation::Electra(att) => att.committee_index(), } } @@ -227,66 +229,90 @@ impl Attestation { } } -impl<'a, E: EthSpec> AttestationRef<'a, E> { - pub fn clone_as_attestation(self) -> Attestation { - match self { - Self::Base(att) => Attestation::Base(att.clone()), - Self::Electra(att) => Attestation::Electra(att.clone()), - } +impl AttestationElectra { + /// Are the aggregation bitfields of these attestations disjoint? + pub fn signers_disjoint_from(&self, other: &Self) -> bool { + self.aggregation_bits + .intersection(&other.aggregation_bits) + .is_zero() } - pub fn is_aggregation_bits_zero(self) -> bool { - match self { - Self::Base(att) => att.aggregation_bits.is_zero(), - Self::Electra(att) => att.aggregation_bits.is_zero(), - } + pub fn committee_index(&self) -> u64 { + *self.get_committee_indices().first().unwrap_or(&0u64) } - pub fn num_set_aggregation_bits(&self) -> usize { - match self { - Self::Base(att) => att.aggregation_bits.num_set_bits(), - Self::Electra(att) => att.aggregation_bits.num_set_bits(), - } + pub fn get_committee_indices(&self) -> Vec { + self.committee_bits + .iter() + .enumerate() + .filter_map(|(index, bit)| if bit { Some(index as u64) } else { None }) + .collect() } - pub fn committee_index(&self) -> Option { - match self { - AttestationRef::Base(att) => Some(att.data.index), - AttestationRef::Electra(att) => att.committee_index(), - } + /// Aggregate another Attestation into this one. + /// + /// The aggregation bitfields must be disjoint, and the data must be the same. + pub fn aggregate(&mut self, other: &Self) { + debug_assert_eq!(self.data, other.data); + debug_assert!(self.signers_disjoint_from(other)); + self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); + self.signature.add_assign_aggregate(&other.signature); } - pub fn set_aggregation_bits(&self) -> Vec { - match self { - Self::Base(att) => att - .aggregation_bits - .iter() - .enumerate() - .filter(|(_i, bit)| *bit) - .map(|(i, _bit)| i) - .collect::>(), - Self::Electra(att) => att - .aggregation_bits - .iter() - .enumerate() - .filter(|(_i, bit)| *bit) - .map(|(i, _bit)| i) - .collect::>(), - } + /// Signs `self`, setting the `committee_position`'th bit of `aggregation_bits` to `true`. + /// + /// Returns an `AlreadySigned` error if the `committee_position`'th bit is already `true`. + pub fn sign( + &mut self, + secret_key: &SecretKey, + committee_position: usize, + fork: &Fork, + genesis_validators_root: Hash256, + spec: &ChainSpec, + ) -> Result<(), Error> { + let domain = spec.get_domain( + self.data.target.epoch, + Domain::BeaconAttester, + fork, + genesis_validators_root, + ); + let message = self.data.signing_root(domain); + + self.add_signature(&secret_key.sign(message), committee_position) } -} -impl AttestationElectra { - pub fn committee_index(&self) -> Option { - self.get_committee_indices().first().cloned() + /// Adds `signature` to `self` and sets the `committee_position`'th bit of `aggregation_bits` to `true`. + /// + /// Returns an `AlreadySigned` error if the `committee_position`'th bit is already `true`. + pub fn add_signature( + &mut self, + signature: &Signature, + committee_position: usize, + ) -> Result<(), Error> { + if self + .aggregation_bits + .get(committee_position) + .map_err(Error::SszTypesError)? + { + Err(Error::AlreadySigned(committee_position)) + } else { + self.aggregation_bits + .set(committee_position, true) + .map_err(Error::SszTypesError)?; + + self.signature.add_assign(signature); + + Ok(()) + } } +} - pub fn get_committee_indices(&self) -> Vec { - self.committee_bits - .iter() - .enumerate() - .filter_map(|(index, bit)| if bit { Some(index as u64) } else { None }) - .collect() +impl AttestationBase { + /// Are the aggregation bitfields of these attestations disjoint? + pub fn signers_disjoint_from(&self, other: &Self) -> bool { + self.aggregation_bits + .intersection(&other.aggregation_bits) + .is_zero() } /// Aggregate another Attestation into this one. @@ -422,71 +448,6 @@ impl SlotData for Attestation { } } -impl<'a, E: EthSpec> SlotData for AttestationRef<'a, E> { - fn get_slot(&self) -> Slot { - self.data().slot - } -} - -#[derive(Debug, Clone, Encode, Decode, PartialEq)] -#[ssz(enum_behaviour = "union")] -pub enum AttestationOnDisk { - Base(AttestationBase), - Electra(AttestationElectra), -} - -impl AttestationOnDisk { - pub fn to_ref(&self) -> AttestationRefOnDisk { - match self { - AttestationOnDisk::Base(att) => AttestationRefOnDisk::Base(att), - AttestationOnDisk::Electra(att) => AttestationRefOnDisk::Electra(att), - } - } -} - -#[derive(Debug, Clone, Encode)] -#[ssz(enum_behaviour = "union")] -pub enum AttestationRefOnDisk<'a, E: EthSpec> { - Base(&'a AttestationBase), - Electra(&'a AttestationElectra), -} - -impl From> for AttestationOnDisk { - fn from(attestation: Attestation) -> Self { - match attestation { - Attestation::Base(attestation) => Self::Base(attestation), - Attestation::Electra(attestation) => Self::Electra(attestation), - } - } -} - -impl From> for Attestation { - fn from(attestation: AttestationOnDisk) -> Self { - match attestation { - AttestationOnDisk::Base(attestation) => Self::Base(attestation), - AttestationOnDisk::Electra(attestation) => Self::Electra(attestation), - } - } -} - -impl<'a, E: EthSpec> From> for AttestationRefOnDisk<'a, E> { - fn from(attestation: AttestationRef<'a, E>) -> Self { - match attestation { - AttestationRef::Base(attestation) => Self::Base(attestation), - AttestationRef::Electra(attestation) => Self::Electra(attestation), - } - } -} - -impl<'a, E: EthSpec> From> for AttestationRef<'a, E> { - fn from(attestation: AttestationRefOnDisk<'a, E>) -> Self { - match attestation { - AttestationRefOnDisk::Base(attestation) => Self::Base(attestation), - AttestationRefOnDisk::Electra(attestation) => Self::Electra(attestation), - } - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index f67a965955c..0381fc104a3 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -12,7 +12,7 @@ use test_random_derive::TestRandom; use tree_hash::TreeHash; use tree_hash_derive::TreeHash; -use self::indexed_attestation::{IndexedAttestationBase, IndexedAttestationElectra}; +use self::indexed_attestation::IndexedAttestationBase; /// A block of the `BeaconChain`. #[superstruct( @@ -328,15 +328,16 @@ impl> BeaconBlockBase { message: header, signature: Signature::empty(), }; - let indexed_attestation = IndexedAttestationBase { - attesting_indices: VariableList::new(vec![ - 0_u64; - E::MaxValidatorsPerCommittee::to_usize() - ]) - .unwrap(), - data: AttestationData::default(), - signature: AggregateSignature::empty(), - }; + let indexed_attestation: IndexedAttestation = + IndexedAttestation::Base(IndexedAttestationBase { + attesting_indices: VariableList::new(vec![ + 0_u64; + E::MaxValidatorsPerCommittee::to_usize() + ]) + .unwrap(), + data: AttestationData::default(), + signature: AggregateSignature::empty(), + }); let deposit_data = DepositData { pubkey: PublicKeyBytes::empty(), @@ -354,12 +355,12 @@ impl> BeaconBlockBase { attestation_2: indexed_attestation, }; - let attestation = AttestationBase { + let attestation: Attestation = Attestation::Base(AttestationBase { aggregation_bits: BitList::with_capacity(E::MaxValidatorsPerCommittee::to_usize()) .unwrap(), data: AttestationData::default(), signature: AggregateSignature::empty(), - }; + }); let deposit = Deposit { proof: FixedVector::from_elem(Hash256::zero()), diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index 1c379f5de42..d367d038a59 100644 --- a/consensus/types/src/eth_spec.rs +++ b/consensus/types/src/eth_spec.rs @@ -63,7 +63,7 @@ pub trait EthSpec: * Misc */ type MaxValidatorsPerCommittee: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; - type MaxValidatorsPerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; + type MaxValidatorsPerCommitteePerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; type MaxCommitteesPerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; /* * Time parameters @@ -368,7 +368,7 @@ impl EthSpec for MainnetEthSpec { type SubnetBitfieldLength = U64; type MaxValidatorsPerCommittee = U2048; type MaxCommitteesPerSlot = U64; - type MaxValidatorsPerSlot = U131072; + type MaxValidatorsPerCommitteePerSlot = U131072; type GenesisEpoch = U0; type SlotsPerEpoch = U32; type EpochsPerEth1VotingPeriod = U64; @@ -450,6 +450,8 @@ impl EthSpec for MinimalEthSpec { SubnetBitfieldLength, SyncCommitteeSubnetCount, MaxValidatorsPerCommittee, + MaxCommitteesPerSlot, + MaxValidatorsPerCommitteePerSlot, GenesisEpoch, HistoricalRootsLimit, ValidatorRegistryLimit, @@ -491,7 +493,7 @@ impl EthSpec for GnosisEthSpec { type SubnetBitfieldLength = U64; type MaxValidatorsPerCommittee = U2048; type MaxCommitteesPerSlot = U64; - type MaxValidatorsPerSlot = U131072; + type MaxValidatorsPerCommitteePerSlot = U131072; type GenesisEpoch = U0; type SlotsPerEpoch = U16; type EpochsPerEth1VotingPeriod = U64; diff --git a/consensus/types/src/indexed_attestation.rs b/consensus/types/src/indexed_attestation.rs index 19d15010753..6ca266e5c30 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -1,7 +1,9 @@ use crate::{test_utils::TestRandom, AggregateSignature, AttestationData, EthSpec, VariableList}; use core::slice::Iter; use derivative::Derivative; +use rand::RngCore; use serde::{Deserialize, Serialize}; +use ssz::Decode; use ssz::Encode; use ssz_derive::{Decode, Encode}; use std::hash::{Hash, Hasher}; @@ -57,7 +59,7 @@ pub struct IndexedAttestation { pub attesting_indices: VariableList, #[superstruct(only(Electra), partial_getter(rename = "attesting_indices_electra"))] #[serde(with = "quoted_variable_list_u64")] - pub attesting_indices: VariableList, + pub attesting_indices: VariableList, pub data: AttestationData, pub signature: AggregateSignature, } @@ -67,16 +69,15 @@ impl IndexedAttestation { /// /// Spec v0.12.1 pub fn is_double_vote(&self, other: &Self) -> bool { - // reuse the ref implementation to ensure logic is the same - self.to_ref().is_double_vote(other.to_ref()) + self.data().target.epoch == other.data().target.epoch && self.data() != other.data() } /// Check if ``attestation_data_1`` surrounds ``attestation_data_2``. /// /// Spec v0.12.1 pub fn is_surround_vote(&self, other: &Self) -> bool { - // reuse the ref implementation to ensure logic is the same - self.to_ref().is_surround_vote(other.to_ref()) + self.data().source.epoch < other.data().source.epoch + && other.data().target.epoch < self.data().target.epoch } pub fn attesting_indices_len(&self) -> usize { @@ -113,77 +114,39 @@ impl IndexedAttestation { IndexedAttestation::Electra(att) => att.attesting_indices.first(), } } - - pub fn to_electra(self) -> IndexedAttestationElectra { - match self { - Self::Base(att) => { - let extended_attesting_indices: VariableList = - VariableList::new(att.attesting_indices.to_vec()) - .expect("MaxValidatorsPerSlot must be >= MaxValidatorsPerCommittee"); - // Note a unit test in consensus/types/src/eth_spec.rs asserts this invariant for - // all known specs - - IndexedAttestationElectra { - attesting_indices: extended_attesting_indices, - data: att.data, - signature: att.signature, - } - } - Self::Electra(att) => att, - } - } } -impl<'a, E: EthSpec> IndexedAttestationRef<'a, E> { - pub fn is_double_vote(&self, other: Self) -> bool { - self.data().target.epoch == other.data().target.epoch && self.data() != other.data() - } - - pub fn is_surround_vote(&self, other: Self) -> bool { - self.data().source.epoch < other.data().source.epoch - && other.data().target.epoch < self.data().target.epoch - } - - pub fn attesting_indices_len(&self) -> usize { - match self { - IndexedAttestationRef::Base(att) => att.attesting_indices.len(), - IndexedAttestationRef::Electra(att) => att.attesting_indices.len(), - } - } - - pub fn attesting_indices_to_vec(&self) -> Vec { - match self { - IndexedAttestationRef::Base(att) => att.attesting_indices.to_vec(), - IndexedAttestationRef::Electra(att) => att.attesting_indices.to_vec(), - } +impl Decode for IndexedAttestation { + fn is_ssz_fixed_len() -> bool { + false } - pub fn attesting_indices_is_empty(&self) -> bool { - match self { - IndexedAttestationRef::Base(att) => att.attesting_indices.is_empty(), - IndexedAttestationRef::Electra(att) => att.attesting_indices.is_empty(), + fn from_ssz_bytes(bytes: &[u8]) -> Result { + if let Ok(result) = IndexedAttestationBase::from_ssz_bytes(bytes) { + return Ok(IndexedAttestation::Base(result)); } - } - pub fn attesting_indices_iter(&self) -> Iter<'_, u64> { - match self { - IndexedAttestationRef::Base(att) => att.attesting_indices.iter(), - IndexedAttestationRef::Electra(att) => att.attesting_indices.iter(), + if let Ok(result) = IndexedAttestationElectra::from_ssz_bytes(bytes) { + return Ok(IndexedAttestation::Electra(result)); } - } - pub fn attesting_indices_first(&self) -> Option<&u64> { - match self { - IndexedAttestationRef::Base(att) => att.attesting_indices.first(), - IndexedAttestationRef::Electra(att) => att.attesting_indices.first(), - } + Err(ssz::DecodeError::BytesInvalid(String::from( + "bytes not valid for any fork variant", + ))) } +} - pub fn clone_as_indexed_attestation(self) -> IndexedAttestation { - match self { - IndexedAttestationRef::Base(att) => IndexedAttestation::Base(att.clone()), - IndexedAttestationRef::Electra(att) => IndexedAttestation::Electra(att.clone()), - } +impl TestRandom for IndexedAttestation { + fn random_for_test(rng: &mut impl RngCore) -> Self { + let attesting_indices = VariableList::random_for_test(rng); + let data = AttestationData::random_for_test(rng); + let signature = AggregateSignature::random_for_test(rng); + + Self::Base(IndexedAttestationBase { + attesting_indices, + data, + signature, + }) } } diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index 26eca19bf15..360e36e8479 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -77,7 +77,8 @@ impl SignedAggregateAndProof { genesis_validators_root, spec, ); - let target_epoch = message.aggregate().data().slot.epoch(E::slots_per_epoch()); + + let target_epoch = message.aggregate.data().slot.epoch(E::slots_per_epoch()); let domain = spec.get_domain( target_epoch, Domain::AggregateAndProof, diff --git a/lcli/src/indexed_attestations.rs b/lcli/src/indexed_attestations.rs index ccc14171128..9441f5e114f 100644 --- a/lcli/src/indexed_attestations.rs +++ b/lcli/src/indexed_attestations.rs @@ -33,14 +33,9 @@ pub fn run(matches: &ArgMatches) -> Result<(), String> { let indexed_attestations = attestations .into_iter() - .map(|att| match att { - Attestation::Base(att) => { - let committee = state.get_beacon_committee(att.data.slot, att.data.index)?; - attesting_indices_base::get_indexed_attestation(committee.committee, &att) - } - Attestation::Electra(att) => { - attesting_indices_electra::get_indexed_attestation_from_state(&state, &att) - } + .map(|att| { + let committee = state.get_beacon_committee(att.data().slot, att.data().index)?; + get_indexed_attestation(committee.committee, &att) }) .collect::, _>>() .map_err(|e| format!("Error constructing indexed attestation: {:?}", e))?; diff --git a/slasher/src/attester_record.rs b/slasher/src/attester_record.rs index 1cd4ba7d4e0..58d62273194 100644 --- a/slasher/src/attester_record.rs +++ b/slasher/src/attester_record.rs @@ -80,7 +80,7 @@ impl IndexedAttesterRecord { #[derive(Debug, Clone, Encode, Decode, TreeHash)] struct IndexedAttestationHeader { - pub attesting_indices: VariableList, + pub attesting_indices: VariableList, pub data_root: Hash256, pub signature: AggregateSignature, } diff --git a/slasher/src/slasher.rs b/slasher/src/slasher.rs index 0bb7c9c3ffe..ab1fe7ec62d 100644 --- a/slasher/src/slasher.rs +++ b/slasher/src/slasher.rs @@ -300,7 +300,7 @@ impl Slasher { self.log, "Found double-vote slashing"; "validator_index" => validator_index, - "epoch" => slashing.attestation_1().data().target.epoch, + "epoch" => slashing.attestation_1.data().target.epoch, ); slashings.insert(slashing); } diff --git a/slasher/src/test_utils.rs b/slasher/src/test_utils.rs index 453d0e66670..d1d59fa25f3 100644 --- a/slasher/src/test_utils.rs +++ b/slasher/src/test_utils.rs @@ -1,10 +1,9 @@ use std::collections::HashSet; use std::sync::Arc; use types::{ - indexed_attestation::{IndexedAttestationBase, IndexedAttestationElectra}, - AggregateSignature, AttestationData, AttesterSlashing, AttesterSlashingBase, - AttesterSlashingElectra, BeaconBlockHeader, ChainSpec, Checkpoint, Epoch, EthSpec, Hash256, - IndexedAttestation, MainnetEthSpec, Signature, SignedBeaconBlockHeader, Slot, + indexed_attestation::IndexedAttestationBase, AggregateSignature, AttestationData, + AttesterSlashing, BeaconBlockHeader, Checkpoint, Epoch, Hash256, IndexedAttestation, + MainnetEthSpec, Signature, SignedBeaconBlockHeader, Slot, }; pub type E = MainnetEthSpec; @@ -15,31 +14,7 @@ pub fn indexed_att_electra( target_epoch: u64, target_root: u64, ) -> IndexedAttestation { - IndexedAttestation::Electra(IndexedAttestationElectra { - attesting_indices: attesting_indices.as_ref().to_vec().into(), - data: AttestationData { - slot: Slot::new(0), - index: 0, - beacon_block_root: Hash256::zero(), - source: Checkpoint { - epoch: Epoch::new(source_epoch), - root: Hash256::from_low_u64_be(0), - }, - target: Checkpoint { - epoch: Epoch::new(target_epoch), - root: Hash256::from_low_u64_be(target_root), - }, - }, - signature: AggregateSignature::empty(), - }) -} - -pub fn indexed_att( - attesting_indices: impl AsRef<[u64]>, - source_epoch: u64, - target_epoch: u64, - target_root: u64, -) -> IndexedAttestation { + // TODO(electra) make fork-agnostic IndexedAttestation::Base(IndexedAttestationBase { attesting_indices: attesting_indices.as_ref().to_vec().into(), data: AttestationData { @@ -121,6 +96,10 @@ pub fn slashed_validators_from_attestations( continue; } + // TODO(electra) in a nested loop, copying to vecs feels bad + let attesting_indices_1 = att1.attesting_indices_to_vec(); + let attesting_indices_2 = att2.attesting_indices_to_vec(); + if att1.is_double_vote(att2) || att1.is_surround_vote(att2) { let attesting_indices_1 = att1.attesting_indices_to_vec(); let attesting_indices_2 = att2.attesting_indices_to_vec(); diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 0cff39546dd..9570f9499ba 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -14,7 +14,10 @@ use std::ops::Deref; use std::sync::Arc; use tokio::time::{sleep, sleep_until, Duration, Instant}; use tree_hash::TreeHash; -use types::{Attestation, AttestationData, ChainSpec, CommitteeIndex, EthSpec, Slot}; +use types::{ + attestation::AttestationBase, AggregateSignature, Attestation, AttestationData, BitList, + ChainSpec, CommitteeIndex, EthSpec, Slot, +}; /// Builds an `AttestationService`. pub struct AttestationServiceBuilder { @@ -373,27 +376,11 @@ impl AttestationService { return None; } - let mut attestation = match Attestation::::empty_for_signing( - duty.committee_index, - duty.committee_length as usize, - attestation_data.slot, - attestation_data.beacon_block_root, - attestation_data.source, - attestation_data.target, - &self.context.eth2_config.spec, - ) { - Ok(attestation) => attestation, - Err(err) => { - crit!( - log, - "Invalid validator duties during signing"; - "validator" => ?duty.pubkey, - "duty" => ?duty, - "err" => ?err, - ); - return None; - } - }; + let mut attestation = Attestation::Base(AttestationBase { + aggregation_bits: BitList::with_capacity(duty.committee_length as usize).unwrap(), + data: attestation_data.clone(), + signature: AggregateSignature::infinity(), + }); match self .validator_store @@ -617,10 +604,10 @@ impl AttestationService { info!( log, "Successfully published attestation"; - "aggregator" => signed_aggregate_and_proof.message().aggregator_index(), + "aggregator" => signed_aggregate_and_proof.message.aggregator_index, "signatures" => attestation.num_set_aggregation_bits(), "head_block" => format!("{:?}", attestation.data().beacon_block_root), - "committee_index" => attestation.committee_index(), + "committee_index" => attestation.data().index, "slot" => attestation.data().slot.as_u64(), "type" => "aggregated", ); @@ -633,8 +620,8 @@ impl AttestationService { log, "Failed to publish attestation"; "error" => %e, - "aggregator" => signed_aggregate_and_proof.message().aggregator_index(), - "committee_index" => attestation.committee_index(), + "aggregator" => signed_aggregate_and_proof.message.aggregator_index, + "committee_index" => attestation.data().index, "slot" => attestation.data().slot.as_u64(), "type" => "aggregated", );