feat: add Electra attestation V2 endpoints by ensi321 · Pull Request #6951 · ChainSafe/lodestar

ensi321 · 2024-07-15T13:19:28Z

This PR adds definition of the new attestation related V2 endpoints in Electra. It also reverts some of the hacks we made to some V1 endpoints during devnet-0.

Note some of the V1 and V2 endpoints are not functioning properly. TODOs are added to ensure their functionalities

Endpoints added:

GET /eth/v2/validator/aggregate_attestation
GET /eth/v2/beacon/blocks/{block_id}/attestations
GET /eth/v2/beacon/pool/attestations
POST /eth/v2/beacon/pool/attestations
GET /eth/v2/beacon/pool/attester_slashings
POST /eth/v2/beacon/pool/attester_slashings (Only support pre-electra request)
POST /eth/v2/validator/aggregate_and_proofs

Endpoints modified:

GET /eth/v1/validator/aggregate_attestation (Broken)
GET /eth/v1/beacon/blocks/{block_id}/attestations
GET /eth/v1/beacon/pool/attestations
GET /eth/v1/beacon/pool/attester_slashings

Remaining todos:

Modify attestation pool to handle pre-electra attestation properly (Related to GET /eth/v1/validator/aggregate_attestation)
Review submitPoolAttesterSlashings flow to handle electra.AttesterSlashing (Related to POST /eth/v2/beacon/pool/attester_slashings)

Passes oapi spec test with the latest master branch of ethereum/beacon-APIs

Partially closes #6895

nflaig

The diff in PR is kinda strange in a few places because we had the hacks previously.

I see two options

either we remove the hacks separately and then rebase this branch on top
or we merge this, do another review on electra branch, and submit fixes afterwards if any

I guess option (2) is favorable as (1) might be too much effort for what it's worth

nflaig · 2024-07-18T12:05:44Z

