diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 32c33f3e617..c7f7bb58dab 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -61,10 +61,9 @@ use std::borrow::Cow; use strum::AsRefStr; use tree_hash::TreeHash; use types::{ - Attestation, AttestationRef, BeaconCommittee, - BeaconStateError::{self, NoCommitteeFound}, - ChainSpec, CommitteeIndex, Epoch, EthSpec, ForkName, Hash256, IndexedAttestation, - SelectionProof, SignedAggregateAndProof, Slot, SubnetId, + Attestation, AttestationRef, BeaconCommittee, BeaconStateError::NoCommitteeFound, ChainSpec, + CommitteeIndex, Epoch, EthSpec, Hash256, IndexedAttestation, SelectionProof, + SignedAggregateAndProof, Slot, SubnetId, }; pub use batch::{batch_verify_aggregated_attestations, batch_verify_unaggregated_attestations}; @@ -266,30 +265,9 @@ pub enum Error { BeaconChainError(BeaconChainError), } -// TODO(electra) the error conversion changes here are to get a test case to pass -// this could easily be cleaned up impl From for Error { fn from(e: BeaconChainError) -> Self { - match &e { - BeaconChainError::BeaconStateError(beacon_state_error) => { - if let BeaconStateError::AggregatorNotInCommittee { aggregator_index } = - beacon_state_error - { - Self::AggregatorNotInCommittee { - aggregator_index: *aggregator_index, - } - } else if let BeaconStateError::InvalidSelectionProof { aggregator_index } = - beacon_state_error - { - Self::InvalidSelectionProof { - aggregator_index: *aggregator_index, - } - } else { - Error::BeaconChainError(e) - } - } - _ => Error::BeaconChainError(e), - } + Self::BeaconChainError(e) } } @@ -1169,7 +1147,7 @@ pub fn verify_propagation_slot_range( let current_fork = spec.fork_name_at_slot::(slot_clock.now().ok_or(BeaconChainError::UnableToReadSlot)?); - let earliest_permissible_slot = if current_fork < ForkName::Deneb { + let earliest_permissible_slot = if !current_fork.deneb_enabled() { one_epoch_prior // EIP-7045 } else { diff --git a/beacon_node/operation_pool/src/attestation.rs b/beacon_node/operation_pool/src/attestation.rs index c6ed6eb7f6e..97d0583e345 100644 --- a/beacon_node/operation_pool/src/attestation.rs +++ b/beacon_node/operation_pool/src/attestation.rs @@ -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(), - // 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!(), + // This code path is obsolete post altair fork, so we just return an empty bitlist here. + CompactIndexedAttestation::Electra(_) => return BitList::with_capacity(0).unwrap(), }; 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 ad4ffe6d3c1..4de9d351f3c 100644 --- a/beacon_node/operation_pool/src/attestation_storage.rs +++ b/beacon_node/operation_pool/src/attestation_storage.rs @@ -165,22 +165,22 @@ impl CompactIndexedAttestation { CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other), ) => this.should_aggregate(other), - // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? _ => false, } } - pub fn aggregate(&mut self, other: &Self) -> Option<()> { + /// Returns `true` if aggregated, otherwise `false`. + pub fn aggregate(&mut self, other: &Self) -> bool { match (self, other) { (CompactIndexedAttestation::Base(this), CompactIndexedAttestation::Base(other)) => { - this.aggregate(other) + this.aggregate(other); + true } ( CompactIndexedAttestation::Electra(this), CompactIndexedAttestation::Electra(other), ) => this.aggregate_same_committee(other), - // TODO(electra) is a mix of electra and base compact indexed attestations an edge case we need to deal with? - _ => None, + _ => false, } } } @@ -192,7 +192,7 @@ impl CompactIndexedAttestationBase { .is_zero() } - pub fn aggregate(&mut self, other: &Self) -> Option<()> { + pub fn aggregate(&mut self, other: &Self) { self.attesting_indices = self .attesting_indices .drain(..) @@ -201,8 +201,6 @@ impl CompactIndexedAttestationBase { .collect(); self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.signature.add_assign_aggregate(&other.signature); - - Some(()) } } @@ -216,9 +214,10 @@ impl CompactIndexedAttestationElectra { .is_zero() } - pub fn aggregate_same_committee(&mut self, other: &Self) -> Option<()> { + /// Returns `true` if aggregated, otherwise `false`. + pub fn aggregate_same_committee(&mut self, other: &Self) -> bool { if self.committee_bits != other.committee_bits { - return None; + return false; } self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.attesting_indices = self @@ -228,7 +227,7 @@ impl CompactIndexedAttestationElectra { .dedup() .collect(); self.signature.add_assign_aggregate(&other.signature); - Some(()) + true } pub fn aggregate_with_disjoint_committees(&mut self, other: &Self) -> Option<()> { @@ -318,8 +317,7 @@ impl AttestationMap { for existing_attestation in attestations.iter_mut() { if existing_attestation.should_aggregate(&indexed) { - existing_attestation.aggregate(&indexed); - aggregated = true; + aggregated = existing_attestation.aggregate(&indexed); } else if *existing_attestation == indexed { aggregated = true; } diff --git a/beacon_node/operation_pool/src/lib.rs b/beacon_node/operation_pool/src/lib.rs index 49fbed56869..a1c9ada03a0 100644 --- a/beacon_node/operation_pool/src/lib.rs +++ b/beacon_node/operation_pool/src/lib.rs @@ -39,7 +39,7 @@ use std::ptr; use types::{ sync_aggregate::Error as SyncAggregateError, typenum::Unsigned, AbstractExecPayload, Attestation, AttestationData, AttesterSlashing, BeaconState, BeaconStateError, ChainSpec, - Epoch, EthSpec, ForkName, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange, + Epoch, EthSpec, ProposerSlashing, SignedBeaconBlock, SignedBlsToExecutionChange, SignedVoluntaryExit, Slot, SyncAggregate, SyncCommitteeContribution, Validator, }; @@ -316,10 +316,10 @@ impl OperationPool { ) .inspect(|_| num_curr_valid += 1); - let curr_epoch_limit = if fork_name < ForkName::Electra { - E::MaxAttestations::to_usize() - } else { + let curr_epoch_limit = if fork_name.electra_enabled() { E::MaxAttestationsElectra::to_usize() + } else { + E::MaxAttestations::to_usize() }; let prev_epoch_limit = if let BeaconState::Base(base_state) = state { std::cmp::min( diff --git a/consensus/state_processing/src/verify_operation.rs b/consensus/state_processing/src/verify_operation.rs index f218b806d20..c4b7c6a026f 100644 --- a/consensus/state_processing/src/verify_operation.rs +++ b/consensus/state_processing/src/verify_operation.rs @@ -48,7 +48,6 @@ pub trait TransformPersist { pub struct SigVerifiedOp { op: T, verified_against: VerifiedAgainst, - //#[ssz(skip_serializing, skip_deserializing)] _phantom: PhantomData, } diff --git a/consensus/types/src/aggregate_and_proof.rs b/consensus/types/src/aggregate_and_proof.rs index 6ae0df4ad7c..223b12e7684 100644 --- a/consensus/types/src/aggregate_and_proof.rs +++ b/consensus/types/src/aggregate_and_proof.rs @@ -33,7 +33,8 @@ use tree_hash_derive::TreeHash; derive(Debug, PartialEq, TreeHash, Serialize,), serde(untagged, bound = "E: EthSpec"), tree_hash(enum_behaviour = "transparent") - ) + ), + map_ref_into(AttestationRef) )] #[derive( arbitrary::Arbitrary, Debug, Clone, PartialEq, Serialize, Deserialize, Encode, TreeHash, @@ -59,19 +60,17 @@ pub struct AggregateAndProof { impl<'a, E: EthSpec> AggregateAndProofRef<'a, E> { /// Returns `true` if `validator_pubkey` signed over `self.aggregate.data.slot`. pub fn aggregate(self) -> AttestationRef<'a, E> { - match self { - AggregateAndProofRef::Base(a) => AttestationRef::Base(&a.aggregate), - AggregateAndProofRef::Electra(a) => AttestationRef::Electra(&a.aggregate), - } + map_aggregate_and_proof_ref_into_attestation_ref!(&'a _, self, |inner, cons| { + cons(&inner.aggregate) + }) } } impl AggregateAndProof { /// Returns `true` if `validator_pubkey` signed over `self.aggregate.data.slot`. - pub fn aggregate(&self) -> AttestationRef { - match self { - AggregateAndProof::Base(a) => AttestationRef::Base(&a.aggregate), - AggregateAndProof::Electra(a) => AttestationRef::Electra(&a.aggregate), - } + pub fn aggregate<'a>(&'a self) -> AttestationRef<'a, E> { + map_aggregate_and_proof_ref_into_attestation_ref!(&'a _, self.to_ref(), |inner, cons| { + cons(&inner.aggregate) + }) } } diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index c8d1c3fb9b7..88993267a94 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -1,6 +1,6 @@ use crate::slot_data::SlotData; +use crate::Checkpoint; use crate::{test_utils::TestRandom, Hash256, Slot}; -use crate::{Checkpoint, ForkName}; use derivative::Derivative; use safe_arith::ArithError; use serde::{Deserialize, Serialize}; @@ -99,7 +99,7 @@ impl Attestation { target: Checkpoint, spec: &ChainSpec, ) -> Result { - if spec.fork_name_at_slot::(slot) >= ForkName::Electra { + if spec.fork_name_at_slot::(slot).electra_enabled() { let mut committee_bits: BitVector = BitVector::default(); committee_bits .set(committee_index as usize, true) @@ -277,16 +277,6 @@ impl<'a, E: EthSpec> AttestationRef<'a, E> { } impl AttestationElectra { - /// Are the aggregation bitfields of these attestations disjoint? - // TODO(electra): check whether the definition from CompactIndexedAttestation::should_aggregate - // is useful where this is used, i.e. only consider attestations disjoint when their committees - // match AND their aggregation bits do not intersect. - pub fn signers_disjoint_from(&self, other: &Self) -> bool { - self.aggregation_bits - .intersection(&other.aggregation_bits) - .is_zero() - } - pub fn committee_index(&self) -> Option { self.get_committee_indices().first().cloned() } @@ -304,7 +294,6 @@ impl AttestationElectra { /// 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); } @@ -358,19 +347,11 @@ impl AttestationElectra { } 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. /// /// 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); } diff --git a/consensus/types/src/fork_name.rs b/consensus/types/src/fork_name.rs index f0810e2bdbe..96f206d4543 100644 --- a/consensus/types/src/fork_name.rs +++ b/consensus/types/src/fork_name.rs @@ -120,6 +120,10 @@ impl ForkName { } } + pub fn deneb_enabled(self) -> bool { + self >= ForkName::Deneb + } + pub fn electra_enabled(self) -> bool { self >= ForkName::Electra } diff --git a/consensus/types/src/signed_aggregate_and_proof.rs b/consensus/types/src/signed_aggregate_and_proof.rs index d339cecaae0..26eca19bf15 100644 --- a/consensus/types/src/signed_aggregate_and_proof.rs +++ b/consensus/types/src/signed_aggregate_and_proof.rs @@ -34,7 +34,9 @@ use tree_hash_derive::TreeHash; ), serde(bound = "E: EthSpec"), arbitrary(bound = "E: EthSpec"), - ) + ), + map_into(Attestation), + map_ref_into(AggregateAndProofRef) )] #[derive( arbitrary::Arbitrary, Debug, Clone, PartialEq, Serialize, Deserialize, Encode, TreeHash, @@ -102,19 +104,17 @@ impl SignedAggregateAndProof { } } - pub fn message(&self) -> AggregateAndProofRef { - match self { - SignedAggregateAndProof::Base(message) => AggregateAndProofRef::Base(&message.message), - SignedAggregateAndProof::Electra(message) => { - AggregateAndProofRef::Electra(&message.message) - } - } + pub fn message<'a>(&'a self) -> AggregateAndProofRef<'a, E> { + map_signed_aggregate_and_proof_ref_into_aggregate_and_proof_ref!( + &'a _, + self.to_ref(), + |inner, cons| { cons(&inner.message) } + ) } pub fn into_attestation(self) -> Attestation { - match self { - Self::Base(att) => Attestation::Base(att.message.aggregate), - Self::Electra(att) => Attestation::Electra(att.message.aggregate), - } + map_signed_aggregate_and_proof_into_attestation!(self, |inner, cons| { + cons(inner.message.aggregate) + }) } } diff --git a/consensus/types/src/sync_committee_contribution.rs b/consensus/types/src/sync_committee_contribution.rs index 81de83ba3be..c348c3e8be3 100644 --- a/consensus/types/src/sync_committee_contribution.rs +++ b/consensus/types/src/sync_committee_contribution.rs @@ -63,13 +63,6 @@ impl SyncCommitteeContribution { }) } - /// Are the aggregation bitfields of these sync contribution disjoint? - pub fn signers_disjoint_from(&self, other: &Self) -> bool { - self.aggregation_bits - .intersection(&other.aggregation_bits) - .is_zero() - } - /// Aggregate another `SyncCommitteeContribution` into this one. /// /// The aggregation bitfields must be disjoint, and the data must be the same. @@ -77,7 +70,6 @@ impl SyncCommitteeContribution { debug_assert_eq!(self.slot, other.slot); debug_assert_eq!(self.beacon_block_root, other.beacon_block_root); debug_assert_eq!(self.subcommittee_index, other.subcommittee_index); - debug_assert!(self.signers_disjoint_from(other)); self.aggregation_bits = self.aggregation_bits.union(&other.aggregation_bits); self.signature.add_assign_aggregate(&other.signature); diff --git a/testing/ef_tests/check_all_files_accessed.py b/testing/ef_tests/check_all_files_accessed.py index 7629d61827f..5f425ab98b4 100755 --- a/testing/ef_tests/check_all_files_accessed.py +++ b/testing/ef_tests/check_all_files_accessed.py @@ -42,7 +42,9 @@ "bls12-381-tests/deserialization_G2", "bls12-381-tests/hash_to_G2", "tests/.*/eip6110", - "tests/.*/whisk" + "tests/.*/whisk", + # TODO(electra): re-enable in https://github.com/sigp/lighthouse/pull/5758 + "tests/.*/.*/ssz_static/IndexedAttestation" ] diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index cbea78dabfc..f886431b400 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -41,6 +41,8 @@ type_name_generic!(AggregateAndProof); type_name_generic!(AggregateAndProofBase, "AggregateAndProof"); type_name_generic!(AggregateAndProofElectra, "AggregateAndProof"); type_name_generic!(Attestation); +type_name_generic!(AttestationBase, "Attestation"); +type_name_generic!(AttestationElectra, "Attestation"); type_name!(AttestationData); type_name_generic!(AttesterSlashing); type_name_generic!(AttesterSlashingBase, "AttesterSlashing"); @@ -76,6 +78,8 @@ type_name!(Fork); type_name!(ForkData); type_name_generic!(HistoricalBatch); type_name_generic!(IndexedAttestation); +type_name_generic!(IndexedAttestationBase, "IndexedAttestation"); +type_name_generic!(IndexedAttestationElectra, "IndexedAttestation"); type_name_generic!(LightClientBootstrap); type_name_generic!(LightClientBootstrapAltair, "LightClientBootstrap"); type_name_generic!(LightClientBootstrapCapella, "LightClientBootstrap"); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 9e8b375b2c1..60685140100 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -232,7 +232,6 @@ mod ssz_static { ssz_static_test!(fork, Fork); ssz_static_test!(fork_data, ForkData); ssz_static_test!(historical_batch, HistoricalBatch<_>); - ssz_static_test!(indexed_attestation, IndexedAttestation<_>); ssz_static_test!(pending_attestation, PendingAttestation<_>); ssz_static_test!(proposer_slashing, ProposerSlashing); ssz_static_test!(