feat(cli): default --attestation-committee-count from validator-config.yaml

[MegaRedHand]

🗒️ Description / Motivation

Make --attestation-committee-count optional and let it default to the
config.attestation_committee_count field already defined in
validator-config.yaml (the shared genesis bundle every Lean client
reads). Falls back to 1 when the field is also absent.

This lets multi-subnet networks like lean-quickstart's ansible-devnet
(which sets attestation_committee_count: 2 in the bundle) drop the
per-client wrapper flag and rely on a single source of truth.

What Changed

• bin/ethlambda/src/main.rs
  • --attestation-committee-count is now Option<u64> with no default.
  • New ValidatorConfigFile / ValidatorConfigBlock structs read the
    optional config.attestation_committee_count while parsing the file
    we already read for the metrics name registry.
  • Resolution order: CLI flag > validator-config.yaml > 1.
  • Logs the resolved value at startup.
  • 5 unit tests covering precedence and YAML shapes (with and without
    the field).

Correctness / Behavior Guarantees

• Pre-existing CLI behavior preserved when --attestation-committee-count
  is supplied: same value, same metric, same swarm config.
• Lower bound (1) still enforced via clap's value parser.
• No change to the SwarmConfig API or downstream callers; only the
  binding site in main switches from a CLI default to the resolved
  value.

Tests Added / Run

• cargo test -p ethlambda --release — 23 pass, including 5 new
  tests::* covering: file parses the field, defaults to None when
  absent, CLI overrides file, file fallback when CLI absent, default
  fallback when neither is set.
• cargo clippy -p ethlambda --tests -- -D warnings — clean.
• cargo fmt --all — clean.
• Live runtime verification with the branch's docker image, all
  three resolution paths fired and logged correctly:

  Scenario	CLI flag	YAML field	Resolved
  File path	(omitted)	2	2
  Default path	(omitted)	(absent)	1
  CLI override	7	2	7

• End-to-end devnet run on lean-quickstart devnet4 branch with
  the branch's image: 8/10 multi-client nodes up, ethlambda elected as
  subnet-0 aggregator, blocks produced/imported, justified_slot=12 vote_count=7 threshold=7.

Related Issues / PRs

• Mirrors the existing attestationCommitteeCount env wiring in
  lean-quickstart/client-cmds/ethlambda-cmd.sh; once this lands, the
  flag forwarding there can be retired in a follow-up.

[github-actions]

🤖 Claude Code Review

Now I have everything I need. Here is the review:

---

PR #337 — feat(cli): default --attestation-committee-count from validator-config.yaml

Overview

Makes --attestation-committee-count optional and adds a three-tier resolution: CLI flag > config.attestation_committee_count from validator-config.yaml > hard default of 1. The YAML parsing is factored into proper named structs, and five unit tests cover the parsing and precedence logic.

The PR is well-motivated and the overall approach is sound. Two issues need attention before merge.

---

Issues

1. Missing range validation for the YAML-sourced value (potential panic)

The CLI flag correctly enforces value_parser = clap::value_parser!(u64).range(1..), but the YAML path has no equivalent guard. attestation_committee_count is used as a modulus in two places:

• crates/net/p2p/src/lib.rs:236 — vid % config.attestation_committee_count
• crates/net/p2p/src/gossipsub/handler.rs:137 — validator % server.attestation_committee_count

A YAML value of 0 reaches both of these as a u64, causing a runtime panic with "attempt to calculate the remainder with a divisor of zero". The fix is to add an explicit check after resolution:

let attestation_committee_count = options
    .attestation_committee_count
    .or(validator_config_file.config.attestation_committee_count)
    .unwrap_or(1);

// Guard for YAML-sourced values which bypass clap's range(1..) validator.
if attestation_committee_count == 0 {
    eyre::bail!("`attestation_committee_count` must be at least 1 (got 0 from validator-config.yaml)");
}

2. Three of five tests are testing Option::or + unwrap_or, not the production code

cli_overrides_file_value, falls_back_to_file_when_cli_absent, and falls_back_to_default_when_neither_set each inline the resolution expression rather than calling the code that runs in production. If the resolution logic in main() is changed (e.g. a different default, an added validation step), all three tests continue to pass unchanged.

