feat: implement EIP-7549 #6689

ensi321 · 2024-04-19T14:18:28Z

NOTE FOR REVIEWERS:

aggregatedAttestationPool.ts and aggregatedAttestationPool.test.ts have already been reviewed in feat: get attestations for electra block #6732. Feel free to skip them
processAttestation() and importBlock has been reviewed. Take the feedbacks in feat: beacon node process electra attestations EIP-7549 #6738 into account when reviewing.

Description

General:

Add containers & presets
Retrieve field values from serialized electra.attestations

Beacon node:

Modify processAttestations
Update attesting indices parsing from electra.Attestation

p2p:

Handle and publish new format of beacon_aggregate_and_proof and beacon_attestation
New validation rule

Validator:

Use MAX_ATTESTER_SLASHINGS_ELECTRA limit when constructing block body
Sign electra.attestations and electra.aggregateAndProof
Come up with new algo to get electra.attestations for block body in AggregatedAttestationPool
Create valid attestation and aggregate when attesting/aggregating

Misc but important:

Update beacon API format for attestation-related endpoints (pending cross-client discussion Update attestation related APIs to be compatible with EIP-7549 ethereum/beacon-APIs#445)
Modify AggregatedAttestationPool to store new attestation format
Modify AttestationPool to aggregate electra.attestations

packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts

* 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>

twoeths · 2024-05-08T01:19:24Z

packages/beacon-node/src/chain/opPools/attestationPool.ts

  const bitIndex = attestation.aggregationBits.getSingleTrueBit();

  // Should never happen, attestations are verified against this exact condition before
  if (bitIndex === null) {
    throw Error("Invalid attestation not exactly one bit set");
  }

+  if ("committeeBits" in attestation && !("committeeBits" in aggregate)) {


use isElectraAttestation() util, same for below

twoeths · 2024-05-08T01:22:07Z

packages/beacon-node/src/chain/validation/aggregateAndProof.ts

+      throw new AttestationError(GossipAction.REJECT, {code: AttestationErrorCode.NOT_EXACTLY_ONE_COMMITTEE_BIT_SET});
+    }
+    // [REJECT] aggregate.data.index == 0
+    if (attData.index === 0) {


Suggested change

if (attData.index === 0) {

if (attData.index !== 0) {

assert.equal should be more convenient
Edit: if (attData.index !== 0) { should be better since it throws gossip error to handle at upper level

twoeths · 2024-05-08T01:24:09Z

packages/beacon-node/src/chain/validation/attestation.ts

+    }
+
+    // [REJECT] aggregate.data.index == 0
+    if (attData.index === 0) {


Suggested change

if (attData.index === 0) {

if (attData.index !== 0) {

twoeths · 2024-05-08T02:27:22Z

packages/beacon-node/src/chain/opPools/attestationPool.ts

+    throw Error("Attempt to aggregate electra attestation into phase0 attestation");
+  }
+
+  if (!("committeeBits" in attestation) && "committeeBits" in aggregate) {


we don't need these validation unless we have to for the type assertion
in order to reach this code, attestation data should be the same hence the same type of attestation

twoeths · 2024-05-08T02:27:52Z

packages/beacon-node/src/chain/validation/attestation.ts

  subnet: number;
  attDataRootHex: RootHex;
 };

 export type AttestationOrBytes = ApiAttestation | GossipAttestation;

 /** attestation from api */
-export type ApiAttestation = {attestation: phase0.Attestation; serializedData: null};
+export type ApiAttestation = {attestation: phase0.Attestation; serializedData: null}; // TODO Electra: add new attestation type


remove TODO?

twoeths · 2024-05-08T02:31:42Z

packages/validator/src/services/validatorStore.ts

+      return {
+        aggregationBits: BitArray.fromSingleBit(duty.committeeLength, duty.validatorCommitteeIndex),
+        data: attestationData,
+        committeeBits: BitArray.fromSingleBit(duty.committeesAtSlot, duty.committeeIndex),


Suggested change

committeeBits: BitArray.fromSingleBit(duty.committeesAtSlot, duty.committeeIndex),

committeeBits: BitArray.fromSingleBit(MAX_COMMITTEES_PER_SLOT, duty.committeeIndex),

twoeths · 2024-05-08T04:14:09Z

packages/beacon-node/src/chain/opPools/attestationPool.ts

+    const attestationCommitteeIndex = attestation.committeeBits.getSingleTrueBit();
+    const aggregateCommitteeIndex = (aggregate as AggregateFastElectra).committeeBits.getSingleTrueBit();
+
+    if (attestationCommitteeIndex !== aggregateCommitteeIndex) {


now same attestation data could belong to different committee indices so we need to add 1 more level to our cache
implemented in #6744

twoeths · 2024-05-08T04:15:17Z

packages/beacon-node/src/chain/opPools/attestationPool.ts

-    signature: aggFast.signature.toBytes(PointFormat.compressed),
-  };
+function fastToAttestation(aggFast: AggregateFast): allForks.Attestation {
+  if ("committeeBits" in aggFast) {


Suggested change

if ("committeeBits" in aggFast) {

return {...aggFast, signature: aggFast.signature.toBytes(PointFormat.compressed)};

g11tech · 2024-05-08T08:22:53Z

packages/types/src/electra/types.ts

@@ -48,4 +48,4 @@ export type SignedConsolidation = ValueOf<typeof ssz.SignedConsolidation>;

 export type PendingBalanceDeposit = ValueOf<typeof ssz.PendingBalanceDeposit>;
 export type PartialWithdrawal = ValueOf<typeof ssz.PartialWithdrawal>;
-export type PendingConsolidation = ValueOf<typeof ssz.PendingConsolidation>;
+export type PendingConsolidation = ValueOf<typeof ssz.PendingConsolidation>;


could we remove this diff? i think newlines got removed

g11tech

merging the current work with electra

* 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>

ensi321 added the status-do-not-merge Merging this issue will break the build. Do not merge! label Apr 19, 2024

This was referenced Apr 29, 2024

Electra tracker #6341

Open

feat: add presets and ssz types for EIP-7549 #6715

Merged

twoeths reviewed May 2, 2024

View reviewed changes

packages/beacon-node/src/chain/opPools/aggregatedAttestationPool.ts Outdated Show resolved Hide resolved

twoeths mentioned this pull request May 4, 2024

feat: get attestations for electra block #6732

Merged

g11tech force-pushed the electra-fork branch from b19abdf to 46ae715 Compare May 4, 2024 18:11

ensi321 mentioned this pull request May 5, 2024

feat: beacon node process electra attestations EIP-7549 #6738

Merged

ensi321 force-pushed the nc/7549 branch from 35e04d1 to f7ae62b Compare May 6, 2024 14:52

ensi321 marked this pull request as ready for review May 7, 2024 19:09

ensi321 requested a review from a team as a code owner May 7, 2024 19:09

ensi321 and others added 13 commits May 7, 2024 22:13

initial commit

b75d75c

lint

6ae90f6

Add getAttestingIndices and update getIndexedAttestation

00f5b17

Update gossip validation

1f1c06c

Update attestation gossip validation

5b78ba1

aggregateAndProof validation

077e216

clean up

371432d

Validator

30f674b

Misc

4be4e55

Fix the build erros

d4f91fb

Fix check-types

8315bea

Address comments

b286351

ensi321 force-pushed the nc/7549 branch from 70e770d to b286351 Compare May 7, 2024 19:21

twoeths reviewed May 8, 2024

View reviewed changes

twoeths mentioned this pull request May 8, 2024

fix: attestation pool for electra #6744

Merged

twoeths reviewed May 8, 2024

View reviewed changes

g11tech reviewed May 8, 2024

View reviewed changes

g11tech approved these changes May 8, 2024

View reviewed changes

g11tech merged commit 838add9 into electra-fork May 8, 2024
11 of 17 checks passed

g11tech deleted the nc/7549 branch May 8, 2024 11:00

ensi321 removed the status-do-not-merge Merging this issue will break the build. Do not merge! label May 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement EIP-7549 #6689

feat: implement EIP-7549 #6689

ensi321 commented Apr 19, 2024 •

edited

twoeths May 8, 2024

twoeths May 8, 2024

twoeths May 8, 2024 •

edited

twoeths May 8, 2024

twoeths May 8, 2024

twoeths May 8, 2024

twoeths May 8, 2024

twoeths May 8, 2024

twoeths May 8, 2024

g11tech May 8, 2024

g11tech left a comment

	committeeBits: BitArray.fromSingleBit(duty.committeesAtSlot, duty.committeeIndex),
	committeeBits: BitArray.fromSingleBit(MAX_COMMITTEES_PER_SLOT, duty.committeeIndex),

	if ("committeeBits" in aggFast) {
	return {...aggFast, signature: aggFast.signature.toBytes(PointFormat.compressed)};

feat: implement EIP-7549 #6689

feat: implement EIP-7549 #6689

Conversation

ensi321 commented Apr 19, 2024 • edited

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twoeths May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech left a comment

Choose a reason for hiding this comment

ensi321 commented Apr 19, 2024 •

edited

twoeths May 8, 2024 •

edited