Fix bugs in proposer calculation post-Fulu (#8101)

As identified by a researcher during the Fusaka security competition, we were computing the proposer index incorrectly in some places by computing without lookahead.


  - [x] Add "low level" checks to computation functions in `consensus/types` to ensure they error cleanly
- [x] Re-work the determination of proposer shuffling decision roots, which are now fork aware.
- [x] Re-work and simplify the beacon proposer cache to be fork-aware.
- [x] Optimise `with_proposer_cache` to use `OnceCell`.
- [x] All tests passing.
- [x] Resolve all remaining `FIXME(sproul)`s.
- [x] Unit tests for `ProtoBlock::proposer_shuffling_root_for_child_block`.
- [x] End-to-end regression test.
- [x] Test on pre-Fulu network.
- [x] Test on post-Fulu network.


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

consensus/types/src/beacon_state.rs

@@ -173,7 +173,21 @@ pub enum Error {
     AggregatorNotInCommittee {
         aggregator_index: u64,
     },
-    PleaseNotifyTheDevs(String),
+    ComputeProposerIndicesPastEpoch {
+        current_epoch: Epoch,
+        request_epoch: Epoch,
+    },
+    ComputeProposerIndicesInsufficientLookahead {
+        current_epoch: Epoch,
+        request_epoch: Epoch,
+    },
+    ComputeProposerIndicesExcessiveLookahead {
+        current_epoch: Epoch,
+        request_epoch: Epoch,
+    },
+    ProposerLookaheadOutOfBounds {
+        i: usize,
+    },
 }
 
 /// Control whether an epoch-indexed field can be indexed at the next epoch or not.
@@ -886,8 +900,9 @@ impl<E: EthSpec> BeaconState<E> {
         &self,
         epoch: Epoch,
         block_root: Hash256,
+        spec: &ChainSpec,
     ) -> Result<Hash256, Error> {
-        let decision_slot = self.proposer_shuffling_decision_slot(epoch);
+        let decision_slot = spec.proposer_shuffling_decision_slot::<E>(epoch);
         if self.slot() <= decision_slot {
             Ok(block_root)
         } else {
@@ -902,19 +917,18 @@ impl<E: EthSpec> BeaconState<E> {
     ///
     /// The `block_root` covers the one-off scenario where the genesis block decides its own
     /// shuffling. It should be set to the latest block applied to `self` or the genesis block root.
-    pub fn proposer_shuffling_decision_root(&self, block_root: Hash256) -> Result<Hash256, Error> {
-        let decision_slot = self.proposer_shuffling_decision_slot(self.current_epoch());
-        if self.slot() == decision_slot {
-            Ok(block_root)
-        } else {
-            self.get_block_root(decision_slot).copied()
-        }
+    pub fn proposer_shuffling_decision_root(
+        &self,
+        block_root: Hash256,
+        spec: &ChainSpec,
+    ) -> Result<Hash256, Error> {
+        self.proposer_shuffling_decision_root_at_epoch(self.current_epoch(), block_root, spec)
     }
 
-    /// Returns the slot at which the proposer shuffling was decided. The block root at this slot
-    /// can be used to key the proposer shuffling for the given epoch.
-    fn proposer_shuffling_decision_slot(&self, epoch: Epoch) -> Slot {
-        epoch.start_slot(E::slots_per_epoch()).saturating_sub(1_u64)
+    pub fn epoch_cache_decision_root(&self, block_root: Hash256) -> Result<Hash256, Error> {
+        // Epoch cache decision root for the current epoch (N) is the block root at the end of epoch
+        // N - 1. This is the same as the root that determines the next epoch attester shuffling.
+        self.attester_shuffling_decision_root(block_root, RelativeEpoch::Next)
     }
 
     /// Returns the block root which decided the attester shuffling for the given `relative_epoch`.
@@ -998,6 +1012,45 @@ impl<E: EthSpec> BeaconState<E> {
         indices: &[usize],
         spec: &ChainSpec,
     ) -> Result<Vec<usize>, Error> {
+        // Regardless of fork, we never support computing proposer indices for past epochs.
+        let current_epoch = self.current_epoch();
+        if epoch < current_epoch {
+            return Err(Error::ComputeProposerIndicesPastEpoch {
+                current_epoch,
+                request_epoch: epoch,
+            });
+        }
+
+        if spec.fork_name_at_epoch(epoch).fulu_enabled() {
+            // Post-Fulu we must never compute proposer indices using insufficient lookahead. This
+            // would be very dangerous as it would lead to conflicts between the *true* proposer as
+            // defined by `self.proposer_lookahead` and the output of this function.
+            // With MIN_SEED_LOOKAHEAD=1 (common config), this is equivalent to checking that the
+            // requested epoch is not the current epoch.
+            //
+            // We do not run this check if this function is called from `upgrade_to_fulu`,
+            // which runs *after* the slot is incremented, and needs to compute the proposer
+            // shuffling for the epoch that was just transitioned into.
+            if self.fork_name_unchecked().fulu_enabled()
+                && epoch < current_epoch.safe_add(spec.min_seed_lookahead)?
+            {
+                return Err(Error::ComputeProposerIndicesInsufficientLookahead {
+                    current_epoch,
+                    request_epoch: epoch,
+                });
+            }
+        } else {
+            // Pre-Fulu the situation is reversed, we *should not* compute proposer indices using
+            // too much lookahead. To do so would make us vulnerable to changes in the proposer
+            // indices caused by effective balance changes.
+            if epoch >= current_epoch.safe_add(spec.min_seed_lookahead)? {
+                return Err(Error::ComputeProposerIndicesExcessiveLookahead {
+                    current_epoch,
+                    request_epoch: epoch,
+                });
+            }
+        }
+
         epoch
             .slot_iter(E::slots_per_epoch())
             .map(|slot| {
@@ -1146,10 +1199,7 @@ impl<E: EthSpec> BeaconState<E> {
             let index = slot.as_usize().safe_rem(E::slots_per_epoch() as usize)?;
             proposer_lookahead
                 .get(index)
-                .ok_or(Error::PleaseNotifyTheDevs(format!(
-                    "Proposer lookahead out of bounds: {} for slot: {}",
-                    index, slot
-                )))
+                .ok_or(Error::ProposerLookaheadOutOfBounds { i: index })
                 .map(|index| *index as usize)
         } else {
             // Pre-Fulu
@@ -1168,6 +1218,25 @@ impl<E: EthSpec> BeaconState<E> {
         epoch: Epoch,
         spec: &ChainSpec,
     ) -> Result<Vec<usize>, Error> {
+        // This isn't in the spec, but we remove the footgun that is requesting the current epoch
+        // for a Fulu state.
+        if let Ok(proposer_lookahead) = self.proposer_lookahead()
+            && epoch >= self.current_epoch()
+            && epoch <= self.next_epoch()?
+        {
+            let slots_per_epoch = E::slots_per_epoch() as usize;
+            let start_offset = if epoch == self.current_epoch() {
+                0
+            } else {
+                slots_per_epoch
+            };
+            return Ok(proposer_lookahead
+                .iter_from(start_offset)?
+                .take(slots_per_epoch)
+                .map(|x| *x as usize)
+                .collect());
+        }
+
         // Not using the cached validator indices since they are shuffled.
         let indices = self.get_active_validator_indices(epoch, spec)?;
 

consensus/types/src/chain_spec.rs

@@ -865,6 +865,28 @@ impl ChainSpec {
         )
     }
 
+    /// Returns the slot at which the proposer shuffling was decided.
+    ///
+    /// The block root at this slot can be used to key the proposer shuffling for the given epoch.
+    pub fn proposer_shuffling_decision_slot<E: EthSpec>(&self, epoch: Epoch) -> Slot {
+        if self.fork_name_at_epoch(epoch).fulu_enabled() {
+            // Post-Fulu the proposer shuffling decision slot for epoch N is the slot at the end
+            // of epoch N - 2 (note: min_seed_lookahead=1 in all current configs).
+            epoch
+                .saturating_sub(self.min_seed_lookahead)
+                .start_slot(E::slots_per_epoch())
+                .saturating_sub(1_u64)
+        } else {
+            // Pre-Fulu the proposer shuffling decision slot for epoch N is the slot at the end of
+            // epoch N - 1 (note: +1 -1 for min_seed_lookahead=1 in all current configs).
+            epoch
+                .saturating_add(Epoch::new(1))
+                .saturating_sub(self.min_seed_lookahead)
+                .start_slot(E::slots_per_epoch())
+                .saturating_sub(1_u64)
+        }
+    }
+
     /// Returns a `ChainSpec` compatible with the Ethereum Foundation specification.
     pub fn mainnet() -> Self {
         Self {

consensus/types/src/epoch_cache.rs

@@ -5,9 +5,13 @@ use std::sync::Arc;
 /// Cache of values which are uniquely determined at the start of an epoch.
 ///
 /// The values are fixed with respect to the last block of the _prior_ epoch, which we refer
-/// to as the "decision block". This cache is very similar to the `BeaconProposerCache` in that
-/// beacon proposers are determined at exactly the same time as the values in this cache, so
-/// the keys for the two caches are identical.
+/// to as the "decision block".
+///
+/// Prior to Fulu this cache was similar to the `BeaconProposerCache` in that beacon proposers were
+/// determined at exactly the same time as the values in this cache, so the keys for the two caches
+/// were identical.
+///
+/// Post-Fulu, we use a different key (the proposers have more lookahead).
 #[cfg_attr(feature = "arbitrary", derive(arbitrary::Arbitrary))]
 #[derive(Debug, PartialEq, Eq, Clone, Default)]
 pub struct EpochCache {