2021-10-01 06:32:38 +00:00
|
|
|
//! Generic tests that make use of the (newer) `InteractiveApiTester`
|
|
|
|
use crate::common::*;
|
2022-12-13 09:57:26 +00:00
|
|
|
use beacon_chain::{
|
|
|
|
chain_config::ReOrgThreshold,
|
|
|
|
test_utils::{AttestationStrategy, BlockStrategy},
|
|
|
|
};
|
2021-10-01 06:32:38 +00:00
|
|
|
use eth2::types::DepositContractData;
|
2022-12-13 09:57:26 +00:00
|
|
|
use execution_layer::{ForkChoiceState, PayloadAttributes};
|
|
|
|
use parking_lot::Mutex;
|
|
|
|
use slot_clock::SlotClock;
|
|
|
|
use state_processing::state_advance::complete_state_advance;
|
|
|
|
use std::collections::HashMap;
|
|
|
|
use std::sync::Arc;
|
|
|
|
use std::time::Duration;
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
use tree_hash::TreeHash;
|
2022-12-13 09:57:26 +00:00
|
|
|
use types::{
|
|
|
|
Address, Epoch, EthSpec, ExecPayload, ExecutionBlockHash, ForkName, FullPayload,
|
|
|
|
MainnetEthSpec, ProposerPreparationData, Slot,
|
|
|
|
};
|
2021-10-01 06:32:38 +00:00
|
|
|
|
|
|
|
type E = MainnetEthSpec;
|
|
|
|
|
|
|
|
// Test that the deposit_contract endpoint returns the correct chain_id and address.
|
|
|
|
// Regression test for https://github.com/sigp/lighthouse/issues/2657
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
async fn deposit_contract_custom_network() {
|
|
|
|
let validator_count = 24;
|
|
|
|
let mut spec = E::default_spec();
|
|
|
|
|
|
|
|
// Rinkeby, which we don't use elsewhere.
|
|
|
|
spec.deposit_chain_id = 4;
|
|
|
|
spec.deposit_network_id = 4;
|
|
|
|
// Arbitrary contract address.
|
|
|
|
spec.deposit_contract_address = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa".parse().unwrap();
|
|
|
|
|
2021-10-11 02:45:06 +00:00
|
|
|
let tester = InteractiveTester::<E>::new(Some(spec.clone()), validator_count).await;
|
2021-10-01 06:32:38 +00:00
|
|
|
let client = &tester.client;
|
|
|
|
|
|
|
|
let result = client.get_config_deposit_contract().await.unwrap().data;
|
|
|
|
|
|
|
|
let expected = DepositContractData {
|
|
|
|
address: spec.deposit_contract_address,
|
|
|
|
chain_id: spec.deposit_chain_id,
|
|
|
|
};
|
|
|
|
|
|
|
|
assert_eq!(result, expected);
|
|
|
|
}
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
|
2022-12-13 09:57:26 +00:00
|
|
|
/// Data structure for tracking fork choice updates received by the mock execution layer.
|
|
|
|
#[derive(Debug, Default)]
|
|
|
|
struct ForkChoiceUpdates {
|
|
|
|
updates: HashMap<ExecutionBlockHash, Vec<ForkChoiceUpdateMetadata>>,
|
|
|
|
}
|
|
|
|
|
|
|
|
#[derive(Debug, Clone)]
|
|
|
|
struct ForkChoiceUpdateMetadata {
|
|
|
|
received_at: Duration,
|
|
|
|
state: ForkChoiceState,
|
|
|
|
payload_attributes: Option<PayloadAttributes>,
|
|
|
|
}
|
|
|
|
|
|
|
|
impl ForkChoiceUpdates {
|
|
|
|
fn insert(&mut self, update: ForkChoiceUpdateMetadata) {
|
|
|
|
self.updates
|
|
|
|
.entry(update.state.head_block_hash)
|
|
|
|
.or_insert_with(Vec::new)
|
|
|
|
.push(update);
|
|
|
|
}
|
|
|
|
|
|
|
|
fn contains_update_for(&self, block_hash: ExecutionBlockHash) -> bool {
|
|
|
|
self.updates.contains_key(&block_hash)
|
|
|
|
}
|
|
|
|
|
|
|
|
/// Find the first fork choice update for `head_block_hash` with payload attributes for a
|
|
|
|
/// block proposal at `proposal_timestamp`.
|
|
|
|
fn first_update_with_payload_attributes(
|
|
|
|
&self,
|
|
|
|
head_block_hash: ExecutionBlockHash,
|
|
|
|
proposal_timestamp: u64,
|
|
|
|
) -> Option<ForkChoiceUpdateMetadata> {
|
|
|
|
self.updates
|
|
|
|
.get(&head_block_hash)?
|
|
|
|
.iter()
|
|
|
|
.find(|update| {
|
|
|
|
update
|
|
|
|
.payload_attributes
|
|
|
|
.as_ref()
|
|
|
|
.map_or(false, |payload_attributes| {
|
|
|
|
payload_attributes.timestamp == proposal_timestamp
|
|
|
|
})
|
|
|
|
})
|
|
|
|
.cloned()
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
pub struct ReOrgTest {
|
|
|
|
head_slot: Slot,
|
|
|
|
/// Number of slots between parent block and canonical head.
|
|
|
|
parent_distance: u64,
|
|
|
|
/// Number of slots between head block and block proposal slot.
|
|
|
|
head_distance: u64,
|
|
|
|
re_org_threshold: u64,
|
|
|
|
max_epochs_since_finalization: u64,
|
|
|
|
percent_parent_votes: usize,
|
|
|
|
percent_empty_votes: usize,
|
|
|
|
percent_head_votes: usize,
|
|
|
|
should_re_org: bool,
|
|
|
|
misprediction: bool,
|
|
|
|
}
|
|
|
|
|
|
|
|
impl Default for ReOrgTest {
|
|
|
|
/// Default config represents a regular easy re-org.
|
|
|
|
fn default() -> Self {
|
|
|
|
Self {
|
|
|
|
head_slot: Slot::new(30),
|
|
|
|
parent_distance: 1,
|
|
|
|
head_distance: 1,
|
|
|
|
re_org_threshold: 20,
|
|
|
|
max_epochs_since_finalization: 2,
|
|
|
|
percent_parent_votes: 100,
|
|
|
|
percent_empty_votes: 100,
|
|
|
|
percent_head_votes: 0,
|
|
|
|
should_re_org: true,
|
|
|
|
misprediction: false,
|
|
|
|
}
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
// Test that the beacon node will try to perform proposer boost re-orgs on late blocks when
|
|
|
|
// configured.
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_zero_weight() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest::default()).await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_epoch_boundary() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(31),
|
|
|
|
should_re_org: false,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_slot_after_epoch_boundary() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(33),
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_bad_ffg() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(64 + 22),
|
|
|
|
should_re_org: false,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_no_finality() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(96),
|
|
|
|
percent_parent_votes: 100,
|
|
|
|
percent_empty_votes: 0,
|
|
|
|
percent_head_votes: 100,
|
|
|
|
should_re_org: false,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_finality() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(129),
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_parent_distance() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(30),
|
|
|
|
parent_distance: 2,
|
|
|
|
should_re_org: false,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_head_distance() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(29),
|
|
|
|
head_distance: 2,
|
|
|
|
should_re_org: false,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_very_unhealthy() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(31),
|
|
|
|
parent_distance: 2,
|
|
|
|
head_distance: 2,
|
|
|
|
percent_parent_votes: 10,
|
|
|
|
percent_empty_votes: 10,
|
|
|
|
percent_head_votes: 10,
|
|
|
|
should_re_org: false,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
/// The head block is late but still receives 30% of the committee vote, leading to a misprediction.
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn proposer_boost_re_org_weight_misprediction() {
|
|
|
|
proposer_boost_re_org_test(ReOrgTest {
|
|
|
|
head_slot: Slot::new(30),
|
|
|
|
percent_empty_votes: 70,
|
|
|
|
percent_head_votes: 30,
|
|
|
|
should_re_org: false,
|
|
|
|
misprediction: true,
|
|
|
|
..Default::default()
|
|
|
|
})
|
|
|
|
.await;
|
|
|
|
}
|
|
|
|
|
|
|
|
/// Run a proposer boost re-org test.
|
|
|
|
///
|
|
|
|
/// - `head_slot`: the slot of the canonical head to be reorged
|
|
|
|
/// - `reorg_threshold`: committee percentage value for reorging
|
|
|
|
/// - `num_empty_votes`: percentage of comm of attestations for the parent block
|
|
|
|
/// - `num_head_votes`: number of attestations for the head block
|
|
|
|
/// - `should_re_org`: whether the proposer should build on the parent rather than the head
|
|
|
|
pub async fn proposer_boost_re_org_test(
|
|
|
|
ReOrgTest {
|
|
|
|
head_slot,
|
|
|
|
parent_distance,
|
|
|
|
head_distance,
|
|
|
|
re_org_threshold,
|
|
|
|
max_epochs_since_finalization,
|
|
|
|
percent_parent_votes,
|
|
|
|
percent_empty_votes,
|
|
|
|
percent_head_votes,
|
|
|
|
should_re_org,
|
|
|
|
misprediction,
|
|
|
|
}: ReOrgTest,
|
|
|
|
) {
|
|
|
|
assert!(head_slot > 0);
|
|
|
|
|
|
|
|
// We require a network with execution enabled so we can check EL message timings.
|
|
|
|
let mut spec = ForkName::Merge.make_genesis_spec(E::default_spec());
|
|
|
|
spec.terminal_total_difficulty = 1.into();
|
|
|
|
|
|
|
|
// Ensure there are enough validators to have `attesters_per_slot`.
|
|
|
|
let attesters_per_slot = 10;
|
|
|
|
let validator_count = E::slots_per_epoch() as usize * attesters_per_slot;
|
|
|
|
let all_validators = (0..validator_count).collect::<Vec<usize>>();
|
|
|
|
let num_initial = head_slot.as_u64().checked_sub(parent_distance + 1).unwrap();
|
|
|
|
|
|
|
|
// Check that the required vote percentages can be satisfied exactly using `attesters_per_slot`.
|
|
|
|
assert_eq!(100 % attesters_per_slot, 0);
|
|
|
|
let percent_per_attester = 100 / attesters_per_slot;
|
|
|
|
assert_eq!(percent_parent_votes % percent_per_attester, 0);
|
|
|
|
assert_eq!(percent_empty_votes % percent_per_attester, 0);
|
|
|
|
assert_eq!(percent_head_votes % percent_per_attester, 0);
|
|
|
|
let num_parent_votes = Some(attesters_per_slot * percent_parent_votes / 100);
|
|
|
|
let num_empty_votes = Some(attesters_per_slot * percent_empty_votes / 100);
|
|
|
|
let num_head_votes = Some(attesters_per_slot * percent_head_votes / 100);
|
|
|
|
|
|
|
|
let tester = InteractiveTester::<E>::new_with_mutator(
|
|
|
|
Some(spec),
|
|
|
|
validator_count,
|
|
|
|
Some(Box::new(move |builder| {
|
|
|
|
builder
|
|
|
|
.proposer_re_org_threshold(Some(ReOrgThreshold(re_org_threshold)))
|
|
|
|
.proposer_re_org_max_epochs_since_finalization(Epoch::new(
|
|
|
|
max_epochs_since_finalization,
|
|
|
|
))
|
|
|
|
})),
|
|
|
|
)
|
|
|
|
.await;
|
|
|
|
let harness = &tester.harness;
|
|
|
|
let mock_el = harness.mock_execution_layer.as_ref().unwrap();
|
|
|
|
let execution_ctx = mock_el.server.ctx.clone();
|
|
|
|
let slot_clock = &harness.chain.slot_clock;
|
|
|
|
|
|
|
|
// Move to terminal block.
|
|
|
|
mock_el.server.all_payloads_valid();
|
|
|
|
execution_ctx
|
|
|
|
.execution_block_generator
|
|
|
|
.write()
|
|
|
|
.move_to_terminal_block()
|
|
|
|
.unwrap();
|
|
|
|
|
|
|
|
// Send proposer preparation data for all validators.
|
|
|
|
let proposer_preparation_data = all_validators
|
|
|
|
.iter()
|
|
|
|
.map(|i| ProposerPreparationData {
|
|
|
|
validator_index: *i as u64,
|
|
|
|
fee_recipient: Address::from_low_u64_be(*i as u64),
|
|
|
|
})
|
|
|
|
.collect::<Vec<_>>();
|
|
|
|
harness
|
|
|
|
.chain
|
|
|
|
.execution_layer
|
|
|
|
.as_ref()
|
|
|
|
.unwrap()
|
|
|
|
.update_proposer_preparation(
|
|
|
|
head_slot.epoch(E::slots_per_epoch()) + 1,
|
|
|
|
&proposer_preparation_data,
|
|
|
|
)
|
|
|
|
.await;
|
|
|
|
|
|
|
|
// Create some chain depth.
|
|
|
|
harness.advance_slot();
|
|
|
|
harness
|
|
|
|
.extend_chain(
|
|
|
|
num_initial as usize,
|
|
|
|
BlockStrategy::OnCanonicalHead,
|
|
|
|
AttestationStrategy::AllValidators,
|
|
|
|
)
|
|
|
|
.await;
|
|
|
|
|
|
|
|
// Start collecting fork choice updates.
|
|
|
|
let forkchoice_updates = Arc::new(Mutex::new(ForkChoiceUpdates::default()));
|
|
|
|
let forkchoice_updates_inner = forkchoice_updates.clone();
|
|
|
|
let chain_inner = harness.chain.clone();
|
|
|
|
|
|
|
|
execution_ctx
|
|
|
|
.hook
|
|
|
|
.lock()
|
|
|
|
.set_forkchoice_updated_hook(Box::new(move |state, payload_attributes| {
|
|
|
|
let received_at = chain_inner.slot_clock.now_duration().unwrap();
|
|
|
|
let state = ForkChoiceState::from(state);
|
|
|
|
let payload_attributes = payload_attributes.map(Into::into);
|
|
|
|
let update = ForkChoiceUpdateMetadata {
|
|
|
|
received_at,
|
|
|
|
state,
|
|
|
|
payload_attributes,
|
|
|
|
};
|
|
|
|
forkchoice_updates_inner.lock().insert(update);
|
|
|
|
None
|
|
|
|
}));
|
|
|
|
|
|
|
|
// We set up the following block graph, where B is a block that arrives late and is re-orged
|
|
|
|
// by C.
|
|
|
|
//
|
|
|
|
// A | B | - |
|
|
|
|
// ^ | - | C |
|
|
|
|
|
|
|
|
let slot_a = Slot::new(num_initial + 1);
|
|
|
|
let slot_b = slot_a + parent_distance;
|
|
|
|
let slot_c = slot_b + head_distance;
|
|
|
|
|
|
|
|
harness.advance_slot();
|
|
|
|
let (block_a_root, block_a, state_a) = harness
|
|
|
|
.add_block_at_slot(slot_a, harness.get_current_state())
|
|
|
|
.await
|
|
|
|
.unwrap();
|
|
|
|
|
|
|
|
// Attest to block A during slot A.
|
|
|
|
let (block_a_parent_votes, _) = harness.make_attestations_with_limit(
|
|
|
|
&all_validators,
|
|
|
|
&state_a,
|
|
|
|
state_a.canonical_root(),
|
|
|
|
block_a_root,
|
|
|
|
slot_a,
|
|
|
|
num_parent_votes,
|
|
|
|
);
|
|
|
|
harness.process_attestations(block_a_parent_votes);
|
|
|
|
|
|
|
|
// Attest to block A during slot B.
|
|
|
|
for _ in 0..parent_distance {
|
|
|
|
harness.advance_slot();
|
|
|
|
}
|
|
|
|
let (block_a_empty_votes, block_a_attesters) = harness.make_attestations_with_limit(
|
|
|
|
&all_validators,
|
|
|
|
&state_a,
|
|
|
|
state_a.canonical_root(),
|
|
|
|
block_a_root,
|
|
|
|
slot_b,
|
|
|
|
num_empty_votes,
|
|
|
|
);
|
|
|
|
harness.process_attestations(block_a_empty_votes);
|
|
|
|
|
|
|
|
let remaining_attesters = all_validators
|
|
|
|
.iter()
|
|
|
|
.copied()
|
|
|
|
.filter(|index| !block_a_attesters.contains(index))
|
|
|
|
.collect::<Vec<_>>();
|
|
|
|
|
|
|
|
// Produce block B and process it halfway through the slot.
|
|
|
|
let (block_b, mut state_b) = harness.make_block(state_a.clone(), slot_b).await;
|
|
|
|
let block_b_root = block_b.canonical_root();
|
|
|
|
|
|
|
|
let obs_time = slot_clock.start_of(slot_b).unwrap() + slot_clock.slot_duration() / 2;
|
|
|
|
slot_clock.set_current_time(obs_time);
|
|
|
|
harness.chain.block_times_cache.write().set_time_observed(
|
|
|
|
block_b_root,
|
|
|
|
slot_b,
|
|
|
|
obs_time,
|
|
|
|
None,
|
|
|
|
None,
|
|
|
|
);
|
|
|
|
harness.process_block_result(block_b.clone()).await.unwrap();
|
|
|
|
|
|
|
|
// Add attestations to block B.
|
|
|
|
let (block_b_head_votes, _) = harness.make_attestations_with_limit(
|
|
|
|
&remaining_attesters,
|
|
|
|
&state_b,
|
|
|
|
state_b.canonical_root(),
|
|
|
|
block_b_root.into(),
|
|
|
|
slot_b,
|
|
|
|
num_head_votes,
|
|
|
|
);
|
|
|
|
harness.process_attestations(block_b_head_votes);
|
|
|
|
|
|
|
|
let payload_lookahead = harness.chain.config.prepare_payload_lookahead;
|
|
|
|
let fork_choice_lookahead = Duration::from_millis(500);
|
|
|
|
while harness.get_current_slot() != slot_c {
|
|
|
|
let current_slot = harness.get_current_slot();
|
|
|
|
let next_slot = current_slot + 1;
|
|
|
|
|
|
|
|
// Simulate the scheduled call to prepare proposers at 8 seconds into the slot.
|
|
|
|
harness.advance_to_slot_lookahead(next_slot, payload_lookahead);
|
|
|
|
harness
|
|
|
|
.chain
|
|
|
|
.prepare_beacon_proposer(current_slot)
|
|
|
|
.await
|
|
|
|
.unwrap();
|
|
|
|
|
|
|
|
// Simulate the scheduled call to fork choice + prepare proposers 500ms before the
|
|
|
|
// next slot.
|
|
|
|
harness.advance_to_slot_lookahead(next_slot, fork_choice_lookahead);
|
|
|
|
harness.chain.recompute_head_at_slot(next_slot).await;
|
|
|
|
harness
|
|
|
|
.chain
|
|
|
|
.prepare_beacon_proposer(current_slot)
|
|
|
|
.await
|
|
|
|
.unwrap();
|
|
|
|
|
|
|
|
harness.advance_slot();
|
|
|
|
harness.chain.per_slot_task().await;
|
|
|
|
}
|
|
|
|
|
|
|
|
// Produce block C.
|
|
|
|
// Advance state_b so we can get the proposer.
|
|
|
|
complete_state_advance(&mut state_b, None, slot_c, &harness.chain.spec).unwrap();
|
|
|
|
|
|
|
|
let proposer_index = state_b
|
|
|
|
.get_beacon_proposer_index(slot_c, &harness.chain.spec)
|
|
|
|
.unwrap();
|
|
|
|
let randao_reveal = harness
|
|
|
|
.sign_randao_reveal(&state_b, proposer_index, slot_c)
|
|
|
|
.into();
|
|
|
|
let unsigned_block_c = tester
|
|
|
|
.client
|
|
|
|
.get_validator_blocks(slot_c, &randao_reveal, None)
|
|
|
|
.await
|
|
|
|
.unwrap()
|
|
|
|
.data;
|
|
|
|
let block_c = harness.sign_beacon_block(unsigned_block_c, &state_b);
|
|
|
|
|
|
|
|
if should_re_org {
|
|
|
|
// Block C should build on A.
|
|
|
|
assert_eq!(block_c.parent_root(), block_a_root.into());
|
|
|
|
} else {
|
|
|
|
// Block C should build on B.
|
|
|
|
assert_eq!(block_c.parent_root(), block_b_root);
|
|
|
|
}
|
|
|
|
|
|
|
|
// Applying block C should cause it to become head regardless (re-org or continuation).
|
|
|
|
let block_root_c = harness
|
|
|
|
.process_block_result(block_c.clone())
|
|
|
|
.await
|
|
|
|
.unwrap()
|
|
|
|
.into();
|
|
|
|
assert_eq!(harness.head_block_root(), block_root_c);
|
|
|
|
|
|
|
|
// Check the fork choice updates that were sent.
|
|
|
|
let forkchoice_updates = forkchoice_updates.lock();
|
|
|
|
let block_a_exec_hash = block_a.message().execution_payload().unwrap().block_hash();
|
|
|
|
let block_b_exec_hash = block_b.message().execution_payload().unwrap().block_hash();
|
|
|
|
|
|
|
|
let block_c_timestamp = block_c.message().execution_payload().unwrap().timestamp();
|
|
|
|
|
|
|
|
// If we re-orged then no fork choice update for B should have been sent.
|
|
|
|
assert_eq!(
|
|
|
|
should_re_org,
|
|
|
|
!forkchoice_updates.contains_update_for(block_b_exec_hash),
|
|
|
|
"{block_b_exec_hash:?}"
|
|
|
|
);
|
|
|
|
|
|
|
|
// Check the timing of the first fork choice update with payload attributes for block C.
|
|
|
|
let c_parent_hash = if should_re_org {
|
|
|
|
block_a_exec_hash
|
|
|
|
} else {
|
|
|
|
block_b_exec_hash
|
|
|
|
};
|
|
|
|
let first_update = forkchoice_updates
|
|
|
|
.first_update_with_payload_attributes(c_parent_hash, block_c_timestamp)
|
|
|
|
.unwrap();
|
|
|
|
let payload_attribs = first_update.payload_attributes.as_ref().unwrap();
|
|
|
|
|
|
|
|
let lookahead = slot_clock
|
|
|
|
.start_of(slot_c)
|
|
|
|
.unwrap()
|
|
|
|
.checked_sub(first_update.received_at)
|
|
|
|
.unwrap();
|
|
|
|
|
|
|
|
if !misprediction {
|
|
|
|
assert_eq!(
|
|
|
|
lookahead, payload_lookahead,
|
|
|
|
"lookahead={lookahead:?}, timestamp={}, prev_randao={:?}",
|
|
|
|
payload_attribs.timestamp, payload_attribs.prev_randao,
|
|
|
|
);
|
|
|
|
} else {
|
|
|
|
// On a misprediction we issue the first fcU 500ms before creating a block!
|
|
|
|
assert_eq!(
|
|
|
|
lookahead, fork_choice_lookahead,
|
|
|
|
"timestamp={}, prev_randao={:?}",
|
|
|
|
payload_attribs.timestamp, payload_attribs.prev_randao,
|
|
|
|
);
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
// Test that running fork choice before proposing results in selection of the correct head.
|
|
|
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
|
|
|
pub async fn fork_choice_before_proposal() {
|
|
|
|
// Validator count needs to be at least 32 or proposer boost gets set to 0 when computing
|
|
|
|
// `validator_count // 32`.
|
|
|
|
let validator_count = 32;
|
|
|
|
let all_validators = (0..validator_count).collect::<Vec<_>>();
|
|
|
|
let num_initial: u64 = 31;
|
|
|
|
|
|
|
|
let tester = InteractiveTester::<E>::new(None, validator_count).await;
|
|
|
|
let harness = &tester.harness;
|
|
|
|
|
|
|
|
// Create some chain depth.
|
|
|
|
harness.advance_slot();
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
harness
|
|
|
|
.extend_chain(
|
|
|
|
num_initial as usize,
|
|
|
|
BlockStrategy::OnCanonicalHead,
|
|
|
|
AttestationStrategy::AllValidators,
|
|
|
|
)
|
|
|
|
.await;
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
|
|
|
|
// We set up the following block graph, where B is a block that is temporarily orphaned by C,
|
|
|
|
// but is then reinstated and built upon by D.
|
|
|
|
//
|
|
|
|
// A | B | - | D |
|
|
|
|
// ^ | - | C |
|
|
|
|
let slot_a = Slot::new(num_initial);
|
|
|
|
let slot_b = slot_a + 1;
|
|
|
|
let slot_c = slot_a + 2;
|
|
|
|
let slot_d = slot_a + 3;
|
|
|
|
|
|
|
|
let state_a = harness.get_current_state();
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
let (block_b, state_b) = harness.make_block(state_a.clone(), slot_b).await;
|
2022-09-23 03:52:42 +00:00
|
|
|
let block_root_b = harness
|
|
|
|
.process_block(slot_b, block_b.canonical_root(), block_b)
|
|
|
|
.await
|
|
|
|
.unwrap();
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
|
|
|
|
// Create attestations to B but keep them in reserve until after C has been processed.
|
|
|
|
let attestations_b = harness.make_attestations(
|
|
|
|
&all_validators,
|
|
|
|
&state_b,
|
|
|
|
state_b.tree_hash_root(),
|
|
|
|
block_root_b,
|
|
|
|
slot_b,
|
|
|
|
);
|
|
|
|
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
let (block_c, state_c) = harness.make_block(state_a, slot_c).await;
|
|
|
|
let block_root_c = harness
|
2022-09-23 03:52:42 +00:00
|
|
|
.process_block(slot_c, block_c.canonical_root(), block_c.clone())
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
.await
|
|
|
|
.unwrap();
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
|
|
|
|
// Create attestations to C from a small number of validators and process them immediately.
|
|
|
|
let attestations_c = harness.make_attestations(
|
|
|
|
&all_validators[..validator_count / 2],
|
|
|
|
&state_c,
|
|
|
|
state_c.tree_hash_root(),
|
|
|
|
block_root_c,
|
|
|
|
slot_c,
|
|
|
|
);
|
|
|
|
harness.process_attestations(attestations_c);
|
|
|
|
|
|
|
|
// Apply the attestations to B, but don't re-run fork choice.
|
|
|
|
harness.process_attestations(attestations_b);
|
|
|
|
|
|
|
|
// Due to proposer boost, the head should be C during slot C.
|
|
|
|
assert_eq!(
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
harness.chain.canonical_head.cached_head().head_block_root(),
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
block_root_c.into()
|
|
|
|
);
|
|
|
|
|
|
|
|
// Ensure that building a block via the HTTP API re-runs fork choice and builds block D upon B.
|
|
|
|
// Manually prod the per-slot task, because the slot timer doesn't run in the background in
|
|
|
|
// these tests.
|
|
|
|
harness.advance_slot();
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
harness.chain.per_slot_task().await;
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
|
|
|
|
let proposer_index = state_b
|
|
|
|
.get_beacon_proposer_index(slot_d, &harness.chain.spec)
|
|
|
|
.unwrap();
|
|
|
|
let randao_reveal = harness
|
|
|
|
.sign_randao_reveal(&state_b, proposer_index, slot_d)
|
|
|
|
.into();
|
|
|
|
let block_d = tester
|
|
|
|
.client
|
|
|
|
.get_validator_blocks::<E, FullPayload<E>>(slot_d, &randao_reveal, None)
|
|
|
|
.await
|
|
|
|
.unwrap()
|
|
|
|
.data;
|
|
|
|
|
|
|
|
// Head is now B.
|
|
|
|
assert_eq!(
|
Use async code when interacting with EL (#3244)
## Overview
This rather extensive PR achieves two primary goals:
1. Uses the finalized/justified checkpoints of fork choice (FC), rather than that of the head state.
2. Refactors fork choice, block production and block processing to `async` functions.
Additionally, it achieves:
- Concurrent forkchoice updates to the EL and cache pruning after a new head is selected.
- Concurrent "block packing" (attestations, etc) and execution payload retrieval during block production.
- Concurrent per-block-processing and execution payload verification during block processing.
- The `Arc`-ification of `SignedBeaconBlock` during block processing (it's never mutated, so why not?):
- I had to do this to deal with sending blocks into spawned tasks.
- Previously we were cloning the beacon block at least 2 times during each block processing, these clones are either removed or turned into cheaper `Arc` clones.
- We were also `Box`-ing and un-`Box`-ing beacon blocks as they moved throughout the networking crate. This is not a big deal, but it's nice to avoid shifting things between the stack and heap.
- Avoids cloning *all the blocks* in *every chain segment* during sync.
- It also has the potential to clean up our code where we need to pass an *owned* block around so we can send it back in the case of an error (I didn't do much of this, my PR is already big enough :sweat_smile:)
- The `BeaconChain::HeadSafetyStatus` struct was removed. It was an old relic from prior merge specs.
For motivation for this change, see https://github.com/sigp/lighthouse/pull/3244#issuecomment-1160963273
## Changes to `canonical_head` and `fork_choice`
Previously, the `BeaconChain` had two separate fields:
```
canonical_head: RwLock<Snapshot>,
fork_choice: RwLock<BeaconForkChoice>
```
Now, we have grouped these values under a single struct:
```
canonical_head: CanonicalHead {
cached_head: RwLock<Arc<Snapshot>>,
fork_choice: RwLock<BeaconForkChoice>
}
```
Apart from ergonomics, the only *actual* change here is wrapping the canonical head snapshot in an `Arc`. This means that we no longer need to hold the `cached_head` (`canonical_head`, in old terms) lock when we want to pull some values from it. This was done to avoid deadlock risks by preventing functions from acquiring (and holding) the `cached_head` and `fork_choice` locks simultaneously.
## Breaking Changes
### The `state` (root) field in the `finalized_checkpoint` SSE event
Consider the scenario where epoch `n` is just finalized, but `start_slot(n)` is skipped. There are two state roots we might in the `finalized_checkpoint` SSE event:
1. The state root of the finalized block, which is `get_block(finalized_checkpoint.root).state_root`.
4. The state root at slot of `start_slot(n)`, which would be the state from (1), but "skipped forward" through any skip slots.
Previously, Lighthouse would choose (2). However, we can see that when [Teku generates that event](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/beaconrestapi/src/main/java/tech/pegasys/teku/beaconrestapi/handlers/v1/events/EventSubscriptionManager.java#L171-L182) it uses [`getStateRootFromBlockRoot`](https://github.com/ConsenSys/teku/blob/de2b2801c89ef5abf983d6bf37867c37fc47121f/data/provider/src/main/java/tech/pegasys/teku/api/ChainDataProvider.java#L336-L341) which uses (1).
I have switched Lighthouse from (2) to (1). I think it's a somewhat arbitrary choice between the two, where (1) is easier to compute and is consistent with Teku.
## Notes for Reviewers
I've renamed `BeaconChain::fork_choice` to `BeaconChain::recompute_head`. Doing this helped ensure I broke all previous uses of fork choice and I also find it more descriptive. It describes an action and can't be confused with trying to get a reference to the `ForkChoice` struct.
I've changed the ordering of SSE events when a block is received. It used to be `[block, finalized, head]` and now it's `[block, head, finalized]`. It was easier this way and I don't think we were making any promises about SSE event ordering so it's not "breaking".
I've made it so fork choice will run when it's first constructed. I did this because I wanted to have a cached version of the last call to `get_head`. Ensuring `get_head` has been run *at least once* means that the cached values doesn't need to wrapped in an `Option`. This was fairly simple, it just involved passing a `slot` to the constructor so it knows *when* it's being run. When loading a fork choice from the store and a slot clock isn't handy I've just used the `slot` that was saved in the `fork_choice_store`. That seems like it would be a faithful representation of the slot when we saved it.
I added the `genesis_time: u64` to the `BeaconChain`. It's small, constant and nice to have around.
Since we're using FC for the fin/just checkpoints, we no longer get the `0x00..00` roots at genesis. You can see I had to remove a work-around in `ef-tests` here: b56be3bc2. I can't find any reason why this would be an issue, if anything I think it'll be better since the genesis-alias has caught us out a few times (0x00..00 isn't actually a real root). Edit: I did find a case where the `network` expected the 0x00..00 alias and patched it here: 3f26ac3e2.
You'll notice a lot of changes in tests. Generally, tests should be functionally equivalent. Here are the things creating the most diff-noise in tests:
- Changing tests to be `tokio::async` tests.
- Adding `.await` to fork choice, block processing and block production functions.
- Refactor of the `canonical_head` "API" provided by the `BeaconChain`. E.g., `chain.canonical_head.cached_head()` instead of `chain.canonical_head.read()`.
- Wrapping `SignedBeaconBlock` in an `Arc`.
- In the `beacon_chain/tests/block_verification`, we can't use the `lazy_static` `CHAIN_SEGMENT` variable anymore since it's generated with an async function. We just generate it in each test, not so efficient but hopefully insignificant.
I had to disable `rayon` concurrent tests in the `fork_choice` tests. This is because the use of `rayon` and `block_on` was causing a panic.
Co-authored-by: Mac L <mjladson@pm.me>
2022-07-03 05:36:50 +00:00
|
|
|
harness.chain.canonical_head.cached_head().head_block_root(),
|
Run fork choice before block proposal (#3168)
## Issue Addressed
Upcoming spec change https://github.com/ethereum/consensus-specs/pull/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 :cry: ). 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.
2022-05-20 05:02:11 +00:00
|
|
|
block_root_b.into()
|
|
|
|
);
|
|
|
|
// D's parent is B.
|
|
|
|
assert_eq!(block_d.parent_root(), block_root_b.into());
|
|
|
|
}
|