[MegaRedHand]

🗒️ Description / Motivation

ethlambda beacon --network <dir> ignored a config's SLOT_DURATION_MS. derive_genesis_fields always set slot_duration_ms = seconds_per_slot * 1000, and Config's container-level serde default fills a missing SECONDS_PER_SLOT with mainnet's 12.

The config.yaml that kurtosis' ethereum-package generates (ethereum-genesis-generator v6.2.7) carries only SLOT_DURATION_MS, since consensus-specs dropped SECONDS_PER_SLOT. So any kurtosis devnet with seconds_per_slot != 12 ran ethlambda on a 12-second clock with no error.

What Changed

The slot-time rule now lives in one place, reconcile_slot_time in network/config_file.rs, called from ConfigFile::parse. Built-in networks and loaded directories both parse through it. It reads the raw key set to tell an absent key from one holding the default:

Keys in the file	Behavior
both	SECONDS_PER_SLOT * 1000 must equal SLOT_DURATION_MS; otherwise startup fails naming both keys and values
only SLOT_DURATION_MS	must be a non-zero whole number of seconds; seconds_per_slot is derived from it
only SECONDS_PER_SLOT	slot_duration_ms derived from it (as before)
neither	mainnet defaults (as before)

• network/mod.rs: derive_genesis_fields now only sets genesis_time.
• Test fixtures devnet and devnet-electra carried a deliberate 6 / 12000 pair to prove seconds won; they now carry 6 / 6000.

Correctness / Behavior Guarantees

• Behavior change: a config whose two slot keys disagree is now refused at startup. Before, SECONDS_PER_SLOT silently won.
• Mainnet, sepolia and hoodi all carry 12 / 12000 and load unchanged.
• Config's shape is unchanged.

Tests Added / Run

• config_file.rs: slot_time_keys_are_reconciled_whichever_are_present (all four combinations)
• dir.rs: a_config_with_only_slot_duration_ms_is_read_from_it, a_slot_duration_ms_that_is_not_whole_seconds_is_rejected_naming_the_field, a_config_with_neither_slot_key_keeps_the_mainnet_default, disagreeing_slot_keys_are_rejected_naming_both_keys_and_values

Ran cargo fmt --all, cargo test -p ethlambda --profile release-fast --bins (231 passed) and cargo clippy --locked -p ethlambda --all-targets -- -D warnings. The workspace-wide make lint/make test were not run locally; CI covers them.

Related Issues / PRs

• Related to ci: add a kurtosis + assertoor battery for the beacon node #679 (kurtosis + assertoor battery, which runs on ethereum-package configs)

[github-actions]

🤖 Kimi Code Review

I'll review this PR which adds support for SLOT_DURATION_MS-only configs (like Kurtosis-generated ones) while maintaining backward compatibility with SECONDS_PER_SLOT.

Overall Assessment

The PR is well-structured with good test coverage and clear documentation. However, I found several issues ranging from minor correctness problems to a potential consensus-critical bug.

---

Critical Issues

1. Integer Division Truncation Risk in derive_genesis_fields (bin/ethlambda/src/network/mod.rs:241)

config.seconds_per_slot = config.slot_duration_ms / 1_000;

Problem: The caller validates slot_duration_ms % 1_000 == 0 in dir.rs, but this validation is not enforced at the type level. If derive_genesis_fields is ever called from another path without validation, truncation silently occurs.

Recommendation: Add a debug assertion or make the function unsafe without validation. At minimum:

debug_assert!(config.slot_duration_ms > 0 && config.slot_duration_ms % 1_000 == 0,
              "slot_duration_ms must be non-zero multiple of 1000");

---

Security / Correctness Issues

2. slot_duration_ms Default Value Attack Surface (bin/ethlambda/src/network/config_file.rs:50)

let slot_duration_only =
    document.contains_key("SLOT_DURATION_MS") && !document.contains_key("SECONDS_PER_SLOT");

Problem: When both keys are present, slot_duration_only is false and SECONDS_PER_SLOT wins. However, if a malicious or buggy config has SLOT_DURATION_MS: 5000 and SECONDS_PER_SLOT: 6, the millisecond value is silently ignored. This could confuse operators.

Recommendation: Consider adding a warning or strict error when both are present with inconsistent values. The current behavior is documented but potentially hazardous.

3. Inconsistent Validation Between built_in.rs and dir.rs

Path	Validates slot_duration_ms whole seconds?
dir.rs	Yes (InvalidSlotDurationMs error)
built_in.rs	No

Problem: built_in.rs at line 71-73 calls derive_genesis_fields with slot_duration_only but never validates slot_duration_ms:

// built_in.rs:71-73
let slot_duration_only = parsed.slot_duration_only;
let mut config = parsed.config;
super::derive_genesis_fields(&mut config, genesis.genesis_time, slot_duration_only);

If a built-in network definition somehow carries an invalid SLOT_DURATION_MS, it passes through unchecked.

Recommendation: Add the same validation in built_in.rs or extract validation into derive_genesis_fields itself.

---

Code Quality Issues

4. Test Helper Panic Message (bin/ethlambda/src/network/dir.rs:350)

assert_ne!(replaced, text, "config no longer carries SLOT_DURATION_MS");

Problem: The panic message is backwards. When assert_ne! fails, it means replaced == text, i.e., the replacement didn't happen because the original text didn't contain SLOT_DURATION_MS: 12000. The message should say "config must contain SLOT_DURATION_MS: 12000 to replace".

5. Magic Number Duplication

The value 1_000 appears in:

• dir.rs:136 (slot_duration_ms % 1_000)
• mod.rs:239 (config.seconds_per_slot = config.slot_duration_ms / 1_000)
• mod.rs:241 (config.slot_duration_ms = config.seconds_per_slot * 1_000)

