Skip to content

Commit 44fa540

Browse files
committed
Persist to DB after setting canonical head (#2547)
## Issue Addressed NA ## Proposed Changes Missed head votes on attestations is a well-known issue. The primary cause is a block getting set as the head *after* the attestation deadline. This PR aims to shorten the overall time between "block received" and "block set as head" by: 1. Persisting the head and fork choice *after* setting the canonical head - Informal measurements show this takes ~200ms 1. Pruning the op pool *after* setting the canonical head. 1. No longer persisting the op pool to disk during `BeaconChain::fork_choice` - Informal measurements show this can take up to 1.2s. I also add some metrics to help measure the effect of these changes. Persistence changes like this run the risk of breaking assumptions downstream. However, I have considered these risks and I think we're fine here. I will describe my reasoning for each change. ## Reasoning ### Change 1: Persisting the head and fork choice *after* setting the canonical head For (1), although the function is called `persist_head_and_fork_choice`, it only persists: - Fork choice - Head tracker - Genesis block root Since `BeaconChain::fork_choice_internal` does not modify these values between the original time we were persisting it and the current time, I assert that the change I've made is non-substantial in terms of what ends up on-disk. There's the possibility that some *other* thread has modified fork choice in the extra time we've given it, but that's totally fine. Since the only time we *read* those values from disk is during startup, I assert that this has no impact during runtime. ### Change 2: Pruning the op pool after setting the canonical head Similar to the argument above, we don't modify the op pool during `BeaconChain::fork_choice_internal` so it shouldn't matter when we prune. This change should be non-substantial. ### Change 3: No longer persisting the op pool to disk during `BeaconChain::fork_choice` This change *is* substantial. With the proposed changes, we'll only be persisting the op pool to disk when we shut down cleanly (i.e., the `BeaconChain` gets dropped). This means we'll save disk IO and time during usual operation, but a `kill -9` or similar "crash" will probably result in an out-of-date op pool when we reboot. An out-of-date op pool can only have an impact when producing blocks or aggregate attestations/sync committees. I think it's pretty reasonable that a crash might result in an out-of-date op pool, since: - Crashes are fairly rare. Practically the only time I see LH suffer a full crash is when the OOM killer shows up, and that's a very serious event. - It's generally quite rare to produce a block/aggregate immediately after a reboot. Just a few slots of runtime is probably enough to have a decent-enough op pool again. ## Additional Info Credits to @macladson for the timings referenced here.
1 parent 1031f79 commit 44fa540

File tree

2 files changed

+13
-6
lines changed

2 files changed

+13
-6
lines changed

beacon_node/beacon_chain/src/beacon_chain.rs

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2810,6 +2810,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
28102810
return Ok(());
28112811
}
28122812

2813+
let lag_timer = metrics::start_timer(&metrics::FORK_CHOICE_SET_HEAD_LAG_TIMES);
2814+
28132815
// At this point we know that the new head block is not the same as the previous one
28142816
metrics::inc_counter(&metrics::FORK_CHOICE_CHANGED_HEAD);
28152817

@@ -2913,12 +2915,6 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
29132915
.slot()
29142916
.epoch(T::EthSpec::slots_per_epoch());
29152917

2916-
if is_epoch_transition || is_reorg {
2917-
self.persist_head_and_fork_choice()?;
2918-
self.op_pool.prune_attestations(self.epoch()?);
2919-
self.persist_op_pool()?;
2920-
}
2921-
29222918
let update_head_timer = metrics::start_timer(&metrics::UPDATE_HEAD_TIMES);
29232919

29242920
// These fields are used for server-sent events
@@ -2934,6 +2930,8 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
29342930
.start_slot(T::EthSpec::slots_per_epoch());
29352931
let head_proposer_index = new_head.beacon_block.message().proposer_index();
29362932

2933+
drop(lag_timer);
2934+
29372935
// Update the snapshot that stores the head of the chain at the time it received the
29382936
// block.
29392937
*self
@@ -2984,6 +2982,11 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
29842982
);
29852983
});
29862984

2985+
if is_epoch_transition || is_reorg {
2986+
self.persist_head_and_fork_choice()?;
2987+
self.op_pool.prune_attestations(self.epoch()?);
2988+
}
2989+
29872990
if new_finalized_checkpoint.epoch != old_finalized_checkpoint.epoch {
29882991
// Due to race conditions, it's technically possible that the head we load here is
29892992
// different to the one earlier in this function.

beacon_node/beacon_chain/src/metrics.rs

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,10 @@ lazy_static! {
261261
"beacon_fork_choice_process_attestation_seconds",
262262
"Time taken to add an attestation to fork choice"
263263
);
264+
pub static ref FORK_CHOICE_SET_HEAD_LAG_TIMES: Result<Histogram> = try_create_histogram(
265+
"beacon_fork_choice_set_head_lag_times",
266+
"Time taken between finding the head and setting the canonical head value"
267+
);
264268
pub static ref BALANCES_CACHE_HITS: Result<IntCounter> =
265269
try_create_int_counter("beacon_balances_cache_hits_total", "Count of times balances cache fulfils request");
266270
pub static ref BALANCES_CACHE_MISSES: Result<IntCounter> =

0 commit comments

Comments
 (0)