Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion beacon_node/beacon_chain/src/beacon_chain.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4175,7 +4175,14 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
};

// Read the cached head prior to taking the fork choice lock to avoid potential deadlocks.
let old_head_slot = self.canonical_head.cached_head().head_slot();
let cached_head = self.canonical_head.cached_head();
let old_head_slot = cached_head.head_slot();

// Compute the expected proposer for `current_slot` on the canonical chain. This is used by
// `on_block` to gate proposer boost on the block's proposer matching the canonical proposer
// (per spec `update_proposer_boost_root` added in v1.7.0-alpha.5).
let canonical_head_proposer_index =
self.canonical_head_proposer_index(current_slot, &cached_head)?;

// Take an upgradable read lock on fork choice so we can check if this block has already
// been imported. We don't want to repeat work importing a block that is already imported.
Expand Down Expand Up @@ -4208,6 +4215,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
block_delay,
&state,
payload_verification_status,
canonical_head_proposer_index,
&self.spec,
)
.map_err(|e| BlockError::BeaconChainError(Box::new(e.into())))?;
Expand Down Expand Up @@ -4950,6 +4958,42 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
}))
}

/// Compute the expected beacon proposer for `slot` on the canonical chain extending `cached_head`.
///
/// Uses the beacon proposer cache to avoid recomputing the shuffling on every block import.
///
/// This is used by `update_proposer_boost_root` to gate proposer boost on the block's proposer
/// matching the canonical proposer, per consensus-specs v1.7.0-alpha.5.
///
/// This function should never error unless there is some corruption of the head state. If a
/// state advance is needed, it will be handled by the proposer cache.
pub fn canonical_head_proposer_index(
&self,
slot: Slot,
cached_head: &CachedHead<T::EthSpec>,
) -> Result<u64, Error> {
let proposal_epoch = slot.epoch(T::EthSpec::slots_per_epoch());
let head_block_root = cached_head.head_block_root();
let head_state = &cached_head.snapshot.beacon_state;

let shuffling_decision_root = head_state.proposer_shuffling_decision_root_at_epoch(
proposal_epoch,
head_block_root,
&self.spec,
)?;

self.with_proposer_cache::<_, Error>(
shuffling_decision_root,
proposal_epoch,
|proposers| {
proposers
.get_slot::<T::EthSpec>(slot)
.map(|p| p.index as u64)
},
|| Ok((cached_head.head_state_root(), head_state.clone())),
)
}

