Log warning on startup if chain spec contains misaligned fork epochs (#5859)

* Log a warning if any fork epoch isn't a multiple of 256.

* Merge branch 'unstable' into warn-invalid-fork-epochs

* Clean up.

beacon_node/src/lib.rs

@@ -17,7 +17,7 @@ use slasher::{DatabaseBackendOverride, Slasher};
 use slog::{info, warn};
 use std::ops::{Deref, DerefMut};
 use std::sync::Arc;
-use types::EthSpec;
+use types::{ChainSpec, Epoch, EthSpec, ForkName};
 
 /// A type-alias to the tighten the definition of a production-intended `Client`.
 pub type ProductionClient<E> =
@@ -78,6 +78,16 @@ impl<E: EthSpec> ProductionBeaconNode<E> {
             TimeoutRwLock::disable_timeouts()
         }
 
+        if let Err(misaligned_forks) = validator_fork_epochs(&spec) {
+            warn!(
+                log,
+                "Fork boundaries are not well aligned / multiples of 256";
+                "info" => "This may cause issues as fork boundaries do not align with the \
+                    start of sync committee period.",
+                "misaligned_forks" => ?misaligned_forks,
+            );
+        }
+
         let builder = ClientBuilder::new(context.eth_spec_instance.clone())
             .runtime_context(context)
             .chain_spec(spec.clone())
@@ -183,6 +193,28 @@ impl<E: EthSpec> ProductionBeaconNode<E> {
     }
 }
 
+fn validator_fork_epochs(spec: &ChainSpec) -> Result<(), Vec<(ForkName, Epoch)>> {
+    // @dapplion: "We try to schedule forks such that the fork epoch is a multiple of 256, to keep
+    // historical vectors in the same fork. Indirectly that makes light client periods align with
+    // fork boundaries."
+    let sync_committee_period = spec.epochs_per_sync_committee_period; // 256
+    let is_fork_boundary_misaligned = |epoch: Epoch| epoch % sync_committee_period != 0;
+
+    let forks_with_misaligned_epochs = ForkName::list_all_fork_epochs(spec)
+        .iter()
+        .filter_map(|(fork, fork_epoch_opt)| {
+            fork_epoch_opt
+                .and_then(|epoch| is_fork_boundary_misaligned(epoch).then_some((*fork, epoch)))
+        })
+        .collect::<Vec<_>>();
+
+    if forks_with_misaligned_epochs.is_empty() {
+        Ok(())
+    } else {
+        Err(forks_with_misaligned_epochs)
+    }
+}
+
 impl<E: EthSpec> Deref for ProductionBeaconNode<E> {
     type Target = ProductionClient<E>;
 
@@ -206,3 +238,23 @@ impl lighthouse_network::discv5::Executor for Discv5Executor {
         self.0.spawn(future, "discv5")
     }
 }
+
+#[cfg(test)]
+mod test {
+    use super::*;
+    use types::MainnetEthSpec;
+
+    #[test]
+    fn test_validator_fork_epoch_alignments() {
+        let mut spec = MainnetEthSpec::default_spec();
+        spec.altair_fork_epoch = Some(Epoch::new(0));
+        spec.bellatrix_fork_epoch = Some(Epoch::new(256));
+        spec.deneb_fork_epoch = Some(Epoch::new(257));
+        spec.electra_fork_epoch = None;
+        let result = validator_fork_epochs(&spec);
+        assert_eq!(
+            result,
+            Err(vec![(ForkName::Deneb, spec.deneb_fork_epoch.unwrap())])
+        );
+    }
+}

consensus/types/src/fork_name.rs

@@ -31,6 +31,16 @@ impl ForkName {
         ]
     }
 
+    pub fn list_all_fork_epochs(spec: &ChainSpec) -> Vec<(ForkName, Option<Epoch>)> {
+        vec![
+            (ForkName::Altair, spec.altair_fork_epoch),
+            (ForkName::Bellatrix, spec.bellatrix_fork_epoch),
+            (ForkName::Capella, spec.capella_fork_epoch),
+            (ForkName::Deneb, spec.deneb_fork_epoch),
+            (ForkName::Electra, spec.electra_fork_epoch),
+        ]
+    }
+
     pub fn latest() -> ForkName {
         // This unwrap is safe as long as we have 1+ forks. It is tested below.
         *ForkName::list_all().last().unwrap()