fix incorrect indexing of attestationdata in attestationpool (#7037)

Author: tersec

AllTests-mainnet.md

@@ -11,6 +11,7 @@ AllTests-mainnet
 + Attestations with disjoint comittee bits and equal data into single on-chain aggregate [Pr OK
 + Can add and retrieve simple electra attestations [Preset: mainnet]                         OK
 + Working with electra aggregates [Preset: mainnet]                                          OK
++ simple add and get with electra nonzero committee [Preset: mainnet]                        OK
 ```
 ## Attestation pool processing [Preset: mainnet]
 ```diff

beacon_chain/consensus_object_pools/attestation_pool.nim

@@ -8,7 +8,6 @@
 {.push raises: [].}
 
 import
-  std/algorithm,
   # Status libraries
   metrics,
   chronicles, stew/byteutils,
@@ -19,6 +18,7 @@ import
   ../fork_choice/fork_choice,
   ../beacon_clock
 
+from std/algorithm import sort
 from std/sequtils import keepItIf, maxIndex
 
 export blockchain_dag, fork_choice
@@ -418,6 +418,11 @@ proc addAttestation(
 
   true
 
+func getAttestationCandidateKey(
+    attestationDataRoot: Eth2Digest, committee_index: CommitteeIndex):
+    Eth2Digest =
+  hash_tree_root([attestationDataRoot, hash_tree_root(committee_index.uint64)])
+
 func getAttestationCandidateKey(
     data: AttestationData,
     committee_index: Opt[CommitteeIndex]): Eth2Digest =
@@ -429,13 +434,7 @@ func getAttestationCandidateKey(
     # i.e. no committees selected, so it can't be an actual Electra attestation
     hash_tree_root(data)
   else:
-    hash_tree_root([hash_tree_root(data),
-                    hash_tree_root(committee_index.get.uint64)])
-
-func getAttestationCandidateKey(
-    attestationDataRoot: Eth2Digest, committee_index: CommitteeIndex):
-    Eth2Digest =
-  hash_tree_root([attestationDataRoot, hash_tree_root(committee_index.uint64)])
+    getAttestationCandidateKey(hash_tree_root(data), committee_index.get)
 
 proc addAttestation*(
     pool: var AttestationPool,
@@ -476,14 +475,20 @@ proc addAttestation*(
   #      creating an unnecessary AttestationEntry on the hot path and avoiding
   #      multiple lookups
   template addAttToPool(attCandidates: untyped, entry: untyped, committee_index: untyped) =
-    let attestation_data_root = getAttestationCandidateKey(entry.data, committee_index)
-
-    attCandidates[candidateIdx.get()].withValue(attestation_data_root, entry) do:
+    # `AttestationData.index == 0` in Electra, but the attestation pool always
+    # represents an AttestationEntry regardless as having the actual committee
+    # index. The entry, therefore, is not the same as the AttestationData, and
+    # thus cannot function as the basis for deriving the hashtable key for the
+    # entry. Instead use the (correctly data.index == 0) attestation passed to
+    # addAttestation.
+    let candidate_key = getAttestationCandidateKey(attestation.data, committee_index)
+
+    attCandidates[candidateIdx.get()].withValue(candidate_key, entry) do:
       if not addAttestation(entry[], attestation, index_in_committee, signature):
         return
     do:
       if not addAttestation(
-          attCandidates[candidateIdx.get()].mgetOrPut(attestation_data_root, entry),
+          attCandidates[candidateIdx.get()].mgetOrPut(candidate_key, entry),
           attestation, index_in_committee, signature):
         # Returns from overall function, not only template
         return

tests/test_attestation_pool.nim

@@ -11,20 +11,22 @@
 import
   # Status lib
   unittest2,
-  chronicles, chronos,
-  stew/[byteutils, endians2],
-  taskpools,
+  chronicles,
   # Internal
-  ../beacon_chain/gossip_processing/[gossip_validation],
-  ../beacon_chain/fork_choice/[fork_choice_types, fork_choice],
   ../beacon_chain/consensus_object_pools/[
-    block_quarantine, blockchain_dag, block_clearance, attestation_pool],
-  ../beacon_chain/spec/[beaconstate, helpers, state_transition, validator],
+    blockchain_dag, block_clearance, attestation_pool],
+  ../beacon_chain/spec/[state_transition, validator],
   ../beacon_chain/beacon_clock,
   # Test utilities
   ./testutil, ./testdbutil, ./testblockutil, ./consensus_spec/fixtures_utils
 
 from std/sequtils import mapIt, toSeq
+from stew/byteutils import `<`
+from ../beacon_chain/consensus_object_pools/block_quarantine import
+  Quarantine, init
+from ../beacon_chain/spec/beaconstate import
+  attester_dependent_root, check_attestation, get_attesting_indices,
+  latest_block_root
 from ./testbcutil import addHeadBlock
 
 func combine(tgt: var (phase0.Attestation | electra.Attestation),
@@ -873,8 +875,7 @@ suite "Attestation pool electra processing" & preset():
          get().aggregation_bits.countOnes() == 2
       # requests to get and aggregate from different committees should be empty
       pool[].getElectraAggregatedAttestation(
-        2.Slot, combined[0].data.beacon_block_root, 1.CommitteeIndex).isNone()
-
+        2.Slot, hash_tree_root(combined[0].data), 1.CommitteeIndex).isNone()
 
   test "Attestations with disjoint comittee bits and equal data into single on-chain aggregate" & preset():
     let
@@ -1176,3 +1177,37 @@ suite "Attestation pool electra processing" & preset():
         state[].electraData.data, attestations[1], {}, cache, true).isOk
       pool[].verifyAttestationSignature(state, cache, attestations[0])
       pool[].verifyAttestationSignature(state, cache, attestations[1])
+
+  test "simple add and get with electra nonzero committee" & preset():
+     let
+       bc0 = get_beacon_committee(
+         state[], getStateField(state[], slot), 0.CommitteeIndex, cache)
+
+       bc1 = get_beacon_committee(
+         state[], getStateField(state[], slot), 1.CommitteeIndex, cache)
+
+       attestation_1 = makeElectraAttestation(
+         state[], state[].latest_block_root, bc0[0], cache)
+
+       attestation_2 = makeElectraAttestation(
+         state[], state[].latest_block_root, bc1[0], cache)
+
+     pool[].addAttestation(
+       attestation_1, @[bc0[0]], attestation_1.aggregation_bits.len,
+       attestation_1.loadSig, attestation_1.data.slot.start_beacon_time)
+
+     pool[].addAttestation(
+       attestation_2, @[bc1[0]], attestation_2.aggregation_bits.len,
+       attestation_2.loadSig, attestation_2.data.slot.start_beacon_time)
+
+     check:
+       process_slots(
+         defaultRuntimeConfig, state[],
+         getStateField(state[], slot) + MIN_ATTESTATION_INCLUSION_DELAY, cache,
+         info, {}).isOk()
+
+     check:
+       pool[].getElectraAggregatedAttestation(1.Slot, hash_tree_root(attestation_1.data),
+           0.CommitteeIndex).isOk
+       pool[].getElectraAggregatedAttestation(1.Slot, hash_tree_root(attestation_2.data),
+           1.CommitteeIndex).isOk

tests/test_mev_calls.nim

@@ -9,7 +9,7 @@
 {.used.}
 
 import
-  stew/[bitseqs, endians2, objects, byteutils],
+  stew/[bitseqs, endians2, objects],
   blscurve, bearssl/rand,
   results, chronos, presto, unittest2,
   chronos/unittest2/asynctests,