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.

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

Minimal single attestation #8676

Closed
wants to merge 12 commits into from

Conversation

tbenr
Copy link
Contributor

@tbenr tbenr commented Oct 3, 2024

This spike seems to work now.

It implements the minimal impact strategy, where we perform SingleAttestation<->Attestation conversion to minimize changes.

In particular:

  • AttestationSubnetSubscriptions uses SingleAttestation schema when creates the electra single attestation topic handler
  • in ValidatableAttestation:
    • attestaiton become modifiable (to cover the conversion transparently)
    • optionally holds SingleAttestation version of the attestation (to be used when gossiping)
  • during attestation validation, if we are dealing with a SingleAttestation, we:
    • change the validation so that we verify signature before checking committee (which triggers the shuffling calculation)
    • convert SingleAttestation attestation to Attestation and cache the original (happening inside ValidatableAttestation)
    • keep creating IndexedAttestation from SingleAttestation but I doubt it is useful
  • during attestation validation, if we are dealing with a non aggregated Attestation produced locally, we:
    • convert Attestation attestation to SingleAttestation (happening inside ValidatableAttestation)
  • in AttestationGossipManager, when publishing unaggregated attestation we get it via validatableAttestation.getSingleAttestation() which:
    • pre-electra will give back the phase0 standard Attestation
    • from electra will give back SingleAttestation as result of the conversion happened during validation.

ethereum/consensus-specs#3900

fixes #8804

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@tbenr tbenr added the DO NOT MERGE Not ready to merge label Oct 4, 2024
@tbenr tbenr force-pushed the minimal-SingleAttestation branch 3 times, most recently from 6c3c34e to 4a4bef9 Compare October 9, 2024 10:08
@tbenr tbenr mentioned this pull request Oct 23, 2024
2 tasks
@tbenr tbenr force-pushed the minimal-SingleAttestation branch 2 times, most recently from 5690d77 to 20225aa Compare November 7, 2024 14:46
@tbenr tbenr force-pushed the minimal-SingleAttestation branch from 20225aa to c5e8836 Compare November 21, 2024 13:11
@tbenr tbenr force-pushed the minimal-SingleAttestation branch from c5e8836 to 0dac48b Compare November 22, 2024 13:27
@tbenr
Copy link
Contributor Author

tbenr commented Dec 4, 2024

closing in favour of #8884

@tbenr tbenr closed this Dec 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DO NOT MERGE Not ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce SingleAttestation
1 participant