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

Ensure GET v2/validator/aggregate_attestation is backwards compatible #6984

Conversation

eserilev
Copy link
Collaborator

Issue Addressed

Closes #6983

Proposed Changes

GET v2/validator/aggregate_attestation is not backwards compatible. It only works for post electra attestations. This PR adds backwards compatibility and additional test coverage. We should include this in the upcoming 7.0 beta release if possible

@eserilev eserilev added electra Required for the Electra/Prague fork bug Something isn't working v7.0.0-beta.0 New release c. Q1 2025 HTTP-API labels Feb 11, 2025
@eserilev eserilev changed the base branch from unstable to release-v7.0.0-beta.0 February 11, 2025 10:08
@eserilev eserilev requested a review from jxs as a code owner February 11, 2025 10:08
@eserilev eserilev force-pushed the aggregate-attestation-v2-pre-electra branch from 8ea91af to a57f64c Compare February 11, 2025 10:10
@eserilev eserilev added the ready-for-review The code is ready for review label Feb 11, 2025
Copy link
Member

@pawanjay176 pawanjay176 left a comment

Choose a reason for hiding this comment

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

This looks good to me.
I tested it by using v2 for both deneb and electra by modifying the lighthouse VC here

beacon_node
.get_validator_aggregate_attestation_v2(
attestation_data.slot,
attestation_data.tree_hash_root(),
committee_index,
)
.await
.map_err(|e| {
format!("Failed to produce an aggregate attestation: {:?}", e)
})?
.ok_or_else(|| format!("No aggregate available for {:?}", attestation_data))
.map(|result| result.data)

It works as expected. Wondering if we v2 all across.

@michaelsproul
Copy link
Member

It works as expected. Wondering if we v2 all across.

I think at this late stage I'm fine with sticking to v1 prior to Electra. We could consider changing this for v7.0.0 proper though

@michaelsproul michaelsproul added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Feb 11, 2025
@michaelsproul
Copy link
Member

@mergify queue

Copy link

mergify bot commented Feb 11, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at ed8086c

@mergify mergify bot merged commit ed8086c into sigp:release-v7.0.0-beta.0 Feb 12, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working electra Required for the Electra/Prague fork HTTP-API ready-for-merge This PR is ready to merge. v7.0.0-beta.0 New release c. Q1 2025
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Electra /eth/v2/validator/aggregate_attestation returning 404s for DENEB
3 participants