The two parsing tests (parses_committee_count_when_present, defaults_to_none_when_field_absent) are solid since they test actual deserialization. For the precedence tests, extract the resolution into a small free function so the tests call real code:

fn resolve_attestation_committee_count(
    cli: Option<u64>,
    from_file: Option<u64>,
) -> u64 {
    cli.or(from_file).unwrap_or(1)
}

Then call resolve_attestation_committee_count(...) from both main() and the three tests. If a validation check is added later, it lives in one place.

---

Minor Notes

• The comment // Resolve attestation_committee_count: CLI flag > validator-config.yaml > 1. is worthwhile — this three-tier precedence is a non-obvious business rule; leaving it is the right call per project guidelines.
• ValidatorConfigEntry naming is clearer than the former anonymous Validator local struct — good improvement.
• #[serde(default)] on both config: ValidatorConfigBlock and attestation_committee_count: Option<u64> correctly handles both a missing config section and a present-but-empty one. No issue there.
• Switching populate_name_registry from consuming into_iter() to borrowing iter() + v.name.clone() is the right tradeoff given that ValidatorConfigFile is needed afterward.

---

Summary

The feature logic is correct and the tests are thorough enough for the parsing layer. The two items above — the 0 panic path from YAML and the hollow precedence tests — should be addressed before merge. Item 1 is the more important of the two.

---

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

[greptile-apps]

Greptile Summary

This PR makes --attestation-committee-count optional and resolves it in priority order: CLI flag → config.attestation_committee_count in validator-config.yaml → 1. The refactoring introduces typed deserialization structs (ValidatorConfigFile, ValidatorConfigBlock, ValidatorConfigEntry) and five unit tests covering all three resolution paths.

• The YAML-sourced value bypasses the range(1..) lower-bound guard that clap enforces for the CLI flag. A validator-config.yaml entry of attestation_committee_count: 0 is silently accepted and propagated to both set_attestation_committee_count and SwarmConfig.

Confidence Score: 4/5

Safe to merge after addressing the missing lower-bound validation for the YAML-sourced committee count.

One P1 finding: YAML path silently accepts attestation_committee_count: 0, bypassing the range(1..) invariant enforced by clap. Logic is otherwise clean and well-tested.

bin/ethlambda/src/main.rs — specifically the ValidatorConfigBlock deserialization and the resolution step.

Important Files Changed

Filename	Overview
bin/ethlambda/src/main.rs	Makes --attestation-committee-count optional with a three-level resolution (CLI > YAML > 1); introduces typed deserialization structs and 5 unit tests. One issue: the YAML path bypasses the range(1..) lower-bound enforced by clap.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Startup] --> B{CLI --attestation-committee-count set?}
    B -- Yes --> C[Use CLI value\nclap enforces range >= 1]
    B -- No --> D{validator-config.yaml\nconfig.attestation_committee_count present?}
    D -- Yes --> E[Use YAML value\n⚠️ no lower-bound check]
    D -- No --> F[Use default: 1]
    C --> G[set_attestation_committee_count]
    E --> G
    F --> G
    G --> H[SwarmConfig.attestation_committee_count]

Loading

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
bin/ethlambda/src/main.rs:266-270
**YAML path bypasses the `range(1..)` lower-bound validator**

When `attestation_committee_count` is supplied via the CLI, clap rejects any value below 1. When it comes from `validator-config.yaml`, there is no equivalent guard. A YAML entry of `attestation_committee_count: 0` is silently accepted, reaches `set_attestation_committee_count(0)`, and is forwarded into `SwarmConfig` — producing the same invalid state that the CLI validator was added to prevent.

Consider adding an `assert!(val >= 1)` (or returning an error) after resolving the value, so the invariant is enforced regardless of the source.

_{Reviews (1): Last reviewed commit: "refactor: simplify code a bit" | Re-trigger Greptile}

[greptile-apps]

YAML path bypasses the range(1..) lower-bound validator

