Skip to content

POST /eth/v2/beacon/pool/attestations bugfixes #6867

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

Merged
merged 12 commits into from
Jan 31, 2025
Next Next commit
Fix v2 endpoint to allow for both SingleAttestation and pre-electra A…
…ttestation, and accept strings for committee_index and attester_index fields
  • Loading branch information
eserilev committed Jan 26, 2025
commit f9ba8dc3203e17f698991ce192153cbaf4fa4a33
4 changes: 2 additions & 2 deletions beacon_node/beacon_chain/src/attestation_verification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,8 +327,8 @@ impl<T: BeaconChainTypes> VerifiedUnaggregatedAttestation<'_, T> {

pub fn single_attestation(&self) -> Option<SingleAttestation> {
Some(SingleAttestation {
committee_index: self.attestation.committee_index()? as usize,
attester_index: self.validator_index,
committee_index: self.attestation.committee_index()?,
attester_index: self.validator_index as u64,
data: self.attestation.data().clone(),
signature: self.attestation.signature().clone(),
})
Expand Down
6 changes: 3 additions & 3 deletions beacon_node/beacon_chain/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1118,15 +1118,15 @@ where
.unwrap();

let single_attestation =
attestation.to_single_attestation_with_attester_index(attester_index)?;
attestation.to_single_attestation_with_attester_index(attester_index as u64)?;

let attestation: Attestation<E> = single_attestation.to_attestation(committee.committee)?;

assert_eq!(
single_attestation.committee_index,
attestation.committee_index().unwrap() as usize
attestation.committee_index().unwrap()
);
assert_eq!(single_attestation.attester_index, validator_index);
assert_eq!(single_attestation.attester_index, validator_index as u64);
Ok(single_attestation)
}

Expand Down
62 changes: 14 additions & 48 deletions beacon_node/http_api/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ mod validator;
mod validator_inclusion;
mod validators;
mod version;

