Gossip verification cleanup (#4219)

* Add ObservedBlobSidecar tests

* Add logging for tricky verification cases

* Update beacon_node/beacon_chain/src/blob_verification.rs

---------

Co-authored-by: realbigsean <seananderson33@GMAIL.com>

beacon_node/beacon_chain/src/blob_verification.rs

@@ -13,6 +13,7 @@ use crate::data_availability_checker::{
 use crate::kzg_utils::{validate_blob, validate_blobs};
 use crate::BeaconChainError;
 use kzg::Kzg;
+use slog::{debug, warn};
 use std::borrow::Cow;
 use types::{
     BeaconBlockRef, BeaconState, BeaconStateError, BlobSidecar, BlobSidecarList, ChainSpec,
@@ -214,10 +215,7 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
         });
     }
 
-    // Note: The spec checks the signature directly against `blob_sidecar.message.proposer_index`
-    // before checking that the provided proposer index is valid w.r.t the current shuffling.
-    //
-    // However, we check that the proposer_index matches against the shuffling first to avoid
+    // Note: We check that the proposer_index matches against the shuffling first to avoid
     // signature verification against an invalid proposer_index.
     let proposer_shuffling_root = signed_blob_sidecar.message.block_parent_root;
 
@@ -229,6 +227,12 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
     let (proposer_index, fork) = if let Some(proposer) = proposer_opt {
         (proposer.index, proposer.fork)
     } else {
+        debug!(
+            chain.log,
+            "Proposer shuffling cache miss for blob verification";
+            "block_root" => %block_root,
+            "index" => %blob_index,
+        );
         // The cached head state is in the same epoch as the blob or the state has already been
         // advanced to the blob's epoch
         let snapshot = &chain.canonical_head.cached_head().snapshot;
@@ -242,6 +246,18 @@ pub fn validate_blob_sidecar_for_gossip<T: BeaconChainTypes>(
         }
         // Need to advance the state to get the proposer index
         else {
+            // Reaching this condition too often might be an issue since we could theoretically have
+            // 5 threads (4 blob indices + 1 block) cloning the state.
+            // We shouldn't be seeing this condition a lot because we try to advance the state
+            // 3 seconds before the start of a slot. However, if this becomes an issue during testing, we should
+            // consider sending a blob for reprocessing to reduce the number of state clones.
+            warn!(
+                chain.log,
+                "Cached head not advanced for blob verification";
+                "block_root" => %block_root,
+                "index" => %blob_index,
+                "action" => "contact the devs if you see this msg too often"
+            );
             // The state produced is only valid for determining proposer/attester shuffling indices.
             let mut cloned_state = snapshot.clone_with(CloneConfig::committee_caches_only());
             let state = cheap_state_advance_to_obtain_committees(

beacon_node/beacon_chain/src/observed_blob_sidecars.rs

@@ -97,3 +97,293 @@ impl<T: EthSpec> ObservedBlobSidecars<T> {
         self.items.retain(|k, _| k.1 > finalized_slot);
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use types::{BlobSidecar, Hash256, MainnetEthSpec};
+
+    type E = MainnetEthSpec;
+
+    fn get_blob_sidecar(slot: u64, block_root: Hash256, index: u64) -> Arc<BlobSidecar<E>> {
+        let mut blob_sidecar = BlobSidecar::empty();
+        blob_sidecar.block_root = block_root;
+        blob_sidecar.slot = slot.into();
+        blob_sidecar.index = index;
+        Arc::new(blob_sidecar)
+    }
+
+    #[test]
+    fn pruning() {
+        let mut cache = ObservedBlobSidecars::default();
+
+        assert_eq!(cache.finalized_slot, 0, "finalized slot is zero");
+        assert_eq!(cache.items.len(), 0, "no slots should be present");
+
+        // Slot 0, index 0
+        let block_root_a = Hash256::random();
+        let sidecar_a = get_blob_sidecar(0, block_root_a, 0);
+
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_a),
+            Ok(false),
+            "can observe proposer, indicates proposer unobserved"
+        );
+
+        /*
+         * Preconditions.
+         */
+
+        assert_eq!(cache.finalized_slot, 0, "finalized slot is zero");
+        assert_eq!(
+            cache.items.len(),
+            1,
+            "only one (slot, root) tuple should be present"
+        );
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_a, Slot::new(0)))
+                .expect("slot zero should be present")
+                .len(),
+            1,
+            "only one item should be present"
+        );
+
+        /*
+         * Check that a prune at the genesis slot does nothing.
+         */
+
+        cache.prune(Slot::new(0));
+
+        assert_eq!(cache.finalized_slot, 0, "finalized slot is zero");
+        assert_eq!(cache.items.len(), 1, "only one slot should be present");
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_a, Slot::new(0)))
+                .expect("slot zero should be present")
+                .len(),
+            1,
+            "only one item should be present"
+        );
+
+        /*
+         * Check that a prune empties the cache
+         */
+
+        cache.prune(E::slots_per_epoch().into());
+        assert_eq!(
+            cache.finalized_slot,
+            Slot::from(E::slots_per_epoch()),
+            "finalized slot is updated"
+        );
+        assert_eq!(cache.items.len(), 0, "no items left");
+
+        /*
+         * Check that we can't insert a finalized sidecar
+         */
+
+        // First slot of finalized epoch
+        let block_b = get_blob_sidecar(E::slots_per_epoch(), Hash256::random(), 0);
+
+        assert_eq!(
+            cache.observe_sidecar(&block_b),
+            Err(Error::FinalizedBlob {
+                slot: E::slots_per_epoch().into(),
+                finalized_slot: E::slots_per_epoch().into(),
+            }),
+            "cant insert finalized sidecar"
+        );
+
+        assert_eq!(cache.items.len(), 0, "sidecar was not added");
+
+        /*
+         * Check that we _can_ insert a non-finalized block
+         */
+
+        let three_epochs = E::slots_per_epoch() * 3;
+
+        // First slot of finalized epoch
+        let block_root_b = Hash256::random();
+        let block_b = get_blob_sidecar(three_epochs, block_root_b, 0);
+
+        assert_eq!(
+            cache.observe_sidecar(&block_b),
+            Ok(false),
+            "can insert non-finalized block"
+        );
+
+        assert_eq!(cache.items.len(), 1, "only one slot should be present");
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_b, Slot::new(three_epochs)))
+                .expect("the three epochs slot should be present")
+                .len(),
+            1,
+            "only one proposer should be present"
+        );
+
+        /*
+         * Check that a prune doesnt wipe later blocks
+         */
+
+        let two_epochs = E::slots_per_epoch() * 2;
+        cache.prune(two_epochs.into());
+
+        assert_eq!(
+            cache.finalized_slot,
+            Slot::from(two_epochs),
+            "finalized slot is updated"
+        );
+
+        assert_eq!(cache.items.len(), 1, "only one slot should be present");
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_b, Slot::new(three_epochs)))
+                .expect("the three epochs slot should be present")
+                .len(),
+            1,
+            "only one proposer should be present"
+        );
+    }
+
+    #[test]
+    fn simple_observations() {
+        let mut cache = ObservedBlobSidecars::default();
+
+        // Slot 0, index 0
+        let block_root_a = Hash256::random();
+        let sidecar_a = get_blob_sidecar(0, block_root_a, 0);
+
+        assert_eq!(
+            cache.is_known(&sidecar_a),
+            Ok(false),
+            "no observation in empty cache"
+        );
+
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_a),
+            Ok(false),
+            "can observe proposer, indicates proposer unobserved"
+        );
+
+        assert_eq!(
+            cache.is_known(&sidecar_a),
+            Ok(true),
+            "observed block is indicated as true"
+        );
+
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_a),
+            Ok(true),
+            "observing again indicates true"
+        );
+
+        assert_eq!(cache.finalized_slot, 0, "finalized slot is zero");
+        assert_eq!(cache.items.len(), 1, "only one slot should be present");
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_a, Slot::new(0)))
+                .expect("slot zero should be present")
+                .len(),
+            1,
+            "only one proposer should be present"
+        );
+
+        // Slot 1, proposer 0
+
+        let block_root_b = Hash256::random();
+        let sidecar_b = get_blob_sidecar(1, block_root_b, 0);
+
+        assert_eq!(
+            cache.is_known(&sidecar_b),
+            Ok(false),
+            "no observation for new slot"
+        );
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_b),
+            Ok(false),
+            "can observe proposer for new slot, indicates proposer unobserved"
+        );
+        assert_eq!(
+            cache.is_known(&sidecar_b),
+            Ok(true),
+            "observed block in slot 1 is indicated as true"
+        );
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_b),
+            Ok(true),
+            "observing slot 1 again indicates true"
+        );
+
+        assert_eq!(cache.finalized_slot, 0, "finalized slot is zero");
+        assert_eq!(cache.items.len(), 2, "two slots should be present");
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_a, Slot::new(0)))
+                .expect("slot zero should be present")
+                .len(),
+            1,
+            "only one proposer should be present in slot 0"
+        );
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_b, Slot::new(1)))
+                .expect("slot zero should be present")
+                .len(),
+            1,
+            "only one proposer should be present in slot 1"
+        );
+
+        // Slot 0, index 1
+        let sidecar_c = get_blob_sidecar(0, block_root_a, 1);
+
+        assert_eq!(
+            cache.is_known(&sidecar_c),
+            Ok(false),
+            "no observation for new index"
+        );
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_c),
+            Ok(false),
+            "can observe new index, indicates sidecar unobserved for new index"
+        );
+        assert_eq!(
+            cache.is_known(&sidecar_c),
+            Ok(true),
+            "observed new sidecar is indicated as true"
+        );
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_c),
+            Ok(true),
+            "observing new sidecar again indicates true"
+        );
+
+        assert_eq!(cache.finalized_slot, 0, "finalized slot is zero");
+        assert_eq!(cache.items.len(), 2, "two slots should be present");
+        assert_eq!(
+            cache
+                .items
+                .get(&(block_root_a, Slot::new(0)))
+                .expect("slot zero should be present")
+                .len(),
+            2,
+            "two blob indices should be present in slot 0"
+        );
+
+        // Try adding an out of bounds index
+        let invalid_index = E::max_blobs_per_block() as u64;
+        let sidecar_d = get_blob_sidecar(0, block_root_a, 4);
+        assert_eq!(
+            cache.observe_sidecar(&sidecar_d),
+            Err(Error::InvalidBlobIndex(invalid_index)),
+            "cannot add an index > MaxBlobsPerBlock"
+        );
+    }
+}