Add `/eth/v2/validator/aggregate_attestation` #14481

saolyn · 2024-09-25T14:55:13Z

What type of PR is this?

Other

What does this PR do? Why is it needed?

Beacon API Electra alignment, add missing endpoint for /eth/v2/validator/aggregate_attestation.
As described in the spec https://ethereum.github.io/beacon-APIs/?urls.primaryName=dev#/Validator/getAggregatedAttestationV2

Which issues(s) does this PR fix?

Part of #14476

Other notes for review

Acknowledgements

I have read CONTRIBUTING.md.
I have made an appropriate entry to CHANGELOG.md.
I have added a description to this PR with sufficient context for reviewers to understand this PR.

beacon-chain/rpc/eth/validator/handlers.go

api/server/structs/endpoints_validator.go

james-prysm · 2024-10-01T19:17:08Z

beacon-chain/rpc/eth/validator/handlers.go

+			return nil
+		}
+	}
+	if match == nil {


this should be moved inside the first match check right?

doesn't matter too much because we assign to an existing variable

beacon-chain/rpc/eth/validator/handlers.go

james-prysm · 2024-10-01T19:19:24Z

beacon-chain/rpc/eth/validator/handlers.go

-func matchingAtt(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byte) (ethpbalpha.Att, error) {
+// GetAggregateAttestationV2 aggregates all attestations matching the given attestation data root and slot, returning the aggregated result.
+func (s *Server) GetAggregateAttestationV2(w http.ResponseWriter, r *http.Request) {
+	_, span := trace.StartSpan(r.Context(), "validator.GetAggregateAttestation")


maybe should update validator.GetAggregateAttestationV2

CHANGELOG.md

api/server/structs/endpoints_validator.go

rkapka · 2024-10-02T08:46:16Z

beacon-chain/rpc/eth/validator/handlers.go

+	if !ok {
+		return
+	}
+


Suggested change

rkapka · 2024-10-02T08:46:21Z

beacon-chain/rpc/eth/validator/handlers.go

+	if !ok {
+		return
+	}
+


Suggested change

rkapka · 2024-10-02T08:56:04Z

beacon-chain/rpc/eth/validator/handlers.go

+			return nil
+		}
+	}
+	if match == nil {


doesn't matter too much because we assign to an existing variable

beacon-chain/rpc/eth/validator/handlers.go

rkapka · 2024-10-02T09:32:59Z

I just noticed that this endpoint never worked correctly. The description says

Aggregates all attestations matching given attestation data root and slot.

This means that if we don't have a matching aggregated attestation, but we have a matching unaggregated one, we have to aggregate these attestations. I think this is a good moment to fix this.

beacon-chain/rpc/eth/validator/handlers.go

rkapka · 2024-10-10T08:59:34Z

validator/client/beacon-api/submit_aggregate_selection_proof.go

@@ -52,7 +53,12 @@ func (c *beaconApiValidatorClient) submitAggregateSelectionProof(
 		return nil, err
 	}

-	aggregatedAttestation, err := convertAttestationToProto(aggregateAttestationResponse.Data)
+	var attData *ethpb.Attestation // Replace with your appropriate struct


You should unmarshal into the API type, not the protobuf one

Not sure how to get it working with the API type

beacon-chain/rpc/eth/validator/handlers.go

prestonvanloon · 2024-10-14T15:27:03Z

beacon-chain/rpc/eth/validator/handlers.go

+	_, attDataRoot, ok := shared.HexFromQuery(w, r, "attestation_data_root", fieldparams.RootLength, true)
+	if !ok {
+		return
+	}
+	_, slot, ok := shared.UintFromQuery(w, r, "slot", true)
+	if !ok {
+		return
+	}
+	_, index, ok := shared.UintFromQuery(w, r, "committee_index", true)
+	if !ok {
+		return
+	}


These should return a 400 / bad request since they are required fields.

They already do, within shared.UintFromQuery we have ValidateUint which handles the http error, same thing applies to shared.HexFromQuery

beacon-chain/rpc/eth/validator/handlers.go

prestonvanloon · 2024-10-14T15:28:26Z

beacon-chain/rpc/eth/validator/handlers.go

 }

-func matchingAtt(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byte) (ethpbalpha.Att, error) {
+func matchingAtt(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byte, index string) (ethpbalpha.Att, error) {


If there are multiple that match, return the best one by most CommitteeBitsVal().Count(), assuming they cannot be further aggregated.

Or consider returning all matching atts.

func matchingAtts(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byte, index string) ([]ethpbalpha.Att, error)

prestonvanloon · 2024-10-14T15:31:19Z

beacon-chain/rpc/eth/validator/handlers.go

+			httputil.HandleError(w, "No matching attestation found", http.StatusNotFound)
+			return nil
+		}
+		_, err = attestations.Aggregate([]ethpbalpha.Att{match})


Suggested change

_, err = attestations.Aggregate([]ethpbalpha.Att{match})

agg, err = attestations.Aggregate([]ethpbalpha.Att{matches})

...

return agg[0]

If they cannot be aggregate to just one (len(agg) == 1), then return best by committee bits count.

rkapka · 2024-10-15T15:50:05Z

beacon-chain/rpc/eth/validator/handlers.go

 }

-func matchingAtt(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byte) (ethpbalpha.Att, error) {
+func matchingAtts(atts []ethpbalpha.Att, slot primitives.Slot, attDataRoot []byte, index primitives.CommitteeIndex) ([]ethpbalpha.Att, error) {
+	if len(atts) == 0 {


So that we don't panic in the declaration of postElectra

rkapka · 2024-10-15T15:50:38Z

beacon-chain/rpc/eth/validator/handlers.go

+	}
+	typedAgg, ok := agg.(*ethpbalpha.Attestation)
+	if !ok {
+		httputil.HandleError(w, fmt.Sprintf("Attestation is not of type %T", &ethpbalpha.Attestation{}), http.StatusInternalServerError)


%T with an empty struct is nicer in my opinion because if the type name changes, you will get a compiler error

saolyn · 2024-10-16T08:55:28Z

beacon-chain/rpc/eth/validator/handlers.go

+
+	match, err := matchingAtts(s.AttestationsPool.AggregatedAttestations(), slot, attDataRoot, index)
+	if err != nil {
+		httputil.HandleError(w, "Could not get matching attestation: "+err.Error(), http.StatusInternalServerError)


Nit

Suggested change

httputil.HandleError(w, "Could not get matching attestation: "+err.Error(), http.StatusInternalServerError)

httputil.HandleError(w, "Could not get matching attestations: "+err.Error(), http.StatusInternalServerError)

rkapka

tests are incorrect

rkapka · 2024-10-18T10:19:17Z

beacon-chain/rpc/eth/validator/handlers_test.go

+		},
+	}
+}
+
 func TestGetAggregateAttestation_SameSlotAndRoot_ReturnMostAggregationBits(t *testing.T) {


This test can be removed because it's a duplicate of one of the new test cases

saolyn added 3 commits September 25, 2024 16:53

add endpoint

97aa3ec

changelog

f9903c4

fix tests

cbd5dd4

saolyn commented Sep 25, 2024

View reviewed changes

beacon-chain/rpc/eth/validator/handlers.go Outdated Show resolved Hide resolved

saolyn marked this pull request as ready for review September 25, 2024 15:45

saolyn requested a review from a team as a code owner September 25, 2024 15:45

saolyn requested review from potuz, terencechain and rkapka September 25, 2024 15:45

saolyn and others added 3 commits September 30, 2024 15:50

fix endpoint

607cd31

Merge branch 'develop' into add-aggregateAttestationV2

fb8e325

remove useless broken code

f189e41

james-prysm reviewed Oct 1, 2024

View reviewed changes

api/server/structs/endpoints_validator.go Outdated Show resolved Hide resolved

james-prysm reviewed Oct 1, 2024

View reviewed changes

beacon-chain/rpc/eth/validator/handlers.go Outdated Show resolved Hide resolved

james-prysm reviewed Oct 1, 2024

View reviewed changes

rkapka requested changes Oct 2, 2024

View reviewed changes

saolyn added 3 commits October 9, 2024 19:32

review + fix endpoint

cb62038

gaz

f94403f

fix aggregate selection proof test

8f8ef67

rkapka reviewed Oct 10, 2024

View reviewed changes

rkapka requested changes Oct 10, 2024

View reviewed changes

fixes

99558f4

prestonvanloon reviewed Oct 14, 2024

View reviewed changes

new way of aggregating

3a4acbd

rkapka reviewed Oct 15, 2024

View reviewed changes

saolyn commented Oct 16, 2024

View reviewed changes

nit

f1a1591

saolyn and others added 5 commits October 16, 2024 11:55

fix part of the tests

55f78d9

fix tests

29fc4f7

Merge branch 'develop' into add-aggregateAttestationV2

3048541

cleanup

b980b91

fix AggSelectionProof test

8f88cb4

rkapka requested changes Oct 18, 2024

View reviewed changes

rkapka reviewed Oct 18, 2024

View reviewed changes

saolyn and others added 2 commits October 31, 2024 13:41

tests

a358698

v1 tests

3ee2e54

rkapka force-pushed the add-aggregateAttestationV2 branch from 090f1fb to 3ee2e54 Compare November 5, 2024 12:20

saolyn and others added 2 commits November 5, 2024 16:35

v2 tests

7d168da

Merge branch 'develop' into add-aggregateAttestationV2

ead84cd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `/eth/v2/validator/aggregate_attestation` #14481

Add `/eth/v2/validator/aggregate_attestation` #14481

saolyn commented Sep 25, 2024 •

edited

Loading

james-prysm Oct 1, 2024

rkapka Oct 2, 2024

james-prysm Oct 1, 2024

rkapka Oct 2, 2024

rkapka Oct 2, 2024

rkapka Oct 2, 2024

rkapka commented Oct 2, 2024 •

edited

Loading

rkapka Oct 10, 2024

saolyn Oct 10, 2024 •

edited

Loading

prestonvanloon Oct 14, 2024

saolyn Oct 16, 2024

prestonvanloon Oct 14, 2024

prestonvanloon Oct 14, 2024

rkapka Oct 15, 2024

rkapka Oct 15, 2024

saolyn Oct 16, 2024

rkapka left a comment

rkapka Oct 18, 2024

-		_, err = attestations.Aggregate([]ethpbalpha.Att{match})
+		agg, err = attestations.Aggregate([]ethpbalpha.Att{matches})
+		...
+		return agg[0]

	httputil.HandleError(w, "Could not get matching attestation: "+err.Error(), http.StatusInternalServerError)
	httputil.HandleError(w, "Could not get matching attestations: "+err.Error(), http.StatusInternalServerError)

Add /eth/v2/validator/aggregate_attestation #14481

Are you sure you want to change the base?

Add /eth/v2/validator/aggregate_attestation #14481

Conversation

saolyn commented Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka commented Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

saolyn Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkapka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Add `/eth/v2/validator/aggregate_attestation` #14481

Add `/eth/v2/validator/aggregate_attestation` #14481

saolyn commented Sep 25, 2024 •

edited

Loading

rkapka commented Oct 2, 2024 •

edited

Loading

saolyn Oct 10, 2024 •

edited

Loading