Update submitPoolAttestationsV2 endpoint#472
Conversation
nflaig
left a comment
nflaig
left a comment
There was a problem hiding this comment.
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
| anyOf: | ||
| - type: array | ||
| items: | ||
| $ref: '../../../beacon-node-oapi.yaml#/components/schemas/Attestation' |
There was a problem hiding this comment.
so the idea is to always submit SingleAttestation even pre-Electra? in this case should remove the anyOf
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
types/attestation.yaml
Outdated
|
|
||
| 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." |
There was a problem hiding this comment.
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
|
I want to echo @eserilev concerns of the non-simplicity of this change here too, see |
| 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: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
| 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: |
There was a problem hiding this comment.
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.
Co-authored-by: Nico Flaig <[email protected]>
9 participants
If the decision is made to introduce a
SingleAttestationobject via ethereum/consensus-specs#3900 it may make more sense forsubmitPoolAttestationsV2to accept a list ofSingleAttestationsinstead of a list ofAttestationobjects.