Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update submitPoolAttestationsV2 endpoint #472

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Update submitPoolAttestationsV2 endpoint #472

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7203294
update post attestation v2 endpoint
eserilev Sep 16, 2024
c581a51
remove anyOf
eserilev Sep 16, 2024
508487a
add a new single_attestation event
eserilev Sep 16, 2024
085e16a
reintroduce anyOf, move type def to electra
eserilev Sep 23, 2024
f2f7969
move Signature field to match spec
eserilev Oct 9, 2024
997ddb7
Merge branch 'master' into post_attestation_v2
eserilev Oct 9, 2024
1df7423
Lint
eserilev Oct 9, 2024
6520c2f
Update types/electra/attestation.yaml
eserilev Nov 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions apis/beacon/pool/attestations.v2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ post:
operationId: submitPoolAttestationsV2
summary: Submit Attestation objects to node
description: |
Submits Attestation objects to the node. Each attestation in the request body is processed individually.
Submits SingleAttestation objects to the node. Each attestation in the request body is processed individually.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might wanna just say "attestations" here as pre-electra we pass phase0.Attestation


If an attestation is validated successfully, the node MUST publish that attestation on the appropriate subnet.

Expand All @@ -79,10 +79,7 @@ post:
anyOf:
- type: array
items:
$ref: '../../../beacon-node-oapi.yaml#/components/schemas/Attestation'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the idea is to always submit SingleAttestation even pre-Electra? in this case should remove the anyOf

Copy link
Author

@eserilev eserilev Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed anyOf

The idea here is that this endpoint should only be used post electra. Since the v2 endpoint is fairly new I thought it'd be ok to repurpose it for post electra usage. Lighthouse is only using the v2 endpoint post electra, but maybe other clients would prefer a v3 endpoint?

Copy link
Collaborator

@nflaig nflaig Sep 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need a v3, this endpoint is effectively only used in devnets right now and would be updated for the next devnet and I don't think anyone is running mixed client setups yet.

In Lodestar we also start using the attestation v2 apis post-electra but what I meant is that after electra is live on mainnet we can be sure that all nodes implement the v2 apis and the v1 apis become unusable since those only support phase0 attestations, this means we could completely remove them from the codebase. We still wanna support earlier forks though, e.g. our sim tests do fork transitions from phase0 to latest fork, but for that the v2 apis need to be backward compatible.

We should clarify what type of attestation should be submitted pre- and post-electra, submitting SingleAttestation should also work for earlier forks, so I don't think this is an issue.

Copy link
Collaborator

@nflaig nflaig Sep 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since ethereum/consensus-specs#3900 now also updates the honest validator spec, wouldn't it make the most sense to submit whatever attestation format the validator works with, i.e. Attestation (phase0) for pre-electra and SingleAttestation post-electra. This means we don't need to do any transformation before gossiping the attestations.

- type: array
items:
$ref: '../../../beacon-node-oapi.yaml#/components/schemas/Electra.Attestation'
$ref: '../../../beacon-node-oapi.yaml#/components/schemas/SingleAttestation'
responses:
"200":
description: Attestations are stored in pool and broadcast on the appropriate subnet
Expand Down
2 changes: 2 additions & 0 deletions beacon-node-oapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,8 @@ components:
$ref: './types/validator.yaml#/Altair/SyncDuty'
SignedAggregateAndProof:
$ref: './types/validator.yaml#/SignedAggregateAndProof'
SingleAttestation:
$ref: './types/attestation.yaml#/SingleAttestation'
Attestation:
$ref: './types/attestation.yaml#/Attestation'
AttestationData:
Expand Down
17 changes: 17 additions & 0 deletions types/attestation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,23 @@ IndexedAttestation:
$ref: './primitive.yaml#/Signature'
description: "The BLS signature of the `IndexedAttestation`, created by the validator of the attestation."

SingleAttestation:
eserilev marked this conversation as resolved.
Show resolved Hide resolved
type: object
description: "The [`SingleAttestation`](https://github.com/ethereum/consensus-specs/blob/v1.3.0/specs/electra/beacon-chain.md#singleattestation) object from the CL spec."
Copy link
Collaborator

@nflaig nflaig Sep 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the spec reference results in a 404, need to wait for upstream PR to be part of a pre-release, or update it later on

required: [committee_index, attester_index, data, signature]
properties:
committee_index:
$ref: "./primitive.yaml#/Uint64"
description: "The attestations committee index."
attester_index:
$ref: "./primitive.yaml#/Uint64"
description: "The validator index that signed this attestation."
data:
$ref: './attestation_data.yaml#/AttestationData'
signature:
$ref: './primitive.yaml#/Signature'
description: "BLS aggregate signature."

Attestation:
type: object
description: "The [`Attestation`](https://github.com/ethereum/consensus-specs/blob/v1.3.0/specs/phase0/beacon-chain.md#attestation) object from the CL spec."
Expand Down