From 245e922c7b4c48a3573859f06439c41ae52a579b Mon Sep 17 00:00:00 2001
From: Jimmy Chen <jchen.tc@gmail.com>
Date: Thu, 16 Feb 2023 23:34:32 +0000
Subject: [PATCH] Improve testing slot clock to allow manipulation of time in
 tests (#3974)

## Issue Addressed

I discovered this issue while implementing [this test](https://github.com/jimmygchen/lighthouse/blob/test-example/beacon_node/network/src/beacon_processor/tests.rs#L895), where I tried to manipulate the slot clock with:

`rig.chain.slot_clock.set_current_time(duration);`

however the change doesn't get reflected in the `slot_clock` in `ReprocessQueue`, and I realised `slot_clock` was cloned a few times in the code, and therefore changing the time in `rig.chain.slot_clock` doesn't have any effect in `ReprocessQueue`.

I've incorporated the suggestion from the @paulhauner and @michaelsproul - wrapping the `ManualSlotClock.current_time` (`RwLock<Duration>)` in an `Arc`, and the above test now passes.

Let's see if this breaks any existing tests :)
---
 common/slot_clock/src/manual_slot_clock.rs |  7 +++--
 lighthouse/tests/beacon_node.rs            | 34 ++++++++++++----------
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/common/slot_clock/src/manual_slot_clock.rs b/common/slot_clock/src/manual_slot_clock.rs
index 296247fe93..61299f74ac 100644
--- a/common/slot_clock/src/manual_slot_clock.rs
+++ b/common/slot_clock/src/manual_slot_clock.rs
@@ -1,6 +1,7 @@
 use super::SlotClock;
 use parking_lot::RwLock;
 use std::convert::TryInto;
+use std::sync::Arc;
 use std::time::Duration;
 use types::Slot;
 
@@ -10,7 +11,7 @@ pub struct ManualSlotClock {
     /// Duration from UNIX epoch to genesis.
     genesis_duration: Duration,
     /// Duration from UNIX epoch to right now.
-    current_time: RwLock<Duration>,
+    current_time: Arc<RwLock<Duration>>,
     /// The length of each slot.
     slot_duration: Duration,
 }
@@ -20,7 +21,7 @@ impl Clone for ManualSlotClock {
         ManualSlotClock {
             genesis_slot: self.genesis_slot,
             genesis_duration: self.genesis_duration,
-            current_time: RwLock::new(*self.current_time.read()),
+            current_time: Arc::clone(&self.current_time),
             slot_duration: self.slot_duration,
         }
     }
@@ -90,7 +91,7 @@ impl SlotClock for ManualSlotClock {
 
         Self {
             genesis_slot,
-            current_time: RwLock::new(genesis_duration),
+            current_time: Arc::new(RwLock::new(genesis_duration)),
             genesis_duration,
             slot_duration,
         }
diff --git a/lighthouse/tests/beacon_node.rs b/lighthouse/tests/beacon_node.rs
index 053a04f879..a07502c58a 100644
--- a/lighthouse/tests/beacon_node.rs
+++ b/lighthouse/tests/beacon_node.rs
@@ -1418,7 +1418,7 @@ fn slasher_slot_offset_flag() {
     CommandLineTest::new()
         .flag("slasher", None)
         .flag("slasher-slot-offset", Some("11.25"))
-        .run()
+        .run_with_zero_port()
         .with_config(|config| {
             let slasher_config = config.slasher.as_ref().unwrap();
             assert_eq!(slasher_config.slot_offset, 11.25);
@@ -1430,7 +1430,7 @@ fn slasher_slot_offset_nan_flag() {
     CommandLineTest::new()
         .flag("slasher", None)
         .flag("slasher-slot-offset", Some("NaN"))
-        .run();
+        .run_with_zero_port();
 }
 #[test]
 fn slasher_history_length_flag() {
@@ -1465,7 +1465,7 @@ fn slasher_attestation_cache_size_flag() {
     CommandLineTest::new()
         .flag("slasher", None)
         .flag("slasher-att-cache-size", Some("10000"))
-        .run()
+        .run_with_zero_port()
         .with_config(|config| {
             let slasher_config = config
                 .slasher
@@ -1569,23 +1569,25 @@ fn ensure_panic_on_failed_launch() {
 
 #[test]
 fn enable_proposer_re_orgs_default() {
-    CommandLineTest::new().run().with_config(|config| {
-        assert_eq!(
-            config.chain.re_org_threshold,
-            Some(DEFAULT_RE_ORG_THRESHOLD)
-        );
-        assert_eq!(
-            config.chain.re_org_max_epochs_since_finalization,
-            DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION,
-        );
-    });
+    CommandLineTest::new()
+        .run_with_zero_port()
+        .with_config(|config| {
+            assert_eq!(
+                config.chain.re_org_threshold,
+                Some(DEFAULT_RE_ORG_THRESHOLD)
+            );
+            assert_eq!(
+                config.chain.re_org_max_epochs_since_finalization,
+                DEFAULT_RE_ORG_MAX_EPOCHS_SINCE_FINALIZATION,
+            );
+        });
 }
 
 #[test]
 fn disable_proposer_re_orgs() {
     CommandLineTest::new()
         .flag("disable-proposer-reorgs", None)
-        .run()
+        .run_with_zero_port()
         .with_config(|config| assert_eq!(config.chain.re_org_threshold, None));
 }
 
@@ -1593,7 +1595,7 @@ fn disable_proposer_re_orgs() {
 fn proposer_re_org_threshold() {
     CommandLineTest::new()
         .flag("proposer-reorg-threshold", Some("90"))
-        .run()
+        .run_with_zero_port()
         .with_config(|config| assert_eq!(config.chain.re_org_threshold.unwrap().0, 90));
 }
 
@@ -1601,7 +1603,7 @@ fn proposer_re_org_threshold() {
 fn proposer_re_org_max_epochs_since_finalization() {
     CommandLineTest::new()
         .flag("proposer-reorg-epochs-since-finalization", Some("8"))
-        .run()
+        .run_with_zero_port()
         .with_config(|config| {
             assert_eq!(
                 config.chain.re_org_max_epochs_since_finalization.as_u64(),