When attestation_committee_count is supplied via the CLI, clap rejects any value below 1. When it comes from validator-config.yaml, there is no equivalent guard. A YAML entry of attestation_committee_count: 0 is silently accepted, reaches set_attestation_committee_count(0), and is forwarded into SwarmConfig — producing the same invalid state that the CLI validator was added to prevent.

Consider adding an assert!(val >= 1) (or returning an error) after resolving the value, so the invariant is enforced regardless of the source.

Prompt To Fix With AI

This is a comment left during a code review.
Path: bin/ethlambda/src/main.rs
Line: 266-270

Comment:
**YAML path bypasses the `range(1..)` lower-bound validator**

When `attestation_committee_count` is supplied via the CLI, clap rejects any value below 1. When it comes from `validator-config.yaml`, there is no equivalent guard. A YAML entry of `attestation_committee_count: 0` is silently accepted, reaches `set_attestation_committee_count(0)`, and is forwarded into `SwarmConfig` — producing the same invalid state that the CLI validator was added to prevent.

Consider adding an `assert!(val >= 1)` (or returning an error) after resolving the value, so the invariant is enforced regardless of the source.

How can I resolve this? If you propose a fix, please make it concise.

[github-actions]

🤖 Kimi Code Review

The changes are well-structured and improve configuration management for the consensus-critical attestation_committee_count parameter. Code is idiomatic and the test coverage is good.

Consensus Safety Note
The attestation_committee_count determines attestation subnet assignment. If nodes in the same network use different values, attestations will be published to mismatched subnets, causing aggregation failures and potential finality delays. The new precedence (CLI > config file > default) is correct, but operators should ensure this value is consistent across all nodes in a deployment. Changing this on a running network requires a coordinated hard fork.

Code Quality

• Line 79-82: Clear documentation of fallback precedence.
• Line 144-149: Correct resolution logic. Consider extracting this into a pure function (e.g., resolve_committee_count(cli, file) -> u64) to avoid duplicating the logic in unit tests (lines 577-596).
• Line 237-264: Good use of #[serde(default)] to handle missing optional fields without Option<T> wrappers for the entire struct.
• Line 275-276: Correct ownership change from into_iter() to iter() + clone() since the config file is now reused after populating the registry.

Minor Suggestions

1. Error context (Line 269-271): Consider including the file path in the panic message to aid debugging:

   let yaml = std::fs::read_to_string(&path)
       .expect(&format!("Failed to read validator config file: {}", path.as_ref().display()));

2. Input validation: The CLI parser enforces >= 1, but consider adding an upper bound check (e.g., <= 64 or whatever the spec MAX_COMMITTEES_PER_SLOT is for this client) to prevent accidental misconfiguration that could cause overflow or memory issues during bitfield calculations.

3. Test duplication: The tests cli_overrides_file_value, falls_back_to_file_when_cli_absent, and falls_back_to_default_when_neither_set duplicate the inline resolution logic. If the precedence rules change, tests must be updated manually. Extracting a helper function would make this self-testing.

Acknowledgment
The refactor properly separates I/O (read_validator_config_file) from side effects (populate_name_registry), improving testability. Memory safety is preserved with no unsafe blocks, and the H256 handling for validator keys is appropriate.

---

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

[github-actions]

🤖 Codex Code Review

1. bin/ethlambda/src/main.rs:149 now accepts config.attestation_committee_count from YAML without reapplying the CLI’s range(1..) invariant. A config value of 0 will flow into % attestation_committee_count at crates/net/p2p/src/lib.rs:236 and crates/net/p2p/src/gossipsub/handler.rs:137, which will panic the node. Reject 0 during deserialization or immediately after resolution; Option<NonZeroU64> would be the idiomatic Rust fix. Add a regression test for attestation_committee_count: 0 as well, since the new tests around bin/ethlambda/src/main.rs:569 only cover precedence, not invalid input.

Otherwise this looks fine. The PR is limited to startup config resolution and metrics wiring; I did not see changes touching fork choice, attestation validation, justification/finalization, state transition, XMSS, or SSZ logic.

I could not run cargo test -p ethlambda --bin ethlambda --quiet in this sandbox because rustup tried to create temp files under a read-only cache.

---

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