Fix attestation shuffling filter (#3629)

## Issue Addressed

Fix a bug in block production that results in blocks with 0 attestations during the first slot of an epoch.

The bug is marked by debug logs of the form:

> DEBG Discarding attestation because of missing ancestor, block_root: 0x3cc00d9c9e0883b2d0db8606278f2b8423d4902f9a1ee619258b5b60590e64f8, pivot_slot: 4042591

It occurs when trying to look up the shuffling decision root for an attestation from a slot which is prior to fork choice's finalized block. This happens frequently when proposing in the first slot of the epoch where we have:

- `current_epoch == n`
- `attestation.data.target.epoch == n - 1`
- attestation shuffling epoch `== n - 3` (decision block being the last block of `n - 3`)
- `state.finalized_checkpoint.epoch == n - 2` (first block of `n - 2` is finalized)

Hence the shuffling decision slot is out of range of the fork choice backwards iterator _by a single slot_.

Unfortunately this bug was hidden when we weren't pruning fork choice, and then reintroduced in v2.5.1 when we fixed the pruning (https://github.com/sigp/lighthouse/releases/tag/v2.5.1). There's no way to turn that off or disable the filtering in our current release, so we need a new release to fix this issue.

Fortunately, it also does not occur on every epoch boundary because of the gradual pruning of fork choice every 256 blocks (~8 epochs):

01e84b71f5/consensus/proto_array/src/proto_array_fork_choice.rs (L16)

01e84b71f5/consensus/proto_array/src/proto_array.rs (L713-L716)

So the probability of proposing a 0-attestation block given a proposal assignment is approximately `1/32 * 1/8 = 0.39%`.

## Proposed Changes

- Load the block's shuffling ID from fork choice and verify it against the expected shuffling ID of the head state. This code was initially written before we had settled on a representation of shuffling IDs, so I think it's a nice simplification to make use of them here rather than more ad-hoc logic that fundamentally does the same thing.

## Additional Info

Thanks to @moshe-blox for noticing this issue and bringing it to our attention.

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -1991,60 +1991,75 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
         target_epoch: Epoch,
         state: &BeaconState<T::EthSpec>,
     ) -> bool {
-        let slots_per_epoch = T::EthSpec::slots_per_epoch();
+        self.shuffling_is_compatible_result(block_root, target_epoch, state)
-        let shuffling_lookahead = 1 + self.spec.min_seed_lookahead.as_u64();
+            .unwrap_or_else(|e| {
-
-        // Shuffling can't have changed if we're in the first few epochs
-        if state.current_epoch() < shuffling_lookahead {
-            return true;
-        }
-
-        // Otherwise the shuffling is determined by the block at the end of the target epoch
-        // minus the shuffling lookahead (usually 2). We call this the "pivot".
-        let pivot_slot =
-            if target_epoch == state.previous_epoch() || target_epoch == state.current_epoch() {
-                (target_epoch - shuffling_lookahead).end_slot(slots_per_epoch)
-            } else {
-                return false;
-            };
-
-        let state_pivot_block_root = match state.get_block_root(pivot_slot) {
-            Ok(root) => *root,
-            Err(e) => {
-                warn!(
-                    &self.log,
-                    "Missing pivot block root for attestation";
-                    "slot" => pivot_slot,
-                    "error" => ?e,
-                );
-                return false;
-            }
-        };
-
-        // Use fork choice's view of the block DAG to quickly evaluate whether the attestation's
-        // pivot block is the same as the current state's pivot block. If it is, then the
-        // attestation's shuffling is the same as the current state's.
-        // To account for skipped slots, find the first block at *or before* the pivot slot.
-        let fork_choice_lock = self.canonical_head.fork_choice_read_lock();
-        let pivot_block_root = fork_choice_lock
-            .proto_array()
-            .core_proto_array()
-            .iter_block_roots(block_root)
-            .find(|(_, slot)| *slot <= pivot_slot)
-            .map(|(block_root, _)| block_root);
-        drop(fork_choice_lock);
-
-        match pivot_block_root {
-            Some(root) => root == state_pivot_block_root,
-            None => {
                 debug!(
-                    &self.log,
+                    self.log,
-                    "Discarding attestation because of missing ancestor";
+                    "Skipping attestation with incompatible shuffling";
-                    "pivot_slot" => pivot_slot.as_u64(),
                     "block_root" => ?block_root,
+                    "target_epoch" => target_epoch,
+                    "reason" => ?e,
                 );
                 false
+            })
+    }
+
+    fn shuffling_is_compatible_result(
+        &self,
+        block_root: &Hash256,
+        target_epoch: Epoch,
+        state: &BeaconState<T::EthSpec>,
+    ) -> Result<bool, Error> {
+        // Compute the shuffling ID for the head state in the `target_epoch`.
+        let relative_epoch = RelativeEpoch::from_epoch(state.current_epoch(), target_epoch)
+            .map_err(|e| Error::BeaconStateError(e.into()))?;
+        let head_shuffling_id =
+            AttestationShufflingId::new(self.genesis_block_root, state, relative_epoch)?;
+
+        // Load the block's shuffling ID from fork choice. We use the variant of `get_block` that
+        // checks descent from the finalized block, so there's one case where we'll spuriously
+        // return `false`: where an attestation for the previous epoch nominates the pivot block
+        // which is the parent block of the finalized block. Such attestations are not useful, so
+        // this doesn't matter.
+        let fork_choice_lock = self.canonical_head.fork_choice_read_lock();
+        let block = fork_choice_lock
+            .get_block(block_root)
+            .ok_or(Error::AttestationHeadNotInForkChoice(*block_root))?;
+        drop(fork_choice_lock);
+
+        let block_shuffling_id = if target_epoch == block.current_epoch_shuffling_id.shuffling_epoch
+        {
+            block.current_epoch_shuffling_id
+        } else if target_epoch == block.next_epoch_shuffling_id.shuffling_epoch {
+            block.next_epoch_shuffling_id
+        } else if target_epoch > block.next_epoch_shuffling_id.shuffling_epoch {
+            AttestationShufflingId {
+                shuffling_epoch: target_epoch,
+                shuffling_decision_block: *block_root,
             }
+        } else {
+            debug!(
+                self.log,
+                "Skipping attestation with incompatible shuffling";
+                "block_root" => ?block_root,
+                "target_epoch" => target_epoch,
+                "reason" => "target epoch less than block epoch"
+            );
+            return Ok(false);
+        };
+
+        if head_shuffling_id == block_shuffling_id {
+            Ok(true)
+        } else {
+            debug!(
+                self.log,
+                "Skipping attestation with incompatible shuffling";
+                "block_root" => ?block_root,
+                "target_epoch" => target_epoch,
+                "head_shuffling_id" => ?head_shuffling_id,
+                "block_shuffling_id" => ?block_shuffling_id,
+            );
+            Ok(false)
         }
     }
 
@@ -4460,7 +4475,7 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
     ///
     /// If the committee for `(head_block_root, shuffling_epoch)` isn't found in the
     /// `shuffling_cache`, we will read a state from disk and then update the `shuffling_cache`.
-    pub(crate) fn with_committee_cache<F, R>(
+    pub fn with_committee_cache<F, R>(
         &self,
         head_block_root: Hash256,
         shuffling_epoch: Epoch,

beacon_node/beacon_chain/tests/store_tests.rs

@@ -811,7 +811,6 @@ async fn shuffling_compatible_linear_chain() {
     let store = get_store(&db_path);
     let harness = get_harness(store.clone(), LOW_VALIDATOR_COUNT);
 
-    // Skip the block at the end of the first epoch.
     let head_block_root = harness
         .extend_chain(
             4 * E::slots_per_epoch() as usize,
@@ -824,10 +823,6 @@ async fn shuffling_compatible_linear_chain() {
         &harness,
         &get_state_for_block(&harness, head_block_root),
         head_block_root,
-        true,
-        true,
-        None,
-        None,
     );
 }
 
@@ -859,10 +854,6 @@ async fn shuffling_compatible_missing_pivot_block() {
         &harness,
         &get_state_for_block(&harness, head_block_root),
         head_block_root,
-        true,
-        true,
-        Some(E::slots_per_epoch() - 2),
-        Some(E::slots_per_epoch() - 2),
     );
 }
 
@@ -880,10 +871,10 @@ async fn shuffling_compatible_simple_fork() {
     let head1_state = get_state_for_block(&harness, head1);
     let head2_state = get_state_for_block(&harness, head2);
 
-    check_shuffling_compatible(&harness, &head1_state, head1, true, true, None, None);
+    check_shuffling_compatible(&harness, &head1_state, head1);
-    check_shuffling_compatible(&harness, &head1_state, head2, false, false, None, None);
+    check_shuffling_compatible(&harness, &head1_state, head2);
-    check_shuffling_compatible(&harness, &head2_state, head1, false, false, None, None);
+    check_shuffling_compatible(&harness, &head2_state, head1);
-    check_shuffling_compatible(&harness, &head2_state, head2, true, true, None, None);
+    check_shuffling_compatible(&harness, &head2_state, head2);
 
     drop(db_path);
 }
@@ -902,21 +893,10 @@ async fn shuffling_compatible_short_fork() {
     let head1_state = get_state_for_block(&harness, head1);
     let head2_state = get_state_for_block(&harness, head2);
 
-    check_shuffling_compatible(&harness, &head1_state, head1, true, true, None, None);
+    check_shuffling_compatible(&harness, &head1_state, head1);
-    check_shuffling_compatible(&harness, &head1_state, head2, false, true, None, None);
+    check_shuffling_compatible(&harness, &head1_state, head2);
-    // NOTE: don't check this case, as block 14 from the first chain appears valid on the second
+    check_shuffling_compatible(&harness, &head2_state, head1);
-    // chain due to it matching the second chain's block 15.
+    check_shuffling_compatible(&harness, &head2_state, head2);
-    // check_shuffling_compatible(&harness, &head2_state, head1, false, true, None, None);
-    check_shuffling_compatible(
-        &harness,
-        &head2_state,
-        head2,
-        true,
-        true,
-        // Required because of the skipped slot.
-        Some(2 * E::slots_per_epoch() - 2),
-        None,
-    );
 
     drop(db_path);
 }
@@ -940,54 +920,82 @@ fn check_shuffling_compatible(
     harness: &TestHarness,
     head_state: &BeaconState<E>,
     head_block_root: Hash256,
-    current_epoch_valid: bool,
-    previous_epoch_valid: bool,
-    current_epoch_cutoff_slot: Option<u64>,
-    previous_epoch_cutoff_slot: Option<u64>,
 ) {
-    let shuffling_lookahead = harness.chain.spec.min_seed_lookahead.as_u64() + 1;
-    let current_pivot_slot =
-        (head_state.current_epoch() - shuffling_lookahead).end_slot(E::slots_per_epoch());
-    let previous_pivot_slot =
-        (head_state.previous_epoch() - shuffling_lookahead).end_slot(E::slots_per_epoch());
-
     for maybe_tuple in harness
         .chain
         .rev_iter_block_roots_from(head_block_root)
         .unwrap()
     {
         let (block_root, slot) = maybe_tuple.unwrap();
-        // Shuffling is compatible targeting the current epoch,
+
-        // if slot is greater than or equal to the current epoch pivot block.
+        // Would an attestation to `block_root` at the current epoch be compatible with the head
-        assert_eq!(
+        // state's shuffling?
-            harness.chain.shuffling_is_compatible(
+        let current_epoch_shuffling_is_compatible = harness.chain.shuffling_is_compatible(
-                &block_root,
+            &block_root,
+            head_state.current_epoch(),
+            &head_state,
+        );
+
+        // Check for consistency with the more expensive shuffling lookup.
+        harness
+            .chain
+            .with_committee_cache(
+                block_root,
                 head_state.current_epoch(),
-                &head_state
+                |committee_cache, _| {
-            ),
+                    let state_cache = head_state.committee_cache(RelativeEpoch::Current).unwrap();
-            current_epoch_valid
+                    if current_epoch_shuffling_is_compatible {
-                && slot >= current_epoch_cutoff_slot.unwrap_or(current_pivot_slot.as_u64())
+                        assert_eq!(committee_cache, state_cache, "block at slot {slot}");
-        );
+                    } else {
+                        assert_ne!(committee_cache, state_cache, "block at slot {slot}");
+                    }
+                    Ok(())
+                },
+            )
+            .unwrap_or_else(|e| {
+                // If the lookup fails then the shuffling must be invalid in some way, e.g. the
+                // block with `block_root` is from a later epoch than `previous_epoch`.
+                assert!(
+                    !current_epoch_shuffling_is_compatible,
+                    "block at slot {slot} has compatible shuffling at epoch {} \
+                     but should be incompatible due to error: {e:?}",
+                    head_state.current_epoch()
+                );
+            });
+
         // Similarly for the previous epoch
-        assert_eq!(
+        let previous_epoch_shuffling_is_compatible = harness.chain.shuffling_is_compatible(
-            harness.chain.shuffling_is_compatible(
+            &block_root,
-                &block_root,
+            head_state.previous_epoch(),
+            &head_state,
+        );
+        harness
+            .chain
+            .with_committee_cache(
+                block_root,
                 head_state.previous_epoch(),
-                &head_state
+                |committee_cache, _| {
-            ),
+                    let state_cache = head_state.committee_cache(RelativeEpoch::Previous).unwrap();
-            previous_epoch_valid
+                    if previous_epoch_shuffling_is_compatible {
-                && slot >= previous_epoch_cutoff_slot.unwrap_or(previous_pivot_slot.as_u64())
+                        assert_eq!(committee_cache, state_cache);
-        );
+                    } else {
-        // Targeting the next epoch should always return false
+                        assert_ne!(committee_cache, state_cache);
-        assert_eq!(
+                    }
-            harness.chain.shuffling_is_compatible(
+                    Ok(())
-                &block_root,
+                },
-                head_state.current_epoch() + 1,
+            )
-                &head_state
+            .unwrap_or_else(|e| {
-            ),
+                // If the lookup fails then the shuffling must be invalid in some way, e.g. the
-            false
+                // block with `block_root` is from a later epoch than `previous_epoch`.
-        );
+                assert!(
-        // Targeting two epochs before the current epoch should also always return false
+                    !previous_epoch_shuffling_is_compatible,
+                    "block at slot {slot} has compatible shuffling at epoch {} \
+                     but should be incompatible due to error: {e:?}",
+                    head_state.previous_epoch()
+                );
+            });
+
+        // Targeting two epochs before the current epoch should always return false
         if head_state.current_epoch() >= 2 {
             assert_eq!(
                 harness.chain.shuffling_is_compatible(