cerc-io/lighthouse
16
0
Fork 0
Code Issues 1 Pull Requests Packages Projects Releases Wiki Activity

Improve valmon inclusion delay calculation (#2618)

Browse Source 
## 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 <micsproul@gmail.com>
This commit is contained in:
Pawan Dhananjay 2021-09-30 01:22:43 +00:00
parent 7d13e57d9f
commit 70441aa554
3 changed files with 70 additions and 20 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
beacon_node/beacon_chain/src/beacon_chain.rs
View File

@ -2439,11 +2439,14 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
+ 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()),
}
}
}

4
beacon_node/beacon_chain/src/metrics.rs
View File

@ -664,8 +664,8 @@ lazy_static! {
"Number of sync contributions seen",
&["src", "validator"]
);
pub static ref VALIDATOR_MONITOR_SYNC_COONTRIBUTIONS_DELAY_SECONDS: Result<HistogramVec> = try_create_histogram_vec(
"validator_monitor_sync_contribtions_delay_seconds",
pub static ref VALIDATOR_MONITOR_SYNC_CONTRIBUTIONS_DELAY_SECONDS: Result<HistogramVec> = 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"]
);

73
beacon_node/beacon_chain/src/validator_monitor.rs
View File

@ -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<u64> {
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<T: EthSpec> ValidatorMonitor<T> {
);
}
// 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<T: EthSpec> ValidatorMonitor<T> {
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<T: EthSpec> ValidatorMonitor<T> {
}
/// 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<T>,
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<T: EthSpec> ValidatorMonitor<T> {
);
validator.with_epoch_summary(epoch, |summary| {
summary.register_attestation_block_inclusion(delay)
summary.register_attestation_block_inclusion(inclusion_distance)
});
}
})
@ -1008,7 +1044,7 @@ impl<T: EthSpec> ValidatorMonitor<T> {
&[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<S: SlotClock>(
.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<T: Ord>(a: Option<T>, b: Option<T>) -> Option<T> {
match (a, b) {
(Some(x), Some(y)) => Some(std::cmp::min(x, y)),
(Some(x), None) => Some(x),
(None, Some(y)) => Some(y),
_ => None,
}
}