Another check to prevent duplicate block imports

beacon_node/beacon_chain/src/beacon_chain.rs

@@ -3889,9 +3889,16 @@ impl<T: BeaconChainTypes> BeaconChain<T> {
                 .map_err(BeaconChainError::from)?;
         }
 
+        // Take an upgradable read lock on fork choice so we can check if this block has already
+        // been imported. We don't want to repeat work importing a block that is already imported.
+        let fork_choice_reader = self.canonical_head.fork_choice_upgradable_read_lock();
+        if fork_choice_reader.contains_block(&block_root) {
+            return Err(BlockError::DuplicateFullyImported(block_root));
+        }
+
         // Take an exclusive write-lock on fork choice. It's very important to prevent deadlocks by
         // avoiding taking other locks whilst holding this lock.
-        let mut fork_choice = self.canonical_head.fork_choice_write_lock();
+        let mut fork_choice = parking_lot::RwLockUpgradableReadGuard::upgrade(fork_choice_reader);
 
         // Do not import a block that doesn't descend from the finalized root.
         let signed_block =

beacon_node/beacon_chain/src/canonical_head.rs

@@ -48,7 +48,7 @@ use fork_choice::{
 };
 use itertools::process_results;
 use logging::crit;
-use parking_lot::{Mutex, RwLock, RwLockReadGuard, RwLockWriteGuard};
+use parking_lot::{Mutex, RwLock, RwLockReadGuard, RwLockUpgradableReadGuard, RwLockWriteGuard};
 use slot_clock::SlotClock;
 use state_processing::AllCaches;
 use std::sync::Arc;
@@ -79,6 +79,10 @@ impl<T> CanonicalHeadRwLock<T> {
         self.0.read()
     }
 
+    fn upgradable_read(&self) -> RwLockUpgradableReadGuard<'_, T> {
+        self.0.upgradable_read()
+    }
+
     fn write(&self) -> RwLockWriteGuard<'_, T> {
         self.0.write()
     }
@@ -389,6 +393,14 @@ impl<T: BeaconChainTypes> CanonicalHead<T> {
         self.fork_choice.read()
     }
 
+    /// Access an upgradable read-lock for fork choice.
+    pub fn fork_choice_upgradable_read_lock(
+        &self,
+    ) -> RwLockUpgradableReadGuard<'_, BeaconForkChoice<T>> {
+        let _timer = metrics::start_timer(&metrics::FORK_CHOICE_UPGRADABLE_READ_LOCK_AQUIRE_TIMES);
+        self.fork_choice.upgradable_read()
+    }
+
     /// Access a write-lock for fork choice.
     pub fn fork_choice_write_lock(&self) -> RwLockWriteGuard<'_, BeaconForkChoice<T>> {
         let _timer = metrics::start_timer(&metrics::FORK_CHOICE_WRITE_LOCK_AQUIRE_TIMES);

beacon_node/beacon_chain/src/metrics.rs

@@ -578,6 +578,14 @@ pub static FORK_CHOICE_READ_LOCK_AQUIRE_TIMES: LazyLock<Result<Histogram>> = Laz
         exponential_buckets(1e-4, 4.0, 7),
     )
 });
+pub static FORK_CHOICE_UPGRADABLE_READ_LOCK_AQUIRE_TIMES: LazyLock<Result<Histogram>> =
+    LazyLock::new(|| {
+        try_create_histogram_with_buckets(
+            "beacon_fork_choice_upgradable_read_lock_aquire_seconds",
+            "Time taken to aquire the fork-choice upgradable read lock",
+            exponential_buckets(1e-4, 4.0, 7),
+        )
+    });
 pub static FORK_CHOICE_WRITE_LOCK_AQUIRE_TIMES: LazyLock<Result<Histogram>> = LazyLock::new(|| {
     try_create_histogram_with_buckets(
         "beacon_fork_choice_write_lock_aquire_seconds",

beacon_node/beacon_chain/tests/block_verification.rs

@@ -1730,6 +1730,8 @@ async fn add_altair_block_to_base_chain() {
     ));
 }
 
+// This is a regression test for this bug:
+// https://github.com/sigp/lighthouse/issues/4332#issuecomment-1565092279
 #[tokio::test]
 async fn import_duplicate_block_unrealized_justification() {
     let spec = MainnetEthSpec::default_spec();
@@ -1791,7 +1793,7 @@ async fn import_duplicate_block_unrealized_justification() {
         .await
         .unwrap();
 
-    // Unrealized justification should NOT have updated.
+    // The store's global unrealized justification should update immediately and match the block.
     let unrealized_justification = {
         let fc = chain.canonical_head.fork_choice_read_lock();
         assert_eq!(fc.justified_checkpoint().epoch, 0);
@@ -1808,9 +1810,12 @@ async fn import_duplicate_block_unrealized_justification() {
     };
 
     // Import the second verified block, simulating a block processed via RPC.
-    import_execution_pending_block(chain.clone(), verified_block2)
-        .await
-        .unwrap();
+    assert_eq!(
+        import_execution_pending_block(chain.clone(), verified_block2)
+            .await
+            .unwrap_err(),
+        format!("DuplicateFullyImported({block_root})")
+    );
 
     // Unrealized justification should still be updated.
     let fc3 = chain.canonical_head.fork_choice_read_lock();