From e32dfcdcad69ab129a42238daae6508faa96ec4e Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 9 May 2024 09:34:56 -0400 Subject: [PATCH 1/2] fix get attesting indices (#5742) * fix get attesting indices * better errors * fix compile * only get committee index once --- .../beacon_chain/src/attestation_verification.rs | 5 +++-- .../src/common/get_attesting_indices.rs | 15 +++++++++++---- consensus/types/src/beacon_state.rs | 3 ++- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/beacon_node/beacon_chain/src/attestation_verification.rs b/beacon_node/beacon_chain/src/attestation_verification.rs index 62e65d5f87..3d722a534b 100644 --- a/beacon_node/beacon_chain/src/attestation_verification.rs +++ b/beacon_node/beacon_chain/src/attestation_verification.rs @@ -1309,10 +1309,11 @@ pub fn obtain_indexed_attestation_and_committees_per_slot( attesting_indices_electra::get_indexed_attestation(&committees, att) .map(|attestation| (attestation, committees_per_slot)) .map_err(|e| { - if e == BlockOperationError::BeaconStateError(NoCommitteeFound) { + let index = att.committee_index(); + if e == BlockOperationError::BeaconStateError(NoCommitteeFound(index)) { Error::NoCommitteeForSlotAndIndex { slot: att.data.slot, - index: att.committee_index(), + index, } } else { Error::Invalid(e) diff --git a/consensus/state_processing/src/common/get_attesting_indices.rs b/consensus/state_processing/src/common/get_attesting_indices.rs index 595cc69f87..9848840e96 100644 --- a/consensus/state_processing/src/common/get_attesting_indices.rs +++ b/consensus/state_processing/src/common/get_attesting_indices.rs @@ -113,11 +113,15 @@ pub mod attesting_indices_electra { .map(|committee| (committee.index, committee)) .collect(); + let committee_count_per_slot = committees.len() as u64; + let mut participant_count = 0; for index in committee_indices { if let Some(&beacon_committee) = committees_map.get(&index) { - if aggregation_bits.len() != beacon_committee.committee.len() { - return Err(BeaconStateError::InvalidBitfield); + // This check is new to the spec's `process_attestation` in Electra. + if index >= committee_count_per_slot { + return Err(BeaconStateError::InvalidCommitteeIndex(index)); } + participant_count.safe_add_assign(beacon_committee.committee.len() as u64)?; let committee_attesters = beacon_committee .committee .iter() @@ -136,10 +140,13 @@ pub mod attesting_indices_electra { committee_offset.safe_add(beacon_committee.committee.len())?; } else { - return Err(Error::NoCommitteeFound); + return Err(Error::NoCommitteeFound(index)); } + } - // TODO(electra) what should we do when theres no committee found for a given index? + // This check is new to the spec's `process_attestation` in Electra. + if participant_count as usize != aggregation_bits.len() { + return Err(BeaconStateError::InvalidBitfield); } let mut indices = output.into_iter().collect_vec(); diff --git a/consensus/types/src/beacon_state.rs b/consensus/types/src/beacon_state.rs index 599c0bfc39..d9c7a78537 100644 --- a/consensus/types/src/beacon_state.rs +++ b/consensus/types/src/beacon_state.rs @@ -159,7 +159,8 @@ pub enum Error { IndexNotSupported(usize), InvalidFlagIndex(usize), MerkleTreeError(merkle_proof::MerkleTreeError), - NoCommitteeFound, + NoCommitteeFound(CommitteeIndex), + InvalidCommitteeIndex(CommitteeIndex), } /// Control whether an epoch-indexed field can be indexed at the next epoch or not. From 07229b76ed89dadd6ce7464699ed5f9a265e8851 Mon Sep 17 00:00:00 2001 From: realbigsean Date: Thu, 9 May 2024 13:40:52 -0400 Subject: [PATCH 2/2] Ef test fixes (#5753) * attestation related ef test fixes * delete commented out stuff --- testing/ef_tests/src/type_name.rs | 7 ++--- testing/ef_tests/tests/tests.rs | 49 +++++++++++++++++++------------ 2 files changed, 33 insertions(+), 23 deletions(-) diff --git a/testing/ef_tests/src/type_name.rs b/testing/ef_tests/src/type_name.rs index 30db5c0e4a..cbea78dabf 100644 --- a/testing/ef_tests/src/type_name.rs +++ b/testing/ef_tests/src/type_name.rs @@ -111,11 +111,8 @@ type_name_generic!(LightClientUpdateDeneb, "LightClientUpdate"); type_name_generic!(PendingAttestation); type_name!(ProposerSlashing); type_name_generic!(SignedAggregateAndProof); -type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProofBase"); -type_name_generic!( - SignedAggregateAndProofElectra, - "SignedAggregateAndProofElectra" -); +type_name_generic!(SignedAggregateAndProofBase, "SignedAggregateAndProof"); +type_name_generic!(SignedAggregateAndProofElectra, "SignedAggregateAndProof"); type_name_generic!(SignedBeaconBlock); type_name!(SignedBeaconBlockHeader); type_name_generic!(SignedContributionAndProof); diff --git a/testing/ef_tests/tests/tests.rs b/testing/ef_tests/tests/tests.rs index 85d9362aae..fb8bdfcae7 100644 --- a/testing/ef_tests/tests/tests.rs +++ b/testing/ef_tests/tests/tests.rs @@ -219,7 +219,6 @@ mod ssz_static { use types::historical_summary::HistoricalSummary; use types::{AttesterSlashingBase, AttesterSlashingElectra, LightClientBootstrapAltair, *}; - ssz_static_test!(aggregate_and_proof, AggregateAndProof<_>); ssz_static_test!(attestation, Attestation<_>); ssz_static_test!(attestation_data, AttestationData); ssz_static_test!(beacon_block, SszStaticWithSpecHandler, BeaconBlock<_>); @@ -249,7 +248,7 @@ mod ssz_static { ssz_static_test!(voluntary_exit, VoluntaryExit); #[test] - fn signed_aggregate_and_proof() { + fn attester_slashing() { SszStaticHandler::, MinimalEthSpec>::pre_electra() .run(); SszStaticHandler::, MainnetEthSpec>::pre_electra() @@ -260,6 +259,36 @@ mod ssz_static { .run(); } + #[test] + fn signed_aggregate_and_proof() { + SszStaticHandler::, MinimalEthSpec>::pre_electra( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::pre_electra( + ) + .run(); + SszStaticHandler::, MinimalEthSpec>::electra_only( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::electra_only( + ) + .run(); + } + + #[test] + fn aggregate_and_proof() { + SszStaticHandler::, MinimalEthSpec>::pre_electra() + .run(); + SszStaticHandler::, MainnetEthSpec>::pre_electra() + .run(); + SszStaticHandler::, MinimalEthSpec>::electra_only( + ) + .run(); + SszStaticHandler::, MainnetEthSpec>::electra_only( + ) + .run(); + } + // BeaconBlockBody has no internal indicator of which fork it is for, so we test it separately. #[test] fn beacon_block_body() { @@ -283,22 +312,6 @@ mod ssz_static { .run(); } - #[test] - fn signed_aggregate_and_proof() { - SszStaticHandler::, MinimalEthSpec>::pre_electra( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::pre_electra( - ) - .run(); - SszStaticHandler::, MinimalEthSpec>::electra_only( - ) - .run(); - SszStaticHandler::, MainnetEthSpec>::electra_only( - ) - .run(); - } - // Altair and later #[test] fn contribution_and_proof() {