pub fn get_expected_withdrawals(
&self,
forkchoice_update_params: &ForkchoiceUpdateParameters,
Expand Down
1 change: 1 addition & 0 deletions beacon_node/beacon_chain/tests/payload_invalidation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1093,6 +1093,7 @@ async fn invalid_parent() {
Duration::from_secs(0),
&state,
PayloadVerificationStatus::Optimistic,
block.message().proposer_index(),
&rig.harness.chain.spec,
),
Err(ForkChoiceError::ProtoArrayStringError(message))
Expand Down
36 changes: 20 additions & 16 deletions beacon_node/http_api/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3450,43 +3450,47 @@ impl ApiTester {
.unwrap()
.unwrap_or(self.chain.head_beacon_block_root());

// Presently, the beacon chain harness never runs the code that primes the proposer
// cache. If this changes in the future then we'll need some smarter logic here, but
// this is succinct and effective for the time being.
assert!(
self.chain
.beacon_proposer_cache
.lock()
.get_epoch::<E>(dependent_root, epoch)
.is_none(),
"the proposer cache should miss initially"
);
// Block import primes the proposer cache for each epoch it runs through (to gate
// proposer boost), so epochs `<= current_epoch` are already cached. The only epoch
// for which we can observe the endpoint's own caching behaviour is
// `current_epoch + 1`, which no block import has touched yet.
if epoch == current_epoch + 1 {
assert!(
self.chain
.beacon_proposer_cache
.lock()
.get_epoch::<E>(dependent_root, epoch)
.is_none(),
"the proposer cache should miss initially for the next epoch"
);
}

let result = self
.client
.get_validator_duties_proposer(epoch)
.await
.unwrap();

// Check that current-epoch requests prime the proposer cache, whilst non-current
// requests don't.
// A current-epoch request should leave the cache primed (block import already did so,
// but this is still a useful end-to-end check). A request for `current_epoch + 1`
// should not prime the cache.
if epoch == current_epoch {
assert!(
self.chain
.beacon_proposer_cache
.lock()
.get_epoch::<E>(dependent_root, epoch)
.is_some(),
"a current-epoch request should prime the proposer cache"
"the proposer cache should be primed for the current epoch"
);
} else {
} else if epoch == current_epoch + 1 {
assert!(
self.chain
.beacon_proposer_cache
.lock()
.get_epoch::<E>(dependent_root, epoch)
.is_none(),
"a non-current-epoch request should not prime the proposer cache"
"a request for the next epoch should not prime the proposer cache"
);
}

Expand Down
17 changes: 10 additions & 7 deletions consensus/fork_choice/src/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -756,6 +756,7 @@ where
block_delay: Duration,
state: &BeaconState<E>,
payload_verification_status: PayloadVerificationStatus,
canonical_head_proposer_index: u64,
spec: &ChainSpec,
) -> Result<(), Error<T::Error>> {
let _timer = metrics::start_timer(&metrics::FORK_CHOICE_ON_BLOCK_TIMES);
Expand Down Expand Up @@ -820,16 +821,18 @@ where

let attestation_threshold = spec.get_attestation_due::<E>(block.slot());

// Add proposer score boost if the block is timely.
// TODO(gloas): the spec's `update_proposer_boost_root` additionally checks that
// `block.proposer_index == get_beacon_proposer_index(head_state)` — i.e. that
// the block's proposer matches the expected proposer on the canonical chain.
// This requires calling `get_head` and advancing the head state to the current
// slot, which is expensive. Implement once we have a cached proposer index.
// Add proposer score boost if the block is the first timely block for this slot and its
// proposer matches the expected proposer on the canonical chain (per spec
// `update_proposer_boost_root`, introduced in v1.7.0-alpha.5).
let is_before_attesting_interval = block_delay < attestation_threshold;

let is_first_block = self.fc_store.proposer_boost_root().is_zero();
if current_slot == block.slot() && is_before_attesting_interval && is_first_block {
let is_canonical_proposer = block.proposer_index() == canonical_head_proposer_index;
if current_slot == block.slot()
&& is_before_attesting_interval
&& is_first_block
&& is_canonical_proposer
{
self.fc_store.set_proposer_boost_root(block_root);
}

Expand Down
2 changes: 2 additions & 0 deletions consensus/fork_choice/tests/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,7 @@ impl ForkChoiceTest {
Duration::from_secs(0),
&state,
PayloadVerificationStatus::Verified,
block.message().proposer_index(),
&self.harness.chain.spec,
)
.unwrap();
Expand Down Expand Up @@ -359,6 +360,7 @@ impl ForkChoiceTest {
Duration::from_secs(0),
&state,
PayloadVerificationStatus::Verified,
block.message().proposer_index(),
&self.harness.chain.spec,
)
.expect_err("on_block did not return an error");
Expand Down
10 changes: 1 addition & 9 deletions testing/ef_tests/src/cases/fork_choice.rs
Original file line number Diff line number Diff line change
Expand Up @@ -335,15 +335,6 @@ impl<E: EthSpec> Case for ForkChoiceTest<E> {
}

fn result(&self, _case_index: usize, fork_name: ForkName) -> Result<(), Error> {
// TODO(gloas): We have not implemented this change to fork choice/proposer boost yet.
// https://github.com/sigp/lighthouse/issues/8689
if self.description == "voting_source_beyond_two_epoch"
|| self.description == "justified_update_not_realized_finality"
|| self.description == "justified_update_always_if_better"
{
return Err(Error::SkippedKnownFailure);
}

let tester = Tester::new(self, testing_spec::<E>(fork_name))?;

for step in &self.steps {
Expand Down Expand Up @@ -791,6 +782,7 @@ impl<E: EthSpec> Tester<E> {
block_delay,
&state,
PayloadVerificationStatus::Irrelevant,
block.message().proposer_index(),
&self.harness.chain.spec,
);

Expand Down
Loading