+    async getPoolAttestationsV2({slot, committeeIndex}) {
+      // Already filtered by slot
+      let attestations = chain.aggregatedAttestationPool.getAll(slot);
      const fork = chain.config.getForkName(slot ?? attestations[0].data.slot) ?? ForkName.phase0;


This logic is unsafe and also incorrect, need to be careful with accessing items in array as typescript does not properly warn here

If array is empty, this will give an error attestations[0].data.slot, need to do attestations[0]?.data.slot

I would also favor a logic like this

const fork = chain.config.getForkName(slot ?? attestations[0]?.data.slot ?? chain.clock.currentSlot);

Honestly what we should do across the code base is type all arrays like this

(Attestation | undefined)[]

instead of

Attestation[]

twoeths · 2024-07-22T02:07:36Z


    this.logger.verbose("Aggregating attestations", logCtx);
-    const res = await this.api.validator.getAggregatedAttestation({
+    const res = await this.api.validator.getAggregatedAttestationV2({


same to the above, vc should follow the spec to call correct api depending on the clock slot. Note that lodestar vc could call bn from other clients as well

we can do a kurtosis run before we merge electra branch to unstable, I have a config which tests interop will all other clients but we will need to use v1 apis pre-electra in any case becasue we don't know if users updated their clients, similar to how we transitioned to block v3 api

nflaig · 2024-07-30T11:23:01Z

 }
+
+export type ForkPreElectra = ForkPreBlobs | ForkName.deneb;
+export type ForkElectra = Exclude<ForkName, ForkPreElectra>;


anyone has an idea for a fancy name here? Looks we like we usually don't use the fork name but rather a feature added in the fork

cc @g11tech

Yes ForkElectra is a little confusing here imo.

Maybe ForkPostElectra.

I'd also be in favor of using that same naming for the other forks (eg ForkPostBellatrix instead of ForkExecution)

I'd also be in favor of using that same naming for the other forks (eg ForkPostBellatrix instead of ForkExecution)

Yea I guess back in the days, each hard fork has a single signature feature (or theme) that you can name after. For electra, there is a bunch of new features that are significant, so naming convention like ForkExecution can't apply to electra.

ForkElectraOnwards?

in favor of pre / post fork terminology, it seems most commonly used and the most concise

nflaig · 2024-07-30T11:24:20Z


    this.logger.verbose("Aggregating attestations", logCtx);
-    const res = await this.api.validator.getAggregatedAttestation({
+    const res = await this.api.validator.getAggregatedAttestationV2({


we can do a kurtosis run before we merge electra branch to unstable, I have a config which tests interop will all other clients but we will need to use v1 apis pre-electra in any case becasue we don't know if users updated their clients, similar to how we transitioned to block v3 api

nflaig · 2024-07-30T11:26:21Z

    };
    try {
-      (await this.api.beacon.submitPoolAttestations({signedAttestations})).assertOk();
+      // TODO Electra: Ensure calling V2 works in pre-electra


just a note here, while all apis should be backward compatible, i.e. work pre-electra, we still can't use them until the fork happens. After the electra fork, we can safely remove all v1 calls and only use v2

nflaig · 2024-07-30T11:36:09Z

    res: undefined,
  },
+  submitPoolAttestationsV2: {
+    args: {signedAttestations: [ssz.phase0.Attestation.defaultValue()]},


I opened an issue to test this with electra data #6987

nflaig

LGTM - can solve remaining todos separately to make it easier to review

nflaig · 2024-07-30T12:10:28Z

      req: {
        writeReqJson: ({signedAttestations}) => {
-          const fork = config.getForkName(signedAttestations[0].data.slot);
+          const fork = config.getForkName(signedAttestations[0]?.data.slot ?? 0);


In practice, we won't be calling this api with an empty array but it's possible based on the types and the spec does not explicitly state that the array is not allowed to be empty.

We don't have access to the clock here, so it's hard as far as I can see to determine the current fork but passing 0 (translating to phase0) should be fine as well since array is empty anyways.

wemeetagain · 2024-08-01T17:20:21Z

Are we waiting on merging this until electra-fork branch is rebased on unstable? Or merging now?

ensi321 · 2024-08-01T18:44:08Z

Are we waiting on merging this until electra-fork branch is rebased on unstable? Or merging now?

@wemeetagain Yes we will wait until the electra-fork branch is rebased

wemeetagain · 2024-08-01T20:25:10Z

Ok, can you base this PR on electra-fork-rebasejul30 in that case?

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

wemeetagain

merging, we can continue the next round of review there

* Initial commit * getAggregatedAttestationV2 * Lint * Fix minor flaw * Add publishAggregateAndProofsV2 * Fix spelling * Fix CI * Fix spec test * Clean up events api * Run against latest beacon api spec * Revert changes to emitted events * Update packages/api/src/beacon/routes/beacon/pool.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com> * Update packages/api/src/beacon/routes/beacon/pool.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com> * Update packages/api/src/beacon/routes/beacon/pool.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com> * Update packages/api/src/beacon/routes/beacon/pool.ts Co-authored-by: Nico Flaig <nflaig@protonmail.com> * Address comment * Add api stub back * Add todos * Review PR * Fix rebase * Lint --------- Co-authored-by: Nico Flaig <nflaig@protonmail.com>

wemeetagain · 2024-09-17T19:08:01Z

🎉 This PR is included in v1.22.0 🎉

nflaig reviewed Jul 15, 2024

View reviewed changes

Comment thread packages/beacon-node/src/api/impl/beacon/pool/index.ts

ensi321 marked this pull request as ready for review July 17, 2024 20:35

ensi321 requested a review from a team as a code owner July 17, 2024 20:35

nflaig reviewed Jul 18, 2024

View reviewed changes

twoeths reviewed Jul 22, 2024

View reviewed changes

Comment thread packages/api/src/beacon/routes/beacon/pool.ts

twoeths reviewed Jul 22, 2024

View reviewed changes

Comment thread packages/beacon-node/src/api/impl/validator/index.ts

twoeths reviewed Jul 22, 2024

View reviewed changes

nflaig reviewed Jul 30, 2024

View reviewed changes

nflaig approved these changes Jul 30, 2024

View reviewed changes

wemeetagain mentioned this pull request Aug 1, 2024

feat: electra fork #6986

Merged

ensi321 and others added 17 commits August 5, 2024 13:10

Initial commit

551f95d

getAggregatedAttestationV2

24ae80a

Lint

ea6acce

Fix minor flaw

1a39d35

Add publishAggregateAndProofsV2

ecf21f2

Fix spelling

1d2b149

Fix CI

a4fb02c

Fix spec test

d468626

Clean up events api

da9fd80

Run against latest beacon api spec

df119e8

Revert changes to emitted events

bd7ce70

Update packages/api/src/beacon/routes/beacon/pool.ts

2556d42

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

Update packages/api/src/beacon/routes/beacon/pool.ts

88e593e

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

Update packages/api/src/beacon/routes/beacon/pool.ts

dadc6b9

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

Update packages/api/src/beacon/routes/beacon/pool.ts

0d3e2bd

Co-authored-by: Nico Flaig <nflaig@protonmail.com>

Address comment

c657fd9

Add api stub back

e4f52b4

ensi321 and others added 2 commits August 5, 2024 13:18

Add todos

9b9836e

Review PR

c9394c6

ensi321 force-pushed the nc/attestation-api-v2 branch from c6e1cca to c9394c6 Compare August 5, 2024 04:19

ensi321 changed the base branch from electra-fork to electra-fork-rebasejul30 August 5, 2024 04:20

ensi321 added 2 commits August 5, 2024 14:56

Fix rebase

b498e6c

Lint

9c0d54e

wemeetagain approved these changes Aug 5, 2024

View reviewed changes

wemeetagain merged commit d0c6d21 into electra-fork-rebasejul30 Aug 5, 2024

wemeetagain deleted the nc/attestation-api-v2 branch August 5, 2024 19:53

Uh oh!

Uh oh!

Conversation

ensi321 commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

nflaig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ensi321 Jul 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nflaig Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nflaig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wemeetagain commented Aug 1, 2024

Uh oh!

ensi321 commented Aug 1, 2024

Uh oh!

wemeetagain commented Aug 1, 2024

Uh oh!

wemeetagain left a comment

Choose a reason for hiding this comment

Uh oh!

wemeetagain commented Sep 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ensi321 commented Jul 15, 2024 •

edited

Loading

ensi321 Jul 31, 2024 •

edited

Loading

nflaig Jul 30, 2024 •

edited

Loading