diff --git a/beacon_node/beacon_chain/src/blob_verification.rs b/beacon_node/beacon_chain/src/blob_verification.rs index 24524bddfa..1e561cf34b 100644 --- a/beacon_node/beacon_chain/src/blob_verification.rs +++ b/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( }); } - // 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( 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( } // 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( diff --git a/beacon_node/beacon_chain/src/observed_blob_sidecars.rs b/beacon_node/beacon_chain/src/observed_blob_sidecars.rs index 1577d5951c..3ff1789049 100644 --- a/beacon_node/beacon_chain/src/observed_blob_sidecars.rs +++ b/beacon_node/beacon_chain/src/observed_blob_sidecars.rs @@ -97,3 +97,293 @@ impl ObservedBlobSidecars { 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> { + 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" + ); + } +}