feat(cli): add the --discovery.* option group

[MegaRedHand]

What

Adds the three operator-facing flags the discv5 work needs, on their own, so
the implementation PR (#579) is confined to the p2p crate.

Flag	Default	Meaning
--discovery.enable	false	turn discv5 peer discovery on
--discovery.port	9000	UDP port for the discv5 socket
--discovery.advertise-ip	unset	IP to advertise in the ENR

The flags parse and validate here. Nothing reads them yet, which is the
point of splitting them out: this is reviewable on its own and cannot change
runtime behaviour of a node that does not pass them.

Why the port validation

--discovery.port and --gossipsub-port are both UDP and both default to
9000, so enabling discovery without moving one of them collides. Left
unchecked, that surfaces at bind time as an opaque EADDRINUSE on whichever
socket loses the race, pointing at neither flag. CliOptions::validate_discovery
rejects it at startup with a message naming both flags and their values.

The check only fires when discovery is enabled, so the shared default is
harmless for every existing deployment.

Why --discovery.advertise-ip

The node binds the wildcard 0.0.0.0, which is not dialable as published. A
node whose reachable address differs from what it listens on (a devnet on
127.0.0.1, or a host behind NAT) needs to say so explicitly. discv5's
PONG-based IP voting may still learn and substitute the real external address
at runtime; this only sets what the ENR carries at startup.

Testing

• make lint clean.
• Colliding ports are rejected by name:

  $ ethlambda ... --discovery.enable
  Error: --discovery.port (9000) must differ from --gossipsub-port (9000): both bind UDP and cannot share a port
  

• Distinct ports pass validation and startup proceeds:

  $ ethlambda ... --discovery.enable --discovery.port 9010
  Error: failed to load node key from /nonexistent/node.key
  

• The group renders under --help with its dotted prefixes intact.

Relationship to #579

#579 carries the discv5 implementation and currently includes these same
flags. If this lands first, #579 rebases onto it and drops the cli.rs hunk.

[greptile-apps]

Your trial has ended. Reactivate Greptile to resume code reviews.

[github-actions]

🤖 Claude Code Review

Good, this confirms nothing else reads the new fields yet, matching the PR's stated intent. Now let's look at the review findings.

Review: feat(cli): add the --discovery.* option group (PR 580)

This is a small, well-scoped diff that adds three inert CLI flags plus validation. Overall it's clean and matches the stated intent (parse-only, nothing reads the values yet, confirmed by grep — only gossipsub_port is read in main.rs).

Findings

1. --discovery.port collision check only covers --gossipsub-port, not --api-port/--metrics-port.
bin/ethlambda/src/cli.rs:145-153 — validate_discovery only compares against gossipsub_port. That's fine since the PR is explicit that this only guards the two UDP sockets (QUIC and discv5), and TCP/UDP namespaces don't collide, so this is correct as scoped. Not a bug, just confirming the check's boundary is intentional and correctly limited — no action needed.

2. No validation on --discovery.advertise-ip for degenerate addresses.
A user could pass 0.0.0.0 or :: (unspecified) or a multicast address to --discovery.advertise-ip, which would be nonsensical for an ENR entry (defeats the purpose of the flag, since the whole point is to avoid publishing the wildcard). Since nothing reads this field yet, it's low-risk today, but worth a // TODO or an issue reference before it lands in the implementation PR (#579), where a garbage-in-ENR value is harder to catch. Minor, not blocking here — the doc comment already partially addresses "why," but doesn't warn against unspecified addresses.

3. Doc comment on advertise_ip states behavior that isn't implemented yet.
bin/ethlambda/src/cli.rs:135-139 says "Defaults to the bind address... discv5's PONG-based IP voting may still replace it at runtime" — this describes behavior that belongs to the (not-yet-merged) implementation PR #579, not to this flag-only PR. It reads fine as forward-looking documentation, but since this PR explicitly says "nothing reads them yet," consider trimming the doc to describe only what the flag is (an optional override), leaving the runtime-behavior explanation for #579 where the code actually exists. Cosmetic/nit.

4. validate_discovery placement.
Defined as an impl CliOptions block sandwiched between DiscoveryConfig's struct definition and the #[cfg(feature = "shadow-integration")] ShadowOptions struct (cli.rs:141-155). This works but slightly interrupts the file's struct-then-impl grouping (three struct/impl blocks intermixed). Purely stylistic — not worth blocking on.

Correctness / security

• No unsafe code, no crypto/consensus-path changes, no SSZ involved — this PR cannot affect fork choice, attestations, or state transition, consistent with its stated scope.
• The port-collision guard is fail-fast at startup (main.rs:84, right after CliOptions::parse()), before any socket binds occur — correct placement, avoids the opaque EADDRINUSE the PR description calls out.
• default_value = "false") on enable/bool fields and #[command(flatten)] usage match existing conventions in the same file (is_aggregator, disable_duty_sync_gate, ShadowOptions).

