From 322694094e1a8cd3b4310c5c63c919759400ae99 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Fri, 17 May 2024 09:51:45 -0400 Subject: [PATCH] fix some todos --- beacon_node/beacon_chain/src/beacon_chain.rs | 44 -------------------- consensus/types/src/attestation.rs | 41 ++++++++++++++---- consensus/types/src/attester_slashing.rs | 11 +++-- consensus/types/src/beacon_block.rs | 3 -- 4 files changed, 42 insertions(+), 57 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 9cbb46afe93..ae649f85907 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -5120,50 +5120,6 @@ impl BeaconChain { }, ); - // TODO(electra): figure out what should *actually* be done here when we have attestations / attester_slashings of the wrong type - match &state { - BeaconState::Base(_) - | BeaconState::Altair(_) - | BeaconState::Bellatrix(_) - | BeaconState::Capella(_) - | BeaconState::Deneb(_) => { - if !attestations_electra.is_empty() { - error!( - self.log, - "Tried to produce block with attestations of the wrong type"; - "slot" => slot, - "attestations" => attestations_electra.len(), - ); - } - if !attester_slashings_electra.is_empty() { - error!( - self.log, - "Tried to produce block with attester slashings of the wrong type"; - "slot" => slot, - "attester_slashings" => attester_slashings_electra.len(), - ); - } - } - BeaconState::Electra(_) => { - if !attestations_base.is_empty() { - error!( - self.log, - "Tried to produce block with attestations of the wrong type"; - "slot" => slot, - "attestations" => attestations_base.len(), - ); - } - if !attester_slashings_base.is_empty() { - error!( - self.log, - "Tried to produce block with attester slashings of the wrong type"; - "slot" => slot, - "attester_slashings" => attester_slashings_base.len(), - ); - } - } - }; - let (inner_block, maybe_blobs_and_proofs, execution_payload_value) = match &state { BeaconState::Base(_) => ( BeaconBlock::Base(BeaconBlockBase { diff --git a/consensus/types/src/attestation.rs b/consensus/types/src/attestation.rs index 8c8a81b90f2..61e1f4332c9 100644 --- a/consensus/types/src/attestation.rs +++ b/consensus/types/src/attestation.rs @@ -428,7 +428,7 @@ mod tests { // This test will only pass with `blst`, if we run these tests with another // BLS library in future we will have to make it generic. #[test] - fn size_of() { + fn size_of_base() { use std::mem::size_of; let aggregation_bits = @@ -441,16 +441,43 @@ mod tests { assert_eq!(signature, 288 + 16); let attestation_expected = aggregation_bits + attestation_data + signature; - // TODO(electra) since we've removed attestation aggregation for electra variant - // i've updated the attestation value expected from 488 544 - // assert_eq!(attestation_expected, 488); assert_eq!(attestation_expected, 488); assert_eq!( - size_of::>(), + size_of::>(), attestation_expected ); } - // TODO(electra): can we do this with both variants or should we? - ssz_and_tree_hash_tests!(AttestationBase); + #[test] + fn size_of_electra() { + use std::mem::size_of; + + let aggregation_bits = + size_of::::MaxValidatorsPerSlot>>(); + let attestation_data = size_of::(); + let committee_bits = + size_of::::MaxCommitteesPerSlot>>(); + let signature = size_of::(); + + assert_eq!(aggregation_bits, 56); + assert_eq!(committee_bits, 56); + assert_eq!(attestation_data, 128); + assert_eq!(signature, 288 + 16); + + let attestation_expected = aggregation_bits + committee_bits + attestation_data + signature; + assert_eq!(attestation_expected, 544); + assert_eq!( + size_of::>(), + attestation_expected + ); + } + + mod base { + use super::*; + ssz_and_tree_hash_tests!(AttestationBase); + } + mod electra { + use super::*; + ssz_and_tree_hash_tests!(AttestationElectra); + } } diff --git a/consensus/types/src/attester_slashing.rs b/consensus/types/src/attester_slashing.rs index 6668f809b82..a8d4e6989c8 100644 --- a/consensus/types/src/attester_slashing.rs +++ b/consensus/types/src/attester_slashing.rs @@ -164,7 +164,12 @@ impl AttesterSlashing { mod tests { use super::*; use crate::*; - - // TODO(electra): should this be done for both variants? - ssz_and_tree_hash_tests!(AttesterSlashingBase); + mod base { + use super::*; + ssz_and_tree_hash_tests!(AttesterSlashingBase); + } + mod electra { + use super::*; + ssz_and_tree_hash_tests!(AttesterSlashingElectra); + } } diff --git a/consensus/types/src/beacon_block.rs b/consensus/types/src/beacon_block.rs index 09f3306c73d..0224f8c855f 100644 --- a/consensus/types/src/beacon_block.rs +++ b/consensus/types/src/beacon_block.rs @@ -606,14 +606,12 @@ impl> BeaconBlockElectra /// Return a Electra block where the block has maximum size. pub fn full(spec: &ChainSpec) -> Self { let base_block: BeaconBlockBase<_, Payload> = BeaconBlockBase::full(spec); - // TODO(electra): check this let indexed_attestation: IndexedAttestationElectra = IndexedAttestationElectra { attesting_indices: VariableList::new(vec![0_u64; E::MaxValidatorsPerSlot::to_usize()]) .unwrap(), data: AttestationData::default(), signature: AggregateSignature::empty(), }; - // TODO(electra): fix this so we calculate this size correctly let attester_slashings = vec![ AttesterSlashingElectra { attestation_1: indexed_attestation.clone(), @@ -626,7 +624,6 @@ impl> BeaconBlockElectra aggregation_bits: BitList::with_capacity(E::MaxValidatorsPerSlot::to_usize()).unwrap(), data: AttestationData::default(), signature: AggregateSignature::empty(), - // TODO(electra): does this actually allocate the size correctly? committee_bits: BitVector::new(), }; let mut attestations_electra = vec![];