From 70441aa554fc8db6c4423e34cb4377269656374f Mon Sep 17 00:00:00 2001 From: Pawan Dhananjay Date: Thu, 30 Sep 2021 01:22:43 +0000 Subject: [PATCH] Improve valmon inclusion delay calculation (#2618) ## Issue Addressed Resolves #2552 ## Proposed Changes Offers some improvement in inclusion distance calculation in the validator monitor. When registering an attestation from a block, instead of doing `block.slot() - attesstation.data.slot()` to get the inclusion distance, we now pass the parent block slot from the beacon chain and do `parent_slot.saturating_sub(attestation.data.slot())`. This allows us to give best effort inclusion distance in scenarios where the attestation was included right after a skip slot. Note that this does not give accurate results in scenarios where the attestation was included few blocks after the skip slot. In this case, if the attestation slot was `b1` and was included in block `b2` with a skip slot in between, we would get the inclusion delay as 0 (by ignoring the skip slot) which is the best effort inclusion delay. ``` b1 <- missed <- b2 ``` Here, if the attestation slot was `b1` and was included in block `b3` with a skip slot and valid block `b2` in between, then we would get the inclusion delay as 2 instead of 1 (by ignoring the skip slot). ``` b1 <- missed <- b2 <- b3 ``` A solution for the scenario 2 would be to count number of slots between included slot and attestation slot ignoring the skip slots in the beacon chain and pass the value to the validator monitor. But I'm concerned that it could potentially lead to db accesses for older blocks in extreme cases. This PR also uses the validator monitor data for logging per epoch inclusion distance. This is useful as we won't get inclusion data in post-altair summaries. Co-authored-by: Michael Sproul --- beacon_node/beacon_chain/src/beacon_chain.rs | 13 ++-- beacon_node/beacon_chain/src/metrics.rs | 4 +- .../beacon_chain/src/validator_monitor.rs | 73 +++++++++++++++---- 3 files changed, 70 insertions(+), 20 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 5f78a4211..f64168527 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -2439,11 +2439,14 @@ impl BeaconChain { + block.slot().as_u64() >= current_slot.as_u64() { - validator_monitor.register_attestation_in_block( - &indexed_attestation, - block.to_ref(), - &self.spec, - ); + match fork_choice.get_block(&block.parent_root()) { + Some(parent_block) => validator_monitor.register_attestation_in_block( + &indexed_attestation, + parent_block.slot, + &self.spec, + ), + None => warn!(self.log, "Failed to get parent block"; "slot" => %block.slot()), + } } } diff --git a/beacon_node/beacon_chain/src/metrics.rs b/beacon_node/beacon_chain/src/metrics.rs index 7be693a7e..203df0134 100644 --- a/beacon_node/beacon_chain/src/metrics.rs +++ b/beacon_node/beacon_chain/src/metrics.rs @@ -664,8 +664,8 @@ lazy_static! { "Number of sync contributions seen", &["src", "validator"] ); - pub static ref VALIDATOR_MONITOR_SYNC_COONTRIBUTIONS_DELAY_SECONDS: Result = try_create_histogram_vec( - "validator_monitor_sync_contribtions_delay_seconds", + pub static ref VALIDATOR_MONITOR_SYNC_CONTRIBUTIONS_DELAY_SECONDS: Result = try_create_histogram_vec( + "validator_monitor_sync_contributions_delay_seconds", "The delay between when the aggregator should send the sync contribution and when it was received.", &["src", "validator"] ); diff --git a/beacon_node/beacon_chain/src/validator_monitor.rs b/beacon_node/beacon_chain/src/validator_monitor.rs index 84bb5d8b9..7d6744b97 100644 --- a/beacon_node/beacon_chain/src/validator_monitor.rs +++ b/beacon_node/beacon_chain/src/validator_monitor.rs @@ -137,9 +137,12 @@ impl EpochSummary { self.sync_signature_contribution_inclusions += 1; } - pub fn register_attestation_block_inclusion(&mut self, delay: Slot) { + pub fn register_attestation_block_inclusion(&mut self, inclusion_distance: Slot) { self.attestation_block_inclusions += 1; - Self::update_if_lt(&mut self.attestation_min_block_inclusion_distance, delay); + Self::update_if_lt( + &mut self.attestation_min_block_inclusion_distance, + inclusion_distance, + ); } pub fn register_sync_signature_block_inclusions(&mut self) { @@ -189,6 +192,22 @@ impl MonitoredValidator { } } + /// Returns minimum inclusion distance for the given epoch as recorded by the validator monitor. + /// + /// Note: this value may be different from the one obtained from epoch summary + /// as the value recorded by the validator monitor ignores skip slots. + fn min_inclusion_distance(&self, epoch: &Epoch) -> Option { + let summaries = self.summaries.read(); + summaries + .get(epoch) + .map(|summary| { + summary + .attestation_min_block_inclusion_distance + .map(Into::into) + }) + .flatten() + } + /// Maps `func` across the `self.summaries`. /// /// ## Warning @@ -474,15 +493,22 @@ impl ValidatorMonitor { ); } - // For pre-Altair, state the inclusion distance. This information is not retained in - // the Altair state. - if let Some(inclusion_info) = summary.previous_epoch_inclusion_info(i) { - if inclusion_info.delay > spec.min_attestation_inclusion_delay { + // Get the minimum value among the validator monitor observed inclusion distance + // and the epoch summary inclusion distance. + // The inclusion data is not retained in the epoch summary post Altair. + let min_inclusion_distance = min_opt( + monitored_validator.min_inclusion_distance(&prev_epoch), + summary + .previous_epoch_inclusion_info(i) + .map(|info| info.delay), + ); + if let Some(inclusion_delay) = min_inclusion_distance { + if inclusion_delay > spec.min_attestation_inclusion_delay { warn!( self.log, - "Sub-optimal inclusion delay"; + "Potential sub-optimal inclusion delay"; "optimal" => spec.min_attestation_inclusion_delay, - "delay" => inclusion_info.delay, + "delay" => inclusion_delay, "epoch" => prev_epoch, "validator" => id, ); @@ -491,7 +517,7 @@ impl ValidatorMonitor { metrics::set_int_gauge( &metrics::VALIDATOR_MONITOR_PREV_EPOCH_ON_CHAIN_INCLUSION_DISTANCE, &[id], - inclusion_info.delay as i64, + inclusion_delay as i64, ); } @@ -828,14 +854,24 @@ impl ValidatorMonitor { } /// Register that the `indexed_attestation` was included in a *valid* `BeaconBlock`. + /// `parent_slot` is the slot corresponding to the parent of the beacon block in which + /// the attestation was included. + /// We use the parent slot instead of block slot to ignore skip slots when calculating inclusion distance. + /// + /// Note: Blocks that get orphaned will skew the inclusion distance calculation. pub fn register_attestation_in_block( &self, indexed_attestation: &IndexedAttestation, - block: BeaconBlockRef<'_, T>, + parent_slot: Slot, spec: &ChainSpec, ) { let data = &indexed_attestation.data; - let delay = (block.slot() - data.slot) - spec.min_attestation_inclusion_delay; + // Best effort inclusion distance which ignores skip slots between the parent + // and the current block. Skipped slots between the attestation slot and the parent + // slot are still counted for simplicity's sake. + let inclusion_distance = parent_slot.saturating_sub(data.slot) + 1; + + let delay = inclusion_distance - spec.min_attestation_inclusion_delay; let epoch = data.slot.epoch(T::slots_per_epoch()); indexed_attestation.attesting_indices.iter().for_each(|i| { @@ -864,7 +900,7 @@ impl ValidatorMonitor { ); validator.with_epoch_summary(epoch, |summary| { - summary.register_attestation_block_inclusion(delay) + summary.register_attestation_block_inclusion(inclusion_distance) }); } }) @@ -1008,7 +1044,7 @@ impl ValidatorMonitor { &[src, id], ); metrics::observe_timer_vec( - &metrics::VALIDATOR_MONITOR_SYNC_COONTRIBUTIONS_DELAY_SECONDS, + &metrics::VALIDATOR_MONITOR_SYNC_CONTRIBUTIONS_DELAY_SECONDS, &[src, id], delay, ); @@ -1432,3 +1468,14 @@ fn get_message_delay_ms( .and_then(|gross_delay| gross_delay.checked_sub(message_production_delay)) .unwrap_or_else(|| Duration::from_secs(0)) } + +/// Returns minimum value from the two options if both are `Some` or the +/// value contained if only one of them is Some. Returns `None` if both options are `None` +fn min_opt(a: Option, b: Option) -> Option { + match (a, b) { + (Some(x), Some(y)) => Some(std::cmp::min(x, y)), + (Some(x), None) => Some(x), + (None, Some(y)) => Some(y), + _ => None, + } +}