electra attestation updates (#6295)

* electra attestation updates

In Electra, we have two attestation formats: on-chain and on-network -
the former combines all committees of a slot in a single committee bit
list.

This PR makes a number of cleanups to move towards fixing this -
attestation packing however still needs to be fixed as it currently
creates attestations with a single committee only which is very
inefficient.

* more attestations in the blocks

* signing and aggregation fixes

* tool fix

* test, import

AllTests-mainnet.md

@@ -5,6 +5,11 @@ AllTests-mainnet
 + ancestorSlot                                                                               OK
 ```
 OK: 1/1 Fail: 0/1 Skip: 0/1
+## Attestation pool electra processing [Preset: mainnet]
+```diff
++ Can add and retrieve simple electra attestations [Preset: mainnet]                         OK
+```
+OK: 1/1 Fail: 0/1 Skip: 0/1
 ## Attestation pool processing [Preset: mainnet]
 ```diff
 + Attestation from different branch [Preset: mainnet]                                        OK
@@ -1020,4 +1025,4 @@ OK: 2/2 Fail: 0/2 Skip: 0/2
 OK: 9/9 Fail: 0/9 Skip: 0/9
 
 ---TOTAL---
-OK: 685/690 Fail: 0/690 Skip: 5/690
+OK: 686/691 Fail: 0/691 Skip: 5/691

beacon_chain/consensus_object_pools/spec_cache.nim

@@ -90,7 +90,7 @@ func compatible_with_shuffling*(
 iterator get_attesting_indices*(shufflingRef: ShufflingRef,
                                 slot: Slot,
                                 committee_index: CommitteeIndex,
-                                bits: CommitteeValidatorsBits):
+                                bits: CommitteeValidatorsBits | ElectraCommitteeValidatorsBits):
                                   ValidatorIndex =
   if not bits.compatible_with_shuffling(shufflingRef, slot, committee_index):
     trace "get_attesting_indices: inconsistent aggregation and committee length"
@@ -104,34 +104,26 @@ iterator get_attesting_indices*(shufflingRef: ShufflingRef,
 iterator get_attesting_indices*(shufflingRef: ShufflingRef,
                                 slot: Slot,
                                 committee_bits: AttestationCommitteeBits,
-                                aggregation_bits: ElectraCommitteeValidatorsBits):
+                                aggregation_bits: ElectraCommitteeValidatorsBits, on_chain: static bool):
                                   ValidatorIndex =
-  debugRaiseAssert "compatible with shuffling? needs checking"
-  #if not aggregation_bits.compatible_with_shuffling(shufflingRef, slot, committee_index):
-  if false:
-    trace "get_attesting_indices: inconsistent aggregation and committee length"
+  when on_chain:
+    var pos = 0
+    for committee_index in get_committee_indices(committee_bits):
+      for _, validator_index in get_beacon_committee(
+          shufflingRef, slot, committee_index):
+
+        if aggregation_bits[pos]:
+          yield validator_index
+          pos += 1
   else:
-    debugComment "replace this implementation with actual iterator, after checking on conditions re repeat vals, ordering, etc; this is almost direct transcription of spec link algorithm in one of the places it doesn't make sense"
-    ## Return the set of attesting indices corresponding to ``aggregation_bits``
-    ## and ``committee_bits``.
-    var output: HashSet[ValidatorIndex]
-    let committee_indices = toSeq(committee_bits.oneIndices)
-    var committee_offset = 0
-    for index in committee_indices:
-      let committee = get_beacon_committee(shufflingRef, slot, index.CommitteeIndex)
-      var committee_attesters: HashSet[ValidatorIndex]
-      for i, index in committee:
-        if aggregation_bits[committee_offset + i]:
-          committee_attesters.incl index
-      output.incl committee_attesters
-
-      committee_offset += len(committee)
-
-    for validatorIndex in output:
-      yield validatorIndex
+    let committee_index = get_committee_index_one(committee_bits)
+    for validator_index in get_attesting_indices(
+        shufflingRef, slot, committee_index, aggregation_bits, on_chain):
+      yield validator_index
 
 iterator get_attesting_indices*(
-    dag: ChainDAGRef, attestation: phase0.TrustedAttestation): ValidatorIndex =
+    dag: ChainDAGRef, attestation: phase0.TrustedAttestation,
+    on_chain: static bool = true): ValidatorIndex =
   block: # `return` is not allowed in an inline iterator
     let
       slot =