use crate::light_client::{get_light_client_bootstrap, get_light_client_updates};
use crate::produce_block::{produce_blinded_block_v2, produce_block_v2, produce_block_v3};
use crate::version::fork_versioned_response;
Expand Down Expand Up @@ -89,8 +88,8 @@ use types::{
AttesterSlashing, BeaconStateError, CommitteeCache, ConfigAndPreset, Epoch, EthSpec, ForkName,
ForkVersionedResponse, Hash256, ProposerPreparationData, ProposerSlashing, RelativeEpoch,
SignedAggregateAndProof, SignedBlindedBeaconBlock, SignedBlsToExecutionChange,
SignedContributionAndProof, SignedValidatorRegistrationData, SignedVoluntaryExit,
SingleAttestation, Slot, SyncCommitteeMessage, SyncContributionData,
SignedContributionAndProof, SignedValidatorRegistrationData, SignedVoluntaryExit, Slot,
SubmitAttestations, SyncCommitteeMessage, SyncContributionData,
};
use validator::pubkey_to_validator_index;
use version::{
Expand Down Expand Up @@ -1835,47 +1834,7 @@ pub fn serve<T: BeaconChainTypes>(
.and(task_spawner_filter.clone())
.and(chain_filter.clone());

let beacon_pool_path_v2 = eth_v2
.and(warp::path("beacon"))
.and(warp::path("pool"))
.and(task_spawner_filter.clone())
.and(chain_filter.clone());

// POST beacon/pool/attestations
let post_beacon_pool_attestations = beacon_pool_path
.clone()
.and(warp::path("attestations"))
.and(warp::path::end())
.and(warp_utils::json::json())
.and(network_tx_filter.clone())
.and(reprocess_send_filter.clone())
.and(log_filter.clone())
.then(
// V1 and V2 are identical except V2 has a consensus version header in the request.
// We only require this header for SSZ deserialization, which isn't supported for
// this endpoint presently.
|task_spawner: TaskSpawner<T::EthSpec>,
chain: Arc<BeaconChain<T>>,
attestations: Vec<Attestation<T::EthSpec>>,
network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
reprocess_tx: Option<Sender<ReprocessQueueMessage>>,
log: Logger| async move {
let attestations = attestations.into_iter().map(Either::Left).collect();
let result = crate::publish_attestations::publish_attestations(
task_spawner,
chain,
attestations,
network_tx,
reprocess_tx,
log,
)
.await
.map(|()| warp::reply::json(&()));
convert_rejection(result).await
},
);

let post_beacon_pool_attestations_v2 = beacon_pool_path_v2
let post_beacon_pool_attestations = beacon_pool_path_any
.clone()
.and(warp::path("attestations"))
.and(warp::path::end())
Expand All @@ -1884,13 +1843,21 @@ pub fn serve<T: BeaconChainTypes>(
.and(reprocess_send_filter)
.and(log_filter.clone())
.then(
|task_spawner: TaskSpawner<T::EthSpec>,
|_endpoint_version: EndpointVersion,
task_spawner: TaskSpawner<T::EthSpec>,
chain: Arc<BeaconChain<T>>,
attestations: Vec<SingleAttestation>,
attestations: SubmitAttestations<T::EthSpec>,
network_tx: UnboundedSender<NetworkMessage<T::EthSpec>>,
reprocess_tx: Option<Sender<ReprocessQueueMessage>>,
log: Logger| async move {
let attestations = attestations.into_iter().map(Either::Right).collect();
let attestations = match attestations {
SubmitAttestations::Attestations(attestations) => {
attestations.into_iter().map(Either::Left).collect()
}
SubmitAttestations::SingleAttestations(attestations) => {
attestations.into_iter().map(Either::Right).collect()
}
};
let result = crate::publish_attestations::publish_attestations(
task_spawner,
chain,
Expand Down Expand Up @@ -4766,7 +4733,6 @@ pub fn serve<T: BeaconChainTypes>(
.uor(post_beacon_blocks_v2)
.uor(post_beacon_blinded_blocks_v2)
.uor(post_beacon_pool_attestations)
.uor(post_beacon_pool_attestations_v2)
.uor(post_beacon_pool_attester_slashings)
.uor(post_beacon_pool_proposer_slashings)
.uor(post_beacon_pool_voluntary_exits)
Expand Down
6 changes: 3 additions & 3 deletions beacon_node/http_api/src/publish_attestations.rs
Original file line number Diff line number Diff line change
Expand Up @@ -163,12 +163,12 @@ fn convert_to_attestation<'a, T: BeaconChainTypes>(
|committee_cache, _| {
let Some(committee) = committee_cache.get_beacon_committee(
single_attestation.data.slot,
single_attestation.committee_index as u64,
single_attestation.committee_index,
) else {
return Err(BeaconChainError::AttestationError(
types::AttestationError::NoCommitteeForSlotAndIndex {
slot: single_attestation.data.slot,
index: single_attestation.committee_index as u64,
index: single_attestation.committee_index,
},
));
};
Expand Down Expand Up @@ -199,7 +199,7 @@ pub async fn publish_attestations<T: BeaconChainTypes>(
.iter()
.map(|att| match att {
Either::Left(att) => (att.data().slot, att.committee_index()),
Either::Right(att) => (att.data.slot, Some(att.committee_index as u64)),
Either::Right(att) => (att.data.slot, Some(att.committee_index)),
})
.collect::<Vec<_>>();

Expand Down
7 changes: 5 additions & 2 deletions beacon_node/http_api/tests/interactive_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ use std::sync::Arc;
use std::time::Duration;
use types::{
Address, Epoch, EthSpec, ExecPayload, ExecutionBlockHash, FixedBytesExtended, ForkName,
Hash256, MainnetEthSpec, MinimalEthSpec, ProposerPreparationData, Slot, Uint256,
Hash256, MainnetEthSpec, MinimalEthSpec, ProposerPreparationData, Slot, SubmitAttestations,
Uint256,
};

type E = MainnetEthSpec;
Expand Down Expand Up @@ -906,9 +907,11 @@ async fn queue_attestations_from_http() {
.flat_map(|attestations| attestations.into_iter().map(|(att, _subnet)| att))
.collect::<Vec<_>>();

let submit_attestations = SubmitAttestations::<E>::SingleAttestations(single_attestations);

tokio::spawn(async move {
client
.post_beacon_pool_attestations_v2(&single_attestations, fork_name)
.post_beacon_pool_attestations_v2(&submit_attestations, fork_name)
.await
.expect("attestations should be processed successfully")
})
Expand Down
32 changes: 24 additions & 8 deletions beacon_node/http_api/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1810,12 +1810,25 @@ impl ApiTester {
self
}

pub async fn test_post_beacon_pool_attestations_valid_v1(mut self) -> Self {
pub async fn test_post_beacon_pool_attestations_valid(mut self) -> Self {
self.client
.post_beacon_pool_attestations_v1(self.attestations.as_slice())
.await
.unwrap();

let fork_name = self
.attestations
.first()
.map(|att| self.chain.spec.fork_name_at_slot::<E>(att.data().slot))
.unwrap();

let submit_attestations = SubmitAttestations::<E>::Attestations(self.attestations.clone());

self.client
.post_beacon_pool_attestations_v2(&submit_attestations, fork_name)
.await
.unwrap();

assert!(
self.network_rx.network_recv.recv().await.is_some(),
"valid attestation should be sent to network"
Expand All @@ -1833,8 +1846,10 @@ impl ApiTester {
.first()
.map(|att| self.chain.spec.fork_name_at_slot::<E>(att.data.slot))
.unwrap();
let submit_attestations =
SubmitAttestations::<E>::SingleAttestations(self.single_attestations.clone());
self.client
.post_beacon_pool_attestations_v2(self.single_attestations.as_slice(), fork_name)
.post_beacon_pool_attestations_v2(&submit_attestations, fork_name)
.await
.unwrap();
assert!(
Expand Down Expand Up @@ -1900,10 +1915,10 @@ impl ApiTester {
.first()
.map(|att| self.chain.spec.fork_name_at_slot::<E>(att.data().slot))
.unwrap();

let submit_attestations = SubmitAttestations::<E>::SingleAttestations(attestations);
let err_v2 = self
.client
.post_beacon_pool_attestations_v2(attestations.as_slice(), fork_name)
.post_beacon_pool_attestations_v2(&submit_attestations, fork_name)
.await
.unwrap_err();

Expand Down Expand Up @@ -6054,9 +6069,10 @@ impl ApiTester {
.chain
.spec
.fork_name_at_slot::<E>(self.chain.slot().unwrap());

let submit_attestations =
SubmitAttestations::<E>::SingleAttestations(self.single_attestations.clone());
self.client
.post_beacon_pool_attestations_v2(&self.single_attestations, fork_name)
.post_beacon_pool_attestations_v2(&submit_attestations, fork_name)
.await
.unwrap();

Expand Down Expand Up @@ -6375,10 +6391,10 @@ async fn post_beacon_blocks_duplicate() {
}

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

Expand Down
2 changes: 1 addition & 1 deletion beacon_node/network/src/network_beacon_processor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ impl<T: BeaconChainTypes> NetworkBeaconProcessor<T> {
|committee_cache, _| {
let Some(committee) = committee_cache.get_beacon_committee(
single_attestation.data.slot,
single_attestation.committee_index as u64,
single_attestation.committee_index,
) else {
warn!(
self.log,
Expand Down
4 changes: 2 additions & 2 deletions common/eth2/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1324,9 +1324,9 @@ impl BeaconNodeHttpClient {
}

/// `POST v2/beacon/pool/attestations`
pub async fn post_beacon_pool_attestations_v2(
pub async fn post_beacon_pool_attestations_v2<E: EthSpec>(
&self,
attestations: &[SingleAttestation],
attestations: &SubmitAttestations<E>,
fork_name: ForkName,
) -> Result<(), Error> {
let mut path = self.eth_path(V2)?;
Expand Down
27 changes: 19 additions & 8 deletions consensus/types/src/attestation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ pub enum Error {
IncorrectStateVariant,
InvalidCommitteeLength,
InvalidCommitteeIndex,
AttesterNotInCommittee(usize),
AttesterNotInCommittee(u64),
InvalidCommittee,
MissingCommittee,
NoCommitteeForSlotAndIndex { slot: Slot, index: CommitteeIndex },
Expand Down Expand Up @@ -238,7 +238,7 @@ impl<E: EthSpec> Attestation<E> {

pub fn to_single_attestation_with_attester_index(
&self,
attester_index: usize,
attester_index: u64,
) -> Result<SingleAttestation, Error> {
match self {
Self::Base(_) => Err(Error::IncorrectStateVariant),
Expand Down Expand Up @@ -375,14 +375,14 @@ impl<E: EthSpec> AttestationElectra<E> {

pub fn to_single_attestation_with_attester_index(
&self,
attester_index: usize,
attester_index: u64,
) -> Result<SingleAttestation, Error> {
let Some(committee_index) = self.committee_index() else {
return Err(Error::InvalidCommitteeIndex);
};

Ok(SingleAttestation {
committee_index: committee_index as usize,
committee_index,
attester_index,
data: self.data.clone(),
signature: self.signature.clone(),
Expand Down Expand Up @@ -579,8 +579,10 @@ impl<E: EthSpec> ForkVersionDeserialize for Vec<Attestation<E>> {
PartialEq,
)]
pub struct SingleAttestation {
pub committee_index: usize,
pub attester_index: usize,
#[serde(with = "serde_utils::quoted_u64")]
pub committee_index: u64,
#[serde(with = "serde_utils::quoted_u64")]
pub attester_index: u64,
pub data: AttestationData,
pub signature: AggregateSignature,
}
Expand All @@ -591,7 +593,7 @@ impl SingleAttestation {
.iter()
.enumerate()
.find_map(|(i, &validator_index)| {
if self.attester_index == validator_index {
if self.attester_index as usize == validator_index {
return Some(i);
}
None
Expand All @@ -600,7 +602,7 @@ impl SingleAttestation {

let mut committee_bits: BitVector<E::MaxCommitteesPerSlot> = BitVector::default();
committee_bits
.set(self.committee_index, true)
.set(self.committee_index as usize, true)
.map_err(|_| Error::InvalidCommitteeIndex)?;

let mut aggregation_bits =
Expand All @@ -617,6 +619,15 @@ impl SingleAttestation {
}
}

#[derive(Debug, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
#[serde(tag = "type")]
#[serde(bound = "E: EthSpec")]
pub enum SubmitAttestations<E: EthSpec> {
Attestations(Vec<Attestation<E>>),
SingleAttestations(Vec<SingleAttestation>),
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
2 changes: 1 addition & 1 deletion consensus/types/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ pub use crate::aggregate_and_proof::{
};
pub use crate::attestation::{
Attestation, AttestationBase, AttestationElectra, AttestationRef, AttestationRefMut,
Error as AttestationError, SingleAttestation,
Error as AttestationError, SingleAttestation, SubmitAttestations,
};
pub use crate::attestation_data::AttestationData;
pub use crate::attestation_duty::AttestationDuty;
Expand Down
2 changes: 1 addition & 1 deletion consensus/types/src/subnet_id.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ impl SubnetId {
) -> Result<SubnetId, ArithError> {
Self::compute_subnet::<E>(
attestation.data.slot,
attestation.committee_index as u64,
attestation.committee_index,
committee_count_per_slot,
spec,
)
Expand Down
Loading
Loading