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 7 commits into
base: master
Choose a base branch
from
Open

Update submitPoolAttestationsV2 endpoint #472

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 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
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
12 changes: 4 additions & 8 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.

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

Expand All @@ -76,13 +76,9 @@ post:
content:
application/json:
schema:
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'
- type: array
items:
$ref: '../../../beacon-node-oapi.yaml#/components/schemas/SingleAttestation'
responses:
"200":
description: Attestations are stored in pool and broadcast on the appropriate subnet
Expand Down
6 changes: 6 additions & 0 deletions apis/eventstream/index.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ get:
- block
- block_gossip
- attestation
- single_attestation
- voluntary_exit
- bls_to_execution_change
- proposer_slashing
Expand Down Expand Up @@ -66,6 +67,11 @@ get:
value: |
event: attestation
data: {"aggregation_bits":"0x01", "signature":"0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505", "data":{"slot":"1", "index":"1", "beacon_block_root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2", "source":{"epoch":"1", "root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"}, "target":{"epoch":"1", "root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"}}}
single_attestation:
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if it's not better to simply add the validator index as a stand-alone field here, so that both index and committee list are available to consumers - ideally, this would be introduced in a way that doesn't require downstream tooling to change.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ideally, this would be introduced in a way that doesn't require downstream tooling to change.

No matter if we go with ethereum/consensus-specs#3900 or not, there will be an impact on downstream consumers since the Attestation is modified in electra.

For single_attestation it would be best to start emitting it even pre-electra (by converting phase0.Attestation to SingleAttestation) so that consumers can switch to the new topic earlier.

Overall, it seems like the cleanest solution would be to also go with ethereum/consensus-specs#3787 and AggregatedAttestation (as noted in ethereum/consensus-specs#3900 (comment)). We could then add two new topics onchain_attestation and aggregate_attestation and completely deprecate attestation post-electra.

This seems cleaner from a consumer point of view as well, you can always subscribe to multiple topics if you want to track all kind of attestations but as it is right now, the attestation topic always emits all 3 different types of attestations which is not great imo.

Copy link
Author

@eserilev eserilev Oct 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I understand it, we need access to the state in order to convert between SingleAttestation to Attestation. So emitting an Attestation event for this new endpoint seems to add complexity

Copy link
Collaborator

Choose a reason for hiding this comment

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

access to the state in order to convert between SingleAttestation to Attestation

you only need access to shuffling but this is already required to validate Attestation

description: The node has received a SingleAttestation (from P2P or API) that passes validation rules of the `beacon_attestation_{subnet_id}` topic
value: |
event: single_attestation
data: {"committee_index":"1", "attester_index": "1", "signature":"0x1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505cc411d61252fb6cb3fa0017b679f8bb2305b26a285fa2737f175668d0dff91cc1b66ac1fb663c9bc59509846d6ec05345bd908eda73e670af888da41af171505", "data":{"slot":"1", "index":"1", "beacon_block_root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2", "source":{"epoch":"1", "root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"}, "target":{"epoch":"1", "root":"0xcf8e0d4e9587369b2301d0790347320302cc0943d5a1884560367e8208d920f2"}}}
eserilev marked this conversation as resolved.
Show resolved Hide resolved
voluntary_exit:
description: The node has received a SignedVoluntaryExit (from P2P or API) that passes validation rules of `voluntary_exit` topic
value: |
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
Loading