feat: get attestations for electra block by twoeths · Pull Request #6732 · ChainSafe/lodestar

twoeths · 2024-05-04T07:47:44Z

Motivation

Implement getAttestationsForBlock() for electra

Description

Group attestations by AttData hex first because different committees could have the same AttestationData
Implement on-chain aggregation for electra when producing blocks

part of #6689

cc @ensi321

ensi321

Minor comment. Good to go when they are addressed 🙂
Learned a lot when reviewing thanks

ensi321 · 2024-05-07T16:58:10Z

+   * Get attestations to be included in a block pre-electra. Returns up to $MAX_ATTESTATIONS items
   */
-  getAttestationsForBlock(
+  getAttestationsForBlockPreElectra(


Return type of getAttestationsForBlockPreElectra() should be phase0.Attestation[]

ensi321 · 2024-05-07T16:58:50Z

+    fork: ForkName,
+    forkChoice: IForkChoice,
+    state: CachedBeaconStateAllForks
+  ): allForks.Attestation[] {


I think we can safely define the return type as electra.Attestation[]

ensi321 · 2024-05-07T17:05:38Z

+          // These properties should not change after being validate in gossip
+          // IF they have to be validated, do it only with one attestation per group since same data
+          // The committeeCountPerSlot can be precomputed once per slot
+          // for (const [i, {attestation, notSeenAttesterCount}] of attestationGroup


Is this line of comment necessary?

let the comment be there for now, later cleanups can alwayss happen

ensi321 · 2024-05-07T17:25:43Z

+    if (attestationNonParticipation !== undefined) {
+      const {attestation} = attestationNonParticipation;
+      committeeBits.set(committeeIndex, true);
+      aggregationBits = [...aggregationBits, ...attestation.aggregationBits.toBoolArray()];


Chain up aggregationBits by using toBoolArray() and later do fromBoolArray() seems like a hack. Looks good for now but this chain up operation should ideally be provided by ssz library

hmm like a concat op

ensi321 · 2024-05-07T17:48:41Z

+        );
+        const aggAttestation = {
+          ...attestationSeed,
+          // aggregationBits: BitArray.fromBoolArray(aggregationBitsArr[i]),


Obsolete comment?

* feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com>

* initial commit * lint * Add getAttestingIndices and update getIndexedAttestation * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * clean up * Validator * Misc * Fix the build erros * feat: get attestations for electra block (#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments --------- Co-authored-by: Nazar Hussain <nazarhussain@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

* feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com>

* initial commit * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * clean up * Misc * Fix the build erros * feat: get attestations for electra block (#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments * Fix the build erros * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * Fix rebase issue * Update committee index count check --------- Co-authored-by: NC <adrninistrator1@protonmail.com> Co-authored-by: Navie Chan <naviechan@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

* initial commit * lint * Add getAttestingIndices and update getIndexedAttestation * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * clean up * Validator * Misc * Fix the build erros * feat: get attestations for electra block (#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments --------- Co-authored-by: Nazar Hussain <nazarhussain@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

* initial commit * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * clean up * Misc * Fix the build erros * feat: get attestations for electra block (#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments * Fix the build erros * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * Fix rebase issue * Update committee index count check --------- Co-authored-by: NC <adrninistrator1@protonmail.com> Co-authored-by: Navie Chan <naviechan@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

* initial commit * lint * Add getAttestingIndices and update getIndexedAttestation * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * clean up * Validator * Misc * Fix the build erros * feat: get attestations for electra block (ChainSafe#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments --------- Co-authored-by: Nazar Hussain <nazarhussain@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

* initial commit * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * clean up * Misc * Fix the build erros * feat: get attestations for electra block (ChainSafe#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments * Fix the build erros * Extend spec runner to be more flexible * Add missing state attributes for electra * Fix ss data types for electra spec * Make the spec runner more flexible * Fix the bug in process attestation * Update the sepc test version * Fix rebase issue * Update committee index count check --------- Co-authored-by: NC <adrninistrator1@protonmail.com> Co-authored-by: Navie Chan <naviechan@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io>

* initial commit * lint * Add getAttestingIndices and update getIndexedAttestation * Update gossip validation * Update attestation gossip validation * aggregateAndProof validation * clean up * Validator * Misc * Fix the build erros * feat: get attestations for electra block (#6732) * feat: getAttestationsForBlock() for electra * chore: fix lint * fix: MAX_ATTESTATIONS_PER_GROUP_ELECTRA and address PR comments * chore: unit test aggregateConsolidation * Fix rebase mistake * Address my own comment :) --------- Co-authored-by: Navie Chan <naviechan@gmail.com> * Fix check-types * Address comments --------- Co-authored-by: Nazar Hussain <nazarhussain@gmail.com> Co-authored-by: tuyennhv <tuyen@chainsafe.io> Co-authored-by: gajinder <develop@g11tech.io>