8424 attestations handler #8456

mehdi-aouadi · 2024-07-19T11:36:25Z

PR Description

Add PostAttestationsRequest for the new post attestations V2 API (remote validator api handler).
The new sendSignedAttesationsV2 isn't used in production code yet. A hidden option will be added in a separate PR in order to activate it progressively (same way for block v3).
The sendSignedAttestations is deprecated and should be progressively replaced by the new sendSignedAttestationsV2.

Fixed Issue(s)

#8424

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.

greptile-apps

PR Summary

Added sendSignedAttestationsV2 method in beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java
Introduced new tests for sendSignedAttestationsV2 in beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandlerTest.java
Added JSON schema for /eth/v2/beacon/pool/attestations in data/beaconrestapi/src/integration-test/resources/tech/pegasys/teku/beaconrestapi/beacon/paths/_eth_v2_beacon_pool_attestations.json
Introduced PostAttestationV2 endpoint in data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/PostAttestationV2.java
Added unit tests for PostAttestationV2 in data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/PostAttestationV2Test.java

_{26 file(s) reviewed, 4 comment(s)}
_{Edit PR Review Bot Settings}

.../java/tech/pegasys/teku/validator/beaconnode/metrics/MetricRecordingValidatorApiChannel.java

...emote/src/main/java/tech/pegasys/teku/validator/remote/sentry/SentryValidatorApiChannel.java

...c/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java

beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java

...tor/remote/src/main/java/tech/pegasys/teku/validator/remote/FailoverValidatorApiHandler.java

greptile-apps

PR Summary

(updates since last review)

Added sendSignedAttestationsV2 method with SpecMilestone parameter in validator/beaconnode/src/main/java/tech/pegasys/teku/validator/beaconnode/metrics/MetricRecordingValidatorApiChannel.java
Removed unnecessary imports and redundant method in validator/remote/src/integration-test/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequestTest.java
Introduced sendSignedAttestationsV2 method with SpecMilestone parameter in validator/remote/src/main/java/tech/pegasys/teku/validator/remote/sentry/SentryValidatorApiChannel.java

