Skip to content

Commit

Permalink
Ensure GET v2/validator/aggregate_attestation is backwards compatib…
Browse files Browse the repository at this point in the history 
…le (#6984)

Closes #6983


  `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
  • Loading branch information
@eserilev
eserilev authored Feb 12, 2025
1 parent 0728140 commit ed8086c
Show file tree
Hide file tree
Showing 2 changed files with 77 additions and 54 deletions.
37 changes: 20 additions & 17 deletions beacon_node/http_api/src/aggregate_attestation.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,14 @@ pub fn get_aggregate_attestation<T: BeaconChainTypes>(
endpoint_version: EndpointVersion,
chain: Arc<BeaconChain<T>>,
) -> Result<Response<Body>, warp::reject::Rejection> {
if endpoint_version == V2 {
let fork_name = chain.spec.fork_name_at_slot::<T::EthSpec>(slot);
let aggregate_attestation = if fork_name.electra_enabled() {
let Some(committee_index) = committee_index else {
return Err(warp_utils::reject::custom_bad_request(
"missing committee index".to_string(),
));
};
let aggregate_attestation = chain
chain
.get_aggregated_attestation_electra(slot, attestation_data_root, committee_index)
.map_err(|e| {
warp_utils::reject::custom_bad_request(format!(
Expand All @@ -34,8 +35,22 @@ pub fn get_aggregate_attestation<T: BeaconChainTypes>(
})?
.ok_or_else(|| {
warp_utils::reject::custom_not_found("no matching aggregate found".to_string())
})?;
let fork_name = chain.spec.fork_name_at_slot::<T::EthSpec>(slot);
})?
} else {
chain
.get_pre_electra_aggregated_attestation_by_slot_and_root(slot, attestation_data_root)
.map_err(|e| {
warp_utils::reject::custom_bad_request(format!(
"unable to fetch aggregate: {:?}",
e
))
})?
.ok_or_else(|| {
warp_utils::reject::custom_not_found("no matching aggregate found".to_string())
})?
};

if endpoint_version == V2 {
let fork_versioned_response = ForkVersionedResponse {
version: Some(fork_name),
metadata: EmptyMetadata {},
Expand All @@ -46,19 +61,7 @@ pub fn get_aggregate_attestation<T: BeaconChainTypes>(
fork_name,
))
} else if endpoint_version == V1 {
let aggregate_attestation = chain
.get_pre_electra_aggregated_attestation_by_slot_and_root(slot, attestation_data_root)
.map_err(|e| {
warp_utils::reject::custom_bad_request(format!(
"unable to fetch aggregate: {:?}",
e
))
})?
.map(GenericResponse::from)
.ok_or_else(|| {
warp_utils::reject::custom_not_found("no matching aggregate found".to_string())
})?;
Ok(warp::reply::json(&aggregate_attestation).into_response())
Ok(warp::reply::json(&GenericResponse::from(aggregate_attestation)).into_response())
} else {
return Err(unsupported_version_rejection(endpoint_version));
}
Expand Down
94 changes: 57 additions & 37 deletions beacon_node/http_api/tests/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3555,44 +3555,48 @@ impl ApiTester {
}

#[allow(clippy::await_holding_lock)] // This is a test, so it should be fine.
pub async fn test_get_validator_aggregate_attestation(self) -> Self {
if self
pub async fn test_get_validator_aggregate_attestation_v1(self) -> Self {
let attestation = self
.chain
.spec
.fork_name_at_slot::<E>(self.chain.slot().unwrap())
.electra_enabled()
{
for attestation in self.chain.naive_aggregation_pool.read().iter() {
let result = self
.client
.get_validator_aggregate_attestation_v2(
attestation.data().slot,
attestation.data().tree_hash_root(),
attestation.committee_index().expect("committee index"),
)
.await
.unwrap()
.unwrap()
.data;
let expected = attestation;
.head_beacon_block()
.message()
.body()
.attestations()
.next()
.unwrap()
.clone_as_attestation();
let result = self
.client
.get_validator_aggregate_attestation_v1(
attestation.data().slot,
attestation.data().tree_hash_root(),
)
.await
.unwrap()
.unwrap()
.data;
let expected = attestation;

assert_eq!(&result, expected);
}
} else {
let attestation = self
.chain
.head_beacon_block()
.message()
.body()
.attestations()
.next()
.unwrap()
.clone_as_attestation();
assert_eq!(result, expected);

self
}

pub async fn test_get_validator_aggregate_attestation_v2(self) -> Self {
let attestations = self
.chain
.naive_aggregation_pool
.read()
.iter()
.cloned()
.collect::<Vec<_>>();
for attestation in attestations {
let result = self
.client
.get_validator_aggregate_attestation_v1(
.get_validator_aggregate_attestation_v2(
attestation.data().slot,
attestation.data().tree_hash_root(),
attestation.committee_index().expect("committee index"),
)
.await
.unwrap()
Expand All @@ -3602,7 +3606,6 @@ impl ApiTester {

assert_eq!(result, expected);
}

self
}

Expand Down Expand Up @@ -6775,19 +6778,36 @@ async fn get_validator_attestation_data_with_skip_slots() {
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_validator_aggregate_attestation() {
async fn get_validator_aggregate_attestation_v1() {
ApiTester::new()
.await
.test_get_validator_aggregate_attestation()
.test_get_validator_aggregate_attestation_v1()
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_validator_aggregate_attestation_v2() {
ApiTester::new()
.await
.test_get_validator_aggregate_attestation_v2()
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_validator_aggregate_attestation_with_skip_slots_v1() {
ApiTester::new()
.await
.skip_slots(E::slots_per_epoch() * 2)
.test_get_validator_aggregate_attestation_v1()
.await;
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn get_validator_aggregate_attestation_with_skip_slots() {
async fn get_validator_aggregate_attestation_with_skip_slots_v2() {
ApiTester::new()
.await
.skip_slots(E::slots_per_epoch() * 2)
.test_get_validator_aggregate_attestation()
.test_get_validator_aggregate_attestation_v2()
.await;
}

Expand Down

0 comments on commit ed8086c

Please sign in to comment.