Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/remove some Electra attestation todos #5817

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 0 additions & 44 deletions beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5120,50 +5120,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
},
);

// 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 {
Expand Down
41 changes: 34 additions & 7 deletions consensus/types/src/attestation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -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::<Attestation<MainnetEthSpec>>(),
size_of::<AttestationBase<MainnetEthSpec>>(),
attestation_expected
);
}

// TODO(electra): can we do this with both variants or should we?
ssz_and_tree_hash_tests!(AttestationBase<MainnetEthSpec>);
#[test]
fn size_of_electra() {
use std::mem::size_of;

let aggregation_bits =
size_of::<BitList<<MainnetEthSpec as EthSpec>::MaxValidatorsPerSlot>>();
let attestation_data = size_of::<AttestationData>();
let committee_bits =
size_of::<BitList<<MainnetEthSpec as EthSpec>::MaxCommitteesPerSlot>>();
let signature = size_of::<AggregateSignature>();

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::<AttestationElectra<MainnetEthSpec>>(),
attestation_expected
);
}

mod base {
use super::*;
ssz_and_tree_hash_tests!(AttestationBase<MainnetEthSpec>);
}
mod electra {
use super::*;
ssz_and_tree_hash_tests!(AttestationElectra<MainnetEthSpec>);
}
}
11 changes: 8 additions & 3 deletions consensus/types/src/attester_slashing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,12 @@ impl<E: EthSpec> AttesterSlashing<E> {
mod tests {
use super::*;
use crate::*;

// TODO(electra): should this be done for both variants?
ssz_and_tree_hash_tests!(AttesterSlashingBase<MainnetEthSpec>);
mod base {
use super::*;
ssz_and_tree_hash_tests!(AttesterSlashingBase<MainnetEthSpec>);
}
mod electra {
use super::*;
ssz_and_tree_hash_tests!(AttesterSlashingElectra<MainnetEthSpec>);
}
}
3 changes: 0 additions & 3 deletions consensus/types/src/beacon_block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -606,14 +606,12 @@ impl<E: EthSpec, Payload: AbstractExecPayload<E>> BeaconBlockElectra<E, Payload>
/// 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<E> = 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(),
Expand All @@ -626,7 +624,6 @@ impl<E: EthSpec, Payload: AbstractExecPayload<E>> BeaconBlockElectra<E, Payload>
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![];
Expand Down
Loading