diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 5f78a42115..f641685272 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 7be693a7e5..203df01347 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 84bb5d8b9c..7d6744b971 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, + } +}