From ed8086c8977a0e0eb7f061bf4dc7484b28cf611d Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Wed, 12 Feb 2025 02:13:05 +0200 Subject: [PATCH] Ensure `GET v2/validator/aggregate_attestation` is backwards compatible (#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 --- .../http_api/src/aggregate_attestation.rs | 37 ++++---- beacon_node/http_api/tests/tests.rs | 94 +++++++++++-------- 2 files changed, 77 insertions(+), 54 deletions(-) diff --git a/beacon_node/http_api/src/aggregate_attestation.rs b/beacon_node/http_api/src/aggregate_attestation.rs index 94b6acd2e6..23af5b0cb5 100644 --- a/beacon_node/http_api/src/aggregate_attestation.rs +++ b/beacon_node/http_api/src/aggregate_attestation.rs @@ -18,13 +18,14 @@ pub fn get_aggregate_attestation( endpoint_version: EndpointVersion, chain: Arc>, ) -> Result, warp::reject::Rejection> { - if endpoint_version == V2 { + let fork_name = chain.spec.fork_name_at_slot::(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!( @@ -34,8 +35,22 @@ pub fn get_aggregate_attestation( })? .ok_or_else(|| { warp_utils::reject::custom_not_found("no matching aggregate found".to_string()) - })?; - let fork_name = chain.spec.fork_name_at_slot::(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 {}, @@ -46,19 +61,7 @@ pub fn get_aggregate_attestation( 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)); } diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index bc3159e074..556ddebad3 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -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::(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::>(); + 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() @@ -3602,7 +3606,6 @@ impl ApiTester { assert_eq!(result, expected); } - self } @@ -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_with_skip_slots() { +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() + .test_get_validator_aggregate_attestation_v1() + .await; +} + +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +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_v2() .await; }