_{3 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Added sendSignedAttestationsV2 method with SpecMilestone parameter in beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java
Added new test cases for sendSignedAttestationsV2 in beacon/validator/src/test/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandlerTest.java
Introduced PostAttestationsV2IntegrationTest for new API endpoint in data/beaconrestapi/src/integration-test/java/tech/pegasys/teku/beaconrestapi/v2/beacon/PostAttestationsV2IntegrationTest.java
Updated JsonTypeDefinitionBeaconRestApi.java to use PostAttestationsV2
Renamed and updated PostAttestationsV2.java and PostAttestationsV2Test.java to reflect new API endpoint

_{17 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps · 2024-07-19T13:43:59Z

...c/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java

+    this.spec = spec;
+  }
+
+  public Optional<PostDataFailureResponse> postAttestations(


Logic: Ensure that the attestations list is not empty before proceeding with the request.

greptile-apps · 2024-07-19T13:44:00Z

...c/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java

+        Map.of(HEADER_CONSENSUS_VERSION, specMilestone.name().toLowerCase(Locale.ROOT)),
+        attestations,
+        attestationsListTypeDefinition,
+        new ResponseHandler<>());


Logic: Handle potential exceptions from the postJson method to avoid unhandled runtime errors.

greptile-apps

PR Summary

(updates since last review)

Introduced sendSignedAttestationsV2 method in beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java
Deprecated sendSignedAttestations method in beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java
sendSignedAttestationsV2 processes attestations individually and converts results into a list of errors
New method not yet used in production; activation via hidden option in a future PR

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

greptile-apps

PR Summary

(updates since last review)

Added new REST API endpoint /eth/v2/beacon/pool/attestations in data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/PostAttestationsV2.java
Updated tests for new API in data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/PostAttestationsV2Test.java
Introduced submitAttestationsV2 method in data/provider/src/main/java/tech/pegasys/teku/api/ValidatorDataProvider.java
Ensure SpecMilestone header is correctly provided and handled
Thorough testing recommended for new API and method

_{3 file(s) reviewed, no comment(s)}
_{Edit PR Review Bot Settings}

greptile-apps

PR Summary

(updates since last review)

Updated data/beaconrestapi/src/integration-test/java/tech/pegasys/teku/beaconrestapi/v2/beacon/PostAttestationsV2IntegrationTest.java to use sendSignedAttestationsV2
Tests now cover successful posts, partial failures, and missing/incorrect consensus headers

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

greptile-apps

PR Summary

(updates since last review)

The pull request introduces the new PostAttestation V2 API to handle Electra attestations, along with several updates and refactors across the codebase to support this new functionality.

Added data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/PostAttestationsV2.java to handle POST requests for submitting attestation objects.
Updated data/provider/src/main/java/tech/pegasys/teku/api/ValidatorDataProvider.java to include a new method createAggregateAndMetaData for the new API.
Introduced data/beaconrestapi/src/test/java/tech/pegasys/teku/beaconrestapi/handlers/v2/beacon/PostAttestationsV2Test.java to cover various scenarios for the new handler.
Modified validator/remote/src/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java to support the new API endpoint.
Updated data/beaconrestapi/src/integration-test/java/tech/pegasys/teku/beaconrestapi/v2/beacon/PostAttestationsV2IntegrationTest.java to use sendSignedAttestationsV2 and cover successful posts, partial failures, and missing/incorrect consensus headers.

_{207 file(s) reviewed, 2 comment(s)}
_{Edit PR Review Bot Settings}

...c/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java

rolfyone · 2024-08-22T02:13:04Z

.../java/tech/pegasys/teku/validator/remote/typedef/handlers/SendSignedAttestationsRequest.java

  public SendSignedAttestationsRequest(
      final HttpUrl baseEndpoint, final OkHttpClient okHttpClient) {
    super(baseEndpoint, okHttpClient);
  }

-  public List<SubmitDataError> submit(final List<Attestation> attestations) {
+  public List<SubmitDataError> submit(final List<Attestation> attestations, final Spec spec) {


i'm lost on prs they all look so similar, but in teh same way i think maybe spec in the constructor is probably better....

It's the same pattern but different APIs that's why they look the same :)
Anyway I refactored the request to pass the spec in the constructor. Left the attestations to the submit method though because we have to check if the list is empty and get the first element to get the slot and the milestone otherwise...

…estationsRequest class - removed an endpoint that had 1 usage in the AbstractTypeDefRequest class- if we're adding a parameter to a fn thats used in one place i'd prefer we dont make a new fn. - new test class for electra attestations (renamed the post test class) Signed-off-by: Paul Harris <[email protected]>

.../java/tech/pegasys/teku/validator/remote/typedef/handlers/SendSignedAttestationsRequest.java

tbenr

generally ok, need to clarify one thing

tbenr

LGTM

mehdi-aouadi self-assigned this Jul 19, 2024

mehdi-aouadi added the EIP7549 label Jul 19, 2024

greptile-apps bot reviewed Jul 19, 2024

View reviewed changes

github-advanced-security bot found potential problems Jul 19, 2024

View reviewed changes

beacon/validator/src/main/java/tech/pegasys/teku/validator/coordinator/ValidatorApiHandler.java Fixed Show fixed Hide fixed

...tor/remote/src/main/java/tech/pegasys/teku/validator/remote/FailoverValidatorApiHandler.java Fixed Show fixed Hide fixed

greptile-apps bot reviewed Jul 19, 2024

View reviewed changes

mehdi-aouadi force-pushed the 8424-attestations-handler branch from a3551d6 to 8e97d97 Compare July 19, 2024 13:34

greptile-apps bot reviewed Jul 19, 2024

View reviewed changes

mehdi-aouadi force-pushed the 8424-attestations-handler branch 2 times, most recently from 74dde7e to 1eab44e Compare August 1, 2024 10:14

mehdi-aouadi marked this pull request as draft August 1, 2024 19:52

mehdi-aouadi marked this pull request as ready for review August 2, 2024 14:58

greptile-apps bot reviewed Aug 2, 2024

View reviewed changes

...c/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java Outdated Show resolved Hide resolved

...c/main/java/tech/pegasys/teku/validator/remote/typedef/handlers/PostAttestationsRequest.java Outdated Show resolved Hide resolved

mehdi-aouadi force-pushed the 8424-attestations-handler branch from 1315851 to fa1bc35 Compare August 2, 2024 18:59

mehdi-aouadi force-pushed the 8424-attestations-handler branch 2 times, most recently from 9a5aa5d to b0ae3eb Compare August 14, 2024 13:15

mehdi-aouadi requested a review from rolfyone August 16, 2024 07:33

mehdi-aouadi mentioned this pull request Aug 21, 2024

Post aggregate and proofs request #8523

Merged

2 tasks

rolfyone reviewed Aug 22, 2024

View reviewed changes

mehdi-aouadi force-pushed the 8424-attestations-handler branch from 55cea7b to 002096f Compare August 22, 2024 12:04

mehdi-aouadi requested a review from rolfyone August 22, 2024 12:08

mehdi-aouadi added 7 commits August 22, 2024 16:19

wip array type definition

93552c4

Add post attestation v2 interface

b6aa8b1

rename json files

f600120

refactoring

f2fb1fc

add integration tests

0963a99

Add post attestation v2 interface

4ecf8d5

add missing experimental tag

f5e61d1

mehdi-aouadi and others added 22 commits August 22, 2024 16:26

add missing json files

e1c7e97

add missing json files

dd21080

rename json files

eb29c1b

refactoring

a254b2f

spotless

b7920b2

add missing changes

0aeacb9

add unit tests

65074a8

add unit test

8efb8f9

fix rebase

e3ea6d8

refactoring

ff593c4

rebase

10b7dda

add empty list check

78e377f

remove unused PostAttestationsRequest

5d7b8e9

refactor pre/post electra

9a67a4e

use spec when submit

06f2634

fix test

8e5675d

refactor test

0d5a8e0

add missing final

490888f

refactor send attestation request

393bd95

rename method

b8d612f

refactoring

e2a9394

mehdi-aouadi force-pushed the 8424-attestations-handler branch from a0c5708 to e2a9394 Compare August 22, 2024 14:31

mehdi-aouadi requested a review from tbenr August 26, 2024 08:37

tbenr reviewed Aug 27, 2024

View reviewed changes

.../java/tech/pegasys/teku/validator/remote/typedef/handlers/SendSignedAttestationsRequest.java Show resolved Hide resolved

tbenr reviewed Aug 27, 2024

View reviewed changes

tbenr approved these changes Aug 27, 2024

View reviewed changes

Merge branch 'master' into 8424-attestations-handler

16edc92

mehdi-aouadi merged commit e4c973f into Consensys:master Aug 27, 2024
17 checks passed

mehdi-aouadi deleted the 8424-attestations-handler branch August 27, 2024 14:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8424 attestations handler #8456

8424 attestations handler #8456

mehdi-aouadi commented Jul 19, 2024 •

edited

Loading

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot Jul 19, 2024

greptile-apps bot Jul 19, 2024

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot left a comment

greptile-apps bot left a comment

rolfyone Aug 22, 2024

mehdi-aouadi Aug 22, 2024 •

edited

Loading

tbenr left a comment

tbenr left a comment

8424 attestations handler #8456

8424 attestations handler #8456

Conversation

mehdi-aouadi commented Jul 19, 2024 • edited Loading

PR Description

Fixed Issue(s)

Documentation

Changelog

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot Jul 19, 2024

Choose a reason for hiding this comment

greptile-apps bot Jul 19, 2024

Choose a reason for hiding this comment

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

greptile-apps bot left a comment

Choose a reason for hiding this comment

PR Summary

rolfyone Aug 22, 2024

Choose a reason for hiding this comment

mehdi-aouadi Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

tbenr left a comment

Choose a reason for hiding this comment

tbenr left a comment

Choose a reason for hiding this comment

mehdi-aouadi commented Jul 19, 2024 •

edited

Loading

mehdi-aouadi Aug 22, 2024 •

edited

Loading