Update submitPoolAttestationsV2 endpoint #472

eserilev · 2024-09-16T01:56:48Z

If the decision is made to introduce a SingleAttestation object via ethereum/consensus-specs#3900 it may make more sense for submitPoolAttestationsV2 to accept a list of SingleAttestations instead of a list of Attestation objects.

nflaig

When submitting an attestation via api or receiving one on p2p we also emit an event. We could consider adding a new single_attestation event to accommodate ethereum/consensus-specs#3900 or convert to Attestation and use existing event, for backward compatibility, we would likely need to emit both events.

beacon-APIs/apis/eventstream/index.yaml

Lines 64 to 65 in ca2f9a8

    
                       attestation: 
        
                         description: The node has received an Attestation (from P2P or API) that passes validation rules of the `beacon_attestation_{subnet_id}` topic

nflaig · 2024-09-16T10:11:57Z

apis/beacon/pool/attestations.v2.yaml

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


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

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?

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.

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.

nflaig · 2024-09-16T10:12:33Z

types/attestation.yaml

@@ -15,6 +15,23 @@ IndexedAttestation:
      $ref: './primitive.yaml#/Signature'
      description: "The BLS signature of the `IndexedAttestation`, created by the validator of the attestation."

+SingleAttestation:
+  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."


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

dapplion · 2024-09-17T09:32:36Z

I want to echo @eserilev concerns of the non-simplicity of this change here too, see

Separate type for unaggregated network attestations consensus-specs#3900 (comment)

types/attestation.yaml

arnetheduck · 2024-10-04T10:24:47Z

apis/eventstream/index.yaml

@@ -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:


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.

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.

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

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

nflaig · 2024-10-05T10:25:21Z

apis/eventstream/index.yaml

@@ -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:


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.

apis/eventstream/index.yaml

update post attestation v2 endpoint

7203294

eserilev mentioned this pull request Sep 16, 2024

Separate type for unaggregated network attestations ethereum/consensus-specs#3900

Open

nflaig reviewed Sep 16, 2024

View reviewed changes

remove anyOf

c581a51

Scutua approved these changes Sep 16, 2024

View reviewed changes

add a new single_attestation event

508487a

nflaig reviewed Sep 20, 2024

View reviewed changes

types/attestation.yaml Outdated Show resolved Hide resolved

Mahmoudhpp approved these changes Sep 20, 2024

View reviewed changes

reintroduce anyOf, move type def to electra

085e16a

arnetheduck reviewed Oct 4, 2024

View reviewed changes

nflaig reviewed Oct 5, 2024

View reviewed changes

poojaranjan mentioned this pull request Oct 8, 2024

Ethereum Cat Herders Meeting #139 ethcatherders/PM#416

Open

3 tasks

eserilev and others added 3 commits October 8, 2024 21:22

move Signature field to match spec

f2f7969

Merge branch 'master' into post_attestation_v2

997ddb7

Lint

1df7423

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update submitPoolAttestationsV2 endpoint #472

Update submitPoolAttestationsV2 endpoint #472

eserilev commented Sep 16, 2024

nflaig left a comment

nflaig Sep 16, 2024

eserilev Sep 16, 2024 •

edited

Loading

nflaig Sep 17, 2024 •

edited

Loading

nflaig Sep 20, 2024 •

edited

Loading

nflaig Sep 16, 2024 •

edited

Loading

dapplion commented Sep 17, 2024

arnetheduck Oct 4, 2024

nflaig Oct 5, 2024

eserilev Oct 14, 2024 •

edited

Loading

nflaig Oct 14, 2024

nflaig Oct 5, 2024

	attestation:
	description: The node has received an Attestation (from P2P or API) that passes validation rules of the `beacon_attestation_{subnet_id}` topic

Update submitPoolAttestationsV2 endpoint #472

Are you sure you want to change the base?

Update submitPoolAttestationsV2 endpoint #472

Conversation

eserilev commented Sep 16, 2024

nflaig left a comment

Choose a reason for hiding this comment

nflaig Sep 16, 2024

Choose a reason for hiding this comment

eserilev Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

nflaig Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

nflaig Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

nflaig Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

dapplion commented Sep 17, 2024

arnetheduck Oct 4, 2024

Choose a reason for hiding this comment

nflaig Oct 5, 2024

Choose a reason for hiding this comment

eserilev Oct 14, 2024 • edited Loading

Choose a reason for hiding this comment

nflaig Oct 14, 2024

Choose a reason for hiding this comment

nflaig Oct 5, 2024

Choose a reason for hiding this comment

eserilev Sep 16, 2024 •

edited

Loading

nflaig Sep 17, 2024 •

edited

Loading

nflaig Sep 20, 2024 •

edited

Loading

nflaig Sep 16, 2024 •

edited

Loading

eserilev Oct 14, 2024 •

edited

Loading