From 738d326db1cdf0c796b8d8cee0e99c23c3aa32a1 Mon Sep 17 00:00:00 2001 From: Michael Sproul Date: Thu, 30 May 2024 14:44:17 +1000 Subject: [PATCH] Keep PendingComponents in da_checker during import_block (#5845) Squashed commit of the following: commit d75321b794ddf07a270e715f9299a53e4a6f5169 Merge: 7c125b848 7f8b600f2 Author: realbigsean Date: Mon May 27 19:52:41 2024 -0400 Merge branch 'unstable' of https://github.com/sigp/lighthouse into time_in_da_checker commit 7c125b848569714a1fc922a5edc908e93dfa70ce Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sat May 25 01:32:02 2024 +0200 Keep PendingComponents in da_checker during import_block commit 7136412062efef3a6c92de6bb08ef01855e0db76 Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Sat May 25 01:01:23 2024 +0200 Simplify BlockProcessStatus commit dbcd7d11c56d0c1b555ccf653bf04aa2fbb9d03d Author: dapplion <35266934+dapplion@users.noreply.github.com> Date: Fri May 24 20:00:05 2024 +0200 Ensure lookup sync checks caches correctly --- beacon_node/beacon_chain/src/beacon_chain.rs | 14 ++++++ .../src/data_availability_checker.rs | 5 +++ .../overflow_lru_cache.rs | 45 +++++++++++++++---- 3 files changed, 56 insertions(+), 8 deletions(-) diff --git a/beacon_node/beacon_chain/src/beacon_chain.rs b/beacon_node/beacon_chain/src/beacon_chain.rs index 6ddac706374..77e1bc095ed 100644 --- a/beacon_node/beacon_chain/src/beacon_chain.rs +++ b/beacon_node/beacon_chain/src/beacon_chain.rs @@ -3336,6 +3336,20 @@ impl BeaconChain { "payload_verification_handle", ) .await??; + + // Remove block components from da_checker AFTER completing block import. Then we can assert + // the following invariant: + // > A valid unfinalized block is either in fork-choice or da_checker. + // + // If we remove the block when it becomes available, there's some time window during + // `import_block` where the block is nowhere. Consumers of the da_checker can handle the + // extend time a block may exist in the da_checker. + // + // If `import_block` errors (only errors with internal errors), the pending components will + // be pruned on data_availability_checker maintenance as finality advances. + self.data_availability_checker + .remove_pending_components(block_root); + Ok(AvailabilityProcessingStatus::Imported(block_root)) } diff --git a/beacon_node/beacon_chain/src/data_availability_checker.rs b/beacon_node/beacon_chain/src/data_availability_checker.rs index 57f718f62d9..e0347d81c39 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker.rs @@ -164,6 +164,11 @@ impl DataAvailabilityChecker { .put_pending_executed_block(executed_block) } + pub fn remove_pending_components(&self, block_root: Hash256) { + self.availability_cache + .remove_pending_components(block_root) + } + /// Verifies kzg commitments for an RpcBlock, returns a `MaybeAvailableBlock` that may /// include the fully available block. /// diff --git a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs index e350181c867..f7aec523d75 100644 --- a/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs +++ b/beacon_node/beacon_chain/src/data_availability_checker/overflow_lru_cache.rs @@ -475,18 +475,18 @@ impl Critical { Ok(()) } - /// Removes and returns the pending_components corresponding to - /// the `block_root` or `None` if it does not exist - pub fn pop_pending_components( + /// Returns the pending_components corresponding to the `block_root` or `None` if it does not + /// exist + pub fn get_pending_components( &mut self, block_root: Hash256, store: &OverflowStore, ) -> Result>, AvailabilityCheckError> { - match self.in_memory.pop_entry(&block_root) { - Some((_, pending_components)) => Ok(Some(pending_components)), + match self.in_memory.get(&block_root) { + Some(pending_components) => Ok(Some(pending_components.clone())), None => { // not in memory, is it in the store? - if self.store_keys.remove(&block_root) { + if self.store_keys.contains(&block_root) { // We don't need to remove the data from the store as we have removed it from // `store_keys` so we won't go looking for it on disk. The maintenance thread // will remove it from disk the next time it runs. @@ -498,6 +498,21 @@ impl Critical { } } + /// Removes and returns the pending_components corresponding to + /// the `block_root` or `None` if it does not exist + pub fn remove_pending_components(&mut self, block_root: Hash256) { + match self.in_memory.pop_entry(&block_root) { + Some { .. } => {} + None => { + // not in memory, is it in the store? + // We don't need to remove the data from the store as we have removed it from + // `store_keys` so we won't go looking for it on disk. The maintenance thread + // will remove it from disk the next time it runs. + self.store_keys.remove(&block_root); + } + } + } + /// Returns the number of pending component entries in memory. pub fn num_blocks(&self) -> usize { self.in_memory.len() @@ -600,13 +615,18 @@ impl OverflowLRUCache { // Grab existing entry or create a new entry. let mut pending_components = write_lock - .pop_pending_components(block_root, &self.overflow_store)? + .get_pending_components(block_root, &self.overflow_store)? .unwrap_or_else(|| PendingComponents::empty(block_root)); // Merge in the blobs. pending_components.merge_blobs(fixed_blobs); if pending_components.is_available() { + write_lock.put_pending_components( + block_root, + pending_components.clone(), + &self.overflow_store, + )?; // No need to hold the write lock anymore drop(write_lock); pending_components.make_available(|diet_block| { @@ -638,7 +658,7 @@ impl OverflowLRUCache { // Grab existing entry or create a new entry. let mut pending_components = write_lock - .pop_pending_components(block_root, &self.overflow_store)? + .get_pending_components(block_root, &self.overflow_store)? .unwrap_or_else(|| PendingComponents::empty(block_root)); // Merge in the block. @@ -646,6 +666,11 @@ impl OverflowLRUCache { // Check if we have all components and entire set is consistent. if pending_components.is_available() { + write_lock.put_pending_components( + block_root, + pending_components.clone(), + &self.overflow_store, + )?; // No need to hold the write lock anymore drop(write_lock); pending_components.make_available(|diet_block| { @@ -661,6 +686,10 @@ impl OverflowLRUCache { } } + pub fn remove_pending_components(&self, block_root: Hash256) { + self.critical.write().remove_pending_components(block_root); + } + /// write all in memory objects to disk pub fn write_all_to_disk(&self) -> Result<(), AvailabilityCheckError> { let maintenance_lock = self.maintenance_lock.lock();