Rust idioms

• Clean use of clap::Args flattening, eyre::bail! with an actionable message naming both flags and values — matches the rest of the file's error-handling style.
• std::net::IpAddr is fully qualified in the advertise_ip field (cli.rs:139) even though IpAddr is already imported at the top of the file (cli.rs:3). Trivial inconsistency — could just be IpAddr for consistency with http_address: IpAddr above.

Overall: solid, low-risk, correctly scoped PR. Only nit-level feedback (item 5 dedup import, item 3 doc-comment scope) — nothing blocking.

---

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

[github-actions]

🤖 Kimi Code Review

The diff adds CLI configuration for discv5 peer discovery with proper validation to prevent UDP port collisions. This is networking infrastructure code and does not touch consensus-critical paths (fork choice, attestation processing, state transition, or XMSS).

General Assessment
Clean, idiomatic Rust with good documentation. The validation logic correctly prevents the primary configuration footgun (UDP port collision between discovery and gossipsub).

Minor Suggestions

1. Consider validating against port 0 (bin/ethlambda/src/cli.rs:149)
   Binding to port 0 triggers ephemeral port allocation. For discovery, this makes the ENR unstable across restarts. Consider rejecting port 0 explicitly:

   if self.discovery.port == 0 {
       eyre::bail!("--discovery.port cannot be 0");
   }

2. Log warning on unspecified advertise address (bin/ethlambda/src/main.rs:84)
   Since advertise_ip: None results in publishing 0.0.0.0 (undialable), consider emitting a warning when discovery is enabled but no advertise IP is set:

   if options.discovery.enable && options.discovery.advertise_ip.is_none() {
       tracing::warn!("Discovery enabled without --discovery.advertise-ip; ENR will advertise 0.0.0.0 which peers cannot dial");
   }

Code Quality Notes

• Line 122: Option<std::net::IpAddr> correctly supports both IPv4 and IPv6.
• Line 149: The validation correctly uses eyre::bail! for concise error propagation, consistent with the crate's error handling style.
• Line 13: #[command(flatten)] is the idiomatic way to nest clap argument structs.

Security
No vulnerabilities identified. The IpAddr type ensures valid address parsing at the CLI level. The port collision check prevents EADDRINUSE race conditions at startup.

---

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

[github-actions]

🤖 Codex Code Review

1. bin/ethlambda/src/main.rs:84 introduces a startup-order regression for test-driver mode. options.validate_discovery()? now runs before the existing HIVE_LEAN_TEST_DRIVER early exit at main.rs:112, even though that mode is explicitly meant to skip consensus/p2p prerequisites. As written, a test-driver invocation that sets --discovery.enable but leaves the default port collision will now fail during argument validation despite never binding either UDP socket. Moving the validation to the normal consensus startup path, after the test-driver short-circuit, would preserve the previous behavior.

No other correctness, security, performance, or consensus-layer concerns stood out in this PR. The actual check in cli.rs:156 is sensible and prevents an otherwise opaque EADDRINUSE failure for the QUIC/discovery UDP port collision.

I could not run a compile check in this environment because rustup failed to create temp files under /home/runner/.rustup (Read-only file system).

---

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