Check ChainSpec consistency with upstream config.yaml (#9008)

Closes:

- https://github.com/sigp/lighthouse/issues/9002


  - Commit `config.yaml` for minimal and mainnet to `consensus/types/configs`. For now we omit any auto-downloading logic, to avoid the hassles of dealing with Github rate limits etc on CI. Unfortunately these files are NOT bundled inside the spec tests.
- Fix the values of `min_builder_withdrawability_delay` for minimal and mainnet. These discrepancies aren't caught by the current spec tests, because the spec tests are missing data: https://github.com/ethereum/consensus-specs/pull/5005. Will be fixed in the next release/when we update to nightly.
- Fix the blob schedule for `minimal`, which should be empty, NOT inherited from mainnet.
- Keep `SECONDS_PER_SLOT` for now because the Kurtosis tests fail upon their complete removal. We will be able to completely remove `SECONDS_PER_SLOT` soon.


Co-Authored-By: Michael Sproul <michael@sigmaprime.io>

consensus/types/src/core/chain_spec.rs

@@ -96,8 +96,7 @@ pub struct ChainSpec {
      * Time parameters
      */
     pub genesis_delay: u64,
-    // TODO deprecate seconds_per_slot
-    pub seconds_per_slot: u64,
+    seconds_per_slot: u64,
     // Private so that this value can't get changed except via the `set_slot_duration_ms` function.
     slot_duration_ms: u64,
     pub min_attestation_inclusion_delay: u64,
@@ -914,6 +913,7 @@ impl ChainSpec {
     /// Set the duration of a slot (in ms).
     pub fn set_slot_duration_ms<E: EthSpec>(mut self, slot_duration_ms: u64) -> Self {
         self.slot_duration_ms = slot_duration_ms;
+        self.seconds_per_slot = slot_duration_ms.saturating_div(1000);
         self.compute_derived_values::<E>()
     }
 
@@ -1235,7 +1235,7 @@ impl ChainSpec {
             gloas_fork_epoch: None,
             builder_payment_threshold_numerator: 6,
             builder_payment_threshold_denominator: 10,
-            min_builder_withdrawability_delay: Epoch::new(4096),
+            min_builder_withdrawability_delay: Epoch::new(64),
             max_request_payloads: 128,
 
             /*
@@ -1381,6 +1381,7 @@ impl ChainSpec {
             // Gloas
             gloas_fork_version: [0x07, 0x00, 0x00, 0x01],
             gloas_fork_epoch: None,
+            min_builder_withdrawability_delay: Epoch::new(2),
 
             /*
              * Derived time values (set by `compute_derived_values()`)
@@ -1391,6 +1392,9 @@ impl ChainSpec {
             sync_message_due: Duration::from_millis(1999),
             contribution_and_proof_due: Duration::from_millis(4000),
 
+            // Networking Fulu
+            blob_schedule: BlobSchedule::default(),
+
             // Other
             network_id: 2, // lighthouse testnet network id
             deposit_chain_id: 5,
@@ -1631,7 +1635,7 @@ impl ChainSpec {
             gloas_fork_epoch: None,
             builder_payment_threshold_numerator: 6,
             builder_payment_threshold_denominator: 10,
-            min_builder_withdrawability_delay: Epoch::new(4096),
+            min_builder_withdrawability_delay: Epoch::new(64),
             max_request_payloads: 128,
 
             /*
@@ -1908,8 +1912,9 @@ pub struct Config {
     #[serde(deserialize_with = "deserialize_fork_epoch")]
     pub gloas_fork_epoch: Option<MaybeQuoted<Epoch>>,
 
-    #[serde(with = "serde_utils::quoted_u64")]
-    seconds_per_slot: u64,
+    #[serde(default)]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    seconds_per_slot: Option<MaybeQuoted<u64>>,
     #[serde(default)]
     #[serde(skip_serializing_if = "Option::is_none")]
     slot_duration_ms: Option<MaybeQuoted<u64>>,
@@ -2064,6 +2069,10 @@ pub struct Config {
     #[serde(default = "default_contribution_due_bps")]
     #[serde(with = "serde_utils::quoted_u64")]
     contribution_due_bps: u64,
+
+    #[serde(default = "default_min_builder_withdrawability_delay")]
+    #[serde(with = "serde_utils::quoted_u64")]
+    min_builder_withdrawability_delay: u64,
 }
 
 fn default_bellatrix_fork_version() -> [u8; 4] {
@@ -2289,6 +2298,10 @@ const fn default_contribution_due_bps() -> u64 {
     6667
 }
 
+const fn default_min_builder_withdrawability_delay() -> u64 {
+    64
+}
+
 fn max_blocks_by_root_request_common(max_request_blocks: u64) -> usize {
     let max_request_blocks = max_request_blocks as usize;
     RuntimeVariableList::<Hash256>::new(
@@ -2459,7 +2472,9 @@ impl Config {
                 .gloas_fork_epoch
                 .map(|epoch| MaybeQuoted { value: epoch }),
 
-            seconds_per_slot: spec.seconds_per_slot,
+            seconds_per_slot: Some(MaybeQuoted {
+                value: spec.seconds_per_slot,
+            }),
             slot_duration_ms: Some(MaybeQuoted {
                 value: spec.slot_duration_ms,
             }),
@@ -2525,6 +2540,8 @@ impl Config {
             aggregate_due_bps: spec.aggregate_due_bps,
             sync_message_due_bps: spec.sync_message_due_bps,
             contribution_due_bps: spec.contribution_due_bps,
+
+            min_builder_withdrawability_delay: spec.min_builder_withdrawability_delay.as_u64(),
         }
     }
 
@@ -2616,12 +2633,21 @@ impl Config {
             aggregate_due_bps,
             sync_message_due_bps,
             contribution_due_bps,
+            min_builder_withdrawability_delay,
         } = self;
 
         if preset_base != E::spec_name().to_string().as_str() {
             return None;
         }
 
+        // Fail if seconds_per_slot and slot_duration_ms are both set but are inconsistent.
+        if let (Some(seconds_per_slot), Some(slot_duration_ms)) =
+            (seconds_per_slot, slot_duration_ms)
+            && seconds_per_slot.value.saturating_mul(1000) != slot_duration_ms.value
+        {
+            return None;
+        }
+
         let spec = ChainSpec {
             config_name: config_name.clone(),
             min_genesis_active_validator_count,
@@ -2642,10 +2668,12 @@ impl Config {
             fulu_fork_version,
             gloas_fork_version,
             gloas_fork_epoch: gloas_fork_epoch.map(|q| q.value),
-            seconds_per_slot,
+            seconds_per_slot: seconds_per_slot
+                .map(|q| q.value)
+                .or_else(|| slot_duration_ms.and_then(|q| q.value.checked_div(1000)))?,
             slot_duration_ms: slot_duration_ms
                 .map(|q| q.value)
-                .unwrap_or_else(|| seconds_per_slot.saturating_mul(1000)),
+                .or_else(|| seconds_per_slot.map(|q| q.value.saturating_mul(1000)))?,
             seconds_per_eth1_block,
             min_validator_withdrawability_delay,
             shard_committee_period,
@@ -2705,6 +2733,8 @@ impl Config {
             sync_message_due_bps,
             contribution_due_bps,
 
+            min_builder_withdrawability_delay: Epoch::new(min_builder_withdrawability_delay),
+
             ..chain_spec.clone()
         };
         Some(spec.compute_derived_values::<E>())
@@ -2853,6 +2883,9 @@ mod yaml_tests {
     use super::*;
     use crate::core::MinimalEthSpec;
     use paste::paste;
+    use std::collections::BTreeSet;
+    use std::env;
+    use std::path::PathBuf;
     use std::sync::Arc;
     use tempfile::NamedTempFile;
 
@@ -2902,6 +2935,67 @@ mod yaml_tests {
         assert_eq!(from, yamlconfig);
     }
 
+    #[test]
+    fn slot_duration_fallback_both_fields() {
+        let mainnet = ChainSpec::mainnet();
+        let mut config = Config::from_chain_spec::<MainnetEthSpec>(&mainnet);
+        config.seconds_per_slot = Some(MaybeQuoted { value: 12 });
+        config.slot_duration_ms = Some(MaybeQuoted { value: 12000 });
+        let spec = config
+            .apply_to_chain_spec::<MainnetEthSpec>(&mainnet)
+            .unwrap();
+        assert_eq!(spec.seconds_per_slot, 12);
+        assert_eq!(spec.slot_duration_ms, 12000);
+    }
+
+    #[test]
+    fn slot_duration_fallback_both_fields_inconsistent() {
+        let mainnet = ChainSpec::mainnet();
+        let mut config = Config::from_chain_spec::<MainnetEthSpec>(&mainnet);
+        config.seconds_per_slot = Some(MaybeQuoted { value: 10 });
+        config.slot_duration_ms = Some(MaybeQuoted { value: 12000 });
+        assert_eq!(config.apply_to_chain_spec::<MainnetEthSpec>(&mainnet), None);
+    }
+
+    #[test]
+    fn slot_duration_fallback_seconds_only() {
+        let mainnet = ChainSpec::mainnet();
+        let mut config = Config::from_chain_spec::<MainnetEthSpec>(&mainnet);
+        config.seconds_per_slot = Some(MaybeQuoted { value: 12 });
+        config.slot_duration_ms = None;
+        let spec = config
+            .apply_to_chain_spec::<MainnetEthSpec>(&mainnet)
+            .unwrap();
+        assert_eq!(spec.seconds_per_slot, 12);
+        assert_eq!(spec.slot_duration_ms, 12000);
+    }
+
+    #[test]
+    fn slot_duration_fallback_ms_only() {
+        let mainnet = ChainSpec::mainnet();
+        let mut config = Config::from_chain_spec::<MainnetEthSpec>(&mainnet);
+        config.seconds_per_slot = None;
+        config.slot_duration_ms = Some(MaybeQuoted { value: 12000 });
+        let spec = config
+            .apply_to_chain_spec::<MainnetEthSpec>(&mainnet)
+            .unwrap();
+        assert_eq!(spec.seconds_per_slot, 12);
+        assert_eq!(spec.slot_duration_ms, 12000);
+    }
+
+    #[test]
+    fn slot_duration_fallback_neither() {
+        let mainnet = ChainSpec::mainnet();
+        let mut config = Config::from_chain_spec::<MainnetEthSpec>(&mainnet);
+        config.seconds_per_slot = None;
+        config.slot_duration_ms = None;
+        assert!(
+            config
+                .apply_to_chain_spec::<MainnetEthSpec>(&mainnet)
+                .is_none()
+        );
+    }
+
     #[test]
     fn blob_schedule_max_blobs_per_block() {
         let spec_contents = r#"
@@ -3375,7 +3469,6 @@ mod yaml_tests {
         // Test slot duration
         let slot_duration = spec.get_slot_duration();
         assert_eq!(slot_duration, Duration::from_millis(12000));
-        assert_eq!(slot_duration, Duration::from_secs(spec.seconds_per_slot));
 
         // Test edge cases with custom spec
         let mut custom_spec = spec.clone();
@@ -3485,4 +3578,133 @@ mod yaml_tests {
         spec.attestation_due_bps = 15000;
         spec.compute_derived_values::<MainnetEthSpec>();
     }
+
+    fn configs_base_path() -> PathBuf {
+        env::var("CARGO_MANIFEST_DIR")
+            .expect("should know manifest dir")
+            .parse::<PathBuf>()
+            .expect("should parse manifest dir as path")
+            .join("configs")
+    }
+
+    /// Upstream config keys that Lighthouse intentionally does not include in its
+    /// `Config` struct. These are forks/features not yet implemented. Update this
+    /// list as new forks are added.
+    const UPSTREAM_KEYS_NOT_IN_LIGHTHOUSE: &[&str] = &[
+        // Forks not yet implemented
+        "HEZE_FORK_VERSION",
+        "HEZE_FORK_EPOCH",
+        "EIP7928_FORK_VERSION",
+        "EIP7928_FORK_EPOCH",
+        // Gloas params not yet in Config
+        "ATTESTATION_DUE_BPS_GLOAS",
+        "AGGREGATE_DUE_BPS_GLOAS",
+        "SYNC_MESSAGE_DUE_BPS_GLOAS",
+        "CONTRIBUTION_DUE_BPS_GLOAS",
+        "PAYLOAD_ATTESTATION_DUE_BPS",
+        "MAX_REQUEST_PAYLOADS",
+        // Gloas fork choice params not yet in Config
+        "REORG_HEAD_WEIGHT_THRESHOLD",
+        "REORG_PARENT_WEIGHT_THRESHOLD",
+        "REORG_MAX_EPOCHS_SINCE_FINALIZATION",
+        // Heze networking
+        "VIEW_FREEZE_CUTOFF_BPS",
+        "INCLUSION_LIST_SUBMISSION_DUE_BPS",
+        "PROPOSER_INCLUSION_LIST_CUTOFF_BPS",
+        "MAX_REQUEST_INCLUSION_LIST",
+        "MAX_BYTES_PER_INCLUSION_LIST",
+    ];
+
+    /// Compare a `ChainSpec` against an upstream consensus-specs config YAML file.
+    ///
+    /// 1. Extracts keys from the raw YAML text (to avoid yaml_serde's inability
+    ///    to parse integers > u64 into `Value`/`Mapping` types) and checks that
+    ///    every key is either known to `Config` or explicitly listed in
+    ///    `UPSTREAM_KEYS_NOT_IN_LIGHTHOUSE`.
+    /// 2. Deserializes the upstream YAML as `Config` (which has custom
+    ///    deserializers for large values like `TERMINAL_TOTAL_DIFFICULTY`) and
+    ///    compares against `Config::from_chain_spec`.
+    fn config_test<E: EthSpec>(spec: &ChainSpec, config_name: &str) {
+        let file_path = configs_base_path().join(format!("{config_name}.yaml"));
+        let upstream_yaml = std::fs::read_to_string(&file_path)
+            .unwrap_or_else(|e| panic!("failed to read {}: {e}", file_path.display()));
+
+        // Extract top-level keys from the raw YAML text. We can't parse as
+        // yaml_serde::Mapping because yaml_serde cannot represent integers
+        // exceeding u64 (e.g. TERMINAL_TOTAL_DIFFICULTY). Config YAML uses a
+        // simple `KEY: value` format with no indentation for top-level keys.
+        let upstream_keys: BTreeSet<String> = upstream_yaml
+            .lines()
+            .filter_map(|line| {
+                // Skip comments, blank lines, and indented lines (nested YAML).
+                if line.is_empty()
+                    || line.starts_with('#')
+                    || line.starts_with(' ')
+                    || line.starts_with('\t')
+                {
+                    return None;
+                }
+                line.split(':').next().map(|k| k.to_string())
+            })
+            .collect();
+
+        // Get the set of keys that Config knows about by serializing and collecting
+        // keys. Also include keys for optional fields that may be skipped during
+        // serialization (e.g. CONFIG_NAME).
+        let our_config = Config::from_chain_spec::<E>(spec);
+        let our_yaml = yaml_serde::to_string(&our_config).expect("failed to serialize Config");
+        let our_mapping: yaml_serde::Mapping =
+            yaml_serde::from_str(&our_yaml).expect("failed to re-parse our Config");
+        let mut known_keys: BTreeSet<String> = our_mapping
+            .keys()
+            .filter_map(|k| k.as_str().map(String::from))
+            .collect();
+        // Fields that Config knows but may skip during serialization.
+        known_keys.insert("CONFIG_NAME".to_string());
+
+        // Check for upstream keys that our Config doesn't know about.
+        let mut missing_keys: Vec<&String> = upstream_keys
+            .iter()
+            .filter(|k| {
+                !known_keys.contains(k.as_str())
+                    && !UPSTREAM_KEYS_NOT_IN_LIGHTHOUSE.contains(&k.as_str())
+            })
+            .collect();
+        missing_keys.sort();
+
+        assert!(
+            missing_keys.is_empty(),
+            "Upstream {config_name} config has keys not present in Lighthouse Config \
+             (add to Config or to UPSTREAM_KEYS_NOT_IN_LIGHTHOUSE): {missing_keys:?}"
+        );
+
+        // Compare values for all fields Config knows about.
+        let mut upstream_config: Config = yaml_serde::from_str(&upstream_yaml)
+            .unwrap_or_else(|e| panic!("failed to parse {config_name} as Config: {e}"));
+
+        // CONFIG_NAME is network metadata (not a spec parameter), so align it
+        // before comparing.
+        upstream_config.config_name = our_config.config_name.clone();
+        // SECONDS_PER_SLOT is deprecated upstream but we still emit it, so
+        // fill it in if the upstream YAML omitted it.
+        if upstream_config.seconds_per_slot.is_none() {
+            upstream_config.seconds_per_slot = our_config.seconds_per_slot;
+        }
+        assert_eq!(
+            upstream_config, our_config,
+            "Config mismatch for {config_name}"
+        );
+    }
+
+    #[test]
+    fn mainnet_config_consistent() {
+        let spec = ChainSpec::mainnet();
+        config_test::<MainnetEthSpec>(&spec, "mainnet");
+    }
+
+    #[test]
+    fn minimal_config_consistent() {
+        let spec = ChainSpec::minimal();
+        config_test::<MinimalEthSpec>(&spec, "minimal");
+    }
 }