-
Notifications
You must be signed in to change notification settings - Fork 781
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Run fork choice before block proposal (#3168)
## Issue Addressed Upcoming spec change ethereum/consensus-specs#2878 ## Proposed Changes 1. Run fork choice at the start of every slot, and wait for this run to complete before proposing a block. 2. As an optimisation, also run fork choice 3/4 of the way through the slot (at 9s), _dequeueing attestations for the next slot_. 3. Remove the fork choice run from the state advance timer that occurred before advancing the state. ## Additional Info ### Block Proposal Accuracy This change makes us more likely to propose on top of the correct head in the presence of re-orgs with proposer boost in play. The main scenario that this change is designed to address is described in the linked spec issue. ### Attestation Accuracy This change _also_ makes us more likely to attest to the correct head. Currently in the case of a skipped slot at `slot` we only run fork choice 9s into `slot - 1`. This means the attestations from `slot - 1` aren't taken into consideration, and any boost applied to the block from `slot - 1` is not removed (it should be). In the language of the linked spec issue, this means we are liable to attest to C, even when the majority voting weight has already caused a re-org to B. ### Why remove the call before the state advance? If we've run fork choice at the start of the slot then it has already dequeued all the attestations from the previous slot, which are the only ones eligible to influence the head in the current slot. Running fork choice again is unnecessary (unless we run it for the next slot and try to pre-empt a re-org, but I don't currently think this is a great idea). ### Performance Based on Prater testing this adds about 5-25ms of runtime to block proposal times, which are 500-1000ms on average (and spike to 5s+ sometimes due to state handling issues 😢 ). I believe this is a small enough penalty to enable it by default, with the option to disable it via the new flag `--fork-choice-before-proposal-timeout 0`. Upcoming work on block packing and state representation will also reduce block production times in general, while removing the spikes. ### Implementation Fork choice gets invoked at the start of the slot via the `per_slot_task` function called from the slot timer. It then uses a condition variable to signal to block production that fork choice has been updated. This is a bit funky, but it seems to work. One downside of the timer-based approach is that it doesn't happen automatically in most of the tests. The test added by this PR has to trigger the run manually.
- Loading branch information
1 parent
54b58fd
commit 6aaeb78
Showing
15 changed files
with
458 additions
and
47 deletions.
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
//! Concurrency helpers for synchronising block proposal with fork choice. | ||
//! | ||
//! The transmitter provides a way for a thread runnning fork choice on a schedule to signal | ||
//! to the receiver that fork choice has been updated for a given slot. | ||
use crate::BeaconChainError; | ||
use parking_lot::{Condvar, Mutex}; | ||
use std::sync::Arc; | ||
use std::time::Duration; | ||
use types::Slot; | ||
|
||
/// Sender, for use by the per-slot task timer. | ||
pub struct ForkChoiceSignalTx { | ||
pair: Arc<(Mutex<Slot>, Condvar)>, | ||
} | ||
|
||
/// Receiver, for use by the beacon chain waiting on fork choice to complete. | ||
pub struct ForkChoiceSignalRx { | ||
pair: Arc<(Mutex<Slot>, Condvar)>, | ||
} | ||
|
||
pub enum ForkChoiceWaitResult { | ||
/// Successfully reached a slot greater than or equal to the awaited slot. | ||
Success(Slot), | ||
/// Fork choice was updated to a lower slot, indicative of lag or processing delays. | ||
Behind(Slot), | ||
/// Timed out waiting for the fork choice update from the sender. | ||
TimeOut, | ||
} | ||
|
||
impl ForkChoiceSignalTx { | ||
pub fn new() -> Self { | ||
let pair = Arc::new((Mutex::new(Slot::new(0)), Condvar::new())); | ||
Self { pair } | ||
} | ||
|
||
pub fn get_receiver(&self) -> ForkChoiceSignalRx { | ||
ForkChoiceSignalRx { | ||
pair: self.pair.clone(), | ||
} | ||
} | ||
|
||
/// Signal to the receiver that fork choice has been updated to `slot`. | ||
/// | ||
/// Return an error if the provided `slot` is strictly less than any previously provided slot. | ||
pub fn notify_fork_choice_complete(&self, slot: Slot) -> Result<(), BeaconChainError> { | ||
let &(ref lock, ref condvar) = &*self.pair; | ||
|
||
let mut current_slot = lock.lock(); | ||
|
||
if slot < *current_slot { | ||
return Err(BeaconChainError::ForkChoiceSignalOutOfOrder { | ||
current: *current_slot, | ||
latest: slot, | ||
}); | ||
} else { | ||
*current_slot = slot; | ||
} | ||
|
||
// We use `notify_all` because there may be multiple block proposals waiting simultaneously. | ||
// Usually there'll be 0-1. | ||
condvar.notify_all(); | ||
|
||
Ok(()) | ||
} | ||
} | ||
|
||
impl Default for ForkChoiceSignalTx { | ||
fn default() -> Self { | ||
Self::new() | ||
} | ||
} | ||
|
||
impl ForkChoiceSignalRx { | ||
pub fn wait_for_fork_choice(&self, slot: Slot, timeout: Duration) -> ForkChoiceWaitResult { | ||
let &(ref lock, ref condvar) = &*self.pair; | ||
|
||
let mut current_slot = lock.lock(); | ||
|
||
// Wait for `current_slot >= slot`. | ||
// | ||
// Do not loop and wait, if we receive an update for the wrong slot then something is | ||
// quite out of whack and we shouldn't waste more time waiting. | ||
if *current_slot < slot { | ||
let timeout_result = condvar.wait_for(&mut current_slot, timeout); | ||
|
||
if timeout_result.timed_out() { | ||
return ForkChoiceWaitResult::TimeOut; | ||
} | ||
} | ||
|
||
if *current_slot >= slot { | ||
ForkChoiceWaitResult::Success(*current_slot) | ||
} else { | ||
ForkChoiceWaitResult::Behind(*current_slot) | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.