Fix proposer shuffling decision slot at boundary (#8128)

Follow-up to the bug fixed in:

- #8121

This fixes the root cause of that bug, which was introduced by me in:

- #8101

Lion identified the issue here:

- #8101 (comment)


  In the methods that compute the proposer shuffling decision root, ensure we don't use lookahead for the Fulu fork epoch itself. This is accomplished by checking if Fulu is enabled at `epoch - 1`, i.e. if `epoch > fulu_fork_epoch`.

I haven't updated the methods that _compute_ shufflings to use these new corrected bounds (e.g. `BeaconState::compute_proposer_indices`), although we could make this change in future. The `get_beacon_proposer_indices` method already gracefully handles the Fulu boundary case by using the `proposer_lookahead` field (if initialised).


Co-Authored-By: Michael Sproul <[email protected]>

Author: michaelsproul

beacon_node/beacon_chain/tests/store_tests.rs

@@ -1195,14 +1195,18 @@ fn check_shuffling_compatible(
 ///
 /// - ProtoBlock::proposer_shuffling_root_for_child_block, and
 /// - BeaconState::proposer_shuffling_decision_root{_at_epoch}
-async fn proposer_shuffling_root_consistency_test(parent_slot: u64, child_slot: u64) {
+async fn proposer_shuffling_root_consistency_test(
+    spec: ChainSpec,
+    parent_slot: u64,
+    child_slot: u64,
+) {
     let child_slot = Slot::new(child_slot);
     let db_path = tempdir().unwrap();
-    let store = get_store(&db_path);
+    let store = get_store_generic(&db_path, Default::default(), spec.clone());
     let validators_keypairs =
         types::test_utils::generate_deterministic_keypairs(LOW_VALIDATOR_COUNT);
     let harness = TestHarness::builder(MinimalEthSpec)
-        .default_spec()
+        .spec(spec.into())
         .keypairs(validators_keypairs)
         .fresh_disk_store(store)
         .mock_execution_layer()
@@ -1268,17 +1272,58 @@ async fn proposer_shuffling_root_consistency_test(parent_slot: u64, child_slot:
 
 #[tokio::test]
 async fn proposer_shuffling_root_consistency_same_epoch() {
-    proposer_shuffling_root_consistency_test(32, 39).await;
+    let spec = test_spec::<E>();
+    proposer_shuffling_root_consistency_test(spec, 32, 39).await;
 }
 
 #[tokio::test]
 async fn proposer_shuffling_root_consistency_next_epoch() {
-    proposer_shuffling_root_consistency_test(32, 47).await;
+    let spec = test_spec::<E>();
+    proposer_shuffling_root_consistency_test(spec, 32, 47).await;
 }
 
 #[tokio::test]
 async fn proposer_shuffling_root_consistency_two_epochs() {
-    proposer_shuffling_root_consistency_test(32, 55).await;
+    let spec = test_spec::<E>();
+    proposer_shuffling_root_consistency_test(spec, 32, 55).await;
+}
+
+#[tokio::test]
+async fn proposer_shuffling_root_consistency_at_fork_boundary() {
+    let mut spec = ForkName::Electra.make_genesis_spec(E::default_spec());
+    spec.fulu_fork_epoch = Some(Epoch::new(4));
+
+    // Parent block in epoch prior to Fulu fork epoch, child block in Fulu fork epoch.
+    proposer_shuffling_root_consistency_test(
+        spec.clone(),
+        3 * E::slots_per_epoch(),
+        4 * E::slots_per_epoch(),
+    )
+    .await;
+
+    // Parent block and child block in Fulu fork epoch.
+    proposer_shuffling_root_consistency_test(
+        spec.clone(),
+        4 * E::slots_per_epoch(),
+        4 * E::slots_per_epoch() + 1,
+    )
+    .await;
+
+    // Parent block in Fulu fork epoch and child block in epoch after.
+    proposer_shuffling_root_consistency_test(
+        spec.clone(),
+        4 * E::slots_per_epoch(),
+        5 * E::slots_per_epoch(),
+    )
+    .await;
+
+    // Parent block in epoch prior and child block in epoch after.
+    proposer_shuffling_root_consistency_test(
+        spec,
+        3 * E::slots_per_epoch(),
+        5 * E::slots_per_epoch(),
+    )
+    .await;
 }
 
 #[tokio::test]

consensus/proto_array/src/proto_array_fork_choice.rs

@@ -172,7 +172,13 @@ impl Block {
     ) -> Hash256 {
         let block_epoch = self.current_epoch_shuffling_id.shuffling_epoch;
 
-        if !spec.fork_name_at_epoch(child_block_epoch).fulu_enabled() {
+        // For child blocks in the Fulu fork epoch itself, we want to use the old logic. There is no
+        // lookahead in the first Fulu epoch. So we check whether Fulu is enabled at
+        // `child_block_epoch - 1`, i.e. whether `child_block_epoch > fulu_fork_epoch`.
+        if !spec
+            .fork_name_at_epoch(child_block_epoch.saturating_sub(1_u64))
+            .fulu_enabled()
+        {
             // Prior to Fulu the proposer shuffling decision root for the current epoch is the same
             // as the attestation shuffling for the *next* epoch, i.e. it is determined at the start
             // of the current epoch.

consensus/types/src/chain_spec.rs

@@ -869,7 +869,13 @@ impl ChainSpec {
     ///
     /// 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() {
+        // At the Fulu fork epoch itself, the shuffling is computed "the old way" with no lookahead.
+        // Therefore for `epoch == fulu_fork_epoch` we must take the `else` branch. Checking if Fulu
+        // is enabled at `epoch - 1` accomplishes this neatly.
+        if self
+            .fork_name_at_epoch(epoch.saturating_sub(1_u64))
+            .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
@@ -2999,4 +3005,32 @@ mod yaml_tests {
             spec.min_epoch_data_availability_boundary(current_epoch)
         );
     }
+
+    #[test]
+    fn proposer_shuffling_decision_root_around_epoch_boundary() {
+        type E = MainnetEthSpec;
+        let fulu_fork_epoch = 5;
+        let spec = {
+            let mut spec = ForkName::Electra.make_genesis_spec(E::default_spec());
+            spec.fulu_fork_epoch = Some(Epoch::new(fulu_fork_epoch));
+            Arc::new(spec)
+        };
+
+        // For epochs prior to AND including the Fulu fork epoch, the decision slot is the end
+        // of the previous epoch (i.e. only 1 slot lookahead).
+        for epoch in (0..=fulu_fork_epoch).map(Epoch::new) {
+            assert_eq!(
+                spec.proposer_shuffling_decision_slot::<E>(epoch),
+                epoch.start_slot(E::slots_per_epoch()) - 1
+            );
+        }
+
+        // For epochs after Fulu, the decision slot is the end of the epoch two epochs prior.
+        for epoch in ((fulu_fork_epoch + 1)..(fulu_fork_epoch + 10)).map(Epoch::new) {
+            assert_eq!(
+                spec.proposer_shuffling_decision_slot::<E>(epoch),
+                (epoch - 1).start_slot(E::slots_per_epoch()) - 1
+            );
+        }
+    }
 }