Update proposer boost calculation (#9215)

Closes:

- https://github.com/sigp/lighthouse/issues/8689


  - Calculate the proposer index on the canonical chain (from canonical head) at `slot` and plumb it through to fork choice so it can be used to determine whether or not to apply the proposer boost. We use the proposer cache to handle state advances and avoid duplicate work.
- Update our FC tests to use `block.message().proposer_index()` (always pass), we are not attempting to test this feature in those tests. The EF tests use the correct canonical proposer idnex via `on_block`, except for invalid blocks which just auto-pass this check (these blocks get rejected by other checks in `on_block` anyway).


Co-Authored-By: Michael Sproul <michael@sigmaprime.io>

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -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.
@@ -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())))?;
@@ -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,

beacon_node/beacon_chain/tests/payload_invalidation.rs

@@ -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))

beacon_node/http_api/tests/tests.rs

@@ -3450,17 +3450,20 @@ 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
@@ -3468,8 +3471,9 @@ impl ApiTester {
                 .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
@@ -3477,16 +3481,16 @@ impl ApiTester {
                         .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"
                 );
             }
 

consensus/fork_choice/src/fork_choice.rs

@@ -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);
@@ -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);
         }
 

consensus/fork_choice/tests/tests.rs

@@ -316,6 +316,7 @@ impl ForkChoiceTest {
                 Duration::from_secs(0),
                 &state,
                 PayloadVerificationStatus::Verified,
+                block.message().proposer_index(),
                 &self.harness.chain.spec,
             )
             .unwrap();
@@ -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");

testing/ef_tests/src/cases/fork_choice.rs

@@ -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 {
@@ -791,6 +782,7 @@ impl<E: EthSpec> Tester<E> {
                     block_delay,
                     &state,
                     PayloadVerificationStatus::Irrelevant,
+                    block.message().proposer_index(),
                     &self.harness.chain.spec,
                 );