@@ -182,8 +174,8 @@ iterator get_attesting_indices*(
       yield validator
 
 iterator get_attesting_indices*(
-    dag: ChainDAGRef, attestation: electra.TrustedAttestation): ValidatorIndex =
-  debugRaiseAssert "bad duplication, mostly to avoid the get_attesting_index call from potentially getting screwed up in deployment version"
+    dag: ChainDAGRef, attestation: electra.TrustedAttestation,
+    on_chain: static bool): ValidatorIndex =
   block: # `return` is not allowed in an inline iterator
     let
       slot =
@@ -220,13 +212,14 @@ iterator get_attesting_indices*(
           break
 
     for validator in get_attesting_indices(
-        shufflingRef, slot, attestation.committee_bits, attestation.aggregation_bits):
+        shufflingRef, slot, attestation.committee_bits,
+        attestation.aggregation_bits, on_chain):
       yield validator
 
 func get_attesting_indices_one*(shufflingRef: ShufflingRef,
                                 slot: Slot,
                                 committee_index: CommitteeIndex,
-                                bits: CommitteeValidatorsBits):
+                                bits: CommitteeValidatorsBits | ElectraCommitteeValidatorsBits):
                                   Option[ValidatorIndex] =
   # A variation on get_attesting_indices that returns the validator index only
   # if only one validator index is set
@@ -239,16 +232,20 @@ func get_attesting_indices_one*(shufflingRef: ShufflingRef,
 
 func get_attesting_indices_one*(shufflingRef: ShufflingRef,
                                 slot: Slot,
-                                committee_indices: AttestationCommitteeBits,
-                                aggregation_bits: ElectraCommitteeValidatorsBits):
-                                  Option[ValidatorIndex] =
+                                committee_bits: AttestationCommitteeBits,
+                                aggregation_bits: ElectraCommitteeValidatorsBits,
+                                on_chain: static bool):
+                                  Opt[ValidatorIndex] =
   # A variation on get_attesting_indices that returns the validator index only
   # if only one validator index is set
-  var res = none(ValidatorIndex)
+  static: doAssert not on_chain, "only on_chain supported"
+
+  var res = Opt.none(ValidatorIndex)
+  let committee_index = ? get_committee_index_one(committee_bits)
   for validator_index in get_attesting_indices(
-      shufflingRef, slot, committee_indices, aggregation_bits):
-    if res.isSome(): return none(ValidatorIndex)
-    res = some(validator_index)
+      shufflingRef, slot, committee_index, aggregation_bits):
+    if res.isSome(): return Opt.none(ValidatorIndex)
+    res = Opt.some(validator_index)
   res
 
 # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.6/specs/phase0/beacon-chain.md#get_attesting_indices
@@ -263,8 +260,11 @@ func get_attesting_indices*(shufflingRef: ShufflingRef,
 func get_attesting_indices*(shufflingRef: ShufflingRef,
                             slot: Slot,
                             committee_index: CommitteeIndex,
-                            bits: ElectraCommitteeValidatorsBits):
+                            bits: ElectraCommitteeValidatorsBits,
+                            on_chain: static bool):
                               seq[ValidatorIndex] =
+  static: doAssert not on_chain, "only on_chain supported"
+
   for idx in get_attesting_indices(shufflingRef, slot, committee_index, bits):
     result.add(idx)
 

beacon_chain/fork_choice/fork_choice.nim

