From a41add95f4ed8c4900aa8649147d28798683cf7e Mon Sep 17 00:00:00 2001
From: Tumas <tumas@grandine.io>
Date: Thu, 5 Dec 2024 17:13:20 +0200
Subject: [PATCH] Fix aggregate construction in electra for block production:

- Iterate over packing results and collect attestations with disjoint
committee bits before computing on chain aggregates
---
 block_producer/src/block_producer.rs          | 61 +++++++++++++++----
 .../src/attestation_agg_pool/tasks.rs         |  2 +-
 2 files changed, 50 insertions(+), 13 deletions(-)

diff --git a/block_producer/src/block_producer.rs b/block_producer/src/block_producer.rs
index 381ac012..90329b2d 100644
--- a/block_producer/src/block_producer.rs
+++ b/block_producer/src/block_producer.rs
@@ -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},
@@ -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);
 
diff --git a/operation_pools/src/attestation_agg_pool/tasks.rs b/operation_pools/src/attestation_agg_pool/tasks.rs
index ff805d7c..365bf025 100644
--- a/operation_pools/src/attestation_agg_pool/tasks.rs
+++ b/operation_pools/src/attestation_agg_pool/tasks.rs
@@ -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(),