From d455c1d29f25d827255e9506892fe0e12689bcd2 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 7 May 2024 13:34:16 +0200 Subject: [PATCH] get tests to pass --- beacon_node/beacon_chain/src/beacon_chain.rs | 42 ++++++++---- .../beacon_chain/src/early_attester_cache.rs | 46 +++++++++---- .../beacon_chain/src/observed_aggregates.rs | 32 +++++---- beacon_node/beacon_chain/src/test_utils.rs | 66 +++++++++++++------ .../operation_pool/src/attestation_storage.rs | 59 +++++++++++++---- beacon_node/store/src/consensus_context.rs | 2 +- consensus/fork_choice/tests/tests.rs | 9 ++- .../src/common/get_attesting_indices.rs | 2 +- .../state_processing/src/consensus_context.rs | 4 +- consensus/types/src/attestation.rs | 6 +- consensus/types/src/beacon_block.rs | 5 +- consensus/types/src/eth_spec.rs | 8 +-- consensus/types/src/indexed_attestation.rs | 2 +- slasher/src/attester_record.rs | 2 +- 14 files changed, 199 insertions(+), 86 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index cd47068977b..973dcaadb47 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -1917,18 +1917,36 @@ impl BeaconChain { }; drop(cache_timer); - // 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(), - })) + if self.spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { + let mut committee_bits = BitVector::default(); + if committee_len > 0 { + committee_bits.set(request_index as usize, true)?; + } + Ok(Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_len)?, + data: AttestationData { + slot: request_slot, + index: 0u64, + beacon_block_root, + source: justified_checkpoint, + target, + }, + committee_bits, + signature: AggregateSignature::empty(), + })) + } else { + 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 diff --git a/beacon_node/beacon_chain/src/early_attester_cache.rs b/beacon_node/beacon_chain/src/early_attester_cache.rs index 595f941235c..8ed4e5db40b 100644 --- a/beacon_node/beacon_chain/src/early_attester_cache.rs +++ b/beacon_node/beacon_chain/src/early_attester_cache.rs @@ -124,18 +124,40 @@ impl EarlyAttesterCache { .get_committee_length::(request_slot, request_index, spec)?; // 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(), - }); + let attestation = if spec.fork_name_at_slot::(request_slot) >= ForkName::Electra { + let mut committee_bits = BitVector::default(); + if committee_len > 0 { + committee_bits + .set(request_index as usize, true) + .map_err(BeaconStateError::from)?; + } + Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_len) + .map_err(BeaconStateError::from)?, + committee_bits, + data: AttestationData { + slot: request_slot, + index: 0u64, + beacon_block_root: item.beacon_block_root, + source: item.source, + target: item.target, + }, + signature: AggregateSignature::empty(), + }) + } else { + 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/observed_aggregates.rs b/beacon_node/beacon_chain/src/observed_aggregates.rs index 857b0edb348..1f14fe62570 100644 --- a/beacon_node/beacon_chain/src/observed_aggregates.rs +++ b/beacon_node/beacon_chain/src/observed_aggregates.rs @@ -20,7 +20,7 @@ pub type ObservedSyncContributions = ObservedAggregates< pub type ObservedAggregateAttestations = ObservedAggregates< Attestation, E, - BitList<::MaxValidatorsPerCommittee>, + BitList<::MaxValidatorsPerSlot>, >; /// A trait use to associate capacity constants with the type being stored in `ObservedAggregates`. @@ -103,29 +103,39 @@ pub trait SubsetItem { } impl<'a, E: EthSpec> SubsetItem for AttestationRef<'a, E> { - type Item = BitList; + type Item = BitList; fn is_subset(&self, other: &Self::Item) -> bool { match self { - Self::Base(att) => att.aggregation_bits.is_subset(other), - // TODO(electra) implement electra variant - Self::Electra(_) => todo!(), + 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), } } fn is_superset(&self, other: &Self::Item) -> bool { match self { - Self::Base(att) => other.is_subset(&att.aggregation_bits), - // TODO(electra) implement electra variant - Self::Electra(_) => todo!(), + 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), } } /// Returns the sync contribution aggregation bits. fn get_item(&self) -> Self::Item { match self { - Self::Base(att) => att.aggregation_bits.clone(), - // TODO(electra) implement electra variant - Self::Electra(_) => todo!(), + Self::Base(att) => { + // TODO(electra) fix unwrap + att.extend_aggregation_bits().unwrap() + } + Self::Electra(att) => att.aggregation_bits.clone(), } } diff --git a/beacon_node/beacon_chain/src/test_utils.rs b/beacon_node/beacon_chain/src/test_utils.rs index ff439742d11..6c096d8bbdc 100644 --- a/beacon_node/beacon_chain/src/test_utils.rs +++ b/beacon_node/beacon_chain/src/test_utils.rs @@ -1032,21 +1032,40 @@ where *state.get_block_root(target_slot)? }; - // 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, + if self.spec.fork_name_at_slot::(slot) >= ForkName::Electra { + let mut committee_bits = BitVector::default(); + committee_bits.set(index as usize, true)?; + Ok(Attestation::Electra(AttestationElectra { + aggregation_bits: BitList::with_capacity(committee_len)?, + committee_bits, + data: AttestationData { + slot, + index: 0u64, + beacon_block_root, + source: state.current_justified_checkpoint(), + target: Checkpoint { + epoch, + root: target_root, + }, }, - }, - signature: AggregateSignature::empty(), - })) + signature: AggregateSignature::empty(), + })) + } else { + 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, + }, + }, + signature: AggregateSignature::empty(), + })) + } } /// A list of attestations for each committee for the given slot. @@ -1121,11 +1140,20 @@ where ) .unwrap(); - attestation - .aggregation_bits_base_mut() - .unwrap() - .set(i, true) - .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.signature_mut() = { let domain = self.spec.get_domain( diff --git a/beacon_node/operation_pool/src/attestation_storage.rs b/beacon_node/operation_pool/src/attestation_storage.rs index 0dde36f0b52..e6a3dc2162d 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -28,7 +28,7 @@ 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))] @@ -164,8 +164,8 @@ impl CompactIndexedAttestation { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.signers_disjoint_from(other) } - (CompactIndexedAttestation::Electra(_), CompactIndexedAttestation::Electra(_)) => { - todo!() + (CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other)) => { + this.signers_disjoint_from(other) } // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => false, @@ -177,8 +177,8 @@ impl CompactIndexedAttestation { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { this.aggregate(other) } - (CompactIndexedAttestation::Electra(_), CompactIndexedAttestation::Electra(_)) => { - todo!() + (CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other)) => { + this.aggregate(other) } // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => (), @@ -205,13 +205,34 @@ impl CompactIndexedAttestationBase { } } +impl CompactIndexedAttestationElectra { + // TODO(electra) update to match spec requirements + pub fn signers_disjoint_from(&self, other: &Self) -> bool { + self.aggregation_bits + .intersection(&other.aggregation_bits) + .is_zero() + } + + // TODO(electra) update to match spec requirements + pub fn aggregate(&mut self, other: &Self) { + self.attesting_indices = self + .attesting_indices + .drain(..) + .merge(other.attesting_indices.iter().copied()) + .dedup() + .collect(); + self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); + self.signature.add_assign_aggregate(&other.signature); + } +} + impl AttestationMap { pub fn insert(&mut self, attestation: Attestation, attesting_indices: Vec) { let SplitAttestation { checkpoint, data, indexed, - } = SplitAttestation::new(attestation, attesting_indices); + } = SplitAttestation::new(attestation.clone(), attesting_indices); let attestation_map = self.checkpoint_map.entry(checkpoint).or_default(); let attestations = attestation_map.attestations.entry(data).or_default(); @@ -220,14 +241,24 @@ impl AttestationMap { // NOTE: this is sub-optimal and in future we will remove this in favour of max-clique // aggregation. let mut aggregated = false; - for existing_attestation in attestations.iter_mut() { - if existing_attestation.signers_disjoint_from(&indexed) { - existing_attestation.aggregate(&indexed); - aggregated = true; - } else if *existing_attestation == indexed { - aggregated = true; - } - } + + match attestation { + Attestation::Base(_) => { + for existing_attestation in attestations.iter_mut() { + if existing_attestation.signers_disjoint_from(&indexed) { + existing_attestation.aggregate(&indexed); + aggregated = true; + } else if *existing_attestation == indexed { + aggregated = true; + } + } + }, + // TODO(electra) in order to be devnet ready, we can skip + // aggregating here for now. this will result in "poorly" + // constructed blocks, but that should be fine for devnet + Attestation::Electra(_) => (), + }; + if !aggregated { attestations.push(indexed); diff --git a/beacon_node/store/src/consensus_context.rs b/beacon_node/store/src/consensus_context.rs index 727423d172f..a1d2e674526 100644 --- a/beacon_node/store/src/consensus_context.rs +++ b/beacon_node/store/src/consensus_context.rs @@ -24,7 +24,7 @@ pub struct OnDiskConsensusContext { indexed_attestations: HashMap< ( AttestationData, - BitList, + BitList, ), IndexedAttestation, >, diff --git a/consensus/fork_choice/tests/tests.rs b/consensus/fork_choice/tests/tests.rs index 63bf3b51bd0..3b05cfab1fd 100644 --- a/consensus/fork_choice/tests/tests.rs +++ b/consensus/fork_choice/tests/tests.rs @@ -830,8 +830,13 @@ async fn invalid_attestation_empty_bitfield() { .await .apply_attestation_to_chain( MutationDelay::NoDelay, - |attestation, _| { - *attestation.attesting_indices_base_mut().unwrap() = vec![].into(); + |attestation, _| match attestation { + IndexedAttestation::Base(ref mut att) => { + att.attesting_indices = vec![].into(); + } + IndexedAttestation::Electra(ref mut att) => { + att.attesting_indices = 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 998559caa67..595cc69f87c 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -99,7 +99,7 @@ pub mod attesting_indices_electra { /// Returns validator indices which participated in the attestation, sorted by increasing index. pub fn get_attesting_indices( committees: &[BeaconCommittee], - aggregation_bits: &BitList, + aggregation_bits: &BitList, committee_bits: &BitVector, ) -> Result, BeaconStateError> { let mut output: HashSet = HashSet::new(); diff --git a/consensus/state_processing/src/consensus_context.rs b/consensus/state_processing/src/consensus_context.rs index c3e92ddb341..3f18acb7921 100644 --- a/consensus/state_processing/src/consensus_context.rs +++ b/consensus/state_processing/src/consensus_context.rs @@ -25,7 +25,7 @@ pub struct ConsensusContext { pub indexed_attestations: HashMap< ( AttestationData, - BitList, + BitList, ), IndexedAttestation, >, @@ -208,7 +208,7 @@ impl ConsensusContext { attestations: HashMap< ( AttestationData, - BitList, + BitList, ), IndexedAttestation, >, diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index d479ca9fc1c..feee4f66879 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -65,7 +65,7 @@ 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, pub signature: AggregateSignature, #[superstruct(only(Electra))] @@ -391,8 +391,8 @@ impl AttestationBase { pub fn extend_aggregation_bits( &self, - ) -> Result, ssz_types::Error> { - let mut extended_aggregation_bits: BitList = + ) -> Result, ssz_types::Error> { + let mut extended_aggregation_bits: BitList = BitList::with_capacity(self.aggregation_bits.len())?; for (i, bit) in self.aggregation_bits.iter().enumerate() { diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index ac0525a3d84..a711a528067 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -610,7 +610,7 @@ impl> BeaconBlockElectra let indexed_attestation: IndexedAttestationElectra = IndexedAttestationElectra { attesting_indices: VariableList::new(vec![ 0_u64; - E::MaxValidatorsPerCommitteePerSlot::to_usize( + E::MaxValidatorsPerSlot::to_usize( ) ]) .unwrap(), @@ -626,10 +626,9 @@ impl> BeaconBlockElectra E::max_attester_slashings_electra() ] .into(); - // TODO(electra): check this let attestation = AttestationElectra { aggregation_bits: BitList::with_capacity( - E::MaxValidatorsPerCommitteePerSlot::to_usize(), + E::MaxValidatorsPerSlot::to_usize(), ) .unwrap(), data: AttestationData::default(), diff --git a/consensus/types/src/eth_spec.rs b/consensus/types/src/eth_spec.rs index e39b0dc9cf6..cec4db2da51 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 MaxValidatorsPerCommitteePerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; + type MaxValidatorsPerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; type MaxCommitteesPerSlot: Unsigned + Clone + Sync + Send + Debug + PartialEq + Eq; /* * Time parameters @@ -352,7 +352,7 @@ impl EthSpec for MainnetEthSpec { type SubnetBitfieldLength = U64; type MaxValidatorsPerCommittee = U2048; type MaxCommitteesPerSlot = U64; - type MaxValidatorsPerCommitteePerSlot = U131072; + type MaxValidatorsPerSlot = U131072; type GenesisEpoch = U0; type SlotsPerEpoch = U32; type EpochsPerEth1VotingPeriod = U64; @@ -433,7 +433,7 @@ impl EthSpec for MinimalEthSpec { SyncCommitteeSubnetCount, MaxValidatorsPerCommittee, MaxCommitteesPerSlot, - MaxValidatorsPerCommitteePerSlot, + MaxValidatorsPerSlot, GenesisEpoch, HistoricalRootsLimit, ValidatorRegistryLimit, @@ -475,7 +475,7 @@ impl EthSpec for GnosisEthSpec { type SubnetBitfieldLength = U64; type MaxValidatorsPerCommittee = U2048; type MaxCommitteesPerSlot = U64; - type MaxValidatorsPerCommitteePerSlot = U131072; + type MaxValidatorsPerSlot = 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 d17d00a3fa8..4e7e061b45f 100644 --- a/consensus/types/src/indexed_attestation.rs +++ b/consensus/types/src/indexed_attestation.rs @@ -59,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, } diff --git a/slasher/src/attester_record.rs b/slasher/src/attester_record.rs index 8de25160725..faba6efce89 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, }