@@ -285,7 +285,7 @@ proc process_block*(self: var ForkChoice,
 
   for attestation in blck.body.attestations:
     if attestation.data.beacon_block_root in self.backend:
-      for validator_index in dag.get_attesting_indices(attestation):
+      for validator_index in dag.get_attesting_indices(attestation, true):
         self.backend.process_attestation(
           validator_index,
           attestation.data.beacon_block_root,

beacon_chain/gossip_processing/block_processor.nim

@@ -603,7 +603,7 @@ proc storeBlock(
         src, wallTime, trustedBlock.message)
 
       for attestation in trustedBlock.message.body.attestations:
-        for validator_index in dag.get_attesting_indices(attestation):
+        for validator_index in dag.get_attesting_indices(attestation, true):
           vm[].registerAttestationInBlock(attestation.data, validator_index,
             trustedBlock.message.slot)
 

beacon_chain/gossip_processing/gossip_validation.nim

@@ -879,7 +879,8 @@ proc validateAttestation*(
   let
     fork = pool.dag.forkAtEpoch(attestation.data.slot.epoch)
     attesting_index = get_attesting_indices_one(
-      shufflingRef, slot, attestation.committee_bits, attestation.aggregation_bits)
+      shufflingRef, slot, attestation.committee_bits,
+      attestation.aggregation_bits, false)
 
   # The number of aggregation bits matches the committee size, which ensures
   # this condition holds.
@@ -1158,7 +1159,7 @@ proc validateAggregate*(
   let
     fork = pool.dag.forkAtEpoch(aggregate.data.slot.epoch)
     attesting_indices = get_attesting_indices(
-      shufflingRef, slot, committee_index, aggregate.aggregation_bits)
+      shufflingRef, slot, committee_index, aggregate.aggregation_bits, false)
 
   let
     sig =

beacon_chain/spec/beaconstate.nim

@@ -596,24 +596,16 @@ iterator get_attesting_indices_iter*(
     aggregation_bits: ElectraCommitteeValidatorsBits,
     committee_bits: auto,
     cache: var StateCache): ValidatorIndex =
-  debugComment "replace this implementation with actual iterator, after checking on conditions re repeat vals, ordering, etc; this is almost direct transcription of spec link algorithm in one of the places it doesn't make sense"
   ## Return the set of attesting indices corresponding to ``aggregation_bits``
   ## and ``committee_bits``.
-  var output: HashSet[ValidatorIndex]
-  let committee_indices = toSeq(committee_bits.oneIndices)
-  var committee_offset = 0
-  for index in committee_indices:
-    let committee = get_beacon_committee(state, data.slot, index.CommitteeIndex, cache)
-    var committee_attesters: HashSet[ValidatorIndex]
-    for i, index in committee:
-      if aggregation_bits[committee_offset + i]:
-        committee_attesters.incl index
-    output.incl committee_attesters
-
-    committee_offset += len(committee)
-
-  for validatorIndex in output:
-    yield validatorIndex
+  var pos = 0
+  for committee_index in get_committee_indices(committee_bits):
+    for _, validator_index in get_beacon_committee(
+        state, data.slot, committee_index, cache):
+
+      if aggregation_bits[pos]:
+        yield validator_index
+      pos += 1
 
 # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.6/specs/phase0/beacon-chain.md#get_attesting_indices
 func get_attesting_indices*(
@@ -886,7 +878,7 @@ func get_base_reward(
 # https://github.com/ethereum/consensus-specs/blob/v1.4.0-beta.6/specs/phase0/beacon-chain.md#attestations
 proc check_attestation*(
     state: ForkyBeaconState, attestation: SomeAttestation, flags: UpdateFlags,
-    cache: var StateCache): Result[void, cstring] =
+    cache: var StateCache, on_chain: static bool = true): Result[void, cstring] =
   ## Check that an attestation follows the rules of being included in the state
   ## at the current slot. When acting as a proposer, the same rules need to
   ## be followed!
@@ -921,7 +913,7 @@ proc check_attestation*(
 proc check_attestation*(
     state: electra.BeaconState,
     attestation: electra.Attestation | electra.TrustedAttestation,
-    flags: UpdateFlags, cache: var StateCache): Result[void, cstring] =
+    flags: UpdateFlags, cache: var StateCache, on_chain: static bool): Result[void, cstring] =
   ## Check that an attestation follows the rules of being included in the state
   ## at the current slot. When acting as a proposer, the same rules need to
   ## be followed!
@@ -937,17 +929,26 @@ proc check_attestation*(
   if not (data.index == 0):
     return err("Electra attestation data index not 0")
 
-  var participants_count = 0
-  debugComment "cache doesn't know about forks"
-  for index in attestation.committee_bits.oneIndices:
-    if not (index.uint64 < get_committee_count_per_slot(
-        state, data.target.epoch, cache)):
-      return err("foo")
-    let committee = get_beacon_committee(state, data.slot, index.CommitteeIndex, cache)
-    participants_count += len(committee)
+  when on_chain:
+    var participants_count = 0'u64
+    debugComment "cache doesn't know about forks"
+    for index in attestation.committee_bits.oneIndices:
+      if not (index.uint64 < get_committee_count_per_slot(
+          state, data.target.epoch, cache)):
+        return err("attestation wrong committee index len")
+      participants_count +=
+        get_beacon_committee_len(state, data.slot, index.CommitteeIndex, cache)
+
+    if not (lenu64(attestation.aggregation_bits) == participants_count):
+      return err("attestation wrong aggregation bit length")
+  else:
+    let
+      committee_index = get_committee_index_one(attestation.committee_bits).valueOr:
+        return err("Network attestation without single committee index")
 
-  if not (len(attestation.aggregation_bits) == participants_count):
-    return err("")
+    if not (lenu64(attestation.aggregation_bits) ==
+        get_beacon_committee_len(state, data.slot, committee_index, cache)):
+      return err("attestation wrong aggregation bit length")
 
   if epoch == get_current_epoch(state):
     if not (data.source == state.current_justified_checkpoint):
@@ -1100,7 +1101,7 @@ proc process_attestation*(
     attestation: electra.Attestation | electra.TrustedAttestation,
     flags: UpdateFlags, base_reward_per_increment: Gwei,
     cache: var StateCache): Result[Gwei, cstring] =
-  ? check_attestation(state, attestation, flags, cache)
+  ? check_attestation(state, attestation, flags, cache, true)
 
   let proposer_index = get_beacon_proposer_index(state, cache).valueOr:
     return err("process_attestation: no beacon proposer index and probably no active validators")

beacon_chain/spec/datatypes/electra.nim

@@ -731,6 +731,7 @@ iterator getValidatorIndices*(attester_slashing: AttesterSlashing | TrustedAttes
 func shortLog*(v: electra.Attestation | electra.TrustedAttestation): auto =
   (
     aggregation_bits: v.aggregation_bits,
+    committee_bits: v.committee_bits,
     data: shortLog(v.data),
     signature: shortLog(v.signature)
   )

beacon_chain/spec/validator.nim

@@ -8,7 +8,9 @@
 
 # Helpers and functions pertaining to managing the validator set
 
-import ./helpers
+import
+  std/algorithm,
+  "."/[crypto, helpers]
 export helpers
 
 const
@@ -541,3 +543,58 @@ func compute_subscribed_subnet(node_id: UInt256, epoch: Epoch, index: uint64):
 iterator compute_subscribed_subnets*(node_id: UInt256, epoch: Epoch): SubnetId =
   for index in 0'u64 ..< SUBNETS_PER_NODE:
     yield compute_subscribed_subnet(node_id, epoch, index)
+
+iterator get_committee_indices*(bits: AttestationCommitteeBits): CommitteeIndex =
+  for index, b in bits:
+    if b:
+      yield CommitteeIndex.init(uint64(index)).valueOr:
+        break # Too many bits! Shouldn't happen
+
+func get_committee_index_one*(bits: AttestationCommitteeBits): Opt[CommitteeIndex] =
+  var res = Opt.none(CommitteeIndex)
+  for committee_index in get_committee_indices(bits):
+    if res.isSome(): return Opt.none(CommitteeIndex)
+    res = Opt.some(committee_index)
+  res
+
+proc compute_on_chain_aggregate*(
+    network_aggregates: openArray[electra.Attestation]): Opt[electra.Attestation] =
+  # aggregates = sorted(network_aggregates, key=lambda a: get_committee_indices(a.committee_bits)[0])
+  let aggregates = network_aggregates.sortedByIt(it.committee_bits.get_committee_index_one().expect("just one"))
+
+  let data = aggregates[0].data
+
+  var agg: AggregateSignature
+  var committee_bits: AttestationCommitteeBits
+
+  var totalLen = 0
+  for i, a in aggregates:
+    totalLen += a.aggregation_bits.len
+
+  var aggregation_bits = ElectraCommitteeValidatorsBits.init(totalLen)
+  var pos = 0
+  for i, a in aggregates:
+    let
+      committee_index = ? get_committee_index_one(a.committee_bits)
+      first = pos == 0
+
+    for b in a.aggregation_bits:
+      aggregation_bits[pos] = b
+      pos += 1
+
+    let sig = ? a.signature.load() # Expensive
+    if first:
+      agg = AggregateSignature.init(sig)
+    else:
+      agg.aggregate(sig)
+
+    committee_bits[int(committee_index)] = true
+
+  let signature = agg.finish()
+
+  ok electra.Attestation(
+      aggregation_bits: aggregation_bits,
+      data: data,
+      committee_bits: committee_bits,
+      signature: signature.toValidatorSig(),
+  )

beacon_chain/validators/beacon_validators.nim

@@ -501,7 +501,7 @@ proc makeBeaconBlockForHeadAndSlot*(
   let
     attestations =
       when PayloadType.kind == ConsensusFork.Electra:
-        default(seq[electra.Attestation])
+        node.attestationPool[].getElectraAttestationsForBlock(state[], cache)
       else:
         node.attestationPool[].getAttestationsForBlock(state[], cache)
     exits = withState(state[]):

ncli/ncli_common.nim

@@ -381,7 +381,7 @@ func collectFromAttestations(
       doAssert base_reward_per_increment > 0.Gwei
       for attestation in forkyBlck.message.body.attestations:
         doAssert check_attestation(
-          forkyState.data, attestation, {}, cache).isOk
+          forkyState.data, attestation, {}, cache, true).isOk
         let proposerReward =
           if attestation.data.target.epoch == get_current_epoch(forkyState.data):
             get_proposer_reward(