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

PART 1 - GET/Attestation Pool API - Handle Electra attestations in the aggregating pool #8431

Merged

Conversation

mehdi-aouadi
Copy link
Contributor

@mehdi-aouadi mehdi-aouadi commented Jul 9, 2024

PR Description

In preparation for the GET attestation from the aggregating attestation pool API.
Handle the Electra attestations when querying attestations from the aggregation pool

Fixed Issue(s)

#8029

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.

@mehdi-aouadi mehdi-aouadi self-assigned this Jul 9, 2024
@mehdi-aouadi mehdi-aouadi force-pushed the 8029-1-aggregating-attestation-pool branch from 1c1679c to cecf11c Compare July 9, 2024 16:23
@mehdi-aouadi mehdi-aouadi marked this pull request as draft July 9, 2024 20:31
@mehdi-aouadi mehdi-aouadi force-pushed the 8029-1-aggregating-attestation-pool branch 2 times, most recently from 1877c1a to f5509c7 Compare July 10, 2024 14:12
@mehdi-aouadi mehdi-aouadi marked this pull request as ready for review July 10, 2024 14:16
@mehdi-aouadi mehdi-aouadi requested a review from tbenr July 10, 2024 14:48
@mehdi-aouadi mehdi-aouadi force-pushed the 8029-1-aggregating-attestation-pool branch from f4a288c to bc75655 Compare July 12, 2024 09:21
@mehdi-aouadi mehdi-aouadi force-pushed the 8029-1-aggregating-attestation-pool branch from bc75655 to 2c6637a Compare July 16, 2024 12:02
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

  • Added handling for Electra attestations in ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPool.java
  • Introduced logic to filter attestations requiring committee bits
  • Ensured correct schema definitions based on slot
  • Added test cases for Electra attestations in ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java
  • Refactored methods and added mock behaviors for different spec milestones

2 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@mehdi-aouadi mehdi-aouadi requested a review from rolfyone July 16, 2024 12:04
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Synchronized getSchemaDefinitions method in ethereum/statetransition/src/main/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPool.java
  • Ensured thread safety for handling Electra attestations
  • No major changes found since last review

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Signed-off-by: Paul Harris <paul.harris@consensys.net>
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Added AttestationElectra class for handling Electra attestations
  • Implemented AttestationElectraSchema for Electra attestation structure
  • Included committeeBits field in AttestationElectra
  • Ensured committeeBits are required for Electra attestations
  • Updated create methods to handle Electra-specific attestation creation

No file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Removed limit on attestations returned from getAttestations in AggregatingAttestationPool
  • Added filtering for attestations requiring committee bits
  • Potential increase in number of attestations returned by getAttestations

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@mehdi-aouadi mehdi-aouadi requested a review from tbenr July 18, 2024 08:49
Copy link
Contributor

@tbenr tbenr left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Refactored waitForNonDefaultExecutionPayload in acceptance-tests/src/testFixtures/java/tech/pegasys/teku/test/acceptance/dsl/TekuBeaconNode.java for readability.
  • Updated getter methods in beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java for SyncCommitteeSubnetSubscription.
  • Introduced PostSyncCommitteeData and SyncCommitteeSubnetSubscription classes in ethereum/json-types/src/main/java/tech/pegasys/teku/ethereum/json/types/validator.
  • Added IPv6 support in networking/p2p/src/main/java/tech/pegasys/teku/networking/p2p/discovery/discv5/DiscV5Service.java.
  • Updated state pruning configurations in storage/src/main/java/tech/pegasys/teku/storage/server/StorageConfiguration.java.

32 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@mehdi-aouadi mehdi-aouadi enabled auto-merge (squash) July 19, 2024 08:56
Copy link

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

  • Updated getAttestations_shouldReturnElectraAttestationsOnly_whenElectraActivatesAndNoSlotProvided method name and comments in ethereum/statetransition/src/test/java/tech/pegasys/teku/statetransition/attestation/AggregatingAttestationPoolTest.java
  • Ensure test cases reflect handling of Electra attestations in the aggregation pool

1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings

@mehdi-aouadi mehdi-aouadi merged commit 135d618 into Consensys:master Jul 19, 2024
15 of 16 checks passed
@mehdi-aouadi mehdi-aouadi deleted the 8029-1-aggregating-attestation-pool branch July 19, 2024 09:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants