From b5ac2a63b6517e8aee6ceb7b2efe9d5fe152fae3 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 3 Aug 2024 18:19:33 -0700 Subject: [PATCH 01/22] refactor to separate attn signing and db slashibility checks --- beacon_node/http_api/tests/fork_tests.rs | 4 +- .../http_api/tests/interactive_tests.rs | 2 +- beacon_node/http_api/tests/tests.rs | 14 +- common/eth2/src/lib.rs | 4 +- .../src/slashing_database.rs | 36 + .../src/attestation_data_service.rs | 84 ++ validator_client/src/attestation_service.rs | 728 ++++++++++++------ validator_client/src/lib.rs | 1 + validator_client/src/validator_store.rs | 85 ++ 9 files changed, 718 insertions(+), 240 deletions(-) create mode 100644 validator_client/src/attestation_data_service.rs diff --git a/beacon_node/http_api/tests/fork_tests.rs b/beacon_node/http_api/tests/fork_tests.rs index db8a0ab2b54..5a38cca19c3 100644 --- a/beacon_node/http_api/tests/fork_tests.rs +++ b/beacon_node/http_api/tests/fork_tests.rs @@ -152,11 +152,11 @@ async fn attestations_across_fork_with_skip_slots() { assert!(!unaggregated_attestations.is_empty()); let fork_name = harness.spec.fork_name_at_slot::(fork_slot); client - .post_beacon_pool_attestations_v1(&unaggregated_attestations) + .post_beacon_pool_attestations_v1(&unaggregated_attestations.iter().collect::>()) .await .unwrap(); client - .post_beacon_pool_attestations_v2(&unaggregated_attestations, fork_name) + .post_beacon_pool_attestations_v2(&unaggregated_attestations.iter().collect::>(), fork_name) .await .unwrap(); diff --git a/beacon_node/http_api/tests/interactive_tests.rs b/beacon_node/http_api/tests/interactive_tests.rs index 2f417cf7ba5..fd41801dcd4 100644 --- a/beacon_node/http_api/tests/interactive_tests.rs +++ b/beacon_node/http_api/tests/interactive_tests.rs @@ -896,7 +896,7 @@ async fn queue_attestations_from_http() { let fork_name = tester.harness.spec.fork_name_at_slot::(attestation_slot); let attestation_future = tokio::spawn(async move { client - .post_beacon_pool_attestations_v2(&attestations, fork_name) + .post_beacon_pool_attestations_v2(&attestations.iter().collect::>(), fork_name) .await .expect("attestations should be processed successfully") }); diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index d51799b8661..8fed1699824 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -1704,7 +1704,7 @@ impl ApiTester { pub async fn test_post_beacon_pool_attestations_valid_v1(mut self) -> Self { self.client - .post_beacon_pool_attestations_v1(self.attestations.as_slice()) + .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) .await .unwrap(); @@ -1723,7 +1723,7 @@ impl ApiTester { .map(|att| self.chain.spec.fork_name_at_slot::(att.data().slot)) .unwrap(); self.client - .post_beacon_pool_attestations_v2(self.attestations.as_slice(), fork_name) + .post_beacon_pool_attestations_v2(&self.attestations.iter().collect::>(), fork_name) .await .unwrap(); assert!( @@ -1747,7 +1747,7 @@ impl ApiTester { let err = self .client - .post_beacon_pool_attestations_v1(attestations.as_slice()) + .post_beacon_pool_attestations_v1(&attestations.iter().collect::>()) .await .unwrap_err(); @@ -1789,7 +1789,7 @@ impl ApiTester { let err_v2 = self .client - .post_beacon_pool_attestations_v2(attestations.as_slice(), fork_name) + .post_beacon_pool_attestations_v2(&attestations.iter().collect::>(), fork_name) .await .unwrap_err(); @@ -3815,7 +3815,7 @@ impl ApiTester { // Attest to the current slot self.client - .post_beacon_pool_attestations_v1(self.attestations.as_slice()) + .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) .await .unwrap(); @@ -5455,7 +5455,7 @@ impl ApiTester { // Attest to the current slot self.client - .post_beacon_pool_attestations_v1(self.attestations.as_slice()) + .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) .await .unwrap(); @@ -5511,7 +5511,7 @@ impl ApiTester { let expected_attestation_len = self.attestations.len(); self.client - .post_beacon_pool_attestations_v1(self.attestations.as_slice()) + .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) .await .unwrap(); diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 6d000f576f9..1ff0c9011ae 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -1264,7 +1264,7 @@ impl BeaconNodeHttpClient { /// `POST v1/beacon/pool/attestations` pub async fn post_beacon_pool_attestations_v1( &self, - attestations: &[Attestation], + attestations: &[&Attestation], ) -> Result<(), Error> { let mut path = self.eth_path(V1)?; @@ -1283,7 +1283,7 @@ impl BeaconNodeHttpClient { /// `POST v2/beacon/pool/attestations` pub async fn post_beacon_pool_attestations_v2( &self, - attestations: &[Attestation], + attestations: &[&Attestation], fork_name: ForkName, ) -> Result<(), Error> { let mut path = self.eth_path(V2)?; diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index 04554786f6f..cf4356fdd16 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -641,6 +641,42 @@ impl SlashingDatabase { Ok(safe) } + pub fn check_attestation_signing_root( + &self, + validator_pubkey: &PublicKeyBytes, + attestation: &AttestationData, + domain: Hash256, + ) -> Result { + let attestation_signing_root = attestation.signing_root(domain).into(); + let mut conn = self.conn_pool.get()?; + let txn = conn.transaction_with_behavior(TransactionBehavior::Deferred)?; + self.check_attestation( + &txn, + validator_pubkey, + attestation.source.epoch, + attestation.target.epoch, + attestation_signing_root, + ) + } + + pub fn insert_attestation_signing_root( + &self, + validator_pubkey: &PublicKeyBytes, + attestation: &AttestationData, + domain: Hash256, + ) -> Result<(), NotSafe> { + let attestation_signing_root = attestation.signing_root(domain).into(); + let mut conn = self.conn_pool.get()?; + let txn = conn.transaction_with_behavior(TransactionBehavior::Exclusive)?; + self.insert_attestation( + &txn, + validator_pubkey, + attestation.source.epoch, + attestation.target.epoch, + attestation_signing_root, + ) + } + /// Transactional variant of `check_and_insert_attestation_signing_root`. fn check_and_insert_attestation_signing_root_txn( &self, diff --git a/validator_client/src/attestation_data_service.rs b/validator_client/src/attestation_data_service.rs new file mode 100644 index 00000000000..0ccb7cab78c --- /dev/null +++ b/validator_client/src/attestation_data_service.rs @@ -0,0 +1,84 @@ +use std::{collections::HashMap, sync::Arc}; + +use slot_clock::SlotClock; +use types::{AttestationData, CommitteeIndex, EthSpec, ForkName, Slot}; + +use crate::{ + beacon_node_fallback::{BeaconNodeFallback, OfflineOnFailure, RequireSynced}, + http_metrics::metrics, +}; + +/// The AttestationDataService is responsible for downloading and caching attestation data at a given slot +/// for a range of committee indexes. It also helps prevent us from re-downloading identical attestation data. +pub struct AttestationDataService { + attestation_data_by_committee: HashMap, + beacon_nodes: Arc>, +} + +impl AttestationDataService { + pub fn new(beacon_nodes: Arc>) -> Self { + Self { + attestation_data_by_committee: HashMap::new(), + beacon_nodes, + } + } + + /// Get previously downloaded attestation data by a given committee index. If the Electra fork is enabled + /// we don't care about the committee index + pub fn get_data_by_committee_index( + &self, + committee_index: CommitteeIndex, + fork_name: ForkName, + ) -> Option { + if fork_name.electra_enabled() { + let data = self.attestation_data_by_committee.iter().next(); + if let Some((_, data)) = data { + return Some(data.clone()); + } + None + } else { + self.attestation_data_by_committee + .get(&committee_index) + .cloned() + } + } + + /// Download attestation data for this slot/committee index from the beacon node. + pub async fn download_data( + &mut self, + committee_index: CommitteeIndex, + slot: Slot, + fork_name: ForkName, + ) -> Result<(), String> { + // If we've already downloaded data for this committee index OR electra is enabled and + // we've already downloaded data for this slot, there's no need to re-download the data. + if let Some(_) = self.get_data_by_committee_index(committee_index, fork_name) { + return Ok(()); + } + + let attestation_data = self + .beacon_nodes + .first_success( + RequireSynced::No, + OfflineOnFailure::Yes, + |beacon_node| async move { + let _timer = metrics::start_timer_vec( + &metrics::ATTESTATION_SERVICE_TIMES, + &[metrics::ATTESTATIONS_HTTP_GET], + ); + beacon_node + .get_validator_attestation_data(slot, committee_index) + .await + .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) + .map(|result| result.data) + }, + ) + .await + .map_err(|e| e.to_string())?; + + self.attestation_data_by_committee + .insert(committee_index, attestation_data); + + Ok(()) + } +} diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 30fe508a2c2..f20e2222137 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -1,3 +1,4 @@ +use crate::attestation_data_service::AttestationDataService; use crate::beacon_node_fallback::{ApiTopic, BeaconNodeFallback, RequireSynced}; use crate::{ duties_service::{DutiesService, DutyAndProof}, @@ -6,6 +7,7 @@ use crate::{ OfflineOnFailure, }; use environment::RuntimeContext; +use eth2::lighthouse::attestation_rewards; use futures::future::join_all; use slog::{crit, debug, error, info, trace, warn}; use slot_clock::SlotClock; @@ -14,7 +16,7 @@ use std::ops::Deref; use std::sync::Arc; use tokio::time::{sleep, sleep_until, Duration, Instant}; use tree_hash::TreeHash; -use types::{Attestation, AttestationData, ChainSpec, CommitteeIndex, EthSpec, Slot}; +use types::{Attestation, AttestationData, ChainSpec, CommitteeIndex, EthSpec, ForkName, Slot}; /// Builds an `AttestationService`. pub struct AttestationServiceBuilder { @@ -172,6 +174,9 @@ impl AttestationService { /// attestation to the beacon node. fn spawn_attestation_tasks(&self, slot_duration: Duration) -> Result<(), String> { let slot = self.slot_clock.now().ok_or("Failed to read slot clock")?; + + let fork_name = self.context.eth2_config.spec.fork_name_at_slot::(slot); + let duration_to_next_slot = self .slot_clock .duration_to_next_slot() @@ -195,24 +200,98 @@ impl AttestationService { map }); - // For each committee index for this slot: - // - // - Create and publish an `Attestation` for all required validators. - // - Create and publish `SignedAggregateAndProof` for all aggregating validators. - duties_by_committee_index - .into_iter() - .for_each(|(committee_index, validator_duties)| { - // Spawn a separate task for each attestation. - self.inner.context.executor.spawn_ignoring_error( - self.clone().publish_attestations_and_aggregates( - slot, - committee_index, - validator_duties, - aggregate_production_instant, - ), - "attestation publish", + let this = self.clone(); + + self.inner.context.executor.spawn( + async move { + let log = this.context.log().clone(); + + let mut attestation_data_service = + AttestationDataService::new(this.beacon_nodes.clone()); + + for (committee_index, _) in duties_by_committee_index.iter() { + let _ = attestation_data_service + .download_data(*committee_index, slot, fork_name) + .await; + } + + let mut handles = vec![]; + + // For each committee index for this slot: + // + // - Create and publish an `Attestation` for all required validators. + // - Create and publish `SignedAggregateAndProof` for all aggregating validators. + duties_by_committee_index.clone().into_iter().for_each( + |(committee_index, validator_duties)| { + validator_duties.into_iter().for_each(|validator_duty| { + if let Some(attestation_data) = attestation_data_service + .get_data_by_committee_index(committee_index, fork_name) + { + let that = this.clone(); + let handle = this.inner.context.executor.spawn_blocking_handle( + move || { + that.sign_attestation( + attestation_data, + validator_duty.clone(), + ) + }, + "Sign attestation", + ); + + if let Some(handle) = handle { + handles.push(handle); + } + } else { + // TODO(attn-slash) log a crit? + } + }) + }, ); - }); + + let mut signed_attestations = vec![]; + + for handle in handles { + if let Ok(result) = handle.await { + let Ok(result) = result.await else { return () }; + if let Some(result) = result { + signed_attestations.push(result); + } + } + } + + let Ok(safe_attestations) = this + .validator_store + .check_and_insert_attestations(signed_attestations) + else { + return (); + }; + + let _ = this.publish_attestations( + &safe_attestations.iter().map(|(a, _)| a).collect(), + fork_name, + ).await; + + for (committee_index, validator_duties) in duties_by_committee_index.iter() { + // TODO(attn-slash) we could make this multi threaded + if let Some(attestation_data) = attestation_data_service + .get_data_by_committee_index(*committee_index, fork_name) + { + match this.produce_and_publish_aggregates( + &attestation_data, + *committee_index, + &validator_duties, + ) + .await { + Ok(_) => (), + Err(_) => () // TODO(attn-slash log a crit), + }; + } else { + // TODO(attn-slash) log a crit? + } + } + }, + "Download and sign attestations", + ); // Schedule pruning of the slashing protection database once all unaggregated // attestations have (hopefully) been signed, i.e. at the same time as aggregate @@ -231,114 +310,185 @@ impl AttestationService { /// /// The given `validator_duties` should already be filtered to only contain those that match /// `slot` and `committee_index`. Critical errors will be logged if this is not the case. - async fn publish_attestations_and_aggregates( + // async fn publish_attestations_and_aggregates( + // self, + // slot: Slot, + // committee_index: CommitteeIndex, + // validator_duties: Vec, + // aggregate_production_instant: Instant, + // ) -> Result<(), ()> { + // let log = self.context.log(); + // let attestations_timer = metrics::start_timer_vec( + // &metrics::ATTESTATION_SERVICE_TIMES, + // &[metrics::ATTESTATIONS], + // ); + + // // There's not need to produce `Attestation` or `SignedAggregateAndProof` if we do not have + // // any validators for the given `slot` and `committee_index`. + // if validator_duties.is_empty() { + // return Ok(()); + // } + + // // Step 1. + // // + // // Download, sign and publish an `Attestation` for each validator. + // let attestation_opt = self + // .produce_and_publish_attestations(slot, committee_index, &validator_duties) + // .await + // .map_err(move |e| { + // crit!( + // log, + // "Error during attestation routine"; + // "error" => format!("{:?}", e), + // "committee_index" => committee_index, + // "slot" => slot.as_u64(), + // ) + // })?; + + // drop(attestations_timer); + + // // Step 2. + // // + // // If an attestation was produced, make an aggregate. + // if let Some(attestation_data) = attestation_opt { + // // First, wait until the `aggregation_production_instant` (2/3rds + // // of the way though the slot). As verified in the + // // `delay_triggers_when_in_the_past` test, this code will still run + // // even if the instant has already elapsed. + // sleep_until(aggregate_production_instant).await; + + // // Start the metrics timer *after* we've done the delay. + // let _aggregates_timer = metrics::start_timer_vec( + // &metrics::ATTESTATION_SERVICE_TIMES, + // &[metrics::AGGREGATES], + // ); + + // // Then download, sign and publish a `SignedAggregateAndProof` for each + // // validator that is elected to aggregate for this `slot` and + // // `committee_index`. + // self.produce_and_publish_aggregates( + // &attestation_data, + // committee_index, + // &validator_duties, + // ) + // .await + // .map_err(move |e| { + // crit!( + // log, + // "Error during attestation routine"; + // "error" => format!("{:?}", e), + // "committee_index" => committee_index, + // "slot" => slot.as_u64(), + // ) + // })?; + // } + + // Ok(()) + // } + + async fn sign_attestation( self, - slot: Slot, - committee_index: CommitteeIndex, - validator_duties: Vec, - aggregate_production_instant: Instant, - ) -> Result<(), ()> { + attestation_data: AttestationData, + validator_duty: DutyAndProof, + ) -> Result, DutyAndProof)>, String> { let log = self.context.log(); + + // TODO(attn-slash) more granular metric names let attestations_timer = metrics::start_timer_vec( &metrics::ATTESTATION_SERVICE_TIMES, &[metrics::ATTESTATIONS], ); - // There's not need to produce `Attestation` or `SignedAggregateAndProof` if we do not have - // any validators for the given `slot` and `committee_index`. - if validator_duties.is_empty() { - return Ok(()); + let current_epoch = self + .slot_clock + .now() + .ok_or("Unable to determine current slot from clock")? + .epoch(E::slots_per_epoch()); + + if !validator_duty + .duty + .match_attestation_data::(&attestation_data, &self.context.eth2_config.spec) + { + crit!( + log, + "Inconsistent validator duties during signing"; + "validator" => ?validator_duty.duty.pubkey, + "duty_slot" => validator_duty.duty.slot, + "attestation_slot" => attestation_data.slot, + "duty_index" => validator_duty.duty.committee_index, + "attestation_index" => attestation_data.index, + ); } - // Step 1. - // - // Download, sign and publish an `Attestation` for each validator. - let attestation_opt = self - .produce_and_publish_attestations(slot, committee_index, &validator_duties) - .await - .map_err(move |e| { + let mut attestation = match Attestation::::empty_for_signing( + validator_duty.duty.committee_index, + validator_duty.duty.committee_length as usize, + attestation_data.slot, + attestation_data.beacon_block_root, + attestation_data.source, + attestation_data.target, + &self.context.eth2_config.spec, + ) { + Ok(attestation) => attestation, + Err(err) => { crit!( log, - "Error during attestation routine"; - "error" => format!("{:?}", e), - "committee_index" => committee_index, - "slot" => slot.as_u64(), - ) - })?; - - drop(attestations_timer); - - // Step 2. - // - // If an attestation was produced, make an aggregate. - if let Some(attestation_data) = attestation_opt { - // First, wait until the `aggregation_production_instant` (2/3rds - // of the way though the slot). As verified in the - // `delay_triggers_when_in_the_past` test, this code will still run - // even if the instant has already elapsed. - sleep_until(aggregate_production_instant).await; - - // Start the metrics timer *after* we've done the delay. - let _aggregates_timer = metrics::start_timer_vec( - &metrics::ATTESTATION_SERVICE_TIMES, - &[metrics::AGGREGATES], - ); + "Invalid validator duties during signing"; + "validator" => ?validator_duty.duty.pubkey, + "duty" => ?validator_duty.duty, + "err" => ?err, + ); + return Ok(None); + } + }; - // Then download, sign and publish a `SignedAggregateAndProof` for each - // validator that is elected to aggregate for this `slot` and - // `committee_index`. - self.produce_and_publish_aggregates( - &attestation_data, - committee_index, - &validator_duties, + let signed_attestation = match self + .validator_store + .sign_attestation_v2( + validator_duty.duty.pubkey, + validator_duty.duty.validator_committee_index as usize, + &mut attestation, + current_epoch, ) .await - .map_err(move |e| { - crit!( + { + Ok(()) => Some((attestation, validator_duty)), + Err(ValidatorStoreError::UnknownPubkey(pubkey)) => { + // A pubkey can be missing when a validator was recently + // removed via the API. + warn!( log, - "Error during attestation routine"; - "error" => format!("{:?}", e), - "committee_index" => committee_index, - "slot" => slot.as_u64(), - ) - })?; - } + "Missing pubkey for attestation"; + "info" => "a validator may have recently been removed from this VC", + "pubkey" => ?pubkey, + "validator" => ?validator_duty.duty.pubkey, + "committee_index" => validator_duty.duty.committee_index, + "slot" => validator_duty.duty.slot.as_u64(), + ); + None + } + Err(e) => { + // crit!( + // log, + // "Failed to sign attestation"; + // "error" => ?e, + // "validator" => ?validator_duty.duty.pubkey, + // "committee_index" => validator_duty.duty.committee_index,, + // "slot" => validator_duty.duty.slot.as_u64(), + // ); + None + } + }; - Ok(()) + Ok(signed_attestation) } - /// Performs the first step of the attesting process: downloading `Attestation` objects, - /// signing them and returning them to the validator. - /// - /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/validator.md#attesting - /// - /// ## Detail - /// - /// The given `validator_duties` should already be filtered to only contain those that match - /// `slot` and `committee_index`. Critical errors will be logged if this is not the case. - /// - /// Only one `Attestation` is downloaded from the BN. It is then cloned and signed by each - /// validator and the list of individually-signed `Attestation` objects is returned to the BN. - async fn produce_and_publish_attestations( + async fn download_attestation( &self, slot: Slot, committee_index: CommitteeIndex, - validator_duties: &[DutyAndProof], - ) -> Result, String> { - let log = self.context.log(); - - if validator_duties.is_empty() { - return Ok(None); - } - - let current_epoch = self - .slot_clock - .now() - .ok_or("Unable to determine current slot from clock")? - .epoch(E::slots_per_epoch()); - - let attestation_data = self - .beacon_nodes + ) -> Result { + self.beacon_nodes .first_success( RequireSynced::No, OfflineOnFailure::Yes, @@ -355,109 +505,15 @@ impl AttestationService { }, ) .await - .map_err(|e| e.to_string())?; - - // Create futures to produce signed `Attestation` objects. - let attestation_data_ref = &attestation_data; - let signing_futures = validator_duties.iter().map(|duty_and_proof| async move { - let duty = &duty_and_proof.duty; - let attestation_data = attestation_data_ref; - - // Ensure that the attestation matches the duties. - if !duty.match_attestation_data::(attestation_data, &self.context.eth2_config.spec) { - crit!( - log, - "Inconsistent validator duties during signing"; - "validator" => ?duty.pubkey, - "duty_slot" => duty.slot, - "attestation_slot" => attestation_data.slot, - "duty_index" => duty.committee_index, - "attestation_index" => attestation_data.index, - ); - return None; - } - - let mut attestation = match Attestation::::empty_for_signing( - duty.committee_index, - duty.committee_length as usize, - attestation_data.slot, - attestation_data.beacon_block_root, - attestation_data.source, - attestation_data.target, - &self.context.eth2_config.spec, - ) { - Ok(attestation) => attestation, - Err(err) => { - crit!( - log, - "Invalid validator duties during signing"; - "validator" => ?duty.pubkey, - "duty" => ?duty, - "err" => ?err, - ); - return None; - } - }; - - match self - .validator_store - .sign_attestation( - duty.pubkey, - duty.validator_committee_index as usize, - &mut attestation, - current_epoch, - ) - .await - { - Ok(()) => Some((attestation, duty.validator_index)), - Err(ValidatorStoreError::UnknownPubkey(pubkey)) => { - // A pubkey can be missing when a validator was recently - // removed via the API. - warn!( - log, - "Missing pubkey for attestation"; - "info" => "a validator may have recently been removed from this VC", - "pubkey" => ?pubkey, - "validator" => ?duty.pubkey, - "committee_index" => committee_index, - "slot" => slot.as_u64(), - ); - None - } - Err(e) => { - crit!( - log, - "Failed to sign attestation"; - "error" => ?e, - "validator" => ?duty.pubkey, - "committee_index" => committee_index, - "slot" => slot.as_u64(), - ); - None - } - } - }); - - // Execute all the futures in parallel, collecting any successful results. - let (ref attestations, ref validator_indices): (Vec<_>, Vec<_>) = join_all(signing_futures) - .await - .into_iter() - .flatten() - .unzip(); - - if attestations.is_empty() { - warn!(log, "No attestations were published"); - return Ok(None); - } - let fork_name = self - .context - .eth2_config - .spec - .fork_name_at_slot::(attestation_data.slot); + .map_err(|e| e.to_string()) + } - // Post the attestations to the BN. - match self - .beacon_nodes + pub async fn publish_attestations( + &self, + attestations: &Vec<&Attestation>, + fork_name: ForkName, + ) -> Result<(), String> { + self.beacon_nodes .request( RequireSynced::No, OfflineOnFailure::Yes, @@ -467,6 +523,7 @@ impl AttestationService { &metrics::ATTESTATION_SERVICE_TIMES, &[metrics::ATTESTATIONS_HTTP_POST], ); + if fork_name.electra_enabled() { beacon_node .post_beacon_pool_attestations_v2(attestations, fork_name) @@ -479,30 +536,245 @@ impl AttestationService { }, ) .await - { - Ok(()) => info!( - log, - "Successfully published attestations"; - "count" => attestations.len(), - "validator_indices" => ?validator_indices, - "head_block" => ?attestation_data.beacon_block_root, - "committee_index" => attestation_data.index, - "slot" => attestation_data.slot.as_u64(), - "type" => "unaggregated", - ), - Err(e) => error!( - log, - "Unable to publish attestations"; - "error" => %e, - "committee_index" => attestation_data.index, - "slot" => slot.as_u64(), - "type" => "unaggregated", - ), - } + .map_err(|_| "Failed to broadcast")?; - Ok(Some(attestation_data)) + Ok(()) } + async fn publish_aggregates( + &self, + attestation_data: &AttestationData, + committee_index: CommitteeIndex, + validator_duties: &Vec, + aggregate_production_instant: Instant, + ) -> Result<(), ()> { + let log = self.context.log(); + + // Step 2. + // + // If an attestation was produced, make an aggregate. + // First, wait until the `aggregation_production_instant` (2/3rds + // of the way though the slot). As verified in the + // `delay_triggers_when_in_the_past` test, this code will still run + // even if the instant has already elapsed. + sleep_until(aggregate_production_instant).await; + + // Start the metrics timer *after* we've done the delay. + let _aggregates_timer = + metrics::start_timer_vec(&metrics::ATTESTATION_SERVICE_TIMES, &[metrics::AGGREGATES]); + + // Then download, sign and publish a `SignedAggregateAndProof` for each + // validator that is elected to aggregate for this `slot` and + // `committee_index`. + self.produce_and_publish_aggregates(attestation_data, committee_index, &validator_duties) + .await + .map_err(move |e| { + crit!( + log, + "Error during attestation routine"; + "error" => format!("{:?}", e), + ) + })?; + + Ok(()) + } + + /// Performs the first step of the attesting process: downloading `Attestation` objects, + /// signing them and returning them to the validator. + /// + /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/validator.md#attesting + /// + /// ## Detail + /// + /// The given `validator_duties` should already be filtered to only contain those that match + /// `slot` and `committee_index`. Critical errors will be logged if this is not the case. + /// + /// Only one `Attestation` is downloaded from the BN. It is then cloned and signed by each + /// validator and the list of individually-signed `Attestation` objects is returned to the BN. + // async fn produce_and_publish_attestations( + // &self, + // slot: Slot, + // committee_index: CommitteeIndex, + // validator_duties: &[DutyAndProof], + // ) -> Result, String> { + // let log = self.context.log(); + + // if validator_duties.is_empty() { + // return Ok(None); + // } + + // let current_epoch = self + // .slot_clock + // .now() + // .ok_or("Unable to determine current slot from clock")? + // .epoch(E::slots_per_epoch()); + + // let attestation_data = self + // .beacon_nodes + // .first_success( + // RequireSynced::No, + // OfflineOnFailure::Yes, + // |beacon_node| async move { + // let _timer = metrics::start_timer_vec( + // &metrics::ATTESTATION_SERVICE_TIMES, + // &[metrics::ATTESTATIONS_HTTP_GET], + // ); + // beacon_node + // .get_validator_attestation_data(slot, committee_index) + // .await + // .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) + // .map(|result| result.data) + // }, + // ) + // .await + // .map_err(|e| e.to_string())?; + + // // Create futures to produce signed `Attestation` objects. + // let attestation_data_ref = &attestation_data; + // let signing_futures = validator_duties.iter().map(|duty_and_proof| async move { + // let duty = &duty_and_proof.duty; + // let attestation_data = attestation_data_ref; + + // // Ensure that the attestation matches the duties. + // if !duty.match_attestation_data::(attestation_data, &self.context.eth2_config.spec) { + // crit!( + // log, + // "Inconsistent validator duties during signing"; + // "validator" => ?duty.pubkey, + // "duty_slot" => duty.slot, + // "attestation_slot" => attestation_data.slot, + // "duty_index" => duty.committee_index, + // "attestation_index" => attestation_data.index, + // ); + // return None; + // } + + // let mut attestation = match Attestation::::empty_for_signing( + // duty.committee_index, + // duty.committee_length as usize, + // attestation_data.slot, + // attestation_data.beacon_block_root, + // attestation_data.source, + // attestation_data.target, + // &self.context.eth2_config.spec, + // ) { + // Ok(attestation) => attestation, + // Err(err) => { + // crit!( + // log, + // "Invalid validator duties during signing"; + // "validator" => ?duty.pubkey, + // "duty" => ?duty, + // "err" => ?err, + // ); + // return None; + // } + // }; + + // match self + // .validator_store + // .sign_attestation( + // duty.pubkey, + // duty.validator_committee_index as usize, + // &mut attestation, + // current_epoch, + // ) + // .await + // { + // Ok(()) => Some((attestation, duty.validator_index)), + // Err(ValidatorStoreError::UnknownPubkey(pubkey)) => { + // // A pubkey can be missing when a validator was recently + // // removed via the API. + // warn!( + // log, + // "Missing pubkey for attestation"; + // "info" => "a validator may have recently been removed from this VC", + // "pubkey" => ?pubkey, + // "validator" => ?duty.pubkey, + // "committee_index" => committee_index, + // "slot" => slot.as_u64(), + // ); + // None + // } + // Err(e) => { + // crit!( + // log, + // "Failed to sign attestation"; + // "error" => ?e, + // "validator" => ?duty.pubkey, + // "committee_index" => committee_index, + // "slot" => slot.as_u64(), + // ); + // None + // } + // } + // }); + + // // Execute all the futures in parallel, collecting any successful results. + // let (ref attestations, ref validator_indices): (Vec<_>, Vec<_>) = join_all(signing_futures) + // .await + // .into_iter() + // .flatten() + // .unzip(); + + // if attestations.is_empty() { + // warn!(log, "No attestations were published"); + // return Ok(None); + // } + // let fork_name = self + // .context + // .eth2_config + // .spec + // .fork_name_at_slot::(attestation_data.slot); + + // // Post the attestations to the BN. + // match self + // .beacon_nodes + // .request( + // RequireSynced::No, + // OfflineOnFailure::Yes, + // ApiTopic::Attestations, + // |beacon_node| async move { + // let _timer = metrics::start_timer_vec( + // &metrics::ATTESTATION_SERVICE_TIMES, + // &[metrics::ATTESTATIONS_HTTP_POST], + // ); + // if fork_name.electra_enabled() { + // beacon_node + // .post_beacon_pool_attestations_v2(attestations.as_ref(), fork_name) + // .await + // } else { + // beacon_node + // .post_beacon_pool_attestations_v1(attestations.as_ref()) + // .await + // } + // }, + // ) + // .await + // { + // Ok(()) => info!( + // log, + // "Successfully published attestations"; + // "count" => attestations.len(), + // "validator_indices" => ?validator_indices, + // "head_block" => ?attestation_data.beacon_block_root, + // "committee_index" => attestation_data.index, + // "slot" => attestation_data.slot.as_u64(), + // "type" => "unaggregated", + // ), + // Err(e) => error!( + // log, + // "Unable to publish attestations"; + // "error" => %e, + // "committee_index" => attestation_data.index, + // "slot" => slot.as_u64(), + // "type" => "unaggregated", + // ), + // } + + // Ok(Some(attestation_data)) + // } + /// Performs the second step of the attesting process: downloading an aggregated `Attestation`, /// converting it into a `SignedAggregateAndProof` and returning it to the BN. /// diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index 729ff62ee30..b3cf326fb92 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -1,3 +1,4 @@ +mod attestation_data_service; mod attestation_service; mod beacon_node_fallback; mod block_service; diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 8a9e125936e..f2ee69b7483 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -1,5 +1,6 @@ use crate::{ doppelganger_service::DoppelgangerService, + duties_service::DutyAndProof, http_metrics::metrics, initialized_validators::InitializedValidators, signing_method::{Error as SigningError, SignableMessage, SigningContext, SigningMethod}, @@ -633,6 +634,46 @@ impl ValidatorStore { } } + pub async fn sign_attestation_v2( + &self, + validator_pubkey: PublicKeyBytes, + validator_committee_position: usize, + attestation: &mut Attestation, + current_epoch: Epoch, + ) -> Result<(), Error> { + // Make sure the target epoch is not higher than the current epoch to avoid potential attacks. + if attestation.data().target.epoch > current_epoch { + return Err(Error::GreaterThanCurrentEpoch { + epoch: attestation.data().target.epoch, + current_epoch, + }); + } + + // Get the signing method and check doppelganger protection. + let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; + + // Checking for slashing conditions. + let signing_epoch = attestation.data().target.epoch; + let signing_context = self.signing_context(Domain::BeaconAttester, signing_epoch); + + let signature = signing_method + .get_signature::>( + SignableMessage::AttestationData(attestation.data()), + signing_context, + &self.spec, + &self.task_executor, + ) + .await?; + + attestation + .add_signature(&signature, validator_committee_position) + .map_err(Error::UnableToSignAttestation)?; + + metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); + + Ok(()) + } + pub async fn sign_attestation( &self, validator_pubkey: PublicKeyBytes, @@ -726,6 +767,50 @@ impl ValidatorStore { } } + pub fn check_and_insert_attestations( + &self, + attestations: Vec<(Attestation, DutyAndProof)>, + ) -> Result, DutyAndProof)>, Error> { + let mut attestations_to_check = vec![]; + let mut safe_attestations = vec![]; + + for (attestation, validator_duty) in attestations.iter() { + let validator_pubkey = validator_duty.duty.pubkey; + let signing_method = + self.doppelganger_checked_signing_method(validator_pubkey.clone())?; + + // Checking for slashing conditions. + let signing_epoch = attestation.data().target.epoch; + let signing_context = self.signing_context(Domain::BeaconAttester, signing_epoch); + let domain_hash = signing_context.domain_hash(&self.spec); + if signing_method + .requires_local_slashing_protection(self.enable_web3signer_slashing_protection) + { + attestations_to_check.push((attestation, validator_duty, domain_hash)); + } else { + safe_attestations.push((attestation.clone(), validator_duty.clone())); + } + } + + for (attestation, validator_duty, domain_hash) in attestations_to_check { + let validator_pubkey = &validator_duty.duty.pubkey; + if let Ok(Safe::Valid) = self.slashing_protection.check_attestation_signing_root( + &validator_pubkey, + attestation.data(), + domain_hash, + ) { + if let Ok(()) = self.slashing_protection.insert_attestation_signing_root( + &validator_pubkey, + attestation.data(), + domain_hash, + ) { + safe_attestations.push((attestation.clone(), validator_duty.clone())); + } + } + } + Ok(safe_attestations) + } + pub async fn sign_voluntary_exit( &self, validator_pubkey: PublicKeyBytes, From a7fe031bf881ae79963493db6dd2a4988fdf114e Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sun, 4 Aug 2024 12:34:36 -0700 Subject: [PATCH 02/22] adding comments, a few logs, some more TODO's --- .../src/slashing_database.rs | 82 ++-- .../src/attestation_data_service.rs | 16 +- validator_client/src/attestation_service.rs | 402 +++--------------- validator_client/src/validator_store.rs | 48 ++- 4 files changed, 142 insertions(+), 406 deletions(-) diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index cf4356fdd16..5cb4a36b0ed 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -11,7 +11,7 @@ use rusqlite::{params, OptionalExtension, Transaction, TransactionBehavior}; use std::fs::File; use std::path::Path; use std::time::Duration; -use types::{AttestationData, BeaconBlockHeader, Epoch, Hash256, PublicKeyBytes, SignedRoot, Slot}; +use types::{Attestation, AttestationData, BeaconBlockHeader, Epoch, Hash256, PublicKeyBytes, SignedRoot, Slot}; type Pool = r2d2::Pool; @@ -599,6 +599,13 @@ impl SlashingDatabase { Ok(safe) } + // pub fn batch_check_and_insert_attestations( + // &self, + // attestations_to_check: Vec<(Attestation, DutyAndProof)>, + // ) { + + // } + /// Check an attestation for slash safety, and if it is safe, record it in the database. /// /// The checking and inserting happen atomically and exclusively. We enforce exclusivity @@ -641,41 +648,44 @@ impl SlashingDatabase { Ok(safe) } - pub fn check_attestation_signing_root( - &self, - validator_pubkey: &PublicKeyBytes, - attestation: &AttestationData, - domain: Hash256, - ) -> Result { - let attestation_signing_root = attestation.signing_root(domain).into(); - let mut conn = self.conn_pool.get()?; - let txn = conn.transaction_with_behavior(TransactionBehavior::Deferred)?; - self.check_attestation( - &txn, - validator_pubkey, - attestation.source.epoch, - attestation.target.epoch, - attestation_signing_root, - ) - } - - pub fn insert_attestation_signing_root( - &self, - validator_pubkey: &PublicKeyBytes, - attestation: &AttestationData, - domain: Hash256, - ) -> Result<(), NotSafe> { - let attestation_signing_root = attestation.signing_root(domain).into(); - let mut conn = self.conn_pool.get()?; - let txn = conn.transaction_with_behavior(TransactionBehavior::Exclusive)?; - self.insert_attestation( - &txn, - validator_pubkey, - attestation.source.epoch, - attestation.target.epoch, - attestation_signing_root, - ) - } + // TODO(attn-slash) previously I had separated checking the signing root + // add inserting. But this doesn't seem necessary and seems to add a bit of unneeded complexity + // will review this before deleting. + // pub fn check_attestation_signing_root( + // &self, + // validator_pubkey: &PublicKeyBytes, + // attestation: &AttestationData, + // domain: Hash256, + // ) -> Result { + // let attestation_signing_root = attestation.signing_root(domain).into(); + // let mut conn = self.conn_pool.get()?; + // let txn = conn.transaction_with_behavior(TransactionBehavior::Deferred)?; + // self.check_attestation( + // &txn, + // validator_pubkey, + // attestation.source.epoch, + // attestation.target.epoch, + // attestation_signing_root, + // ) + // } + + // pub fn insert_attestation_signing_root( + // &self, + // validator_pubkey: &PublicKeyBytes, + // attestation: &AttestationData, + // domain: Hash256, + // ) -> Result<(), NotSafe> { + // let attestation_signing_root = attestation.signing_root(domain).into(); + // let mut conn = self.conn_pool.get()?; + // let txn = conn.transaction_with_behavior(TransactionBehavior::Exclusive)?; + // self.insert_attestation( + // &txn, + // validator_pubkey, + // attestation.source.epoch, + // attestation.target.epoch, + // attestation_signing_root, + // ) + // } /// Transactional variant of `check_and_insert_attestation_signing_root`. fn check_and_insert_attestation_signing_root_txn( diff --git a/validator_client/src/attestation_data_service.rs b/validator_client/src/attestation_data_service.rs index 0ccb7cab78c..755e52a2efd 100644 --- a/validator_client/src/attestation_data_service.rs +++ b/validator_client/src/attestation_data_service.rs @@ -27,8 +27,8 @@ impl AttestationDataService { /// we don't care about the committee index pub fn get_data_by_committee_index( &self, - committee_index: CommitteeIndex, - fork_name: ForkName, + committee_index: &CommitteeIndex, + fork_name: &ForkName, ) -> Option { if fork_name.electra_enabled() { let data = self.attestation_data_by_committee.iter().next(); @@ -38,7 +38,7 @@ impl AttestationDataService { None } else { self.attestation_data_by_committee - .get(&committee_index) + .get(committee_index) .cloned() } } @@ -46,9 +46,9 @@ impl AttestationDataService { /// Download attestation data for this slot/committee index from the beacon node. pub async fn download_data( &mut self, - committee_index: CommitteeIndex, - slot: Slot, - fork_name: ForkName, + committee_index: &CommitteeIndex, + slot: &Slot, + fork_name: &ForkName, ) -> Result<(), String> { // If we've already downloaded data for this committee index OR electra is enabled and // we've already downloaded data for this slot, there's no need to re-download the data. @@ -67,7 +67,7 @@ impl AttestationDataService { &[metrics::ATTESTATIONS_HTTP_GET], ); beacon_node - .get_validator_attestation_data(slot, committee_index) + .get_validator_attestation_data(slot.clone(), *committee_index) .await .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) .map(|result| result.data) @@ -77,7 +77,7 @@ impl AttestationDataService { .map_err(|e| e.to_string())?; self.attestation_data_by_committee - .insert(committee_index, attestation_data); + .insert(*committee_index, attestation_data); Ok(()) } diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index f20e2222137..d5d88db7354 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -7,7 +7,6 @@ use crate::{ OfflineOnFailure, }; use environment::RuntimeContext; -use eth2::lighthouse::attestation_rewards; use futures::future::join_all; use slog::{crit, debug, error, info, trace, warn}; use slot_clock::SlotClock; @@ -209,25 +208,25 @@ impl AttestationService { let mut attestation_data_service = AttestationDataService::new(this.beacon_nodes.clone()); - for (committee_index, _) in duties_by_committee_index.iter() { - let _ = attestation_data_service - .download_data(*committee_index, slot, fork_name) - .await; - } + this.produce_attestation_data( + &mut attestation_data_service, + &duties_by_committee_index, + &slot, + &fork_name + ).await; let mut handles = vec![]; - // For each committee index for this slot: - // - // - Create and publish an `Attestation` for all required validators. - // - Create and publish `SignedAggregateAndProof` for all aggregating validators. + // Sign an `Attestation` for all required validators. duties_by_committee_index.clone().into_iter().for_each( |(committee_index, validator_duties)| { validator_duties.into_iter().for_each(|validator_duty| { + // Get the previously downloaded attestation data for this committee index. if let Some(attestation_data) = attestation_data_service - .get_data_by_committee_index(committee_index, fork_name) + .get_data_by_committee_index(&committee_index, &fork_name) { let that = this.clone(); + // Have the validator sign the attestation. let handle = this.inner.context.executor.spawn_blocking_handle( move || { that.sign_attestation( @@ -259,6 +258,7 @@ impl AttestationService { } } + // Check that the signed attestations are not slash-able (if slash-ability checks are enabled). let Ok(safe_attestations) = this .validator_store .check_and_insert_attestations(signed_attestations) @@ -266,15 +266,19 @@ impl AttestationService { return (); }; - let _ = this.publish_attestations( + match this.publish_attestations( &safe_attestations.iter().map(|(a, _)| a).collect(), fork_name, - ).await; + ).await { + Ok(_) => (), + Err(_) => (), // TODO(attn-slash) add logging + }; + // Create and publish `SignedAggregateAndProof` for all aggregating validators. for (committee_index, validator_duties) in duties_by_committee_index.iter() { // TODO(attn-slash) we could make this multi threaded if let Some(attestation_data) = attestation_data_service - .get_data_by_committee_index(*committee_index, fork_name) + .get_data_by_committee_index(committee_index, &fork_name) { match this.produce_and_publish_aggregates( &attestation_data, @@ -283,7 +287,7 @@ impl AttestationService { ) .await { Ok(_) => (), - Err(_) => () // TODO(attn-slash log a crit), + Err(_) => () // TODO(attn-slash) log a crit }; } else { // TODO(attn-slash) log a crit? @@ -301,8 +305,28 @@ impl AttestationService { Ok(()) } - /// Performs the first step of the attesting process: downloading `Attestation` objects, - /// signing them and returning them to the validator. + /// Performs the first step of the attesting process: downloading `AttestationData` objects. + /// Pre Electra: Only one `AttestationData` is downloaded from the BN for each committee index. + /// Post Electra: Only one `AttestationData` is downloaded from the BN for each slot. + pub async fn produce_attestation_data( + &self, + attestation_data_service: &mut AttestationDataService, + duties_by_committee_index: &HashMap>, + slot: &Slot, + fork_name: &ForkName + ) { + for (committee_index, _) in duties_by_committee_index.iter() { + match attestation_data_service + .download_data(committee_index, slot, fork_name) + .await { + Ok(_) => (), + Err(_) => (), // TODO(attn-slash) add logging + }; + } + } + + /// Performs the second step of the attesting process: signing the downloaded + /// `Attestation` objects. /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/validator.md#attesting /// @@ -310,82 +334,6 @@ impl AttestationService { /// /// The given `validator_duties` should already be filtered to only contain those that match /// `slot` and `committee_index`. Critical errors will be logged if this is not the case. - // async fn publish_attestations_and_aggregates( - // self, - // slot: Slot, - // committee_index: CommitteeIndex, - // validator_duties: Vec, - // aggregate_production_instant: Instant, - // ) -> Result<(), ()> { - // let log = self.context.log(); - // let attestations_timer = metrics::start_timer_vec( - // &metrics::ATTESTATION_SERVICE_TIMES, - // &[metrics::ATTESTATIONS], - // ); - - // // There's not need to produce `Attestation` or `SignedAggregateAndProof` if we do not have - // // any validators for the given `slot` and `committee_index`. - // if validator_duties.is_empty() { - // return Ok(()); - // } - - // // Step 1. - // // - // // Download, sign and publish an `Attestation` for each validator. - // let attestation_opt = self - // .produce_and_publish_attestations(slot, committee_index, &validator_duties) - // .await - // .map_err(move |e| { - // crit!( - // log, - // "Error during attestation routine"; - // "error" => format!("{:?}", e), - // "committee_index" => committee_index, - // "slot" => slot.as_u64(), - // ) - // })?; - - // drop(attestations_timer); - - // // Step 2. - // // - // // If an attestation was produced, make an aggregate. - // if let Some(attestation_data) = attestation_opt { - // // First, wait until the `aggregation_production_instant` (2/3rds - // // of the way though the slot). As verified in the - // // `delay_triggers_when_in_the_past` test, this code will still run - // // even if the instant has already elapsed. - // sleep_until(aggregate_production_instant).await; - - // // Start the metrics timer *after* we've done the delay. - // let _aggregates_timer = metrics::start_timer_vec( - // &metrics::ATTESTATION_SERVICE_TIMES, - // &[metrics::AGGREGATES], - // ); - - // // Then download, sign and publish a `SignedAggregateAndProof` for each - // // validator that is elected to aggregate for this `slot` and - // // `committee_index`. - // self.produce_and_publish_aggregates( - // &attestation_data, - // committee_index, - // &validator_duties, - // ) - // .await - // .map_err(move |e| { - // crit!( - // log, - // "Error during attestation routine"; - // "error" => format!("{:?}", e), - // "committee_index" => committee_index, - // "slot" => slot.as_u64(), - // ) - // })?; - // } - - // Ok(()) - // } - async fn sign_attestation( self, attestation_data: AttestationData, @@ -483,31 +431,13 @@ impl AttestationService { Ok(signed_attestation) } - async fn download_attestation( - &self, - slot: Slot, - committee_index: CommitteeIndex, - ) -> Result { - self.beacon_nodes - .first_success( - RequireSynced::No, - OfflineOnFailure::Yes, - |beacon_node| async move { - let _timer = metrics::start_timer_vec( - &metrics::ATTESTATION_SERVICE_TIMES, - &[metrics::ATTESTATIONS_HTTP_GET], - ); - beacon_node - .get_validator_attestation_data(slot, committee_index) - .await - .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) - .map(|result| result.data) - }, - ) - .await - .map_err(|e| e.to_string()) - } - + /// Performs the third step of the attesting process: broadcasting the signed attestations + /// to the network. + /// + /// ## Detail + /// + /// If slash-ability checks are enabled the broadcasted attestations only include + /// attestations that were deemed "safe". pub async fn publish_attestations( &self, attestations: &Vec<&Attestation>, @@ -541,241 +471,7 @@ impl AttestationService { Ok(()) } - async fn publish_aggregates( - &self, - attestation_data: &AttestationData, - committee_index: CommitteeIndex, - validator_duties: &Vec, - aggregate_production_instant: Instant, - ) -> Result<(), ()> { - let log = self.context.log(); - - // Step 2. - // - // If an attestation was produced, make an aggregate. - // First, wait until the `aggregation_production_instant` (2/3rds - // of the way though the slot). As verified in the - // `delay_triggers_when_in_the_past` test, this code will still run - // even if the instant has already elapsed. - sleep_until(aggregate_production_instant).await; - - // Start the metrics timer *after* we've done the delay. - let _aggregates_timer = - metrics::start_timer_vec(&metrics::ATTESTATION_SERVICE_TIMES, &[metrics::AGGREGATES]); - - // Then download, sign and publish a `SignedAggregateAndProof` for each - // validator that is elected to aggregate for this `slot` and - // `committee_index`. - self.produce_and_publish_aggregates(attestation_data, committee_index, &validator_duties) - .await - .map_err(move |e| { - crit!( - log, - "Error during attestation routine"; - "error" => format!("{:?}", e), - ) - })?; - - Ok(()) - } - - /// Performs the first step of the attesting process: downloading `Attestation` objects, - /// signing them and returning them to the validator. - /// - /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/validator.md#attesting - /// - /// ## Detail - /// - /// The given `validator_duties` should already be filtered to only contain those that match - /// `slot` and `committee_index`. Critical errors will be logged if this is not the case. - /// - /// Only one `Attestation` is downloaded from the BN. It is then cloned and signed by each - /// validator and the list of individually-signed `Attestation` objects is returned to the BN. - // async fn produce_and_publish_attestations( - // &self, - // slot: Slot, - // committee_index: CommitteeIndex, - // validator_duties: &[DutyAndProof], - // ) -> Result, String> { - // let log = self.context.log(); - - // if validator_duties.is_empty() { - // return Ok(None); - // } - - // let current_epoch = self - // .slot_clock - // .now() - // .ok_or("Unable to determine current slot from clock")? - // .epoch(E::slots_per_epoch()); - - // let attestation_data = self - // .beacon_nodes - // .first_success( - // RequireSynced::No, - // OfflineOnFailure::Yes, - // |beacon_node| async move { - // let _timer = metrics::start_timer_vec( - // &metrics::ATTESTATION_SERVICE_TIMES, - // &[metrics::ATTESTATIONS_HTTP_GET], - // ); - // beacon_node - // .get_validator_attestation_data(slot, committee_index) - // .await - // .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) - // .map(|result| result.data) - // }, - // ) - // .await - // .map_err(|e| e.to_string())?; - - // // Create futures to produce signed `Attestation` objects. - // let attestation_data_ref = &attestation_data; - // let signing_futures = validator_duties.iter().map(|duty_and_proof| async move { - // let duty = &duty_and_proof.duty; - // let attestation_data = attestation_data_ref; - - // // Ensure that the attestation matches the duties. - // if !duty.match_attestation_data::(attestation_data, &self.context.eth2_config.spec) { - // crit!( - // log, - // "Inconsistent validator duties during signing"; - // "validator" => ?duty.pubkey, - // "duty_slot" => duty.slot, - // "attestation_slot" => attestation_data.slot, - // "duty_index" => duty.committee_index, - // "attestation_index" => attestation_data.index, - // ); - // return None; - // } - - // let mut attestation = match Attestation::::empty_for_signing( - // duty.committee_index, - // duty.committee_length as usize, - // attestation_data.slot, - // attestation_data.beacon_block_root, - // attestation_data.source, - // attestation_data.target, - // &self.context.eth2_config.spec, - // ) { - // Ok(attestation) => attestation, - // Err(err) => { - // crit!( - // log, - // "Invalid validator duties during signing"; - // "validator" => ?duty.pubkey, - // "duty" => ?duty, - // "err" => ?err, - // ); - // return None; - // } - // }; - - // match self - // .validator_store - // .sign_attestation( - // duty.pubkey, - // duty.validator_committee_index as usize, - // &mut attestation, - // current_epoch, - // ) - // .await - // { - // Ok(()) => Some((attestation, duty.validator_index)), - // Err(ValidatorStoreError::UnknownPubkey(pubkey)) => { - // // A pubkey can be missing when a validator was recently - // // removed via the API. - // warn!( - // log, - // "Missing pubkey for attestation"; - // "info" => "a validator may have recently been removed from this VC", - // "pubkey" => ?pubkey, - // "validator" => ?duty.pubkey, - // "committee_index" => committee_index, - // "slot" => slot.as_u64(), - // ); - // None - // } - // Err(e) => { - // crit!( - // log, - // "Failed to sign attestation"; - // "error" => ?e, - // "validator" => ?duty.pubkey, - // "committee_index" => committee_index, - // "slot" => slot.as_u64(), - // ); - // None - // } - // } - // }); - - // // Execute all the futures in parallel, collecting any successful results. - // let (ref attestations, ref validator_indices): (Vec<_>, Vec<_>) = join_all(signing_futures) - // .await - // .into_iter() - // .flatten() - // .unzip(); - - // if attestations.is_empty() { - // warn!(log, "No attestations were published"); - // return Ok(None); - // } - // let fork_name = self - // .context - // .eth2_config - // .spec - // .fork_name_at_slot::(attestation_data.slot); - - // // Post the attestations to the BN. - // match self - // .beacon_nodes - // .request( - // RequireSynced::No, - // OfflineOnFailure::Yes, - // ApiTopic::Attestations, - // |beacon_node| async move { - // let _timer = metrics::start_timer_vec( - // &metrics::ATTESTATION_SERVICE_TIMES, - // &[metrics::ATTESTATIONS_HTTP_POST], - // ); - // if fork_name.electra_enabled() { - // beacon_node - // .post_beacon_pool_attestations_v2(attestations.as_ref(), fork_name) - // .await - // } else { - // beacon_node - // .post_beacon_pool_attestations_v1(attestations.as_ref()) - // .await - // } - // }, - // ) - // .await - // { - // Ok(()) => info!( - // log, - // "Successfully published attestations"; - // "count" => attestations.len(), - // "validator_indices" => ?validator_indices, - // "head_block" => ?attestation_data.beacon_block_root, - // "committee_index" => attestation_data.index, - // "slot" => attestation_data.slot.as_u64(), - // "type" => "unaggregated", - // ), - // Err(e) => error!( - // log, - // "Unable to publish attestations"; - // "error" => %e, - // "committee_index" => attestation_data.index, - // "slot" => slot.as_u64(), - // "type" => "unaggregated", - // ), - // } - - // Ok(Some(attestation_data)) - // } - - /// Performs the second step of the attesting process: downloading an aggregated `Attestation`, + /// Performs the fourth step of the attesting process: downloading an aggregated `Attestation`, /// converting it into a `SignedAggregateAndProof` and returning it to the BN. /// /// https://github.com/ethereum/eth2.0-specs/blob/v0.12.1/specs/phase0/validator.md#broadcast-aggregate diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index f2ee69b7483..4762f3b6045 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -794,19 +794,49 @@ impl ValidatorStore { for (attestation, validator_duty, domain_hash) in attestations_to_check { let validator_pubkey = &validator_duty.duty.pubkey; - if let Ok(Safe::Valid) = self.slashing_protection.check_attestation_signing_root( + + let slashing_status = self.slashing_protection.check_and_insert_attestation( &validator_pubkey, attestation.data(), domain_hash, - ) { - if let Ok(()) = self.slashing_protection.insert_attestation_signing_root( - &validator_pubkey, - attestation.data(), - domain_hash, - ) { + ); + + match slashing_status { + Ok(Safe::Valid) => { safe_attestations.push((attestation.clone(), validator_duty.clone())); - } - } + metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); + }, + Ok(Safe::SameData) => { + warn!( + self.log, + "Skipping previously signed attestation" + ); + }, + Err(NotSafe::UnregisteredValidator(pk)) => { + warn!( + self.log, + "Not signing attestation for unregistered validator"; + "msg" => "Carefully consider running with --init-slashing-protection (see --help)", + "public_key" => format!("{:?}", pk) + ); + metrics::inc_counter_vec( + &metrics::SIGNED_ATTESTATIONS_TOTAL, + &[metrics::UNREGISTERED], + ); + }, + Err(e) => { + crit!( + self.log, + "Not signing slashable attestation"; + "attestation" => format!("{:?}", attestation.data()), + "error" => format!("{:?}", e) + ); + metrics::inc_counter_vec( + &metrics::SIGNED_ATTESTATIONS_TOTAL, + &[metrics::SLASHABLE], + ); + }, + }; } Ok(safe_attestations) } From ee691f915be708381c3778ee5b94b9511fbc12e1 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 5 Aug 2024 11:03:45 -0700 Subject: [PATCH 03/22] only download attn data once and mutate index when required --- .../src/slashing_database.rs | 2 ++ .../src/attestation_data_service.rs | 28 ++++++++----------- 2 files changed, 14 insertions(+), 16 deletions(-) diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index 5cb4a36b0ed..961ca5da2e9 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -599,6 +599,8 @@ impl SlashingDatabase { Ok(safe) } + // TODO(attn-slash) we could do a bulk check and insert using a single write txn + // instead of doing this process individually and opening and closing a bunch of db txns // pub fn batch_check_and_insert_attestations( // &self, // attestations_to_check: Vec<(Attestation, DutyAndProof)>, diff --git a/validator_client/src/attestation_data_service.rs b/validator_client/src/attestation_data_service.rs index 755e52a2efd..8f6d973dd8b 100644 --- a/validator_client/src/attestation_data_service.rs +++ b/validator_client/src/attestation_data_service.rs @@ -11,35 +11,33 @@ use crate::{ /// The AttestationDataService is responsible for downloading and caching attestation data at a given slot /// for a range of committee indexes. It also helps prevent us from re-downloading identical attestation data. pub struct AttestationDataService { - attestation_data_by_committee: HashMap, + attestation_data: Option, beacon_nodes: Arc>, } impl AttestationDataService { pub fn new(beacon_nodes: Arc>) -> Self { Self { - attestation_data_by_committee: HashMap::new(), + attestation_data: None, beacon_nodes, } } - /// Get previously downloaded attestation data by a given committee index. If the Electra fork is enabled - /// we don't care about the committee index + /// Get previously downloaded attestation data. If the Electra fork is enabled + /// we don't care about the committee index. If we're pre-Electra, pub fn get_data_by_committee_index( &self, committee_index: &CommitteeIndex, fork_name: &ForkName, ) -> Option { if fork_name.electra_enabled() { - let data = self.attestation_data_by_committee.iter().next(); - if let Some((_, data)) = data { - return Some(data.clone()); - } - None + self.attestation_data.clone() } else { - self.attestation_data_by_committee - .get(committee_index) - .cloned() + let Some(mut attestation_data) = self.attestation_data.clone() else { + return None; + }; + attestation_data.index = *committee_index; + return Some(attestation_data) } } @@ -50,8 +48,7 @@ impl AttestationDataService { slot: &Slot, fork_name: &ForkName, ) -> Result<(), String> { - // If we've already downloaded data for this committee index OR electra is enabled and - // we've already downloaded data for this slot, there's no need to re-download the data. + // If we've already downloaded attestation data for this slot, there's no need to re-download the data. if let Some(_) = self.get_data_by_committee_index(committee_index, fork_name) { return Ok(()); } @@ -76,8 +73,7 @@ impl AttestationDataService { .await .map_err(|e| e.to_string())?; - self.attestation_data_by_committee - .insert(*committee_index, attestation_data); + self.attestation_data = Some(attestation_data); Ok(()) } From 14cbae84e062d259b5143cc3f0de033ecaadd9ce Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 5 Aug 2024 15:48:44 -0700 Subject: [PATCH 04/22] batch db txn --- Cargo.lock | 1 + beacon_node/http_api/tests/fork_tests.rs | 5 +- beacon_node/http_api/tests/tests.rs | 5 +- testing/web3signer_tests/src/lib.rs | 10 +- validator_client/Cargo.toml | 1 + .../src/interchange_test.rs | 9 + .../slashing_protection/src/parallel_tests.rs | 20 +- .../src/slashing_database.rs | 29 +-- .../slashing_protection/src/test_utils.rs | 26 ++- .../src/attestation_data_service.rs | 9 +- validator_client/src/attestation_service.rs | 111 +++++---- .../src/http_api/tests/keystores.rs | 6 +- validator_client/src/validator_store.rs | 221 ++++++++++-------- 13 files changed, 276 insertions(+), 177 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9afb3635f12..2ab1730c13f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9060,6 +9060,7 @@ dependencies = [ "malloc_utils", "monitoring_api", "parking_lot 0.12.3", + "r2d2_sqlite", "rand", "reqwest", "ring 0.16.20", diff --git a/beacon_node/http_api/tests/fork_tests.rs b/beacon_node/http_api/tests/fork_tests.rs index 5a38cca19c3..305308d51c2 100644 --- a/beacon_node/http_api/tests/fork_tests.rs +++ b/beacon_node/http_api/tests/fork_tests.rs @@ -156,7 +156,10 @@ async fn attestations_across_fork_with_skip_slots() { .await .unwrap(); client - .post_beacon_pool_attestations_v2(&unaggregated_attestations.iter().collect::>(), fork_name) + .post_beacon_pool_attestations_v2( + &unaggregated_attestations.iter().collect::>(), + fork_name, + ) .await .unwrap(); diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index 8fed1699824..f2fab5edd72 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -1723,7 +1723,10 @@ impl ApiTester { .map(|att| self.chain.spec.fork_name_at_slot::(att.data().slot)) .unwrap(); self.client - .post_beacon_pool_attestations_v2(&self.attestations.iter().collect::>(), fork_name) + .post_beacon_pool_attestations_v2( + &self.attestations.iter().collect::>(), + fork_name, + ) .await .unwrap(); assert!( diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index 13d92d2d855..04c9ac3554c 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -602,7 +602,7 @@ mod tests { .assert_signatures_match("attestation", |pubkey, validator_store| async move { let mut attestation = get_attestation(); validator_store - .sign_attestation(pubkey, 0, &mut attestation, Epoch::new(0)) + .sign_attestation_v2(pubkey, 0, &mut attestation, Epoch::new(0)) .await .unwrap(); attestation @@ -827,7 +827,7 @@ mod tests { .assert_signatures_match("first_attestation", |pubkey, validator_store| async move { let mut attestation = first_attestation(); validator_store - .sign_attestation(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) .await .unwrap(); attestation @@ -838,7 +838,7 @@ mod tests { move |pubkey, validator_store| async move { let mut attestation = double_vote_attestation(); validator_store - .sign_attestation(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) .await }, slashable_message_should_sign, @@ -849,7 +849,7 @@ mod tests { move |pubkey, validator_store| async move { let mut attestation = surrounding_attestation(); validator_store - .sign_attestation(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) .await }, slashable_message_should_sign, @@ -860,7 +860,7 @@ mod tests { move |pubkey, validator_store| async move { let mut attestation = surrounded_attestation(); validator_store - .sign_attestation(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) .await }, slashable_message_should_sign, diff --git a/validator_client/Cargo.toml b/validator_client/Cargo.toml index bff40b41d5f..c5356a46e9b 100644 --- a/validator_client/Cargo.toml +++ b/validator_client/Cargo.toml @@ -47,6 +47,7 @@ warp = { workspace = true } hyper = { workspace = true } ethereum_serde_utils = { workspace = true } libsecp256k1 = { workspace = true } +r2d2_sqlite = "0.21.0" ring = { workspace = true } rand = { workspace = true, features = ["small_rng"] } lighthouse_metrics = { workspace = true } diff --git a/validator_client/slashing_protection/src/interchange_test.rs b/validator_client/slashing_protection/src/interchange_test.rs index d99647bc936..b07574cddf5 100644 --- a/validator_client/slashing_protection/src/interchange_test.rs +++ b/validator_client/slashing_protection/src/interchange_test.rs @@ -3,6 +3,7 @@ use crate::{ test_utils::{pubkey, DEFAULT_GENESIS_VALIDATORS_ROOT}, SigningRoot, SlashingDatabase, }; +use rusqlite::TransactionBehavior; use serde::{Deserialize, Serialize}; use std::collections::HashSet; use tempfile::tempdir; @@ -132,12 +133,18 @@ impl MultiTestCase { } } + let mut conn = slashing_db.get_db_connection().unwrap(); + let txn = conn + .transaction_with_behavior(TransactionBehavior::Exclusive) + .unwrap(); + for (i, att) in test_case.attestations.iter().enumerate() { match slashing_db.check_and_insert_attestation_signing_root( &att.pubkey, att.source_epoch, att.target_epoch, SigningRoot::from(att.signing_root), + &txn, ) { Ok(safe) if !att.should_succeed => { panic!( @@ -154,6 +161,8 @@ impl MultiTestCase { _ => (), } } + + slashing_db.commit(txn).unwrap(); } } } diff --git a/validator_client/slashing_protection/src/parallel_tests.rs b/validator_client/slashing_protection/src/parallel_tests.rs index e3cc1a0d567..90346e88237 100644 --- a/validator_client/slashing_protection/src/parallel_tests.rs +++ b/validator_client/slashing_protection/src/parallel_tests.rs @@ -6,6 +6,7 @@ use crate::block_tests::block; use crate::test_utils::*; use crate::*; use rayon::prelude::*; +use rusqlite::TransactionBehavior; use tempfile::tempdir; #[test] @@ -44,11 +45,18 @@ fn attestation_same_target() { let results = (0..num_attestations) .into_par_iter() .map(|i| { - slashing_db.check_and_insert_attestation( + let mut conn = slashing_db.get_db_connection().unwrap(); + let txn = conn + .transaction_with_behavior(TransactionBehavior::Exclusive) + .unwrap(); + let result = slashing_db.check_and_insert_attestation( &pk, &attestation_data_builder(i, num_attestations), DEFAULT_DOMAIN, - ) + &txn, + ); + slashing_db.commit(txn).unwrap(); + result }) .collect::>(); @@ -72,8 +80,14 @@ fn attestation_surround_fest() { let results = (0..num_attestations) .into_par_iter() .map(|i| { + let mut conn = slashing_db.get_db_connection().unwrap(); + let txn = conn + .transaction_with_behavior(TransactionBehavior::Exclusive) + .unwrap(); let att = attestation_data_builder(i, 2 * num_attestations - i); - slashing_db.check_and_insert_attestation(&pk, &att, DEFAULT_DOMAIN) + let result = slashing_db.check_and_insert_attestation(&pk, &att, DEFAULT_DOMAIN, &txn); + slashing_db.commit(txn).unwrap(); + result }) .collect::>(); diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index 961ca5da2e9..f5c368cfa75 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -6,12 +6,13 @@ use crate::signed_attestation::InvalidAttestation; use crate::signed_block::InvalidBlock; use crate::{signing_root_from_row, NotSafe, Safe, SignedAttestation, SignedBlock, SigningRoot}; use filesystem::restrict_file_permissions; +use r2d2::PooledConnection; use r2d2_sqlite::SqliteConnectionManager; use rusqlite::{params, OptionalExtension, Transaction, TransactionBehavior}; use std::fs::File; use std::path::Path; use std::time::Duration; -use types::{Attestation, AttestationData, BeaconBlockHeader, Epoch, Hash256, PublicKeyBytes, SignedRoot, Slot}; +use types::{AttestationData, BeaconBlockHeader, Epoch, Hash256, PublicKeyBytes, SignedRoot, Slot}; type Pool = r2d2::Pool; @@ -599,14 +600,15 @@ impl SlashingDatabase { Ok(safe) } - // TODO(attn-slash) we could do a bulk check and insert using a single write txn - // instead of doing this process individually and opening and closing a bunch of db txns - // pub fn batch_check_and_insert_attestations( - // &self, - // attestations_to_check: Vec<(Attestation, DutyAndProof)>, - // ) { - - // } + pub fn get_db_connection(&self) -> Result, NotSafe> { + let conn = self.conn_pool.get()?; + Ok(conn) + } + + pub fn commit(&self, txn: Transaction) -> Result<(), NotSafe> { + txn.commit()?; + Ok(()) + } /// Check an attestation for slash safety, and if it is safe, record it in the database. /// @@ -619,6 +621,7 @@ impl SlashingDatabase { validator_pubkey: &PublicKeyBytes, attestation: &AttestationData, domain: Hash256, + txn: &Transaction, ) -> Result { let attestation_signing_root = attestation.signing_root(domain).into(); self.check_and_insert_attestation_signing_root( @@ -626,6 +629,7 @@ impl SlashingDatabase { attestation.source.epoch, attestation.target.epoch, attestation_signing_root, + txn, ) } @@ -636,17 +640,15 @@ impl SlashingDatabase { att_source_epoch: Epoch, att_target_epoch: Epoch, att_signing_root: SigningRoot, + txn: &Transaction, ) -> Result { - let mut conn = self.conn_pool.get()?; - let txn = conn.transaction_with_behavior(TransactionBehavior::Exclusive)?; let safe = self.check_and_insert_attestation_signing_root_txn( validator_pubkey, att_source_epoch, att_target_epoch, att_signing_root, - &txn, + txn, )?; - txn.commit()?; Ok(safe) } @@ -854,6 +856,7 @@ impl SlashingDatabase { ) -> Result { let mut conn = self.conn_pool.get()?; let txn = &conn.transaction()?; + println!("hmm.."); self.export_interchange_info_in_txn(genesis_validators_root, selected_pubkeys, txn) } diff --git a/validator_client/slashing_protection/src/test_utils.rs b/validator_client/slashing_protection/src/test_utils.rs index efdeb9bc6ba..b50ea852335 100644 --- a/validator_client/slashing_protection/src/test_utils.rs +++ b/validator_client/slashing_protection/src/test_utils.rs @@ -1,4 +1,5 @@ use crate::*; +use rusqlite::TransactionBehavior; use tempfile::{tempdir, TempDir}; use types::{test_utils::generate_deterministic_keypair, AttestationData, BeaconBlockHeader}; @@ -82,15 +83,32 @@ impl StreamTest { check_registration_invariants(&slashing_db, &self.registered_validators); + println!("1"); + let mut conn = slashing_db.get_db_connection().unwrap(); + println!("2"); + + let txn = conn + .transaction_with_behavior(TransactionBehavior::Exclusive) + .unwrap(); + for (i, test) in self.cases.iter().enumerate() { assert_eq!( - slashing_db.check_and_insert_attestation(&test.pubkey, &test.data, test.domain), + slashing_db.check_and_insert_attestation( + &test.pubkey, + &test.data, + test.domain, + &txn + ), test.expected, "attestation {} not processed as expected", i ); } + println!("3"); + slashing_db.commit(txn).unwrap(); + println!("4"); + drop(conn); roundtrip_database(&dir, &slashing_db, self.registered_validators.is_empty()); } } @@ -123,18 +141,22 @@ impl StreamTest { // This function roundtrips the database, but applies minification in order to be compatible with // the implicit minification done on import. fn roundtrip_database(dir: &TempDir, db: &SlashingDatabase, is_empty: bool) { + println!("a"); let exported = db .export_all_interchange_info(DEFAULT_GENESIS_VALIDATORS_ROOT) .unwrap(); + println!("b"); let new_db = SlashingDatabase::create(&dir.path().join("roundtrip_slashing_protection.sqlite")).unwrap(); + println!("c"); new_db .import_interchange_info(exported.clone(), DEFAULT_GENESIS_VALIDATORS_ROOT) .unwrap(); + println!("d"); let reexported = new_db .export_all_interchange_info(DEFAULT_GENESIS_VALIDATORS_ROOT) .unwrap(); - + println!("e"); assert!(exported .minify() .unwrap() diff --git a/validator_client/src/attestation_data_service.rs b/validator_client/src/attestation_data_service.rs index 8f6d973dd8b..5a0ab244447 100644 --- a/validator_client/src/attestation_data_service.rs +++ b/validator_client/src/attestation_data_service.rs @@ -8,8 +8,8 @@ use crate::{ http_metrics::metrics, }; -/// The AttestationDataService is responsible for downloading and caching attestation data at a given slot -/// for a range of committee indexes. It also helps prevent us from re-downloading identical attestation data. +/// The AttestationDataService is responsible for downloading and caching attestation data at a given slot. +/// It also helps prevent us from re-downloading identical attestation data. pub struct AttestationDataService { attestation_data: Option, beacon_nodes: Arc>, @@ -24,7 +24,8 @@ impl AttestationDataService { } /// Get previously downloaded attestation data. If the Electra fork is enabled - /// we don't care about the committee index. If we're pre-Electra, + /// we don't care about the committee index. If we're pre-Electra, we insert + /// the correct committee index. pub fn get_data_by_committee_index( &self, committee_index: &CommitteeIndex, @@ -37,7 +38,7 @@ impl AttestationDataService { return None; }; attestation_data.index = *committee_index; - return Some(attestation_data) + return Some(attestation_data); } } diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index d5d88db7354..bcaced594b0 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -199,21 +199,43 @@ impl AttestationService { map }); - let this = self.clone(); + // Signs unaggregated attestations and broadcasts them to the network. + // Downloads aggregated attestations, signs them, and broadcasts to the network. + self.spawn_attestation_sign_and_broadcast_task(duties_by_committee_index, slot, fork_name); + + // Schedule pruning of the slashing protection database once all unaggregated + // attestations have (hopefully) been signed, i.e. at the same time as aggregate + // production. + self.spawn_slashing_protection_pruning_task(slot, aggregate_production_instant); + + Ok(()) + } + + /// Spawn a blocking task to run the attestation signing and broadcasting process + /// for both unaggregated and aggregated attestations. + fn spawn_attestation_sign_and_broadcast_task( + &self, + duties_by_committee_index: HashMap>, + slot: Slot, + fork_name: ForkName, + ) { + let inner_self = self.clone(); self.inner.context.executor.spawn( async move { - let log = this.context.log().clone(); + let log = inner_self.context.log().clone(); let mut attestation_data_service = - AttestationDataService::new(this.beacon_nodes.clone()); + AttestationDataService::new(inner_self.beacon_nodes.clone()); - this.produce_attestation_data( - &mut attestation_data_service, - &duties_by_committee_index, - &slot, - &fork_name - ).await; + inner_self + .produce_attestation_data( + &mut attestation_data_service, + &duties_by_committee_index, + &slot, + &fork_name, + ) + .await; let mut handles = vec![]; @@ -225,17 +247,18 @@ impl AttestationService { if let Some(attestation_data) = attestation_data_service .get_data_by_committee_index(&committee_index, &fork_name) { - let that = this.clone(); + let this = inner_self.clone(); // Have the validator sign the attestation. - let handle = this.inner.context.executor.spawn_blocking_handle( - move || { - that.sign_attestation( - attestation_data, - validator_duty.clone(), - ) - }, - "Sign attestation", - ); + let handle = + inner_self.inner.context.executor.spawn_blocking_handle( + move || { + this.sign_attestation( + attestation_data, + validator_duty.clone(), + ) + }, + "Sign attestation", + ); if let Some(handle) = handle { handles.push(handle); @@ -259,17 +282,21 @@ impl AttestationService { } // Check that the signed attestations are not slash-able (if slash-ability checks are enabled). - let Ok(safe_attestations) = this + let Ok(safe_attestations) = inner_self .validator_store .check_and_insert_attestations(signed_attestations) else { + // TODO(attn-slash) add logging return (); }; - match this.publish_attestations( - &safe_attestations.iter().map(|(a, _)| a).collect(), - fork_name, - ).await { + match inner_self + .publish_attestations( + &safe_attestations.iter().map(|(a, _)| a).collect(), + fork_name, + ) + .await + { Ok(_) => (), Err(_) => (), // TODO(attn-slash) add logging }; @@ -280,14 +307,16 @@ impl AttestationService { if let Some(attestation_data) = attestation_data_service .get_data_by_committee_index(committee_index, &fork_name) { - match this.produce_and_publish_aggregates( - &attestation_data, - *committee_index, - &validator_duties, - ) - .await { + match inner_self + .produce_and_publish_aggregates( + &attestation_data, + *committee_index, + &validator_duties, + ) + .await + { Ok(_) => (), - Err(_) => () // TODO(attn-slash) log a crit + Err(_) => (), // TODO(attn-slash) log a crit }; } else { // TODO(attn-slash) log a crit? @@ -296,32 +325,26 @@ impl AttestationService { }, "Download and sign attestations", ); - - // Schedule pruning of the slashing protection database once all unaggregated - // attestations have (hopefully) been signed, i.e. at the same time as aggregate - // production. - self.spawn_slashing_protection_pruning_task(slot, aggregate_production_instant); - - Ok(()) } /// Performs the first step of the attesting process: downloading `AttestationData` objects. - /// Pre Electra: Only one `AttestationData` is downloaded from the BN for each committee index. + /// Pre Electra: Only one `AttestationData` is downloaded from the BN for each committee index. /// Post Electra: Only one `AttestationData` is downloaded from the BN for each slot. pub async fn produce_attestation_data( &self, attestation_data_service: &mut AttestationDataService, duties_by_committee_index: &HashMap>, slot: &Slot, - fork_name: &ForkName + fork_name: &ForkName, ) { for (committee_index, _) in duties_by_committee_index.iter() { match attestation_data_service .download_data(committee_index, slot, fork_name) - .await { - Ok(_) => (), - Err(_) => (), // TODO(attn-slash) add logging - }; + .await + { + Ok(_) => (), + Err(_) => (), // TODO(attn-slash) add logging + }; } } diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index b6923d1c788..c819c55db86 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -1097,7 +1097,7 @@ async fn generic_migration_test( let current_epoch = attestation.data().target.epoch; tester1 .validator_store - .sign_attestation(public_key, 0, &mut attestation, current_epoch) + .sign_attestation_v2(public_key, 0, &mut attestation, current_epoch) .await .unwrap(); } @@ -1173,7 +1173,7 @@ async fn generic_migration_test( let current_epoch = attestation.data().target.epoch; match tester2 .validator_store - .sign_attestation(public_key, 0, &mut attestation, current_epoch) + .sign_attestation_v2(public_key, 0, &mut attestation, current_epoch) .await { Ok(()) => assert!(should_succeed), @@ -1306,7 +1306,7 @@ async fn delete_concurrent_with_signing() { let mut att = make_attestation(j, j + 1); for (_validator_id, public_key) in thread_pubkeys.iter().enumerate() { let _ = validator_store - .sign_attestation(*public_key, 0, &mut att, Epoch::new(j + 1)) + .sign_attestation_v2(*public_key, 0, &mut att, Epoch::new(j + 1)) .await; } } diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 4762f3b6045..5a3096ef160 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -8,6 +8,7 @@ use crate::{ }; use account_utils::validator_definitions::{PasswordStorage, ValidatorDefinition}; use parking_lot::{Mutex, RwLock}; +use r2d2_sqlite::rusqlite::TransactionBehavior; use slashing_protection::{ interchange::Interchange, InterchangeError, NotSafe, Safe, SlashingDatabase, }; @@ -41,6 +42,7 @@ pub enum Error { GreaterThanCurrentEpoch { epoch: Epoch, current_epoch: Epoch }, UnableToSignAttestation(AttestationError), UnableToSign(SigningError), + SlashingDbError, } impl From for Error { @@ -674,98 +676,98 @@ impl ValidatorStore { Ok(()) } - pub async fn sign_attestation( - &self, - validator_pubkey: PublicKeyBytes, - validator_committee_position: usize, - attestation: &mut Attestation, - current_epoch: Epoch, - ) -> Result<(), Error> { - // Make sure the target epoch is not higher than the current epoch to avoid potential attacks. - if attestation.data().target.epoch > current_epoch { - return Err(Error::GreaterThanCurrentEpoch { - epoch: attestation.data().target.epoch, - current_epoch, - }); - } - - // Get the signing method and check doppelganger protection. - let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; - - // Checking for slashing conditions. - let signing_epoch = attestation.data().target.epoch; - let signing_context = self.signing_context(Domain::BeaconAttester, signing_epoch); - let domain_hash = signing_context.domain_hash(&self.spec); - let slashing_status = if signing_method - .requires_local_slashing_protection(self.enable_web3signer_slashing_protection) - { - self.slashing_protection.check_and_insert_attestation( - &validator_pubkey, - attestation.data(), - domain_hash, - ) - } else { - Ok(Safe::Valid) - }; - - match slashing_status { - // We can safely sign this attestation. - Ok(Safe::Valid) => { - let signature = signing_method - .get_signature::>( - SignableMessage::AttestationData(attestation.data()), - signing_context, - &self.spec, - &self.task_executor, - ) - .await?; - attestation - .add_signature(&signature, validator_committee_position) - .map_err(Error::UnableToSignAttestation)?; - - metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); - - Ok(()) - } - Ok(Safe::SameData) => { - warn!( - self.log, - "Skipping signing of previously signed attestation" - ); - metrics::inc_counter_vec( - &metrics::SIGNED_ATTESTATIONS_TOTAL, - &[metrics::SAME_DATA], - ); - Err(Error::SameData) - } - Err(NotSafe::UnregisteredValidator(pk)) => { - warn!( - self.log, - "Not signing attestation for unregistered validator"; - "msg" => "Carefully consider running with --init-slashing-protection (see --help)", - "public_key" => format!("{:?}", pk) - ); - metrics::inc_counter_vec( - &metrics::SIGNED_ATTESTATIONS_TOTAL, - &[metrics::UNREGISTERED], - ); - Err(Error::Slashable(NotSafe::UnregisteredValidator(pk))) - } - Err(e) => { - crit!( - self.log, - "Not signing slashable attestation"; - "attestation" => format!("{:?}", attestation.data()), - "error" => format!("{:?}", e) - ); - metrics::inc_counter_vec( - &metrics::SIGNED_ATTESTATIONS_TOTAL, - &[metrics::SLASHABLE], - ); - Err(Error::Slashable(e)) - } - } - } + // pub async fn sign_attestation( + // &self, + // validator_pubkey: PublicKeyBytes, + // validator_committee_position: usize, + // attestation: &mut Attestation, + // current_epoch: Epoch, + // ) -> Result<(), Error> { + // // Make sure the target epoch is not higher than the current epoch to avoid potential attacks. + // if attestation.data().target.epoch > current_epoch { + // return Err(Error::GreaterThanCurrentEpoch { + // epoch: attestation.data().target.epoch, + // current_epoch, + // }); + // } + + // // Get the signing method and check doppelganger protection. + // let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; + + // // Checking for slashing conditions. + // let signing_epoch = attestation.data().target.epoch; + // let signing_context = self.signing_context(Domain::BeaconAttester, signing_epoch); + // let domain_hash = signing_context.domain_hash(&self.spec); + // let slashing_status = if signing_method + // .requires_local_slashing_protection(self.enable_web3signer_slashing_protection) + // { + // self.slashing_protection.check_and_insert_attestation( + // &validator_pubkey, + // attestation.data(), + // domain_hash, + // ) + // } else { + // Ok(Safe::Valid) + // }; + + // match slashing_status { + // // We can safely sign this attestation. + // Ok(Safe::Valid) => { + // let signature = signing_method + // .get_signature::>( + // SignableMessage::AttestationData(attestation.data()), + // signing_context, + // &self.spec, + // &self.task_executor, + // ) + // .await?; + // attestation + // .add_signature(&signature, validator_committee_position) + // .map_err(Error::UnableToSignAttestation)?; + + // metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); + + // Ok(()) + // } + // Ok(Safe::SameData) => { + // warn!( + // self.log, + // "Skipping signing of previously signed attestation" + // ); + // metrics::inc_counter_vec( + // &metrics::SIGNED_ATTESTATIONS_TOTAL, + // &[metrics::SAME_DATA], + // ); + // Err(Error::SameData) + // } + // Err(NotSafe::UnregisteredValidator(pk)) => { + // warn!( + // self.log, + // "Not signing attestation for unregistered validator"; + // "msg" => "Carefully consider running with --init-slashing-protection (see --help)", + // "public_key" => format!("{:?}", pk) + // ); + // metrics::inc_counter_vec( + // &metrics::SIGNED_ATTESTATIONS_TOTAL, + // &[metrics::UNREGISTERED], + // ); + // Err(Error::Slashable(NotSafe::UnregisteredValidator(pk))) + // } + // Err(e) => { + // crit!( + // self.log, + // "Not signing slashable attestation"; + // "attestation" => format!("{:?}", attestation.data()), + // "error" => format!("{:?}", e) + // ); + // metrics::inc_counter_vec( + // &metrics::SIGNED_ATTESTATIONS_TOTAL, + // &[metrics::SLASHABLE], + // ); + // Err(Error::Slashable(e)) + // } + // } + // } pub fn check_and_insert_attestations( &self, @@ -792,6 +794,15 @@ impl ValidatorStore { } } + let Ok(mut conn) = self.slashing_protection.get_db_connection() else { + // TODO(attn-slash) crit log + return Err(Error::SlashingDbError); + }; + let Ok(txn) = conn.transaction_with_behavior(TransactionBehavior::Exclusive) else { + // TODO(attn-slash) crit log + return Err(Error::SlashingDbError); + }; + for (attestation, validator_duty, domain_hash) in attestations_to_check { let validator_pubkey = &validator_duty.duty.pubkey; @@ -799,19 +810,20 @@ impl ValidatorStore { &validator_pubkey, attestation.data(), domain_hash, + &txn, ); match slashing_status { Ok(Safe::Valid) => { safe_attestations.push((attestation.clone(), validator_duty.clone())); - metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); - }, - Ok(Safe::SameData) => { - warn!( - self.log, - "Skipping previously signed attestation" + metrics::inc_counter_vec( + &metrics::SIGNED_ATTESTATIONS_TOTAL, + &[metrics::SUCCESS], ); - }, + } + Ok(Safe::SameData) => { + warn!(self.log, "Skipping previously signed attestation"); + } Err(NotSafe::UnregisteredValidator(pk)) => { warn!( self.log, @@ -823,7 +835,7 @@ impl ValidatorStore { &metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::UNREGISTERED], ); - }, + } Err(e) => { crit!( self.log, @@ -835,9 +847,16 @@ impl ValidatorStore { &metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SLASHABLE], ); - }, + } }; } + + // TODO(attn-slash) add logging + match self.slashing_protection.commit(txn) { + Err(_) => (), + Ok(_) => (), + }; + Ok(safe_attestations) } From 9b1302ab1fa93e1aad3542088a7c003d5b0815d6 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 6 Aug 2024 16:50:56 -0700 Subject: [PATCH 05/22] fix some tests, linting, logs --- testing/web3signer_tests/src/lib.rs | 10 +- .../src/slashing_database.rs | 39 ---- .../src/attestation_data_service.rs | 11 +- validator_client/src/attestation_service.rs | 106 ++++++--- .../src/http_api/tests/keystores.rs | 110 ++++++++- validator_client/src/http_metrics/metrics.rs | 2 +- validator_client/src/validator_store.rs | 220 +++++------------- 7 files changed, 247 insertions(+), 251 deletions(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index 04c9ac3554c..13d92d2d855 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -602,7 +602,7 @@ mod tests { .assert_signatures_match("attestation", |pubkey, validator_store| async move { let mut attestation = get_attestation(); validator_store - .sign_attestation_v2(pubkey, 0, &mut attestation, Epoch::new(0)) + .sign_attestation(pubkey, 0, &mut attestation, Epoch::new(0)) .await .unwrap(); attestation @@ -827,7 +827,7 @@ mod tests { .assert_signatures_match("first_attestation", |pubkey, validator_store| async move { let mut attestation = first_attestation(); validator_store - .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await .unwrap(); attestation @@ -838,7 +838,7 @@ mod tests { move |pubkey, validator_store| async move { let mut attestation = double_vote_attestation(); validator_store - .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await }, slashable_message_should_sign, @@ -849,7 +849,7 @@ mod tests { move |pubkey, validator_store| async move { let mut attestation = surrounding_attestation(); validator_store - .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await }, slashable_message_should_sign, @@ -860,7 +860,7 @@ mod tests { move |pubkey, validator_store| async move { let mut attestation = surrounded_attestation(); validator_store - .sign_attestation_v2(pubkey, 0, &mut attestation, current_epoch) + .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await }, slashable_message_should_sign, diff --git a/validator_client/slashing_protection/src/slashing_database.rs b/validator_client/slashing_protection/src/slashing_database.rs index f5c368cfa75..f754d6fd020 100644 --- a/validator_client/slashing_protection/src/slashing_database.rs +++ b/validator_client/slashing_protection/src/slashing_database.rs @@ -652,45 +652,6 @@ impl SlashingDatabase { Ok(safe) } - // TODO(attn-slash) previously I had separated checking the signing root - // add inserting. But this doesn't seem necessary and seems to add a bit of unneeded complexity - // will review this before deleting. - // pub fn check_attestation_signing_root( - // &self, - // validator_pubkey: &PublicKeyBytes, - // attestation: &AttestationData, - // domain: Hash256, - // ) -> Result { - // let attestation_signing_root = attestation.signing_root(domain).into(); - // let mut conn = self.conn_pool.get()?; - // let txn = conn.transaction_with_behavior(TransactionBehavior::Deferred)?; - // self.check_attestation( - // &txn, - // validator_pubkey, - // attestation.source.epoch, - // attestation.target.epoch, - // attestation_signing_root, - // ) - // } - - // pub fn insert_attestation_signing_root( - // &self, - // validator_pubkey: &PublicKeyBytes, - // attestation: &AttestationData, - // domain: Hash256, - // ) -> Result<(), NotSafe> { - // let attestation_signing_root = attestation.signing_root(domain).into(); - // let mut conn = self.conn_pool.get()?; - // let txn = conn.transaction_with_behavior(TransactionBehavior::Exclusive)?; - // self.insert_attestation( - // &txn, - // validator_pubkey, - // attestation.source.epoch, - // attestation.target.epoch, - // attestation_signing_root, - // ) - // } - /// Transactional variant of `check_and_insert_attestation_signing_root`. fn check_and_insert_attestation_signing_root_txn( &self, diff --git a/validator_client/src/attestation_data_service.rs b/validator_client/src/attestation_data_service.rs index 5a0ab244447..e3fce1f4279 100644 --- a/validator_client/src/attestation_data_service.rs +++ b/validator_client/src/attestation_data_service.rs @@ -1,4 +1,4 @@ -use std::{collections::HashMap, sync::Arc}; +use std::sync::Arc; use slot_clock::SlotClock; use types::{AttestationData, CommitteeIndex, EthSpec, ForkName, Slot}; @@ -38,7 +38,7 @@ impl AttestationDataService { return None; }; attestation_data.index = *committee_index; - return Some(attestation_data); + Some(attestation_data) } } @@ -50,7 +50,10 @@ impl AttestationDataService { fork_name: &ForkName, ) -> Result<(), String> { // If we've already downloaded attestation data for this slot, there's no need to re-download the data. - if let Some(_) = self.get_data_by_committee_index(committee_index, fork_name) { + if self + .get_data_by_committee_index(committee_index, fork_name) + .is_some() + { return Ok(()); } @@ -65,7 +68,7 @@ impl AttestationDataService { &[metrics::ATTESTATIONS_HTTP_GET], ); beacon_node - .get_validator_attestation_data(slot.clone(), *committee_index) + .get_validator_attestation_data(*slot, *committee_index) .await .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) .map(|result| result.data) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index bcaced594b0..f4ab6fbc6d6 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -221,6 +221,12 @@ impl AttestationService { ) { let inner_self = self.clone(); + // TODO(attn-slash) more granular metric names + let attestations_timer = metrics::start_timer_vec( + &metrics::ATTESTATION_SERVICE_TIMES, + &[metrics::ATTESTATIONS], + ); + self.inner.context.executor.spawn( async move { let log = inner_self.context.log().clone(); @@ -264,7 +270,12 @@ impl AttestationService { handles.push(handle); } } else { - // TODO(attn-slash) log a crit? + crit!( + log, + "Failed to fetch attestation data"; + "committee_index" => format!("{:?}",&committee_index), + "slot" => format!("{}", &slot), + ) } }) }, @@ -272,33 +283,46 @@ impl AttestationService { let mut signed_attestations = vec![]; - for handle in handles { - if let Ok(result) = handle.await { - let Ok(result) = result.await else { return () }; - if let Some(result) = result { - signed_attestations.push(result); - } + let results = join_all(handles).await; + + for result in results.into_iter().flatten() { + if let Ok(Some(result)) = result.await { + signed_attestations.push(result); } } // Check that the signed attestations are not slash-able (if slash-ability checks are enabled). - let Ok(safe_attestations) = inner_self + let safe_attestations = match inner_self .validator_store .check_and_insert_attestations(signed_attestations) - else { - // TODO(attn-slash) add logging - return (); + { + Ok(attestations) => attestations, + Err(e) => { + crit!( + log, + "An error occurred when checking for slashable attestations"; + "error" => format!("{:?}", e) + ); + return; + } }; match inner_self .publish_attestations( - &safe_attestations.iter().map(|(a, _)| a).collect(), + &safe_attestations.iter().map(|(a, _)| a).collect::>(), fork_name, ) .await { Ok(_) => (), - Err(_) => (), // TODO(attn-slash) add logging + Err(e) => { + crit!( + log, + "Failed to broadcast signed attestations"; + "slot" => format!("{}", slot), + "error" => format!("{:?}", e), + ); + } }; // Create and publish `SignedAggregateAndProof` for all aggregating validators. @@ -311,20 +335,34 @@ impl AttestationService { .produce_and_publish_aggregates( &attestation_data, *committee_index, - &validator_duties, + validator_duties, ) .await { Ok(_) => (), - Err(_) => (), // TODO(attn-slash) log a crit + Err(e) => { + crit!( + log, + "Failed to produce and publish attestation aggregates"; + "slot" => format!("{}", slot), + "error" => format!("{:?}", e), + ); + } }; } else { - // TODO(attn-slash) log a crit? + crit!( + log, + "Failed to fetch attestation data"; + "committee_index" => format!("{:?}",&committee_index), + "slot" => format!("{}", &slot), + ) } } }, "Download and sign attestations", ); + + drop(attestations_timer); } /// Performs the first step of the attesting process: downloading `AttestationData` objects. @@ -337,13 +375,22 @@ impl AttestationService { slot: &Slot, fork_name: &ForkName, ) { + let log = self.context.log().clone(); for (committee_index, _) in duties_by_committee_index.iter() { match attestation_data_service .download_data(committee_index, slot, fork_name) .await { Ok(_) => (), - Err(_) => (), // TODO(attn-slash) add logging + Err(e) => { + crit!( + log, + "Failed to download attestation data"; + "slot" => format!("{}", slot), + "committee_index" => format!("{}", committee_index), + "error" => format!("{:?}", e) + ); + } }; } } @@ -364,12 +411,6 @@ impl AttestationService { ) -> Result, DutyAndProof)>, String> { let log = self.context.log(); - // TODO(attn-slash) more granular metric names - let attestations_timer = metrics::start_timer_vec( - &metrics::ATTESTATION_SERVICE_TIMES, - &[metrics::ATTESTATIONS], - ); - let current_epoch = self .slot_clock .now() @@ -415,7 +456,7 @@ impl AttestationService { let signed_attestation = match self .validator_store - .sign_attestation_v2( + .sign_attestation( validator_duty.duty.pubkey, validator_duty.duty.validator_committee_index as usize, &mut attestation, @@ -439,14 +480,13 @@ impl AttestationService { None } Err(e) => { - // crit!( - // log, - // "Failed to sign attestation"; - // "error" => ?e, - // "validator" => ?validator_duty.duty.pubkey, - // "committee_index" => validator_duty.duty.committee_index,, - // "slot" => validator_duty.duty.slot.as_u64(), - // ); + crit!( + log, + "Failed to sign attestation"; + "error" => ?e, + "validator" => ?validator_duty.duty.pubkey, + "slot" => validator_duty.duty.slot.as_u64(), + ); None } }; @@ -463,7 +503,7 @@ impl AttestationService { /// attestations that were deemed "safe". pub async fn publish_attestations( &self, - attestations: &Vec<&Attestation>, + attestations: &[&Attestation], fork_name: ForkName, ) -> Result<(), String> { self.beacon_nodes diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index c819c55db86..dfb719e458e 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -1,5 +1,6 @@ use super::super::super::validator_store::DEFAULT_GAS_LIMIT; use super::*; +use crate::duties_service::DutyAndProof; use account_utils::random_password_string; use bls::PublicKeyBytes; use eth2::lighthouse_vc::types::UpdateFeeRecipientRequest; @@ -8,6 +9,7 @@ use eth2::lighthouse_vc::{ std_types::{KeystoreJsonStr as Keystore, *}, types::Web3SignerValidatorRequest, }; +use eth2::types::AttesterData; use itertools::Itertools; use rand::{rngs::SmallRng, Rng, SeedableRng}; use slashing_protection::interchange::{Interchange, InterchangeMetadata}; @@ -1091,17 +1093,53 @@ async fn generic_migration_test( .unwrap(); check_keystore_import_response(&import_res, all_imported(keystores.len())); + let attestations_and_duties = first_vc_attestations + .into_iter() + .map(|(validator_index, attestation)| { + ( + attestation.clone(), + DutyAndProof::new_without_selection_proof( + AttesterData { + pubkey: keystore_pubkey(&keystores[validator_index]), + validator_index: validator_index as u64, + committees_at_slot: 0, + committee_index: 0, + committee_length: 0, + validator_committee_index: 0, + slot: attestation.data().slot, + }, + attestation.data().slot, + ), + ) + }) + .collect::>(); + // Sign attestations on VC1. - for (validator_index, mut attestation) in first_vc_attestations { - let public_key = keystore_pubkey(&keystores[validator_index]); + for (mut attestation, validator_duty) in attestations_and_duties.clone() { let current_epoch = attestation.data().target.epoch; tester1 .validator_store - .sign_attestation_v2(public_key, 0, &mut attestation, current_epoch) + .sign_attestation( + validator_duty.duty.pubkey, + 0, + &mut attestation, + current_epoch, + ) .await .unwrap(); } + tester1 + .validator_store + .check_and_insert_attestations( + attestations_and_duties + .clone() + .into_iter() + .map(|(a, b)| (a.clone(), b.clone())) + .collect::>(), + ) + .unwrap(); + // Delete the selected keys from VC1. let delete_res = tester1 .client @@ -1114,6 +1152,7 @@ async fn generic_migration_test( }) .await .unwrap(); + check_keystore_delete_response(&delete_res, all_deleted(delete_indices.len())); // Check that slashing protection data was returned for all selected validators. @@ -1167,17 +1206,70 @@ async fn generic_migration_test( .unwrap(); check_keystore_import_response(&import_res, all_imported(import_indices.len())); + let attestations_and_duties = second_vc_attestations + .into_iter() + .map(|(validator_index, attestation, should_succeed)| { + ( + attestation.clone(), + DutyAndProof::new_without_selection_proof( + AttesterData { + pubkey: keystore_pubkey(&keystores[validator_index]), + validator_index: validator_index as u64, + committees_at_slot: 0, + committee_index: 0, + committee_length: 0, + validator_committee_index: 0, + slot: attestation.data().slot, + }, + attestation.data().slot, + ), + should_succeed, + ) + }) + .collect::>(); + // Sign attestations on the second VC. - for (validator_index, mut attestation, should_succeed) in second_vc_attestations { - let public_key = keystore_pubkey(&keystores[validator_index]); + for (mut attestation, validator_duty, should_succeed) in attestations_and_duties.clone() { let current_epoch = attestation.data().target.epoch; + match tester2 .validator_store - .sign_attestation_v2(public_key, 0, &mut attestation, current_epoch) + .sign_attestation( + validator_duty.duty.pubkey, + 0, + &mut attestation, + current_epoch, + ) .await { - Ok(()) => assert!(should_succeed), - Err(e) => assert!(!should_succeed, "{:?}", e), + Ok(_) => (), + Err(e) => { + if should_succeed { + panic!("Should succeed"); + } else { + continue; + } + } + }; + + let attestation_and_duty = vec![(attestation, validator_duty)]; + + let Ok(safe_attestation) = tester2.validator_store.check_and_insert_attestations( + attestation_and_duty + .clone() + .into_iter() + .map(|(a, b)| (a.clone(), b.clone())) + .collect::>(), + ) else { + panic!("Should succeed"); + }; + + if safe_attestation.len() > 0 && !should_succeed { + panic!("should fail") + } + + if safe_attestation.len() == 0 && should_succeed { + panic!("should succeed") } } }) @@ -1306,7 +1398,7 @@ async fn delete_concurrent_with_signing() { let mut att = make_attestation(j, j + 1); for (_validator_id, public_key) in thread_pubkeys.iter().enumerate() { let _ = validator_store - .sign_attestation_v2(*public_key, 0, &mut att, Epoch::new(j + 1)) + .sign_attestation(*public_key, 0, &mut att, Epoch::new(j + 1)) .await; } } diff --git a/validator_client/src/http_metrics/metrics.rs b/validator_client/src/http_metrics/metrics.rs index 8bc569c67a2..dd08216e214 100644 --- a/validator_client/src/http_metrics/metrics.rs +++ b/validator_client/src/http_metrics/metrics.rs @@ -17,7 +17,7 @@ pub const BLINDED_BEACON_BLOCK_HTTP_POST: &str = "blinded_beacon_block_http_post pub const ATTESTATIONS: &str = "attestations"; pub const ATTESTATIONS_HTTP_GET: &str = "attestations_http_get"; pub const ATTESTATIONS_HTTP_POST: &str = "attestations_http_post"; -pub const AGGREGATES: &str = "aggregates"; +// pub const AGGREGATES: &str = "aggregates"; TODO(attn-slash) re-introduce this metric pub const AGGREGATES_HTTP_GET: &str = "aggregates_http_get"; pub const AGGREGATES_HTTP_POST: &str = "aggregates_http_post"; pub const CURRENT_EPOCH: &str = "current_epoch"; diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 5a3096ef160..37989ea51be 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -42,7 +42,6 @@ pub enum Error { GreaterThanCurrentEpoch { epoch: Epoch, current_epoch: Epoch }, UnableToSignAttestation(AttestationError), UnableToSign(SigningError), - SlashingDbError, } impl From for Error { @@ -636,7 +635,7 @@ impl ValidatorStore { } } - pub async fn sign_attestation_v2( + pub async fn sign_attestation( &self, validator_pubkey: PublicKeyBytes, validator_committee_position: usize, @@ -676,110 +675,25 @@ impl ValidatorStore { Ok(()) } - // pub async fn sign_attestation( - // &self, - // validator_pubkey: PublicKeyBytes, - // validator_committee_position: usize, - // attestation: &mut Attestation, - // current_epoch: Epoch, - // ) -> Result<(), Error> { - // // Make sure the target epoch is not higher than the current epoch to avoid potential attacks. - // if attestation.data().target.epoch > current_epoch { - // return Err(Error::GreaterThanCurrentEpoch { - // epoch: attestation.data().target.epoch, - // current_epoch, - // }); - // } - - // // Get the signing method and check doppelganger protection. - // let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; - - // // Checking for slashing conditions. - // let signing_epoch = attestation.data().target.epoch; - // let signing_context = self.signing_context(Domain::BeaconAttester, signing_epoch); - // let domain_hash = signing_context.domain_hash(&self.spec); - // let slashing_status = if signing_method - // .requires_local_slashing_protection(self.enable_web3signer_slashing_protection) - // { - // self.slashing_protection.check_and_insert_attestation( - // &validator_pubkey, - // attestation.data(), - // domain_hash, - // ) - // } else { - // Ok(Safe::Valid) - // }; - - // match slashing_status { - // // We can safely sign this attestation. - // Ok(Safe::Valid) => { - // let signature = signing_method - // .get_signature::>( - // SignableMessage::AttestationData(attestation.data()), - // signing_context, - // &self.spec, - // &self.task_executor, - // ) - // .await?; - // attestation - // .add_signature(&signature, validator_committee_position) - // .map_err(Error::UnableToSignAttestation)?; - - // metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); - - // Ok(()) - // } - // Ok(Safe::SameData) => { - // warn!( - // self.log, - // "Skipping signing of previously signed attestation" - // ); - // metrics::inc_counter_vec( - // &metrics::SIGNED_ATTESTATIONS_TOTAL, - // &[metrics::SAME_DATA], - // ); - // Err(Error::SameData) - // } - // Err(NotSafe::UnregisteredValidator(pk)) => { - // warn!( - // self.log, - // "Not signing attestation for unregistered validator"; - // "msg" => "Carefully consider running with --init-slashing-protection (see --help)", - // "public_key" => format!("{:?}", pk) - // ); - // metrics::inc_counter_vec( - // &metrics::SIGNED_ATTESTATIONS_TOTAL, - // &[metrics::UNREGISTERED], - // ); - // Err(Error::Slashable(NotSafe::UnregisteredValidator(pk))) - // } - // Err(e) => { - // crit!( - // self.log, - // "Not signing slashable attestation"; - // "attestation" => format!("{:?}", attestation.data()), - // "error" => format!("{:?}", e) - // ); - // metrics::inc_counter_vec( - // &metrics::SIGNED_ATTESTATIONS_TOTAL, - // &[metrics::SLASHABLE], - // ); - // Err(Error::Slashable(e)) - // } - // } - // } - pub fn check_and_insert_attestations( &self, attestations: Vec<(Attestation, DutyAndProof)>, ) -> Result, DutyAndProof)>, Error> { - let mut attestations_to_check = vec![]; let mut safe_attestations = vec![]; + let mut conn = match self.slashing_protection.get_db_connection() { + Ok(conn) => conn, + Err(e) => return Err(Error::Slashable(e)), + }; + + let txn = match conn.transaction_with_behavior(TransactionBehavior::Exclusive) { + Ok(txn) => txn, + Err(e) => return Err(Error::Slashable(e.into())), + }; + for (attestation, validator_duty) in attestations.iter() { let validator_pubkey = validator_duty.duty.pubkey; - let signing_method = - self.doppelganger_checked_signing_method(validator_pubkey.clone())?; + let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; // Checking for slashing conditions. let signing_epoch = attestation.data().target.epoch; @@ -788,73 +702,59 @@ impl ValidatorStore { if signing_method .requires_local_slashing_protection(self.enable_web3signer_slashing_protection) { - attestations_to_check.push((attestation, validator_duty, domain_hash)); + let validator_pubkey = &validator_duty.duty.pubkey; + + let slashing_status = self.slashing_protection.check_and_insert_attestation( + validator_pubkey, + attestation.data(), + domain_hash, + &txn, + ); + + match slashing_status { + Ok(Safe::Valid) => { + safe_attestations.push((attestation.clone(), validator_duty.clone())); + metrics::inc_counter_vec( + &metrics::SIGNED_ATTESTATIONS_TOTAL, + &[metrics::SUCCESS], + ); + } + Ok(Safe::SameData) => { + warn!(self.log, "Skipping previously signed attestation"); + } + Err(NotSafe::UnregisteredValidator(pk)) => { + warn!( + self.log, + "Not signing attestation for unregistered validator"; + "msg" => "Carefully consider running with --init-slashing-protection (see --help)", + "public_key" => format!("{:?}", pk) + ); + metrics::inc_counter_vec( + &metrics::SIGNED_ATTESTATIONS_TOTAL, + &[metrics::UNREGISTERED], + ); + } + Err(e) => { + crit!( + self.log, + "Not signing slashable attestation"; + "attestation" => format!("{:?}", attestation.data()), + "error" => format!("{:?}", e) + ); + metrics::inc_counter_vec( + &metrics::SIGNED_ATTESTATIONS_TOTAL, + &[metrics::SLASHABLE], + ); + } + }; } else { safe_attestations.push((attestation.clone(), validator_duty.clone())); + metrics::inc_counter_vec(&metrics::SIGNED_ATTESTATIONS_TOTAL, &[metrics::SUCCESS]); } } - let Ok(mut conn) = self.slashing_protection.get_db_connection() else { - // TODO(attn-slash) crit log - return Err(Error::SlashingDbError); - }; - let Ok(txn) = conn.transaction_with_behavior(TransactionBehavior::Exclusive) else { - // TODO(attn-slash) crit log - return Err(Error::SlashingDbError); - }; - - for (attestation, validator_duty, domain_hash) in attestations_to_check { - let validator_pubkey = &validator_duty.duty.pubkey; - - let slashing_status = self.slashing_protection.check_and_insert_attestation( - &validator_pubkey, - attestation.data(), - domain_hash, - &txn, - ); - - match slashing_status { - Ok(Safe::Valid) => { - safe_attestations.push((attestation.clone(), validator_duty.clone())); - metrics::inc_counter_vec( - &metrics::SIGNED_ATTESTATIONS_TOTAL, - &[metrics::SUCCESS], - ); - } - Ok(Safe::SameData) => { - warn!(self.log, "Skipping previously signed attestation"); - } - Err(NotSafe::UnregisteredValidator(pk)) => { - warn!( - self.log, - "Not signing attestation for unregistered validator"; - "msg" => "Carefully consider running with --init-slashing-protection (see --help)", - "public_key" => format!("{:?}", pk) - ); - metrics::inc_counter_vec( - &metrics::SIGNED_ATTESTATIONS_TOTAL, - &[metrics::UNREGISTERED], - ); - } - Err(e) => { - crit!( - self.log, - "Not signing slashable attestation"; - "attestation" => format!("{:?}", attestation.data()), - "error" => format!("{:?}", e) - ); - metrics::inc_counter_vec( - &metrics::SIGNED_ATTESTATIONS_TOTAL, - &[metrics::SLASHABLE], - ); - } - }; - } - - // TODO(attn-slash) add logging - match self.slashing_protection.commit(txn) { - Err(_) => (), - Ok(_) => (), + if let Err(e) = self.slashing_protection.commit(txn) { + return Err(Error::Slashable(e)); }; Ok(safe_attestations) From 57bd6934ef2f006f431bccc1f415eaa1538362f8 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 6 Aug 2024 17:03:30 -0700 Subject: [PATCH 06/22] remove unneeded clones --- validator_client/src/attestation_service.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index f4ab6fbc6d6..185767fc567 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -246,21 +246,22 @@ impl AttestationService { let mut handles = vec![]; // Sign an `Attestation` for all required validators. - duties_by_committee_index.clone().into_iter().for_each( + duties_by_committee_index.iter().for_each( |(committee_index, validator_duties)| { - validator_duties.into_iter().for_each(|validator_duty| { + validator_duties.iter().for_each(|validator_duty| { // Get the previously downloaded attestation data for this committee index. if let Some(attestation_data) = attestation_data_service .get_data_by_committee_index(&committee_index, &fork_name) { let this = inner_self.clone(); + let duty = validator_duty.clone(); // Have the validator sign the attestation. let handle = inner_self.inner.context.executor.spawn_blocking_handle( move || { this.sign_attestation( attestation_data, - validator_duty.clone(), + duty, ) }, "Sign attestation", From 73f1d5567100e89a6c7f21a697fcde51ff654adb Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 6 Aug 2024 17:04:42 -0700 Subject: [PATCH 07/22] linnt --- validator_client/src/http_api/tests/keystores.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/validator_client/src/http_api/tests/keystores.rs b/validator_client/src/http_api/tests/keystores.rs index dfb719e458e..997ae108339 100644 --- a/validator_client/src/http_api/tests/keystores.rs +++ b/validator_client/src/http_api/tests/keystores.rs @@ -1243,7 +1243,7 @@ async fn generic_migration_test( .await { Ok(_) => (), - Err(e) => { + Err(_) => { if should_succeed { panic!("Should succeed"); } else { From d07564c1891ff23d3eb97bc0f7a248a6258815dd Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 6 Aug 2024 17:13:14 -0700 Subject: [PATCH 08/22] fmt --- validator_client/src/attestation_service.rs | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 185767fc567..091c32d90e4 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -246,8 +246,9 @@ impl AttestationService { let mut handles = vec![]; // Sign an `Attestation` for all required validators. - duties_by_committee_index.iter().for_each( - |(committee_index, validator_duties)| { + duties_by_committee_index + .iter() + .for_each(|(committee_index, validator_duties)| { validator_duties.iter().for_each(|validator_duty| { // Get the previously downloaded attestation data for this committee index. if let Some(attestation_data) = attestation_data_service @@ -258,12 +259,7 @@ impl AttestationService { // Have the validator sign the attestation. let handle = inner_self.inner.context.executor.spawn_blocking_handle( - move || { - this.sign_attestation( - attestation_data, - duty, - ) - }, + move || this.sign_attestation(attestation_data, duty), "Sign attestation", ); @@ -279,8 +275,7 @@ impl AttestationService { ) } }) - }, - ); + }); let mut signed_attestations = vec![]; From 420ce5723c76765f2d7720a9f3c05f7408832e36 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 9 Aug 2024 16:43:13 -0700 Subject: [PATCH 09/22] working on test fixes --- testing/web3signer_tests/src/lib.rs | 75 ++++++++++++++++++- .../slashing_protection/src/test_utils.rs | 9 --- validator_client/src/attestation_service.rs | 2 +- validator_client/src/lib.rs | 2 +- 4 files changed, 76 insertions(+), 12 deletions(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index 13d92d2d855..5070c5c2f0d 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -20,6 +20,7 @@ mod tests { use account_utils::validator_definitions::{ SigningDefinition, ValidatorDefinition, ValidatorDefinitions, Web3SignerDefinition, }; + use eth2::types::AttesterData; use eth2_keystore::KeystoreBuilder; use eth2_network_config::Eth2NetworkConfig; use parking_lot::Mutex; @@ -40,6 +41,7 @@ mod tests { use tokio::time::sleep; use types::{attestation::AttestationBase, *}; use url::Url; + use validator_client::duties_service::DutyAndProof; use validator_client::{ initialized_validators::{ load_pem_certificate, load_pkcs12_identity, InitializedValidators, @@ -519,7 +521,7 @@ mod tests { ) -> Self where F: Fn(PublicKeyBytes, Arc>) -> R, - R: Future>, + R: Future>, { for validator_rig in &self.validator_rigs { let result = @@ -837,9 +839,33 @@ mod tests { "double_vote_attestation", move |pubkey, validator_store| async move { let mut attestation = double_vote_attestation(); + let validator_duty = DutyAndProof::new_without_selection_proof( + AttesterData { + pubkey: pubkey, + validator_index: 0, + committees_at_slot: 0, + committee_index: 0, + committee_length: 0, + validator_committee_index: 0, + slot: attestation.data().slot, + }, + attestation.data().slot, + ); + validator_store .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await + .unwrap(); + + let safe_attestations = validator_store + .check_and_insert_attestations(vec![(attestation, validator_duty)]) + .unwrap(); + + if !slashable_message_should_sign && safe_attestations.len() > 0 { + return Err(()); + } + + Ok(()) }, slashable_message_should_sign, ) @@ -848,9 +874,32 @@ mod tests { "surrounding_attestation", move |pubkey, validator_store| async move { let mut attestation = surrounding_attestation(); + let validator_duty = DutyAndProof::new_without_selection_proof( + AttesterData { + pubkey: pubkey, + validator_index: 0, + committees_at_slot: 0, + committee_index: 0, + committee_length: 0, + validator_committee_index: 0, + slot: attestation.data().slot, + }, + attestation.data().slot, + ); validator_store .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await + .unwrap(); + + let safe_attestations = validator_store + .check_and_insert_attestations(vec![(attestation, validator_duty)]) + .unwrap(); + + if !slashable_message_should_sign && safe_attestations.len() > 0 { + return Err(()); + } + + Ok(()) }, slashable_message_should_sign, ) @@ -859,9 +908,32 @@ mod tests { "surrounded_attestation", move |pubkey, validator_store| async move { let mut attestation = surrounded_attestation(); + let validator_duty = DutyAndProof::new_without_selection_proof( + AttesterData { + pubkey: pubkey, + validator_index: 0, + committees_at_slot: 0, + committee_index: 0, + committee_length: 0, + validator_committee_index: 0, + slot: attestation.data().slot, + }, + attestation.data().slot, + ); validator_store .sign_attestation(pubkey, 0, &mut attestation, current_epoch) .await + .unwrap(); + + let safe_attestations = validator_store + .check_and_insert_attestations(vec![(attestation, validator_duty)]) + .unwrap(); + + if !slashable_message_should_sign && safe_attestations.len() > 0 { + return Err(()); + } + + Ok(()) }, slashable_message_should_sign, ) @@ -884,6 +956,7 @@ mod tests { .sign_block(pubkey, block, slot) .await .map(|_| ()) + .map_err(|_| ()) }, slashable_message_should_sign, ) diff --git a/validator_client/slashing_protection/src/test_utils.rs b/validator_client/slashing_protection/src/test_utils.rs index b50ea852335..168cf603fb2 100644 --- a/validator_client/slashing_protection/src/test_utils.rs +++ b/validator_client/slashing_protection/src/test_utils.rs @@ -83,9 +83,7 @@ impl StreamTest { check_registration_invariants(&slashing_db, &self.registered_validators); - println!("1"); let mut conn = slashing_db.get_db_connection().unwrap(); - println!("2"); let txn = conn .transaction_with_behavior(TransactionBehavior::Exclusive) @@ -105,9 +103,7 @@ impl StreamTest { ); } - println!("3"); slashing_db.commit(txn).unwrap(); - println!("4"); drop(conn); roundtrip_database(&dir, &slashing_db, self.registered_validators.is_empty()); } @@ -141,22 +137,17 @@ impl StreamTest { // This function roundtrips the database, but applies minification in order to be compatible with // the implicit minification done on import. fn roundtrip_database(dir: &TempDir, db: &SlashingDatabase, is_empty: bool) { - println!("a"); let exported = db .export_all_interchange_info(DEFAULT_GENESIS_VALIDATORS_ROOT) .unwrap(); - println!("b"); let new_db = SlashingDatabase::create(&dir.path().join("roundtrip_slashing_protection.sqlite")).unwrap(); - println!("c"); new_db .import_interchange_info(exported.clone(), DEFAULT_GENESIS_VALIDATORS_ROOT) .unwrap(); - println!("d"); let reexported = new_db .export_all_interchange_info(DEFAULT_GENESIS_VALIDATORS_ROOT) .unwrap(); - println!("e"); assert!(exported .minify() .unwrap() diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 091c32d90e4..767821b7816 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -252,7 +252,7 @@ impl AttestationService { validator_duties.iter().for_each(|validator_duty| { // Get the previously downloaded attestation data for this committee index. if let Some(attestation_data) = attestation_data_service - .get_data_by_committee_index(&committee_index, &fork_name) + .get_data_by_committee_index(committee_index, &fork_name) { let this = inner_self.clone(); let duty = validator_duty.clone(); diff --git a/validator_client/src/lib.rs b/validator_client/src/lib.rs index b3cf326fb92..403d3c3ff65 100644 --- a/validator_client/src/lib.rs +++ b/validator_client/src/lib.rs @@ -4,7 +4,7 @@ mod beacon_node_fallback; mod block_service; mod check_synced; mod cli; -mod duties_service; +pub mod duties_service; mod graffiti_file; mod http_metrics; mod key_cache; From e9112b17f109d12c2d6f19c506af6e6b0f6cb379 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Mon, 12 Aug 2024 09:03:29 -0700 Subject: [PATCH 10/22] fix test --- Cargo.lock | 2 ++ testing/web3signer_tests/Cargo.toml | 2 ++ testing/web3signer_tests/src/lib.rs | 13 ++++++++----- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 2ab1730c13f..cc65c3a52f6 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -9400,6 +9400,7 @@ dependencies = [ "account_utils", "async-channel", "environment", + "eth2", "eth2_keystore", "eth2_network_config", "futures", @@ -9408,6 +9409,7 @@ dependencies = [ "serde", "serde_json", "serde_yaml", + "slashing_protection", "slot_clock", "task_executor", "tempfile", diff --git a/testing/web3signer_tests/Cargo.toml b/testing/web3signer_tests/Cargo.toml index 7321fc13849..8aea81d4746 100644 --- a/testing/web3signer_tests/Cargo.toml +++ b/testing/web3signer_tests/Cargo.toml @@ -9,6 +9,7 @@ edition = { workspace = true } [dev-dependencies] async-channel = { workspace = true } +eth2 = { workspace = true } eth2_keystore = { workspace = true } types = { workspace = true } tempfile = { workspace = true } @@ -16,6 +17,7 @@ tokio = { workspace = true } reqwest = { workspace = true } url = { workspace = true } validator_client = { workspace = true } +slashing_protection = { workspace = true } slot_clock = { workspace = true } futures = { workspace = true } task_executor = { workspace = true } diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index 5070c5c2f0d..630f40ef3d7 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -26,6 +26,7 @@ mod tests { use parking_lot::Mutex; use reqwest::Client; use serde::Serialize; + use slashing_protection::NotSafe; use slot_clock::{SlotClock, TestingSlotClock}; use std::env; use std::fmt::Debug; @@ -521,7 +522,7 @@ mod tests { ) -> Self where F: Fn(PublicKeyBytes, Arc>) -> R, - R: Future>, + R: Future>, { for validator_rig in &self.validator_rigs { let result = @@ -862,7 +863,8 @@ mod tests { .unwrap(); if !slashable_message_should_sign && safe_attestations.len() > 0 { - return Err(()); + // return a validator store error to get the test to pass + return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } Ok(()) @@ -896,7 +898,8 @@ mod tests { .unwrap(); if !slashable_message_should_sign && safe_attestations.len() > 0 { - return Err(()); + // return a validator store error to get the test to pass + return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } Ok(()) @@ -930,7 +933,8 @@ mod tests { .unwrap(); if !slashable_message_should_sign && safe_attestations.len() > 0 { - return Err(()); + // return a validator store error to get the test to pass + return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } Ok(()) @@ -956,7 +960,6 @@ mod tests { .sign_block(pubkey, block, slot) .await .map(|_| ()) - .map_err(|_| ()) }, slashable_message_should_sign, ) From ecf42f07285eae2a8ddb2e9802e1b5103a844571 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Tue, 13 Aug 2024 16:32:46 -0700 Subject: [PATCH 11/22] prevent db commit when slashing is not avail --- validator_client/src/attestation_service.rs | 29 +++++++++++++++++---- validator_client/src/validator_store.rs | 17 ++++++++++++ 2 files changed, 41 insertions(+), 5 deletions(-) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 767821b7816..46a49f1504f 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -287,22 +287,41 @@ impl AttestationService { } } - // Check that the signed attestations are not slash-able (if slash-ability checks are enabled). - let safe_attestations = match inner_self + let slashing_checks_enabled = match inner_self .validator_store - .check_and_insert_attestations(signed_attestations) + .attestation_slashing_checks_enabled(&signed_attestations) { - Ok(attestations) => attestations, + Ok(slashing_checks_enabled) => slashing_checks_enabled, Err(e) => { crit!( log, - "An error occurred when checking for slashable attestations"; + "An error occurred when checking if slashing checks are enabled"; "error" => format!("{:?}", e) ); return; } }; + // Check that the signed attestations are not slash-able (if slash-ability checks are enabled). + let safe_attestations = if slashing_checks_enabled { + match inner_self + .validator_store + .check_and_insert_attestations(signed_attestations) + { + Ok(attestations) => attestations, + Err(e) => { + crit!( + log, + "An error occurred when checking for slashable attestations"; + "error" => format!("{:?}", e) + ); + return; + } + } + } else { + signed_attestations + }; + match inner_self .publish_attestations( &safe_attestations.iter().map(|(a, _)| a).collect::>(), diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index 37989ea51be..f0aab1ad832 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -675,6 +675,23 @@ impl ValidatorStore { Ok(()) } + /// If a single validator in the list of attestation duties requires slashing protection return true + pub fn attestation_slashing_checks_enabled( + &self, + attestations: &Vec<(Attestation, DutyAndProof)>, + ) -> Result { + for (_, validator_duty) in attestations.iter() { + let validator_pubkey = validator_duty.duty.pubkey; + let signing_method = self.doppelganger_checked_signing_method(validator_pubkey)?; + if signing_method + .requires_local_slashing_protection(self.enable_web3signer_slashing_protection) + { + return Ok(true); + } + } + Ok(false) + } + pub fn check_and_insert_attestations( &self, attestations: Vec<(Attestation, DutyAndProof)>, From b469d65712f232a04601bc6f270263d3253edbbc Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 15 Aug 2024 14:39:22 -0700 Subject: [PATCH 12/22] add more granular metrics --- validator_client/src/attestation_service.rs | 18 ++++++++++++++---- validator_client/src/http_metrics/metrics.rs | 4 +++- validator_client/src/validator_store.rs | 2 +- 3 files changed, 18 insertions(+), 6 deletions(-) diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 46a49f1504f..ff0905d19c0 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -221,8 +221,7 @@ impl AttestationService { ) { let inner_self = self.clone(); - // TODO(attn-slash) more granular metric names - let attestations_timer = metrics::start_timer_vec( + let _attestations_timer = metrics::start_timer_vec( &metrics::ATTESTATION_SERVICE_TIMES, &[metrics::ATTESTATIONS], ); @@ -304,6 +303,10 @@ impl AttestationService { // Check that the signed attestations are not slash-able (if slash-ability checks are enabled). let safe_attestations = if slashing_checks_enabled { + let _timer = metrics::start_timer_vec( + &metrics::ATTESTATION_SERVICE_TIMES, + &[metrics::ATTESTATION_SLASHABILITY_CHECK], + ); match inner_self .validator_store .check_and_insert_attestations(signed_attestations) @@ -340,6 +343,10 @@ impl AttestationService { } }; + let _timer = metrics::start_timer_vec( + &metrics::ATTESTATION_SERVICE_TIMES, + &[metrics::AGGREGATES], + ); // Create and publish `SignedAggregateAndProof` for all aggregating validators. for (committee_index, validator_duties) in duties_by_committee_index.iter() { // TODO(attn-slash) we could make this multi threaded @@ -376,8 +383,6 @@ impl AttestationService { }, "Download and sign attestations", ); - - drop(attestations_timer); } /// Performs the first step of the attesting process: downloading `AttestationData` objects. @@ -391,6 +396,7 @@ impl AttestationService { fork_name: &ForkName, ) { let log = self.context.log().clone(); + for (committee_index, _) in duties_by_committee_index.iter() { match attestation_data_service .download_data(committee_index, slot, fork_name) @@ -425,6 +431,10 @@ impl AttestationService { validator_duty: DutyAndProof, ) -> Result, DutyAndProof)>, String> { let log = self.context.log(); + let _timer = metrics::start_timer_vec( + &metrics::ATTESTATION_SERVICE_TIMES, + &[metrics::ATTESTATION_SIGN], + ); let current_epoch = self .slot_clock diff --git a/validator_client/src/http_metrics/metrics.rs b/validator_client/src/http_metrics/metrics.rs index dd08216e214..aa45f122b21 100644 --- a/validator_client/src/http_metrics/metrics.rs +++ b/validator_client/src/http_metrics/metrics.rs @@ -17,7 +17,9 @@ pub const BLINDED_BEACON_BLOCK_HTTP_POST: &str = "blinded_beacon_block_http_post pub const ATTESTATIONS: &str = "attestations"; pub const ATTESTATIONS_HTTP_GET: &str = "attestations_http_get"; pub const ATTESTATIONS_HTTP_POST: &str = "attestations_http_post"; -// pub const AGGREGATES: &str = "aggregates"; TODO(attn-slash) re-introduce this metric +pub const ATTESTATION_SIGN: &str = "attestation_sign"; +pub const ATTESTATION_SLASHABILITY_CHECK: &str = "attestation_slashability_check"; +pub const AGGREGATES: &str = "aggregates"; pub const AGGREGATES_HTTP_GET: &str = "aggregates_http_get"; pub const AGGREGATES_HTTP_POST: &str = "aggregates_http_post"; pub const CURRENT_EPOCH: &str = "current_epoch"; diff --git a/validator_client/src/validator_store.rs b/validator_client/src/validator_store.rs index f0aab1ad832..0dcb548b53c 100644 --- a/validator_client/src/validator_store.rs +++ b/validator_client/src/validator_store.rs @@ -678,7 +678,7 @@ impl ValidatorStore { /// If a single validator in the list of attestation duties requires slashing protection return true pub fn attestation_slashing_checks_enabled( &self, - attestations: &Vec<(Attestation, DutyAndProof)>, + attestations: &[(Attestation, DutyAndProof)], ) -> Result { for (_, validator_duty) in attestations.iter() { let validator_pubkey = validator_duty.duty.pubkey; From cdf219b07529ad32e4d1ced65650ed087c576281 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 22 Aug 2024 18:43:13 -0700 Subject: [PATCH 13/22] fix test --- testing/web3signer_tests/src/lib.rs | 27 ++++++++++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index 630f40ef3d7..597be2112e4 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -863,7 +863,14 @@ mod tests { .unwrap(); if !slashable_message_should_sign && safe_attestations.len() > 0 { - // return a validator store error to get the test to pass + // if slashability checks are disabled and we don't return any safe attestations + // we raise an error to indicate that thi test case has failed + return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); + } + + if slashable_message_should_sign && !safe_attestations.len() > 0 { + // if slashability checks are eabled and we return safe attestations + // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } @@ -898,7 +905,14 @@ mod tests { .unwrap(); if !slashable_message_should_sign && safe_attestations.len() > 0 { - // return a validator store error to get the test to pass + // if slashability checks are disabled and we don't return any safe attestations + // we raise an error to indicate that thi test case has failed + return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); + } + + if slashable_message_should_sign && !safe_attestations.len() > 0 { + // if slashability checks are eabled and we return safe attestations + // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } @@ -933,7 +947,14 @@ mod tests { .unwrap(); if !slashable_message_should_sign && safe_attestations.len() > 0 { - // return a validator store error to get the test to pass + // if slashability checks are disabled and we don't return any safe attestations + // we raise an error to indicate that thi test case has failed + return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); + } + + if slashable_message_should_sign && !safe_attestations.len() > 0 { + // if slashability checks are eabled and we return safe attestations + // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } From 4715750a29fb5f3eb7ade4e6d9fc47e6c0c683fb Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 3 Oct 2024 10:46:27 -0700 Subject: [PATCH 14/22] Resolve merge conflicts --- .../src/attestation_data_service.rs | 31 +++++++------------ validator_client/src/attestation_service.rs | 2 +- 2 files changed, 13 insertions(+), 20 deletions(-) diff --git a/validator_client/src/attestation_data_service.rs b/validator_client/src/attestation_data_service.rs index e3fce1f4279..86bfdb7e56a 100644 --- a/validator_client/src/attestation_data_service.rs +++ b/validator_client/src/attestation_data_service.rs @@ -3,10 +3,7 @@ use std::sync::Arc; use slot_clock::SlotClock; use types::{AttestationData, CommitteeIndex, EthSpec, ForkName, Slot}; -use crate::{ - beacon_node_fallback::{BeaconNodeFallback, OfflineOnFailure, RequireSynced}, - http_metrics::metrics, -}; +use crate::{beacon_node_fallback::BeaconNodeFallback, http_metrics::metrics}; /// The AttestationDataService is responsible for downloading and caching attestation data at a given slot. /// It also helps prevent us from re-downloading identical attestation data. @@ -59,21 +56,17 @@ impl AttestationDataService { let attestation_data = self .beacon_nodes - .first_success( - RequireSynced::No, - OfflineOnFailure::Yes, - |beacon_node| async move { - let _timer = metrics::start_timer_vec( - &metrics::ATTESTATION_SERVICE_TIMES, - &[metrics::ATTESTATIONS_HTTP_GET], - ); - beacon_node - .get_validator_attestation_data(*slot, *committee_index) - .await - .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) - .map(|result| result.data) - }, - ) + .first_success(|beacon_node| async move { + let _timer = metrics::start_timer_vec( + &metrics::ATTESTATION_SERVICE_TIMES, + &[metrics::ATTESTATIONS_HTTP_GET], + ); + beacon_node + .get_validator_attestation_data(*slot, *committee_index) + .await + .map_err(|e| format!("Failed to produce attestation data: {:?}", e)) + .map(|result| result.data) + }) .await .map_err(|e| e.to_string())?; diff --git a/validator_client/src/attestation_service.rs b/validator_client/src/attestation_service.rs index 224f4b25df7..430acb3947a 100644 --- a/validator_client/src/attestation_service.rs +++ b/validator_client/src/attestation_service.rs @@ -1,5 +1,5 @@ use crate::attestation_data_service::AttestationDataService; -use crate::beacon_node_fallback::{ApiTopic, BeaconNodeFallback, RequireSynced}; +use crate::beacon_node_fallback::{ApiTopic, BeaconNodeFallback}; use crate::{ duties_service::{DutiesService, DutyAndProof}, http_metrics::metrics, From f5dacb6e93c9c2e4b244b0af1a96c850a75770c4 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Fri, 22 Nov 2024 23:51:12 +0700 Subject: [PATCH 15/22] remove unused import --- testing/web3signer_tests/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index e8e4916b64a..224718a1f29 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -20,7 +20,6 @@ mod tests { use account_utils::validator_definitions::{ SigningDefinition, ValidatorDefinition, ValidatorDefinitions, Web3SignerDefinition, }; - use eth2::types::AttesterData; use eth2_keystore::KeystoreBuilder; use eth2_network_config::Eth2NetworkConfig; use initialized_validators::{ From 98b2f7c396b2cb730e12ca0724ff996d052f16dd Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 23 Nov 2024 21:29:21 +0700 Subject: [PATCH 16/22] fix --- testing/web3signer_tests/src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index 224718a1f29..e0aadfcf867 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -991,6 +991,7 @@ mod tests { #[tokio::test] async fn slashing_protection_disabled_locally() { + test_lighthouse_slashing_protection(SlashingProtectionConfig { local: false }, 4253).await } From 67c6f3a20c01cdde5e3aafc7462797a07277a62f Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Sat, 23 Nov 2024 21:29:35 +0700 Subject: [PATCH 17/22] retry --- testing/web3signer_tests/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index e0aadfcf867..224718a1f29 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -991,7 +991,6 @@ mod tests { #[tokio::test] async fn slashing_protection_disabled_locally() { - test_lighthouse_slashing_protection(SlashingProtectionConfig { local: false }, 4253).await } From 6ece2bebaec963086e46838d96756ae020cbd250 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 3 Apr 2025 18:24:40 -0700 Subject: [PATCH 18/22] fix test --- beacon_node/http_api/tests/fork_tests.rs | 2 +- beacon_node/http_api/tests/tests.rs | 10 +++++----- common/eth2/src/lib.rs | 2 +- .../validator_services/src/attestation_service.rs | 9 ++++++--- 4 files changed, 13 insertions(+), 10 deletions(-) diff --git a/beacon_node/http_api/tests/fork_tests.rs b/beacon_node/http_api/tests/fork_tests.rs index 3156e3b2089..10e1d015368 100644 --- a/beacon_node/http_api/tests/fork_tests.rs +++ b/beacon_node/http_api/tests/fork_tests.rs @@ -152,7 +152,7 @@ async fn attestations_across_fork_with_skip_slots() { assert!(!unaggregated_attestations.is_empty()); let fork_name = harness.spec.fork_name_at_slot::(fork_slot); client - .post_beacon_pool_attestations_v1(&unaggregated_attestations.iter().collect::>()) + .post_beacon_pool_attestations_v1(&unaggregated_attestations) .await .unwrap(); diff --git a/beacon_node/http_api/tests/tests.rs b/beacon_node/http_api/tests/tests.rs index e9db80cde7f..ff355c61a7c 100644 --- a/beacon_node/http_api/tests/tests.rs +++ b/beacon_node/http_api/tests/tests.rs @@ -1862,7 +1862,7 @@ impl ApiTester { pub async fn test_post_beacon_pool_attestations_valid(mut self) -> Self { self.client - .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) + .post_beacon_pool_attestations_v1(&self.attestations) .await .unwrap(); @@ -1923,7 +1923,7 @@ impl ApiTester { let err = self .client - .post_beacon_pool_attestations_v1(&attestations.iter().collect::>()) + .post_beacon_pool_attestations_v1(&attestations) .await .unwrap_err(); @@ -4062,7 +4062,7 @@ impl ApiTester { // Attest to the current slot self.client - .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) + .post_beacon_pool_attestations_v1(&self.attestations) .await .unwrap(); @@ -5801,7 +5801,7 @@ impl ApiTester { // Attest to the current slot self.client - .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) + .post_beacon_pool_attestations_v1(&self.attestations) .await .unwrap(); @@ -5857,7 +5857,7 @@ impl ApiTester { let expected_attestation_len = self.attestations.len(); self.client - .post_beacon_pool_attestations_v1(&self.attestations.iter().collect::>()) + .post_beacon_pool_attestations_v1(&self.attestations) .await .unwrap(); diff --git a/common/eth2/src/lib.rs b/common/eth2/src/lib.rs index 989f99ac4a1..1c55220b507 100644 --- a/common/eth2/src/lib.rs +++ b/common/eth2/src/lib.rs @@ -1347,7 +1347,7 @@ impl BeaconNodeHttpClient { /// `POST v1/beacon/pool/attestations` pub async fn post_beacon_pool_attestations_v1( &self, - attestations: &[&Attestation], + attestations: &[Attestation], ) -> Result<(), Error> { let mut path = self.eth_path(V1)?; diff --git a/validator_client/validator_services/src/attestation_service.rs b/validator_client/validator_services/src/attestation_service.rs index e758987d898..d6397f8f0ac 100644 --- a/validator_client/validator_services/src/attestation_service.rs +++ b/validator_client/validator_services/src/attestation_service.rs @@ -257,7 +257,7 @@ impl AttestationService { // Have the validator sign the attestation. let handle = inner_self.inner.context.executor.spawn_blocking_handle( - async move || { + || async { this.sign_attestation(attestation_data, duty).await }, "Sign attestation", @@ -327,7 +327,10 @@ impl AttestationService { match inner_self .publish_attestations( - &safe_attestations.iter().map(|(a, _)| a).collect::>(), + &safe_attestations + .into_iter() + .map(|(a, _)| a) + .collect::>(), &validator_indices, slot, fork_name, @@ -521,7 +524,7 @@ impl AttestationService { /// attestations that were deemed "safe". pub async fn publish_attestations( &self, - attestations: &[&Attestation], + attestations: &[Attestation], validator_indices: &[u64], slot: Slot, fork_name: ForkName, From dece5b606c584957ccac80b4d8263bbdca3d738b Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 3 Apr 2025 20:23:46 -0700 Subject: [PATCH 19/22] optimize publish aggs --- testing/web3signer_tests/src/lib.rs | 2 +- .../src/attestation_service.rs | 84 +++++++++++-------- 2 files changed, 52 insertions(+), 34 deletions(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index c143dd0d9b0..fa2cf79e07d 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -856,7 +856,7 @@ mod tests { } if slashable_message_should_sign && !safe_attestations.len() > 0 { - // if slashability checks are eabled and we return safe attestations + // if slashability checks are en abled and we return safe attestations // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } diff --git a/validator_client/validator_services/src/attestation_service.rs b/validator_client/validator_services/src/attestation_service.rs index d6397f8f0ac..e249767bd74 100644 --- a/validator_client/validator_services/src/attestation_service.rs +++ b/validator_client/validator_services/src/attestation_service.rs @@ -255,15 +255,17 @@ impl AttestationService { let this = inner_self.clone(); let duty = validator_duty.clone(); // Have the validator sign the attestation. - let handle = + let unaggregated_attestation_handle = inner_self.inner.context.executor.spawn_blocking_handle( || async { this.sign_attestation(attestation_data, duty).await }, "Sign attestation", ); - if let Some(handle) = handle { - handles.push(handle); + if let Some(unaggregated_attestation_handle) = + unaggregated_attestation_handle + { + handles.push(unaggregated_attestation_handle); } } else { crit!( @@ -351,37 +353,53 @@ impl AttestationService { &validator_metrics::ATTESTATION_SERVICE_TIMES, &[validator_metrics::AGGREGATES], ); + + let mut handles = vec![]; // Create and publish `SignedAggregateAndProof` for all aggregating validators. - for (committee_index, validator_duties) in duties_by_committee_index.iter() { - // TODO(attn-slash) we could make this multi threaded - if let Some(attestation_data) = attestation_data_service - .get_data_by_committee_index(committee_index, &fork_name) - { - match inner_self - .produce_and_publish_aggregates( - &attestation_data, - *committee_index, - validator_duties, - ) - .await + duties_by_committee_index + .into_iter() + .for_each(|(committee_index, validator_duties)| { + if let Some(attestation_data) = attestation_data_service + .get_data_by_committee_index(&committee_index, &fork_name) { - Ok(_) => (), - Err(e) => { - crit!( - %slot, - error = ?e, - "Failed to produce and publish attestation aggregates", - ); + let this = inner_self.clone(); + let attn_data = attestation_data.clone(); + let handle = inner_self.inner.context.executor.spawn_blocking_handle( + async move || { + match this + .produce_and_publish_aggregates( + &attn_data, + committee_index, + &validator_duties, + ) + .await + { + Ok(_) => (), + Err(e) => { + crit!( + %slot, + error = ?e, + "Failed to produce and publish attestation aggregates", + ); + } + }; + }, + "Produce and publish aggregates", + ); + + if let Some(handle) = handle { + handles.push(handle) } - }; - } else { - crit!( - committee_index, - %slot, - "Failed to fetch attestation data", - ) - } - } + } else { + crit!( + committee_index, + %slot, + "Failed to fetch attestation data", + ) + } + }); + + join_all(handles).await; }, "Download and sign attestations", ); @@ -447,8 +465,8 @@ impl AttestationService { validator = ?validator_duty.duty.pubkey, duty_slot = %validator_duty.duty.slot, attestation_slot = %attestation_data.slot, - duty_index = validator_duty.duty.committee_index, - attestation_index = attestation_data.index, // TODO(batch-attestation) index isnt in attestation anymore + committee_index = validator_duty.duty.committee_index, + attestation_index = attestation_data.index, "Inconsistent validator duties during signing" ); } From d9e27c6ad05ed258f7ca3afb3052a96e5f8a99a8 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 3 Apr 2025 21:34:23 -0700 Subject: [PATCH 20/22] fix lint --- .../validator_services/src/attestation_service.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/validator_client/validator_services/src/attestation_service.rs b/validator_client/validator_services/src/attestation_service.rs index e249767bd74..48f3350eeb4 100644 --- a/validator_client/validator_services/src/attestation_service.rs +++ b/validator_client/validator_services/src/attestation_service.rs @@ -363,12 +363,11 @@ impl AttestationService { .get_data_by_committee_index(&committee_index, &fork_name) { let this = inner_self.clone(); - let attn_data = attestation_data.clone(); let handle = inner_self.inner.context.executor.spawn_blocking_handle( - async move || { + move || async move { match this .produce_and_publish_aggregates( - &attn_data, + &attestation_data, committee_index, &validator_duties, ) From a463e8d797173447a5a5d392570904022e689c57 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 3 Apr 2025 21:38:31 -0700 Subject: [PATCH 21/22] fmt --- .../validator_services/src/attestation_service.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/validator_client/validator_services/src/attestation_service.rs b/validator_client/validator_services/src/attestation_service.rs index 48f3350eeb4..c2b2c0b14fb 100644 --- a/validator_client/validator_services/src/attestation_service.rs +++ b/validator_client/validator_services/src/attestation_service.rs @@ -356,9 +356,8 @@ impl AttestationService { let mut handles = vec![]; // Create and publish `SignedAggregateAndProof` for all aggregating validators. - duties_by_committee_index - .into_iter() - .for_each(|(committee_index, validator_duties)| { + duties_by_committee_index.into_iter().for_each( + |(committee_index, validator_duties)| { if let Some(attestation_data) = attestation_data_service .get_data_by_committee_index(&committee_index, &fork_name) { @@ -396,7 +395,8 @@ impl AttestationService { "Failed to fetch attestation data", ) } - }); + }, + ); join_all(handles).await; }, From d8d6da94fa7ecee14bae99f9c506d0d3ad7aca39 Mon Sep 17 00:00:00 2001 From: Eitan Seri-Levi Date: Thu, 3 Apr 2025 22:17:58 -0700 Subject: [PATCH 22/22] linting --- testing/web3signer_tests/src/lib.rs | 12 ++++++------ validator_client/http_api/src/tests/keystores.rs | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/testing/web3signer_tests/src/lib.rs b/testing/web3signer_tests/src/lib.rs index fa2cf79e07d..7288e185dae 100644 --- a/testing/web3signer_tests/src/lib.rs +++ b/testing/web3signer_tests/src/lib.rs @@ -849,13 +849,13 @@ mod tests { .check_and_insert_attestations(vec![(attestation, pubkey)]) .unwrap(); - if !slashable_message_should_sign && safe_attestations.len() > 0 { + if !slashable_message_should_sign && !safe_attestations.is_empty() { // if slashability checks are disabled and we don't return any safe attestations // we raise an error to indicate that thi test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } - if slashable_message_should_sign && !safe_attestations.len() > 0 { + if slashable_message_should_sign && safe_attestations.is_empty() { // if slashability checks are en abled and we return safe attestations // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); @@ -880,13 +880,13 @@ mod tests { .check_and_insert_attestations(vec![(attestation, pubkey)]) .unwrap(); - if !slashable_message_should_sign && safe_attestations.len() > 0 { + if !slashable_message_should_sign && !safe_attestations.is_empty() { // if slashability checks are disabled and we don't return any safe attestations // we raise an error to indicate that thi test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } - if slashable_message_should_sign && !safe_attestations.len() > 0 { + if slashable_message_should_sign && safe_attestations.is_empty() { // if slashability checks are eabled and we return safe attestations // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); @@ -911,13 +911,13 @@ mod tests { .check_and_insert_attestations(vec![(attestation, pubkey)]) .unwrap(); - if !slashable_message_should_sign && safe_attestations.len() > 0 { + if !slashable_message_should_sign && !safe_attestations.is_empty() { // if slashability checks are disabled and we don't return any safe attestations // we raise an error to indicate that thi test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); } - if slashable_message_should_sign && !safe_attestations.len() > 0 { + if slashable_message_should_sign && safe_attestations.is_empty() { // if slashability checks are eabled and we return safe attestations // we raise an error to indicate that this test case has failed return Err(ValidatorStoreError::Slashable(NotSafe::ConsistencyError)); diff --git a/validator_client/http_api/src/tests/keystores.rs b/validator_client/http_api/src/tests/keystores.rs index 2332aef55bb..5634a883882 100644 --- a/validator_client/http_api/src/tests/keystores.rs +++ b/validator_client/http_api/src/tests/keystores.rs @@ -1136,7 +1136,7 @@ async fn generic_migration_test( attestations_and_duties .clone() .into_iter() - .map(|(a, b)| (a.clone(), b.duty.pubkey.clone())) + .map(|(a, b)| (a.clone(), b.duty.pubkey)) .collect::>(), ) .unwrap(); @@ -1259,17 +1259,17 @@ async fn generic_migration_test( attestation_and_duty .clone() .into_iter() - .map(|(a, b)| (a.clone(), b.duty.pubkey.clone())) + .map(|(a, b)| (a.clone(), b.duty.pubkey)) .collect::>(), ) else { panic!("Should succeed"); }; - if safe_attestation.len() > 0 && !should_succeed { + if !safe_attestation.is_empty() && !should_succeed { panic!("should fail") } - if safe_attestation.len() == 0 && should_succeed { + if safe_attestation.is_empty() && should_succeed { panic!("should succeed") } }