Consider a const MS_PER_SECOND: u64 = 1_000;.

6. Test Coverage Gap: built_in.rs Path

No tests verify the BuiltInNetwork path with slot_duration_only: true. The validation gap in Point 3 would be caught by even a basic test.

---

Minor Suggestions

7. Documentation Clarity (`bin/ethlambda/src/network/mod.rs:224-240)

The doc comment says "The caller must have checked it is a non-zero multiple of 1000" but this is only true for the slot_duration_only: true branch. Consider making this explicit:

/// # Panics
/// If `slot_duration_only` is `true` and `config.slot_duration_ms` is not a non-zero
/// multiple of 1000. The `dir.rs` loader enforces this; do not call with unchecked input.

---

Positive Notes

1. Good test coverage for the NetworkDir path with three distinct cases (only ms, invalid ms, neither key)
2. Clear documentation of the Kurtosis use case and why the raw key set matters
3. Backward compatibility maintained — existing SECONDS_PER_SLOT configs work unchanged
4. Explicit error variant InvalidSlotDurationMs with descriptive message

---

Summary

Priority	Item	Location
High	Add validation in built_in.rs or derive_genesis_fields	built_in.rs:71-73, mod.rs:238
Medium	Fix misleading panic message	dir.rs:350
Medium	Add debug assertion for division safety	mod.rs:239
Low	Consider warning on conflicting keys	config_file.rs:50
Low	Extract MS_PER_SECOND constant	multiple
Low	Add built_in.rs test coverage	built_in.rs

---

Automated review by Kimi (Moonshot AI) · kimi-k2.6 · custom prompt

[github-actions]

🤖 Claude Code Review

Review: PR 691, read SLOT_DURATION_MS when SECONDS_PER_SLOT is absent

Overall: The fix is correct and narrowly scoped. I found no blocking issues. I read the diff only and did not build or run the tests.

Correctness

• ConfigFile::parse detects "ms-only" from the raw key set (config_file.rs, the slot_duration_only line). This is the right approach. A serde container-level default can't tell a missing key from one holding the default value, as the doc comment says.
• The derive_genesis_fields branch is sound. It derives seconds from milliseconds only after NetworkDir::load has checked that the value is non-zero and a multiple of 1000, so the integer division is exact.
• Files with SECONDS_PER_SLOT behave as before, whether or not SLOT_DURATION_MS agrees. That keeps the existing contract and means mainnet, sepolia and hoodi are unaffected.
• Rejecting 0 and 6500 at startup, with an error that names the field, is better than silently rounding.

Minor observations (non-blocking)

1. Validation lives only in dir.rs. derive_genesis_fields documents "the caller must have checked" its precondition, and built_in.rs passes the flag through without any check. That is safe today because every embedded config has SECONDS_PER_SLOT. A later embedded chain with an ms-only config would divide unvalidated (a 6500 would truncate to 6). Consider moving the check into derive_genesis_fields, making it return a Result, or adding a debug_assert! there so the invariant is enforced where it matters.
2. slot_duration_only is computed from key presence only. SECONDS_PER_SLOT: with a null value would deserialize through serde's default, and the key still counts as present. This is an unlikely edge case, and the behavior is arguably fine.
3. Test coverage. The three new tests cover the main paths. A small unit test for ConfigFile::parse setting the flag in all four key combinations would pin the detection logic directly, without going through NetworkDir::load.
4. Docs. CLAUDE.md says slot_duration_ms is "always derived" in this area. Check whether docs/cli.md or docs/beacon_wire.md repeat that claim and need a line about the ms-only case.

Security / consensus: Nothing concerning. This is a startup-time config parse that fails closed on bad input. Before the fix, an ms-only config silently ran on a 12-second clock. After it, the configured slot time is used.

Verdict: Looks good, and I'd approve. Item 1 is a cheap hardening step worth considering.

---

Automated review by Claude (Anthropic) · sonnet · custom prompt

[github-actions]

🤖 Codex Code Review

Looks good overall: the change is narrowly scoped to config loading, closes a real correctness gap for generated configs that omit SECONDS_PER_SLOT, and the added tests cover the new behavior well.

• No consensus-critical concerns in fork choice, attestation handling, finalization, state transition, XMSS, or SSZ paths: this PR only changes startup config reconciliation.
• Correctness improves in bin/ethlambda/src/network/mod.rs:236 by making SLOT_DURATION_MS authoritative only when SECONDS_PER_SLOT is absent, which matches the documented generated-config case.
• The new validation in bin/ethlambda/src/network/dir.rs:136 is a good safety check: it rejects SLOT_DURATION_MS values that would otherwise silently truncate when deriving seconds_per_slot.

One small thing to consider:

• bin/ethlambda/src/network/built_in.rs:74 now calls derive_genesis_fields(..., slot_duration_only) but does not perform the same slot_duration_ms == 0 || % 1000 != 0 validation that NetworkDir::load does in bin/ethlambda/src/network/dir.rs:136.
  • Today that seems harmless because the built-in assets all include SECONDS_PER_SLOT, so slot_duration_only is false.
  • Still, this creates an invariant that is enforced only on directory-backed networks, not built-ins. If a future built-in config ever ships with only SLOT_DURATION_MS, derive_genesis_fields at bin/ethlambda/src/network/mod.rs:239 would silently floor-divide.
  • Suggestion: either move the validation next to derive_genesis_fields or add the same guard in BuiltInNetwork::resolve at bin/ethlambda/src/network/built_in.rs:61.

Aside from that, I don’t see bugs, security issues, or performance regressions in this diff.

---

Automated review by OpenAI Codex · gpt-5.4 · custom prompt