Skip to content

Commit

Permalink
Fix aggregate construction in electra for block production:
Browse files Browse the repository at this point in the history
- Iterate over packing results and collect attestations with disjoint
committee bits before computing on chain aggregates
  • Loading branch information
Tumas committed Dec 9, 2024
1 parent dc0d3e0 commit a41add9
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 13 deletions.
61 changes: 49 additions & 12 deletions block_producer/src/block_producer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,13 +75,13 @@ use types::{
phase0::{
consts::{FAR_FUTURE_EPOCH, GENESIS_SLOT},
containers::{
Attestation, AttesterSlashing as Phase0AttesterSlashing,
Attestation, AttestationData, AttesterSlashing as Phase0AttesterSlashing,
BeaconBlock as Phase0BeaconBlock, BeaconBlockBody as Phase0BeaconBlockBody, Deposit,
Eth1Data, ProposerSlashing, SignedVoluntaryExit,
},
primitives::{
DepositIndex, Epoch, ExecutionAddress, ExecutionBlockHash, Slot, Uint256,
ValidatorIndex, H256,
CommitteeIndex, DepositIndex, Epoch, ExecutionAddress, ExecutionBlockHash, Slot,
Uint256, ValidatorIndex, H256,
},
},
preset::{Preset, SyncSubcommitteeSize},
Expand Down Expand Up @@ -847,18 +847,55 @@ impl<P: Preset, W: Wait> BlockBuildContext<P, W> {
{
ContiguousList::default()
} else {
// TODO(feature/electra): don't ignore errors
let attestations = attestations
.into_iter()
.filter_map(|attestation| {
operation_pools::convert_to_electra_attestation(attestation).ok()
// Store results in a vec to preserve insertion order and thus the results of the packing algorithm
let mut results: Vec<(
AttestationData,
HashSet<CommitteeIndex>,
Vec<ElectraAttestation<P>>,
)> = Vec::new();

for (electra_attestation, committee_index) in
attestations.into_iter().filter_map(|attestation| {
let committee_index = attestation.data.index;

match operation_pools::convert_to_electra_attestation(attestation) {
Ok(electra_attestation) => {
Some((electra_attestation, committee_index))
}
Err(error) => {
warn!("unable to convert to electra attestation: {error:?}");
None
}
}
})
.chunk_by(|attestation| attestation.data);
{
if let Some((_, indices, attestations)) =
results.iter_mut().find(|(data, indices, _)| {
*data == electra_attestation.data
&& !indices.contains(&committee_index)
})
{
indices.insert(committee_index);
attestations.push(electra_attestation);
} else {
results.push((
electra_attestation.data,
HashSet::from([committee_index]),
vec![electra_attestation],
))
}
}

let attestations = attestations
let attestations = results
.into_iter()
.filter_map(|(_, attestations)| {
Self::compute_on_chain_aggregate(attestations).ok()
.filter_map(|(_, _, attestations)| {
match Self::compute_on_chain_aggregate(attestations.into_iter()) {
Ok(electra_aggregate) => Some(electra_aggregate),
Err(error) => {
warn!("unable to compute on chain aggregate: {error:?}");
None
}
}
})
.take(P::MaxAttestationsElectra::USIZE);

Expand Down
2 changes: 1 addition & 1 deletion operation_pools/src/attestation_agg_pool/tasks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ impl<P: Preset, W: Wait> PoolTask for BestProposableAttestationsTask<P, W> {
return Ok(attestations);
}

DebugAttestationPacker.warn("no optimal attestations for slot: {slot}");
DebugAttestationPacker.warn(format_args!("no optimal attestations for slot: {slot}"));

let attestation_packer = AttestationPacker::new(
controller.chain_config().clone_arc(),
Expand Down

0 comments on commit a41add9

Please sign in to comment.